diff mbox series

videobuf2: always re-init queue memory consistency attribute

Message ID 20200609060417.8066-1-sergey.senozhatsky@gmail.com (mailing list archive)
State Accepted, archived
Delegated to: Hans Verkuil
Headers
Series videobuf2: always re-init queue memory consistency attribute |

Commit Message

Sergey Senozhatsky June 9, 2020, 6:04 a.m. UTC 
  We need a combination of two factors in order to permit modification
of queue's memory consistency attribute in set_queue_consistency():
  (1) queue should allow user-space cache hints
  (2) queue should be used for MMAP-ed I/O

Therefore the code in videobuf2 core looks as follows:

	q->memory = req->memory;
	set_queue_consistency(q, consistent);

This works when we do something like this (suppose that queue allows
cache hints)

	reqbufs.memory = V4L2_MEMORY_DMABUF;
	reqbufs.flags = 0;
	doioctl(node, VIDIOC_REQBUFS, &reqbufs);

	reqbufs.memory = V4L2_MEMORY_MMAP;
	reqbufs.flags = V4L2_FLAG_MEMORY_NON_CONSISTENT;
	doioctl(node, VIDIOC_REQBUFS, &reqbufs);

However, this doesn't work the other way around

	reqbufs.memory = V4L2_MEMORY_MMAP;
	reqbufs.flags = V4L2_FLAG_MEMORY_NON_CONSISTENT;
	doioctl(node, VIDIOC_REQBUFS, &reqbufs);

	reqbufs.memory = V4L2_MEMORY_DMABUF;
	reqbufs.flags = 0;
	doioctl(node, VIDIOC_REQBUFS, &reqbufs);

because __vb2_queue_free() doesn't clear queue's ->dma_attrs
once its don't freeing queue buffers, and by the time we call
set_queue_consistency() the queue is DMABUF so (2) is false
and we never clear the stale consistency attribute.

Re-implement set_queue_consistency() - it must always clear
queue's non-consistency attr and set it only if (1) and (2).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/media/common/videobuf2/videobuf2-core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Hans Verkuil June 10, 2020, 9:54 a.m. UTC | #1 
Hi Sergey,

On 09/06/2020 08:04, Sergey Senozhatsky wrote:
> We need a combination of two factors in order to permit modification
> of queue's memory consistency attribute in set_queue_consistency():
>   (1) queue should allow user-space cache hints
>   (2) queue should be used for MMAP-ed I/O
> 
> Therefore the code in videobuf2 core looks as follows:
> 
> 	q->memory = req->memory;
> 	set_queue_consistency(q, consistent);
> 
> This works when we do something like this (suppose that queue allows
> cache hints)
> 
> 	reqbufs.memory = V4L2_MEMORY_DMABUF;
> 	reqbufs.flags = 0;
> 	doioctl(node, VIDIOC_REQBUFS, &reqbufs);
> 
> 	reqbufs.memory = V4L2_MEMORY_MMAP;
> 	reqbufs.flags = V4L2_FLAG_MEMORY_NON_CONSISTENT;
> 	doioctl(node, VIDIOC_REQBUFS, &reqbufs);
> 
> However, this doesn't work the other way around
> 
> 	reqbufs.memory = V4L2_MEMORY_MMAP;
> 	reqbufs.flags = V4L2_FLAG_MEMORY_NON_CONSISTENT;
> 	doioctl(node, VIDIOC_REQBUFS, &reqbufs);
> 
> 	reqbufs.memory = V4L2_MEMORY_DMABUF;
> 	reqbufs.flags = 0;
> 	doioctl(node, VIDIOC_REQBUFS, &reqbufs);
> 
> because __vb2_queue_free() doesn't clear queue's ->dma_attrs
> once its don't freeing queue buffers, and by the time we call
> set_queue_consistency() the queue is DMABUF so (2) is false
> and we never clear the stale consistency attribute.
> 
> Re-implement set_queue_consistency() - it must always clear
> queue's non-consistency attr and set it only if (1) and (2).
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 7e081716b8da..37d0186ba330 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -704,12 +704,11 @@ EXPORT_SYMBOL(vb2_verify_memory_type);
>  
>  static void set_queue_consistency(struct vb2_queue *q, bool consistent_mem)
>  {
> +	q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT;
> +
>  	if (!vb2_queue_allows_cache_hints(q))
>  		return;
> -
> -	if (consistent_mem)
> -		q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT;
> -	else
> +	if (!consistent_mem)
>  		q->dma_attrs |= DMA_ATTR_NON_CONSISTENT;
>  }
>  
> 

Is it OK with you if I fold this patch into the original patch series and make a
new PR for it? I prefer to have a series merged without a bug, rather than fixing
it in a final patch.

