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

  Extend vb2_buffer and vb2_queue structs with cache management
members.

V4L2 UAPI already contains two buffer flags which user-space,
supposedly, can use to control buffer cache sync:

- V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
- V4L2_BUF_FLAG_NO_CACHE_CLEAN

None of these, however, do anything at the moment. This patch
set is intended to change it.

Since user-space cache management hints are supposed to be
implemented on a per-buffer basis we need to extend vb2_buffer
struct with two new members ->need_cache_sync_on_prepare and
->need_cache_sync_on_finish, which will store corresponding
user-space hints.

In order to preserve the existing behaviour, user-space cache
managements flags will be handled only by those drivers that
permit user-space cache hints. That's the purpose of vb2_queue
->allow_cache_hints member. Driver must set ->allow_cache_hints
during queue initialisation to enable cache management hints
mechanism.

Only drivers that set ->allow_cache_hints during queue initialisation
will handle user-space cache management hints. Otherwise hints
will be ignored.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 include/media/videobuf2-core.h | 10 ++++++++++
 1 file changed, 10 insertions(+)
  

On 02/03/2020 05:12, Sergey Senozhatsky wrote:
> Extend vb2_buffer and vb2_queue structs with cache management
> members.
> 
> V4L2 UAPI already contains two buffer flags which user-space,
> supposedly, can use to control buffer cache sync:
> 
> - V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
> - V4L2_BUF_FLAG_NO_CACHE_CLEAN
> 
> None of these, however, do anything at the moment. This patch
> set is intended to change it.
> 
> Since user-space cache management hints are supposed to be
> implemented on a per-buffer basis we need to extend vb2_buffer
> struct with two new members ->need_cache_sync_on_prepare and
> ->need_cache_sync_on_finish, which will store corresponding
> user-space hints.
> 
> In order to preserve the existing behaviour, user-space cache
> managements flags will be handled only by those drivers that
> permit user-space cache hints. That's the purpose of vb2_queue
> ->allow_cache_hints member. Driver must set ->allow_cache_hints
> during queue initialisation to enable cache management hints
> mechanism.
> 
> Only drivers that set ->allow_cache_hints during queue initialisation
> will handle user-space cache management hints. Otherwise hints
> will be ignored.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  include/media/videobuf2-core.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index a2b2208b02da..4a19170672ac 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -263,6 +263,10 @@ struct vb2_buffer {
>  	 *			after the 'buf_finish' op is called.
>  	 * copied_timestamp:	the timestamp of this capture buffer was copied
>  	 *			from an output buffer.
> +	 * need_cache_sync_on_prepare: when set buffer's ->prepare() function
> +	 *			performs cache sync/invalidation.
> +	 * need_cache_sync_on_finish: when set buffer's ->finish() function
> +	 *			performs cache sync/invalidation.
>  	 * queued_entry:	entry on the queued buffers list, which holds
>  	 *			all buffers queued from userspace
>  	 * done_entry:		entry on the list that stores all buffers ready
> @@ -273,6 +277,8 @@ struct vb2_buffer {
>  	unsigned int		synced:1;
>  	unsigned int		prepared:1;
>  	unsigned int		copied_timestamp:1;
> +	unsigned int		need_cache_sync_on_prepare:1;
> +	unsigned int		need_cache_sync_on_finish:1;
>  
>  	struct vb2_plane	planes[VB2_MAX_PLANES];
>  	struct list_head	queued_entry;
> @@ -491,6 +497,9 @@ struct vb2_buf_ops {
>   * @uses_requests: requests are used for this queue. Set to 1 the first time
>   *		a request is queued. Set to 0 when the queue is canceled.
>   *		If this is 1, then you cannot queue buffers directly.
> + * @allow_cache_hints: when set user-space can pass cache management hints in
> + * 		order to skip cache flush/invalidation on ->prepare() or/and
> + * 		->finish().

checkpatch complains about a space before a tab in the two lines above.

Regards,

	Hans

>   * @lock:	pointer to a mutex that protects the &struct vb2_queue. The
>   *		driver can set this to a mutex to let the v4l2 core serialize
>   *		the queuing ioctls. If the driver wants to handle locking
> @@ -564,6 +573,7 @@ struct vb2_queue {
>  	unsigned			requires_requests:1;
>  	unsigned			uses_qbuf:1;
>  	unsigned			uses_requests:1;
> +	unsigned			allow_cache_hints:1;
>  
>  	struct mutex			*lock;
>  	void				*owner;
>
  

On (20/03/06 14:57), Hans Verkuil wrote:
[..]
> > @@ -491,6 +497,9 @@ struct vb2_buf_ops {
> >   * @uses_requests: requests are used for this queue. Set to 1 the first time
> >   *		a request is queued. Set to 0 when the queue is canceled.
> >   *		If this is 1, then you cannot queue buffers directly.
> > + * @allow_cache_hints: when set user-space can pass cache management hints in
> > + * 		order to skip cache flush/invalidation on ->prepare() or/and
> > + * 		->finish().
> 
> checkpatch complains about a space before a tab in the two lines above.

I see them. Sorry. Fixed now.

	-ss
  

On (20/03/06 14:57), Hans Verkuil wrote:
[..]
> >   * @lock:	pointer to a mutex that protects the &struct vb2_queue. The
> >   *		driver can set this to a mutex to let the v4l2 core serialize
> >   *		the queuing ioctls. If the driver wants to handle locking
> > @@ -564,6 +573,7 @@ struct vb2_queue {
> >  	unsigned			requires_requests:1;
> >  	unsigned			uses_qbuf:1;
> >  	unsigned			uses_requests:1;
> > +	unsigned			allow_cache_hints:1;

Shall I use "unsigned int" here instead of "unsigned"?

	-ss
  

On 07/03/2020 10:46, Sergey Senozhatsky wrote:
> On (20/03/06 14:57), Hans Verkuil wrote:
> [..]
>>>   * @lock:	pointer to a mutex that protects the &struct vb2_queue. The
>>>   *		driver can set this to a mutex to let the v4l2 core serialize
>>>   *		the queuing ioctls. If the driver wants to handle locking
>>> @@ -564,6 +573,7 @@ struct vb2_queue {
>>>  	unsigned			requires_requests:1;
>>>  	unsigned			uses_qbuf:1;
>>>  	unsigned			uses_requests:1;
>>> +	unsigned			allow_cache_hints:1;
> 
> Shall I use "unsigned int" here instead of "unsigned"?

The vb2_queue bitfields are the only places in that header were 'unsigned' is
used. I think that that should be fixed in a separate patch. It's nice to have
it consistent.

Put that patch in the beginning of the series, that way I can pick it up in the
next pull request.

Regards,

	Hans

> 
> 	-ss
>
  

On (20/03/07 11:10), Hans Verkuil wrote:
[..]
> >>> @@ -564,6 +573,7 @@ struct vb2_queue {
> >>>  	unsigned			requires_requests:1;
> >>>  	unsigned			uses_qbuf:1;
> >>>  	unsigned			uses_requests:1;
> >>> +	unsigned			allow_cache_hints:1;
> > 
> > Shall I use "unsigned int" here instead of "unsigned"?
> 
> The vb2_queue bitfields are the only places in that header were 'unsigned' is
> used. I think that that should be fixed in a separate patch. It's nice to have
> it consistent.
> 
> Put that patch in the beginning of the series, that way I can pick it up in the
> next pull request.

OK, done.

For the time being the series has moved to github public repo [0],
I'll try to run more 'twisty' cases and re-submit once it survives
beating.

[0] https://github.com/sergey-senozhatsky/v4l2-mmap-cache-flags

	-ss
  

On 07/03/2020 12:28, Sergey Senozhatsky wrote:
> On (20/03/07 11:10), Hans Verkuil wrote:
> [..]
>>>>> @@ -564,6 +573,7 @@ struct vb2_queue {
>>>>>  	unsigned			requires_requests:1;
>>>>>  	unsigned			uses_qbuf:1;
>>>>>  	unsigned			uses_requests:1;
>>>>> +	unsigned			allow_cache_hints:1;
>>>
>>> Shall I use "unsigned int" here instead of "unsigned"?
>>
>> The vb2_queue bitfields are the only places in that header were 'unsigned' is
>> used. I think that that should be fixed in a separate patch. It's nice to have
>> it consistent.
>>
>> Put that patch in the beginning of the series, that way I can pick it up in the
>> next pull request.
> 
> OK, done.
> 
> For the time being the series has moved to github public repo [0],
> I'll try to run more 'twisty' cases and re-submit once it survives
> beating.

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?).

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().

Regards,

	Hans

> 
> [0] https://github.com/sergey-senozhatsky/v4l2-mmap-cache-flags
> 
> 	-ss
>
  

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:

---
 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;
 }
  

On (20/03/09 12:27), 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".

No. I'll just move NO_CACHE_INVALIDATE/NO_CACHE_CLEAN handling to
set_buffer_cache_hints(). This is where we take care of q->memory
and q->allow_cache_hints, this is where we set/clear the
->need_cache_sync flags, this is where we also can clear v4l2_buffer
->flags. Seems like a better place than vb2_fill_vb2_v4l2_buffer().

---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index b4b379f3bf98..0a6bd4a1f58e 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -199,15 +199,6 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
 	vbuf->request_fd = -1;
 	vbuf->is_held = false;
 
-	/*
-	 * Clear buffer cache flags if queue does not support user space hints.
-	 * That's to indicate to userspace that these flags won't work.
-	 */
-	if (!vb2_queue_allows_cache_hints(q)) {
-		b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
-		b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
-	}
-
 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
 		switch (b->memory) {
 		case VB2_MEMORY_USERPTR:
@@ -368,8 +359,16 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
 	vb->need_cache_sync_on_prepare = 1;
 	vb->need_cache_sync_on_finish = 1;
 
-	if (!vb2_queue_allows_cache_hints(q))
+	if (!vb2_queue_allows_cache_hints(q)) {
+		/*
+		 * Clear buffer cache flags if queue does not support user
+		 * space hints. That's to indicate to userspace that these
+		 * flags won't work.
+		 */
+		b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
+		b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
 		return;
+	}
 
 	/*
 	 * ->finish() cache sync can be avoided when queue direction is
  

On (20/03/07 12:47), Hans Verkuil wrote:
> 
> 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
>
[..]
>
> All these tests can be done in testReqBufs().

MEMORY_NON_CONSISTENT is a queue property, we set it during queue setup.
NO_CACHE_INVALIDATE/FLAG_NO_CACHE_CLEAN is a buffer property, we set it
when we qbuf. I'm not sure if all of these can be done in testReqBufts().

	-ss
  

On 3/9/20 5:03 AM, Sergey Senozhatsky wrote:
> On (20/03/07 12:47), Hans Verkuil wrote:
>>
>> 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
>>
> [..]
>>
>> All these tests can be done in testReqBufs().
> 
> MEMORY_NON_CONSISTENT is a queue property, we set it during queue setup.
> NO_CACHE_INVALIDATE/FLAG_NO_CACHE_CLEAN is a buffer property, we set it
> when we qbuf. I'm not sure if all of these can be done in testReqBufts().

testReqBufs can call qbuf, but not streamon. Since you don't need streaming
for this, testReqBufs should be fine.

Regards,

	Hans

> 
> 	-ss
>
  

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;
>  }
>
  

On (20/03/09 08:21), Hans Verkuil wrote:
> 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?).

[..]

> > 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.

I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
is never set" then?

	-ss
  

On 3/9/20 8:25 AM, Sergey Senozhatsky wrote:
> On (20/03/09 08:21), Hans Verkuil wrote:
>> 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?).
> 
> [..]
> 
>>> 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.
> 
> I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
> is never set" then?

Not sure I understand your question. When requesting buffers for non-MMAP memory,
this capability must never be returned. That has nothing to do with a cache_hints
module option.

Regards,

	Hans

> 
> 	-ss
>
  

On (20/03/09 08:28), Hans Verkuil wrote:
> >>> 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.
> > 
> > I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
> > is never set" then?
> 
> Not sure I understand your question. When requesting buffers for non-MMAP memory,
> this capability must never be returned. That has nothing to do with a cache_hints
> module option.

OK. What I thought was that not every MMAP memory node can have cache
hints enabled, so it should be verified that only MMAP nodes which were
configured to allow_cache_hints should report that CAP, the rest of MMAP
nodes (if any) should have that CAP bit clear.

	-ss
  

On Mon, Mar 9, 2020 at 4:28 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 3/9/20 8:25 AM, Sergey Senozhatsky wrote:
> > On (20/03/09 08:21), Hans Verkuil wrote:
> >> 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?).
> >
> > [..]
> >
> >>> 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.
> >
> > I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
> > is never set" then?
>
> Not sure I understand your question. When requesting buffers for non-MMAP memory,
> this capability must never be returned. That has nothing to do with a cache_hints
> module option.

Have we decided that we explicitly don't want to support this for
USERPTR memory, even though technically possible and without much
extra code needed?

Best regards,
Tomasz
  

On (20/03/09 17:58), Tomasz Figa wrote:
[..]
> > > I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
> > > is never set" then?
> >
> > Not sure I understand your question. When requesting buffers for non-MMAP memory,
> > this capability must never be returned. That has nothing to do with a cache_hints
> > module option.
>
> Have we decided that we explicitly don't want to support this for
> USERPTR memory, even though technically possible and without much
> extra code needed?

My irrelevant 5 cents (sorry), I'd probably prefer to land MMAP
first + test drivers patches + v4l-util patches. The effort
required to land this is getting bigger.

	-ss
  

On Mon, Mar 9, 2020 at 6:08 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (20/03/09 17:58), Tomasz Figa wrote:
> [..]
> > > > I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
> > > > is never set" then?
> > >
> > > Not sure I understand your question. When requesting buffers for non-MMAP memory,
> > > this capability must never be returned. That has nothing to do with a cache_hints
> > > module option.
> >
> > Have we decided that we explicitly don't want to support this for
> > USERPTR memory, even though technically possible and without much
> > extra code needed?
>
> My irrelevant 5 cents (sorry), I'd probably prefer to land MMAP
> first + test drivers patches + v4l-util patches. The effort
> required to land this is getting bigger.

I think that's fine, but we need to make it explicit. :)

Best regards,
Tomasz
  

On (20/03/09 18:17), Tomasz Figa wrote:
> 
> I think that's fine, but we need to make it explicit. :)
> 

I'll improve Docs.

	-ss