Re: [PATCHv4 01/11] videobuf2: add cache management members

[Hans Verkuil]

On 3/9/20 4:27 AM, Sergey Senozhatsky wrote:
> On (20/03/07 12:47), Hans Verkuil wrote:
>>
>> Create those tests in v4l2-compliance: that's where they belong.
>>
>> You need these tests:
>>
>> For non-MMAP modes:
>>
>> 1) test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is never set.
>>
>> If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is not set, then:
>>
>> 1) attempting to use V4L2_FLAG_MEMORY_NON_CONSISTENT will clear the flag
>>    upon return (test with both reqbufs and create_bufs).
>> 2) attempting to use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
>>    will clear those flags upon return (do we actually do that in the patch series?).
> 
> NO_CACHE_INVALIDATE/NO_CACHE_CLEAN are cleared in vb2_fill_vb2_v4l2_buffer()
> [as was suggested], then the struct is copied back to user. But I think it
> would be better to clear those flags when we clear
> V4L2_FLAG_MEMORY_NON_CONSISTENT. We have 4 places which do something
> like
> 	"if !vb2_queue_allows_cache_hints(q) then clear flags bit".
> 
> Besides, vb2_fill_vb2_v4l2_buffer() is called only for !prepared
> buffers, so the flags won't be cleared if the buffer is already
> prepared.
> 
> Another thing is that, it seems, I need to patch compat32 code. It
> copies to user structs member by member so I need to add ->flags.
> 
>> If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is set, then:
>>
>> 1) set V4L2_FLAG_MEMORY_NON_CONSISTENT in reqbufs, but clear in create_bufs:
>>    this should fail.
>> 2) clear V4L2_FLAG_MEMORY_NON_CONSISTENT in reqbufs, but set in create_bufs:
>>    this should fail.
>> 3) set V4L2_FLAG_MEMORY_NON_CONSISTENT in both reqbufs and create_bufs: this should
>>    work.
>> 4) clear V4L2_FLAG_MEMORY_NON_CONSISTENT in both reqbufs and create_bufs: this should
>>    work.
>> 5) you can use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
>>    without these flags being cleared in v4l2_buffer.
>>
>> All these tests can be done in testReqBufs().
> 
> I'm looking into it. Will it work if I patch the vivid test driver to
> enable/disable ->allow_cache_hints bit per-node and include the patch
> into the series. So then v4l2 tests can create some nodes with
> ->allow_cache_hints.  Something like this:

I would add a 'cache_hints' module parameter (array of bool) to tell vivid
whether cache hints should be set or not for each instance. It would be useful
to have this in vivid. Don't forget to update Documentation/media/v4l-drivers/vivid.rst
as well.

Regards,

	Hans

> 
> ---
>  drivers/media/platform/vivid/vivid-core.c     | 6 +++++-
>  drivers/media/platform/vivid/vivid-core.h     | 1 +
>  drivers/media/platform/vivid/vivid-meta-cap.c | 3 +++
>  drivers/media/platform/vivid/vivid-meta-out.c | 3 +++
>  4 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
> index c62c068b6b60..9acbb59d240c 100644
> --- a/drivers/media/platform/vivid/vivid-core.c
> +++ b/drivers/media/platform/vivid/vivid-core.c
> @@ -129,7 +129,8 @@ MODULE_PARM_DESC(node_types, " node types, default is 0xe1d3d. Bitmask with the
>  			     "\t\t    bit 16: Framebuffer for testing overlays\n"
>  			     "\t\t    bit 17: Metadata Capture node\n"
>  			     "\t\t    bit 18: Metadata Output node\n"
> -			     "\t\t    bit 19: Touch Capture node\n");
> +			     "\t\t    bit 19: Touch Capture node\n"
> +			     "\t\t    bit 20: Node supports cache-hints\n");
>  
>  /* Default: 4 inputs */
>  static unsigned num_inputs[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = 4 };
> @@ -977,6 +978,9 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		return -EINVAL;
>  	}
>  
> +	/* do we enable user-space cache management hints */
> +	dev->allow_cache_hints = node_type & 0x100000;
> +
>  	/* do we create a radio receiver device? */
>  	dev->has_radio_rx = node_type & 0x0010;
>  
> diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
> index 99e69b8f770f..2d311fc33619 100644
> --- a/drivers/media/platform/vivid/vivid-core.h
> +++ b/drivers/media/platform/vivid/vivid-core.h
> @@ -206,6 +206,7 @@ struct vivid_dev {
>  	bool				has_meta_out;
>  	bool				has_tv_tuner;
>  	bool				has_touch_cap;
> +	bool				allow_cache_hints;
>  
>  	bool				can_loop_video;
>  
> diff --git a/drivers/media/platform/vivid/vivid-meta-cap.c b/drivers/media/platform/vivid/vivid-meta-cap.c
> index 780f96860a6d..6c28034d3d58 100644
> --- a/drivers/media/platform/vivid/vivid-meta-cap.c
> +++ b/drivers/media/platform/vivid/vivid-meta-cap.c
> @@ -33,6 +33,9 @@ static int meta_cap_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>  	if (vq->num_buffers + *nbuffers < 2)
>  		*nbuffers = 2 - vq->num_buffers;
>  
> +	if (dev->allow_cache_hints)
> +		vq->allow_cache_hints = true;
> +
>  	*nplanes = 1;
>  	return 0;
>  }
> diff --git a/drivers/media/platform/vivid/vivid-meta-out.c b/drivers/media/platform/vivid/vivid-meta-out.c
> index ff8a039aba72..d7b808aa5f6d 100644
> --- a/drivers/media/platform/vivid/vivid-meta-out.c
> +++ b/drivers/media/platform/vivid/vivid-meta-out.c
> @@ -33,6 +33,9 @@ static int meta_out_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>  	if (vq->num_buffers + *nbuffers < 2)
>  		*nbuffers = 2 - vq->num_buffers;
>  
> +	if (dev->allow_cache_hints)
> +		vq->allow_cache_hints = true;
> +
>  	*nplanes = 1;
>  	return 0;
>  }
>