Regards,

	Hans
Sergey Senozhatsky June 10, 2020, 10:10 a.m. UTC | #2 
Hi Hans,

On (20/06/10 11:54), Hans Verkuil wrote:
> 
> Is it OK with you if I fold this patch into the original patch series and make a
> new PR for it? I prefer to have a series merged without a bug, rather than fixing
> it in a final patch.

Absolutely!

	-ss
Tomasz Figa June 10, 2020, 10:17 a.m. UTC | #3 
On Wed, Jun 10, 2020 at 11:54 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Sergey,
>
> On 09/06/2020 08:04, Sergey Senozhatsky wrote:
> > We need a combination of two factors in order to permit modification
> > of queue's memory consistency attribute in set_queue_consistency():
> >   (1) queue should allow user-space cache hints
> >   (2) queue should be used for MMAP-ed I/O
> >
> > Therefore the code in videobuf2 core looks as follows:
> >
> >       q->memory = req->memory;
> >       set_queue_consistency(q, consistent);
> >
> > This works when we do something like this (suppose that queue allows
> > cache hints)
> >
> >       reqbufs.memory = V4L2_MEMORY_DMABUF;
> >       reqbufs.flags = 0;
> >       doioctl(node, VIDIOC_REQBUFS, &reqbufs);
> >
> >       reqbufs.memory = V4L2_MEMORY_MMAP;
> >       reqbufs.flags = V4L2_FLAG_MEMORY_NON_CONSISTENT;
> >       doioctl(node, VIDIOC_REQBUFS, &reqbufs);
> >
> > However, this doesn't work the other way around
> >
> >       reqbufs.memory = V4L2_MEMORY_MMAP;
> >       reqbufs.flags = V4L2_FLAG_MEMORY_NON_CONSISTENT;
> >       doioctl(node, VIDIOC_REQBUFS, &reqbufs);
> >
> >       reqbufs.memory = V4L2_MEMORY_DMABUF;
> >       reqbufs.flags = 0;
> >       doioctl(node, VIDIOC_REQBUFS, &reqbufs);
> >
> > because __vb2_queue_free() doesn't clear queue's ->dma_attrs
> > once its don't freeing queue buffers, and by the time we call
> > set_queue_consistency() the queue is DMABUF so (2) is false
> > and we never clear the stale consistency attribute.
> >
> > Re-implement set_queue_consistency() - it must always clear
> > queue's non-consistency attr and set it only if (1) and (2).
> >
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  drivers/media/common/videobuf2/videobuf2-core.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index 7e081716b8da..37d0186ba330 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -704,12 +704,11 @@ EXPORT_SYMBOL(vb2_verify_memory_type);
> >
> >  static void set_queue_consistency(struct vb2_queue *q, bool consistent_mem)
> >  {
> > +     q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT;
> > +
> >       if (!vb2_queue_allows_cache_hints(q))
> >               return;
> > -
> > -     if (consistent_mem)
> > -             q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT;
> > -     else
> > +     if (!consistent_mem)
> >               q->dma_attrs |= DMA_ATTR_NON_CONSISTENT;
> >  }
> >
> >
>
> Is it OK with you if I fold this patch into the original patch series and make a
> new PR for it? I prefer to have a series merged without a bug, rather than fixing
> it in a final patch.

I believe this didn't introduce any real bug, because dma_attrs would
end up used only for MMAP buffers anyway. Still, the current behavior
could end up being confusing for whoever has to deal with vb2 in the
future, so should be fixed.

Best regards,
Tomasz
Sergey Senozhatsky June 10, 2020, 10:48 a.m. UTC | #4 
On (20/06/10 12:17), Tomasz Figa wrote:
[..]
> > Is it OK with you if I fold this patch into the original patch series and make a
> > new PR for it? I prefer to have a series merged without a bug, rather than fixing
> > it in a final patch.
> 
> I believe this didn't introduce any real bug, because dma_attrs would
> end up used only for MMAP buffers anyway. Still, the current behavior
> could end up being confusing for whoever has to deal with vb2 in the
> future, so should be fixed.

This also requires user-space code that constantly use different ->memory
types and ->flags in inoctl()-s on the same queue.

	-ss
diff mbox series

Patch

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 7e081716b8da..37d0186ba330 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -704,12 +704,11 @@  EXPORT_SYMBOL(vb2_verify_memory_type);
 
 static void set_queue_consistency(struct vb2_queue *q, bool consistent_mem)
 {
+	q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT;
+
 	if (!vb2_queue_allows_cache_hints(q))
 		return;
-
-	if (consistent_mem)
-		q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT;
-	else
+	if (!consistent_mem)
 		q->dma_attrs |= DMA_ATTR_NON_CONSISTENT;
 }