[PATCH] media: vb2: Fix regression on poll() for RW mode

[PATCH] media: vb2: Fix regression on poll() for RW mode

[Ricardo Ribalda Delgado]

When using a device is read/write mode, vb2 does not handle properly the
first select/poll operation. It allways return POLLERR.

The reason for this is that when this code has been refactored, some of
the operations have changed their order, and now fileio emulator is not
started by poll, due to a previous check.

Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
Cc: Junghak Sung <jh1009.sung@samsung.com>
Cc: stable@vger.kernel.org
Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
 drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 5d016f496e0e..199c65dbe330 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
 		return POLLERR;
 
 	/*
+	 * For compatibility with vb1: if QBUF hasn't been called yet, then
+	 * return POLLERR as well. This only affects capture queues, output
+	 * queues will always initialize waiting_for_buffers to false.
+	 */
+	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
+		return POLLERR;
+
+	/*
 	 * For output streams you can call write() as long as there are fewer
 	 * buffers queued than there are buffers available.
 	 */
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index 91f552124050..c9bad9ef2104 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -818,14 +818,6 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 			poll_wait(file, &fh->wait, wait);
 	}
 
-	/*
-	 * For compatibility with vb1: if QBUF hasn't been called yet, then
-	 * return POLLERR as well. This only affects capture queues, output
-	 * queues will always initialize waiting_for_buffers to false.
-	 */
-	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
-		return POLLERR;
-
 	return res | vb2_core_poll(q, file, wait);
 }
 EXPORT_SYMBOL_GPL(vb2_poll);
-- 
2.8.0.rc3

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Hans Verkuil]

Hi Ricardo,

On 04/21/2016 11:15 AM, Ricardo Ribalda Delgado wrote:
> When using a device is read/write mode, vb2 does not handle properly the
> first select/poll operation. It allways return POLLERR.
> 
> The reason for this is that when this code has been refactored, some of
> the operations have changed their order, and now fileio emulator is not
> started by poll, due to a previous check.
> 
> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
> Cc: Junghak Sung <jh1009.sung@samsung.com>
> Cc: stable@vger.kernel.org
> Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 5d016f496e0e..199c65dbe330 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
>  		return POLLERR;
>  
>  	/*
> +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
> +	 * return POLLERR as well. This only affects capture queues, output
> +	 * queues will always initialize waiting_for_buffers to false.
> +	 */
> +	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
> +		return POLLERR;

The problem I have with this is that this should be specific to V4L2. The only
reason we do this is that we had to stay backwards compatible with vb1.

This is the reason this code was placed in videobuf2-v4l2.c. But you are correct
that this causes a regression, and I see no other choice but to put it in core.c.

That said, I would still only honor this when called from v4l2, so I suggest that
a new flag 'check_waiting_for_buffers' is added that is only set in vb2_queue_init
in videobuf2-v4l2.c.

So the test above becomes:

	if (q->check_waiting_for_buffers && q->waiting_for_buffers &&
	    (req_events & (POLLIN | POLLRDNORM)))

It's not ideal, but at least this keeps this v4l2 specific.

Regards,

	Hans

> +
> +	/*
>  	 * For output streams you can call write() as long as there are fewer
>  	 * buffers queued than there are buffers available.
>  	 */
> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index 91f552124050..c9bad9ef2104 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -818,14 +818,6 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>  			poll_wait(file, &fh->wait, wait);
>  	}
>  
> -	/*
> -	 * For compatibility with vb1: if QBUF hasn't been called yet, then
> -	 * return POLLERR as well. This only affects capture queues, output
> -	 * queues will always initialize waiting_for_buffers to false.
> -	 */
> -	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
> -		return POLLERR;
> -
>  	return res | vb2_core_poll(q, file, wait);
>  }
>  EXPORT_SYMBOL_GPL(vb2_poll);
> 

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Hans Verkuil]

On 04/22/2016 11:47 AM, Ricardo Ribalda Delgado wrote:
> Thanks for your review!
> 
> I will fix it that way. Do you mind to wait until Monday for a patch?

No problem.

	Hans

> 
> Regards
> 
> On 22 Apr 2016 11:19, "Hans Verkuil" <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
> 
>     Hi Ricardo,
> 
>     On 04/21/2016 11:15 AM, Ricardo Ribalda Delgado wrote:
>     > When using a device is read/write mode, vb2 does not handle properly the
>     > first select/poll operation. It allways return POLLERR.
>     >
>     > The reason for this is that when this code has been refactored, some of
>     > the operations have changed their order, and now fileio emulator is not
>     > started by poll, due to a previous check.
>     >
>     > Reported-by: Dimitrios Katsaros <patcherwork@gmail.com <mailto:patcherwork@gmail.com>>
>     > Cc: Junghak Sung <jh1009.sung@samsung.com <mailto:jh1009.sung@samsung.com>>
>     > Cc: stable@vger.kernel.org <mailto:stable@vger.kernel.org>
>     > Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
>     > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com <mailto:ricardo.ribalda@gmail.com>>
>     > ---
>     >  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
>     >  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
>     >  2 files changed, 8 insertions(+), 8 deletions(-)
>     >
>     > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>     > index 5d016f496e0e..199c65dbe330 100644
>     > --- a/drivers/media/v4l2-core/videobuf2-core.c
>     > +++ b/drivers/media/v4l2-core/videobuf2-core.c
>     > @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
>     >               return POLLERR;
>     >
>     >       /*
>     > +      * For compatibility with vb1: if QBUF hasn't been called yet, then
>     > +      * return POLLERR as well. This only affects capture queues, output
>     > +      * queues will always initialize waiting_for_buffers to false.
>     > +      */
>     > +     if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
>     > +             return POLLERR;
> 
>     The problem I have with this is that this should be specific to V4L2. The only
>     reason we do this is that we had to stay backwards compatible with vb1.
> 
>     This is the reason this code was placed in videobuf2-v4l2.c. But you are correct
>     that this causes a regression, and I see no other choice but to put it in core.c.
> 
>     That said, I would still only honor this when called from v4l2, so I suggest that
>     a new flag 'check_waiting_for_buffers' is added that is only set in vb2_queue_init
>     in videobuf2-v4l2.c.
> 
>     So the test above becomes:
> 
>             if (q->check_waiting_for_buffers && q->waiting_for_buffers &&
>                 (req_events & (POLLIN | POLLRDNORM)))
> 
>     It's not ideal, but at least this keeps this v4l2 specific.
> 
>     Regards,
> 
>             Hans
> 
>     > +
>     > +     /*
>     >        * For output streams you can call write() as long as there are fewer
>     >        * buffers queued than there are buffers available.
>     >        */
>     > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
>     > index 91f552124050..c9bad9ef2104 100644
>     > --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
>     > +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
>     > @@ -818,14 +818,6 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>     >                       poll_wait(file, &fh->wait, wait);
>     >       }
>     >
>     > -     /*
>     > -      * For compatibility with vb1: if QBUF hasn't been called yet, then
>     > -      * return POLLERR as well. This only affects capture queues, output
>     > -      * queues will always initialize waiting_for_buffers to false.
>     > -      */
>     > -     if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
>     > -             return POLLERR;
>     > -
>     >       return res | vb2_core_poll(q, file, wait);
>     >  }
>     >  EXPORT_SYMBOL_GPL(vb2_poll);
>     >
> 

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Mauro Carvalho Chehab]

Em Thu, 21 Apr 2016 11:15:16 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> escreveu:

> When using a device is read/write mode, vb2 does not handle properly the
> first select/poll operation. It allways return POLLERR.
> 
> The reason for this is that when this code has been refactored, some of
> the operations have changed their order, and now fileio emulator is not
> started by poll, due to a previous check.
> 
> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
> Cc: Junghak Sung <jh1009.sung@samsung.com>
> Cc: stable@vger.kernel.org
> Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 5d016f496e0e..199c65dbe330 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
>  		return POLLERR;
>  
>  	/*
> +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
> +	 * return POLLERR as well. This only affects capture queues, output
> +	 * queues will always initialize waiting_for_buffers to false.
> +	 */
> +	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
> +		return POLLERR;
> +

No, we shouldn't do VB1 backward compatibility at the core, as this
is a special case that only applies for V4L2. The hole idea of splitting
the core and the v4l2-specific code is to allow VB2 to be used on other
places, like on DVB.

So, we need some other approach that would keep this specific to V4L2.

> +	/*
>  	 * For output streams you can call write() as long as there are fewer
>  	 * buffers queued than there are buffers available.
>  	 */
> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index 91f552124050..c9bad9ef2104 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -818,14 +818,6 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>  			poll_wait(file, &fh->wait, wait);
>  	}
>  
> -	/*
> -	 * For compatibility with vb1: if QBUF hasn't been called yet, then
> -	 * return POLLERR as well. This only affects capture queues, output
> -	 * queues will always initialize waiting_for_buffers to false.
> -	 */
> -	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
> -		return POLLERR;
> -
>  	return res | vb2_core_poll(q, file, wait);
>  }
>  EXPORT_SYMBOL_GPL(vb2_poll);


-- 
Thanks,
Mauro

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Mauro Carvalho Chehab]

Em Fri, 22 Apr 2016 11:19:09 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> Hi Ricardo,
> 
> On 04/21/2016 11:15 AM, Ricardo Ribalda Delgado wrote:
> > When using a device is read/write mode, vb2 does not handle properly the
> > first select/poll operation. It allways return POLLERR.
> > 
> > The reason for this is that when this code has been refactored, some of
> > the operations have changed their order, and now fileio emulator is not
> > started by poll, due to a previous check.
> > 
> > Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
> > Cc: Junghak Sung <jh1009.sung@samsung.com>
> > Cc: stable@vger.kernel.org
> > Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
> >  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index 5d016f496e0e..199c65dbe330 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
> >  		return POLLERR;
> >  
> >  	/*
> > +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
> > +	 * return POLLERR as well. This only affects capture queues, output
> > +	 * queues will always initialize waiting_for_buffers to false.
> > +	 */
> > +	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
> > +		return POLLERR;  
> 
> The problem I have with this is that this should be specific to V4L2. The only
> reason we do this is that we had to stay backwards compatible with vb1.
> 
> This is the reason this code was placed in videobuf2-v4l2.c. But you are correct
> that this causes a regression, and I see no other choice but to put it in core.c.
> 
> That said, I would still only honor this when called from v4l2, so I suggest that
> a new flag 'check_waiting_for_buffers' is added that is only set in vb2_queue_init
> in videobuf2-v4l2.c.
> 
> So the test above becomes:
> 
> 	if (q->check_waiting_for_buffers && q->waiting_for_buffers &&
> 	    (req_events & (POLLIN | POLLRDNORM)))
> 
> It's not ideal, but at least this keeps this v4l2 specific.

I don't like the above approach, for two reasons:

1) it is not obvious that this is V4L2 specific from the code;

2) we should not mess the core due to some V4L2 mess.


> 
> Regards,
> 
> 	Hans
> 
> > +
> > +	/*
> >  	 * For output streams you can call write() as long as there are fewer
> >  	 * buffers queued than there are buffers available.
> >  	 */
> > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > index 91f552124050..c9bad9ef2104 100644
> > --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> > +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > @@ -818,14 +818,6 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
> >  			poll_wait(file, &fh->wait, wait);
> >  	}
> >  
> > -	/*
> > -	 * For compatibility with vb1: if QBUF hasn't been called yet, then
> > -	 * return POLLERR as well. This only affects capture queues, output
> > -	 * queues will always initialize waiting_for_buffers to false.
> > -	 */
> > -	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
> > -		return POLLERR;
> > -
> >  	return res | vb2_core_poll(q, file, wait);
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_poll);
> >   
> 


-- 
Thanks,
Mauro

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Hans Verkuil]

On 04/22/2016 02:31 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 22 Apr 2016 11:19:09 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> Hi Ricardo,
>>
>> On 04/21/2016 11:15 AM, Ricardo Ribalda Delgado wrote:
>>> When using a device is read/write mode, vb2 does not handle properly the
>>> first select/poll operation. It allways return POLLERR.
>>>
>>> The reason for this is that when this code has been refactored, some of
>>> the operations have changed their order, and now fileio emulator is not
>>> started by poll, due to a previous check.
>>>
>>> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
>>> Cc: Junghak Sung <jh1009.sung@samsung.com>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>>> ---
>>>  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
>>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
>>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>>> index 5d016f496e0e..199c65dbe330 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
>>>  		return POLLERR;
>>>  
>>>  	/*
>>> +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
>>> +	 * return POLLERR as well. This only affects capture queues, output
>>> +	 * queues will always initialize waiting_for_buffers to false.
>>> +	 */
>>> +	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
>>> +		return POLLERR;  
>>
>> The problem I have with this is that this should be specific to V4L2. The only
>> reason we do this is that we had to stay backwards compatible with vb1.
>>
>> This is the reason this code was placed in videobuf2-v4l2.c. But you are correct
>> that this causes a regression, and I see no other choice but to put it in core.c.
>>
>> That said, I would still only honor this when called from v4l2, so I suggest that
>> a new flag 'check_waiting_for_buffers' is added that is only set in vb2_queue_init
>> in videobuf2-v4l2.c.
>>
>> So the test above becomes:
>>
>> 	if (q->check_waiting_for_buffers && q->waiting_for_buffers &&
>> 	    (req_events & (POLLIN | POLLRDNORM)))
>>
>> It's not ideal, but at least this keeps this v4l2 specific.
> 
> I don't like the above approach, for two reasons:
> 
> 1) it is not obvious that this is V4L2 specific from the code;

s/check_waiting_for_buffers/v4l2_needs_to_wait_for_buffers/

> 
> 2) we should not mess the core due to some V4L2 mess.

Well, the only other alternative I see is to split vb2_core_poll() into two
since the check has to happen in the middle. The v4l2 code would call core_poll1(),
then do the check and afterwards call core_poll2(). And that would really be ugly.
I would probably NACK that.

Better ideas are welcome.

Regards,

	Hans

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Mauro Carvalho Chehab]

Em Fri, 22 Apr 2016 14:37:07 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 04/22/2016 02:31 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 22 Apr 2016 11:19:09 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> Hi Ricardo,
> >>
> >> On 04/21/2016 11:15 AM, Ricardo Ribalda Delgado wrote:  
> >>> When using a device is read/write mode, vb2 does not handle properly the
> >>> first select/poll operation. It allways return POLLERR.
> >>>
> >>> The reason for this is that when this code has been refactored, some of
> >>> the operations have changed their order, and now fileio emulator is not
> >>> started by poll, due to a previous check.
> >>>
> >>> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
> >>> Cc: Junghak Sung <jh1009.sung@samsung.com>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
> >>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> >>> ---
> >>>  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
> >>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
> >>>  2 files changed, 8 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> >>> index 5d016f496e0e..199c65dbe330 100644
> >>> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >>> @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
> >>>  		return POLLERR;
> >>>  
> >>>  	/*
> >>> +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
> >>> +	 * return POLLERR as well. This only affects capture queues, output
> >>> +	 * queues will always initialize waiting_for_buffers to false.
> >>> +	 */
> >>> +	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
> >>> +		return POLLERR;    
> >>
> >> The problem I have with this is that this should be specific to V4L2. The only
> >> reason we do this is that we had to stay backwards compatible with vb1.
> >>
> >> This is the reason this code was placed in videobuf2-v4l2.c. But you are correct
> >> that this causes a regression, and I see no other choice but to put it in core.c.
> >>
> >> That said, I would still only honor this when called from v4l2, so I suggest that
> >> a new flag 'check_waiting_for_buffers' is added that is only set in vb2_queue_init
> >> in videobuf2-v4l2.c.
> >>
> >> So the test above becomes:
> >>
> >> 	if (q->check_waiting_for_buffers && q->waiting_for_buffers &&
> >> 	    (req_events & (POLLIN | POLLRDNORM)))
> >>
> >> It's not ideal, but at least this keeps this v4l2 specific.  
> > 
> > I don't like the above approach, for two reasons:
> > 
> > 1) it is not obvious that this is V4L2 specific from the code;  
> 
> s/check_waiting_for_buffers/v4l2_needs_to_wait_for_buffers/

Better, but still hell of a hack. Maybe we could add a quirks
flag and add a flag like:
	VB2_FLAG_ENABLE_POLLERR_IF_WAITING_BUFFERS_AND_NO_QBUF
(or some better naming, I'm not inspired today...)

Of course, such quirk should be properly documented.

> > 
> > 2) we should not mess the core due to some V4L2 mess.  
> 
> Well, the only other alternative I see is to split vb2_core_poll() into two
> since the check has to happen in the middle. The v4l2 code would call core_poll1(),
> then do the check and afterwards call core_poll2(). And that would really be ugly.

Actually, the first callback would be better called as
vb2_core_poll_check() - or something with similar name.

IMHO, this is the cleaner solution, although it adds an extra cost.


> I would probably NACK that.
> 
> Better ideas are welcome.
> 
> Regards,
> 
> 	Hans


-- 
Thanks,
Mauro

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Hans Verkuil]

On 04/22/2016 04:21 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 22 Apr 2016 14:37:07 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 04/22/2016 02:31 PM, Mauro Carvalho Chehab wrote:
>>> Em Fri, 22 Apr 2016 11:19:09 +0200
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>   
>>>> Hi Ricardo,
>>>>
>>>> On 04/21/2016 11:15 AM, Ricardo Ribalda Delgado wrote:  
>>>>> When using a device is read/write mode, vb2 does not handle properly the
>>>>> first select/poll operation. It allways return POLLERR.
>>>>>
>>>>> The reason for this is that when this code has been refactored, some of
>>>>> the operations have changed their order, and now fileio emulator is not
>>>>> started by poll, due to a previous check.
>>>>>
>>>>> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
>>>>> Cc: Junghak Sung <jh1009.sung@samsung.com>
>>>>> Cc: stable@vger.kernel.org
>>>>> Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
>>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>>>>> ---
>>>>>  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
>>>>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
>>>>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>>>>> index 5d016f496e0e..199c65dbe330 100644
>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>>>> @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
>>>>>  		return POLLERR;
>>>>>  
>>>>>  	/*
>>>>> +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
>>>>> +	 * return POLLERR as well. This only affects capture queues, output
>>>>> +	 * queues will always initialize waiting_for_buffers to false.
>>>>> +	 */
>>>>> +	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
>>>>> +		return POLLERR;    
>>>>
>>>> The problem I have with this is that this should be specific to V4L2. The only
>>>> reason we do this is that we had to stay backwards compatible with vb1.
>>>>
>>>> This is the reason this code was placed in videobuf2-v4l2.c. But you are correct
>>>> that this causes a regression, and I see no other choice but to put it in core.c.
>>>>
>>>> That said, I would still only honor this when called from v4l2, so I suggest that
>>>> a new flag 'check_waiting_for_buffers' is added that is only set in vb2_queue_init
>>>> in videobuf2-v4l2.c.
>>>>
>>>> So the test above becomes:
>>>>
>>>> 	if (q->check_waiting_for_buffers && q->waiting_for_buffers &&
>>>> 	    (req_events & (POLLIN | POLLRDNORM)))
>>>>
>>>> It's not ideal, but at least this keeps this v4l2 specific.  
>>>
>>> I don't like the above approach, for two reasons:
>>>
>>> 1) it is not obvious that this is V4L2 specific from the code;  
>>
>> s/check_waiting_for_buffers/v4l2_needs_to_wait_for_buffers/
> 
> Better, but still hell of a hack. Maybe we could add a quirks
> flag and add a flag like:
> 	VB2_FLAG_ENABLE_POLLERR_IF_WAITING_BUFFERS_AND_NO_QBUF
> (or some better naming, I'm not inspired today...)
> 
> Of course, such quirk should be properly documented.

How about 'quirk_poll_must_check_waiting_for_buffers'? Something with 'quirk' in the
name is a good idea.

> 
>>>
>>> 2) we should not mess the core due to some V4L2 mess.  
>>
>> Well, the only other alternative I see is to split vb2_core_poll() into two
>> since the check has to happen in the middle. The v4l2 code would call core_poll1(),
>> then do the check and afterwards call core_poll2(). And that would really be ugly.
> 
> Actually, the first callback would be better called as
> vb2_core_poll_check() - or something with similar name.
> 
> IMHO, this is the cleaner solution, although it adds an extra cost.

I really don't like this. This has a much larger impact on vb2 core then adding
a simple quirk flag.

Regards,

	Hans

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Mauro Carvalho Chehab]

Em Fri, 22 Apr 2016 16:31:28 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 04/22/2016 04:21 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 22 Apr 2016 14:37:07 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> On 04/22/2016 02:31 PM, Mauro Carvalho Chehab wrote:  
> >>> Em Fri, 22 Apr 2016 11:19:09 +0200
> >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>     
> >>>> Hi Ricardo,
> >>>>
> >>>> On 04/21/2016 11:15 AM, Ricardo Ribalda Delgado wrote:    
> >>>>> When using a device is read/write mode, vb2 does not handle properly the
> >>>>> first select/poll operation. It allways return POLLERR.
> >>>>>
> >>>>> The reason for this is that when this code has been refactored, some of
> >>>>> the operations have changed their order, and now fileio emulator is not
> >>>>> started by poll, due to a previous check.
> >>>>>
> >>>>> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
> >>>>> Cc: Junghak Sung <jh1009.sung@samsung.com>
> >>>>> Cc: stable@vger.kernel.org
> >>>>> Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
> >>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> >>>>> ---
> >>>>>  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
> >>>>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
> >>>>>  2 files changed, 8 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> >>>>> index 5d016f496e0e..199c65dbe330 100644
> >>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >>>>> @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
> >>>>>  		return POLLERR;
> >>>>>  
> >>>>>  	/*
> >>>>> +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
> >>>>> +	 * return POLLERR as well. This only affects capture queues, output
> >>>>> +	 * queues will always initialize waiting_for_buffers to false.
> >>>>> +	 */
> >>>>> +	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
> >>>>> +		return POLLERR;      
> >>>>
> >>>> The problem I have with this is that this should be specific to V4L2. The only
> >>>> reason we do this is that we had to stay backwards compatible with vb1.
> >>>>
> >>>> This is the reason this code was placed in videobuf2-v4l2.c. But you are correct
> >>>> that this causes a regression, and I see no other choice but to put it in core.c.
> >>>>
> >>>> That said, I would still only honor this when called from v4l2, so I suggest that
> >>>> a new flag 'check_waiting_for_buffers' is added that is only set in vb2_queue_init
> >>>> in videobuf2-v4l2.c.
> >>>>
> >>>> So the test above becomes:
> >>>>
> >>>> 	if (q->check_waiting_for_buffers && q->waiting_for_buffers &&
> >>>> 	    (req_events & (POLLIN | POLLRDNORM)))
> >>>>
> >>>> It's not ideal, but at least this keeps this v4l2 specific.    
> >>>
> >>> I don't like the above approach, for two reasons:
> >>>
> >>> 1) it is not obvious that this is V4L2 specific from the code;    
> >>
> >> s/check_waiting_for_buffers/v4l2_needs_to_wait_for_buffers/  
> > 
> > Better, but still hell of a hack. Maybe we could add a quirks
> > flag and add a flag like:
> > 	VB2_FLAG_ENABLE_POLLERR_IF_WAITING_BUFFERS_AND_NO_QBUF
> > (or some better naming, I'm not inspired today...)
> > 
> > Of course, such quirk should be properly documented.  
> 
> How about 'quirk_poll_must_check_waiting_for_buffers'? Something with 'quirk' in the
> name is a good idea.

works for me, provided that we add the field as a flag. So it would be like:

#define QUIRK_POLL_MUST_CHECK_WAITING_FOR_BUFFERS 0

 	if (test_bit(q->quirk, QUIRK_POLL_MUST_CHECK_WAITING_FOR_BUFFERS) &&
	    q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))

> 
> >   
> >>>
> >>> 2) we should not mess the core due to some V4L2 mess.    
> >>
> >> Well, the only other alternative I see is to split vb2_core_poll() into two
> >> since the check has to happen in the middle. The v4l2 code would call core_poll1(),
> >> then do the check and afterwards call core_poll2(). And that would really be ugly.  
> > 
> > Actually, the first callback would be better called as
> > vb2_core_poll_check() - or something with similar name.
> > 
> > IMHO, this is the cleaner solution, although it adds an extra cost.  
> 
> I really don't like this. This has a much larger impact on vb2 core then adding
> a simple quirk flag.
> 
> Regards,
> 
> 	Hans


-- 
Thanks,
Mauro

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Hans Verkuil]

On 04/22/2016 04:48 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 22 Apr 2016 16:31:28 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 04/22/2016 04:21 PM, Mauro Carvalho Chehab wrote:
>>> Em Fri, 22 Apr 2016 14:37:07 +0200
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>   
>>>> On 04/22/2016 02:31 PM, Mauro Carvalho Chehab wrote:  
>>>>> Em Fri, 22 Apr 2016 11:19:09 +0200
>>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>>     
>>>>>> Hi Ricardo,
>>>>>>
>>>>>> On 04/21/2016 11:15 AM, Ricardo Ribalda Delgado wrote:    
>>>>>>> When using a device is read/write mode, vb2 does not handle properly the
>>>>>>> first select/poll operation. It allways return POLLERR.
>>>>>>>
>>>>>>> The reason for this is that when this code has been refactored, some of
>>>>>>> the operations have changed their order, and now fileio emulator is not
>>>>>>> started by poll, due to a previous check.
>>>>>>>
>>>>>>> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
>>>>>>> Cc: Junghak Sung <jh1009.sung@samsung.com>
>>>>>>> Cc: stable@vger.kernel.org
>>>>>>> Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
>>>>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
>>>>>>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
>>>>>>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>>>>>>> index 5d016f496e0e..199c65dbe330 100644
>>>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>>>>>> @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
>>>>>>>  		return POLLERR;
>>>>>>>  
>>>>>>>  	/*
>>>>>>> +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
>>>>>>> +	 * return POLLERR as well. This only affects capture queues, output
>>>>>>> +	 * queues will always initialize waiting_for_buffers to false.
>>>>>>> +	 */
>>>>>>> +	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
>>>>>>> +		return POLLERR;      
>>>>>>
>>>>>> The problem I have with this is that this should be specific to V4L2. The only
>>>>>> reason we do this is that we had to stay backwards compatible with vb1.
>>>>>>
>>>>>> This is the reason this code was placed in videobuf2-v4l2.c. But you are correct
>>>>>> that this causes a regression, and I see no other choice but to put it in core.c.
>>>>>>
>>>>>> That said, I would still only honor this when called from v4l2, so I suggest that
>>>>>> a new flag 'check_waiting_for_buffers' is added that is only set in vb2_queue_init
>>>>>> in videobuf2-v4l2.c.
>>>>>>
>>>>>> So the test above becomes:
>>>>>>
>>>>>> 	if (q->check_waiting_for_buffers && q->waiting_for_buffers &&
>>>>>> 	    (req_events & (POLLIN | POLLRDNORM)))
>>>>>>
>>>>>> It's not ideal, but at least this keeps this v4l2 specific.    
>>>>>
>>>>> I don't like the above approach, for two reasons:
>>>>>
>>>>> 1) it is not obvious that this is V4L2 specific from the code;    
>>>>
>>>> s/check_waiting_for_buffers/v4l2_needs_to_wait_for_buffers/  
>>>
>>> Better, but still hell of a hack. Maybe we could add a quirks
>>> flag and add a flag like:
>>> 	VB2_FLAG_ENABLE_POLLERR_IF_WAITING_BUFFERS_AND_NO_QBUF
>>> (or some better naming, I'm not inspired today...)
>>>
>>> Of course, such quirk should be properly documented.  
>>
>> How about 'quirk_poll_must_check_waiting_for_buffers'? Something with 'quirk' in the
>> name is a good idea.
> 
> works for me, provided that we add the field as a flag. So it would be like:
> 
> #define QUIRK_POLL_MUST_CHECK_WAITING_FOR_BUFFERS 0
> 
>  	if (test_bit(q->quirk, QUIRK_POLL_MUST_CHECK_WAITING_FOR_BUFFERS) &&
> 	    q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))

Why should it be a flag? What is wrong with a bitfield?

Just curious what the reasoning is for that. I don't see any obvious
advantage of a flag over a bitfield.

Regards,

	Hans

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Mauro Carvalho Chehab]

Em Fri, 22 Apr 2016 16:56:00 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 04/22/2016 04:48 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 22 Apr 2016 16:31:28 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> On 04/22/2016 04:21 PM, Mauro Carvalho Chehab wrote:  
> >>> Em Fri, 22 Apr 2016 14:37:07 +0200
> >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>     
> >>>> On 04/22/2016 02:31 PM, Mauro Carvalho Chehab wrote:    
> >>>>> Em Fri, 22 Apr 2016 11:19:09 +0200
> >>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>>>       
> >>>>>> Hi Ricardo,
> >>>>>>
> >>>>>> On 04/21/2016 11:15 AM, Ricardo Ribalda Delgado wrote:      
> >>>>>>> When using a device is read/write mode, vb2 does not handle properly the
> >>>>>>> first select/poll operation. It allways return POLLERR.
> >>>>>>>
> >>>>>>> The reason for this is that when this code has been refactored, some of
> >>>>>>> the operations have changed their order, and now fileio emulator is not
> >>>>>>> started by poll, due to a previous check.
> >>>>>>>
> >>>>>>> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
> >>>>>>> Cc: Junghak Sung <jh1009.sung@samsung.com>
> >>>>>>> Cc: stable@vger.kernel.org
> >>>>>>> Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
> >>>>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> >>>>>>> ---
> >>>>>>>  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
> >>>>>>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
> >>>>>>>  2 files changed, 8 insertions(+), 8 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> >>>>>>> index 5d016f496e0e..199c65dbe330 100644
> >>>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >>>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >>>>>>> @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
> >>>>>>>  		return POLLERR;
> >>>>>>>  
> >>>>>>>  	/*
> >>>>>>> +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
> >>>>>>> +	 * return POLLERR as well. This only affects capture queues, output
> >>>>>>> +	 * queues will always initialize waiting_for_buffers to false.
> >>>>>>> +	 */
> >>>>>>> +	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
> >>>>>>> +		return POLLERR;        
> >>>>>>
> >>>>>> The problem I have with this is that this should be specific to V4L2. The only
> >>>>>> reason we do this is that we had to stay backwards compatible with vb1.
> >>>>>>
> >>>>>> This is the reason this code was placed in videobuf2-v4l2.c. But you are correct
> >>>>>> that this causes a regression, and I see no other choice but to put it in core.c.
> >>>>>>
> >>>>>> That said, I would still only honor this when called from v4l2, so I suggest that
> >>>>>> a new flag 'check_waiting_for_buffers' is added that is only set in vb2_queue_init
> >>>>>> in videobuf2-v4l2.c.
> >>>>>>
> >>>>>> So the test above becomes:
> >>>>>>
> >>>>>> 	if (q->check_waiting_for_buffers && q->waiting_for_buffers &&
> >>>>>> 	    (req_events & (POLLIN | POLLRDNORM)))
> >>>>>>
> >>>>>> It's not ideal, but at least this keeps this v4l2 specific.      
> >>>>>
> >>>>> I don't like the above approach, for two reasons:
> >>>>>
> >>>>> 1) it is not obvious that this is V4L2 specific from the code;      
> >>>>
> >>>> s/check_waiting_for_buffers/v4l2_needs_to_wait_for_buffers/    
> >>>
> >>> Better, but still hell of a hack. Maybe we could add a quirks
> >>> flag and add a flag like:
> >>> 	VB2_FLAG_ENABLE_POLLERR_IF_WAITING_BUFFERS_AND_NO_QBUF
> >>> (or some better naming, I'm not inspired today...)
> >>>
> >>> Of course, such quirk should be properly documented.    
> >>
> >> How about 'quirk_poll_must_check_waiting_for_buffers'? Something with 'quirk' in the
> >> name is a good idea.  
> > 
> > works for me, provided that we add the field as a flag. So it would be like:
> > 
> > #define QUIRK_POLL_MUST_CHECK_WAITING_FOR_BUFFERS 0
> > 
> >  	if (test_bit(q->quirk, QUIRK_POLL_MUST_CHECK_WAITING_FOR_BUFFERS) &&
> > 	    q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))  
> 
> Why should it be a flag? What is wrong with a bitfield?
> 
> Just curious what the reasoning is for that. I don't see any obvious
> advantage of a flag over a bitfield.

Huh? Flags are implemented as bitfields. See the above code: it is
using test_bit() for the new q->quirk flags/bitfield.

Regards,
Mauro

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Hans Verkuil]

On 04/22/2016 05:21 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 22 Apr 2016 16:56:00 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 04/22/2016 04:48 PM, Mauro Carvalho Chehab wrote:
>>> Em Fri, 22 Apr 2016 16:31:28 +0200
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>   
>>>> On 04/22/2016 04:21 PM, Mauro Carvalho Chehab wrote:  
>>>>> Em Fri, 22 Apr 2016 14:37:07 +0200
>>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>>     
>>>>>> On 04/22/2016 02:31 PM, Mauro Carvalho Chehab wrote:    
>>>>>>> Em Fri, 22 Apr 2016 11:19:09 +0200
>>>>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>>>>       
>>>>>>>> Hi Ricardo,
>>>>>>>>
>>>>>>>> On 04/21/2016 11:15 AM, Ricardo Ribalda Delgado wrote:      
>>>>>>>>> When using a device is read/write mode, vb2 does not handle properly the
>>>>>>>>> first select/poll operation. It allways return POLLERR.
>>>>>>>>>
>>>>>>>>> The reason for this is that when this code has been refactored, some of
>>>>>>>>> the operations have changed their order, and now fileio emulator is not
>>>>>>>>> started by poll, due to a previous check.
>>>>>>>>>
>>>>>>>>> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
>>>>>>>>> Cc: Junghak Sung <jh1009.sung@samsung.com>
>>>>>>>>> Cc: stable@vger.kernel.org
>>>>>>>>> Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
>>>>>>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
>>>>>>>>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
>>>>>>>>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>>>>>>>>> index 5d016f496e0e..199c65dbe330 100644
>>>>>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>>>>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>>>>>>>> @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
>>>>>>>>>  		return POLLERR;
>>>>>>>>>  
>>>>>>>>>  	/*
>>>>>>>>> +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
>>>>>>>>> +	 * return POLLERR as well. This only affects capture queues, output
>>>>>>>>> +	 * queues will always initialize waiting_for_buffers to false.
>>>>>>>>> +	 */
>>>>>>>>> +	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
>>>>>>>>> +		return POLLERR;        
>>>>>>>>
>>>>>>>> The problem I have with this is that this should be specific to V4L2. The only
>>>>>>>> reason we do this is that we had to stay backwards compatible with vb1.
>>>>>>>>
>>>>>>>> This is the reason this code was placed in videobuf2-v4l2.c. But you are correct
>>>>>>>> that this causes a regression, and I see no other choice but to put it in core.c.
>>>>>>>>
>>>>>>>> That said, I would still only honor this when called from v4l2, so I suggest that
>>>>>>>> a new flag 'check_waiting_for_buffers' is added that is only set in vb2_queue_init
>>>>>>>> in videobuf2-v4l2.c.
>>>>>>>>
>>>>>>>> So the test above becomes:
>>>>>>>>
>>>>>>>> 	if (q->check_waiting_for_buffers && q->waiting_for_buffers &&
>>>>>>>> 	    (req_events & (POLLIN | POLLRDNORM)))
>>>>>>>>
>>>>>>>> It's not ideal, but at least this keeps this v4l2 specific.      
>>>>>>>
>>>>>>> I don't like the above approach, for two reasons:
>>>>>>>
>>>>>>> 1) it is not obvious that this is V4L2 specific from the code;      
>>>>>>
>>>>>> s/check_waiting_for_buffers/v4l2_needs_to_wait_for_buffers/    
>>>>>
>>>>> Better, but still hell of a hack. Maybe we could add a quirks
>>>>> flag and add a flag like:
>>>>> 	VB2_FLAG_ENABLE_POLLERR_IF_WAITING_BUFFERS_AND_NO_QBUF
>>>>> (or some better naming, I'm not inspired today...)
>>>>>
>>>>> Of course, such quirk should be properly documented.    
>>>>
>>>> How about 'quirk_poll_must_check_waiting_for_buffers'? Something with 'quirk' in the
>>>> name is a good idea.  
>>>
>>> works for me, provided that we add the field as a flag. So it would be like:
>>>
>>> #define QUIRK_POLL_MUST_CHECK_WAITING_FOR_BUFFERS 0
>>>
>>>  	if (test_bit(q->quirk, QUIRK_POLL_MUST_CHECK_WAITING_FOR_BUFFERS) &&
>>> 	    q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))  
>>
>> Why should it be a flag? What is wrong with a bitfield?
>>
>> Just curious what the reasoning is for that. I don't see any obvious
>> advantage of a flag over a bitfield.
> 
> Huh? Flags are implemented as bitfields. See the above code: it is
> using test_bit() for the new q->quirk flags/bitfield.

I mean C bitfields like this:

        unsigned                        fileio_read_once:1;
        unsigned                        fileio_write_immediately:1;
        unsigned                        allow_zero_bytesused:1;

This is already used in struct vb2_queue, so my proposal would be to add:

	unsigned			quirk_poll_must_check_waiting_for_buffers:1;

Regards,

	Hans

Re: [PATCH] media: vb2: Fix regression on poll() for RW mode

[Mauro Carvalho Chehab]

Em Fri, 22 Apr 2016 18:45:41 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 04/22/2016 05:21 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 22 Apr 2016 16:56:00 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> On 04/22/2016 04:48 PM, Mauro Carvalho Chehab wrote:  
> >>> Em Fri, 22 Apr 2016 16:31:28 +0200
> >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>     
> >>>> On 04/22/2016 04:21 PM, Mauro Carvalho Chehab wrote:    
> >>>>> Em Fri, 22 Apr 2016 14:37:07 +0200
> >>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>>>       
> >>>>>> On 04/22/2016 02:31 PM, Mauro Carvalho Chehab wrote:      
> >>>>>>> Em Fri, 22 Apr 2016 11:19:09 +0200
> >>>>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>>>>>         
> >>>>>>>> Hi Ricardo,
> >>>>>>>>
> >>>>>>>> On 04/21/2016 11:15 AM, Ricardo Ribalda Delgado wrote:        
> >>>>>>>>> When using a device is read/write mode, vb2 does not handle properly the
> >>>>>>>>> first select/poll operation. It allways return POLLERR.
> >>>>>>>>>
> >>>>>>>>> The reason for this is that when this code has been refactored, some of
> >>>>>>>>> the operations have changed their order, and now fileio emulator is not
> >>>>>>>>> started by poll, due to a previous check.
> >>>>>>>>>
> >>>>>>>>> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
> >>>>>>>>> Cc: Junghak Sung <jh1009.sung@samsung.com>
> >>>>>>>>> Cc: stable@vger.kernel.org
> >>>>>>>>> Fixes: 49d8ab9feaf2 ("media] media: videobuf2: Separate vb2_poll()")
> >>>>>>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> >>>>>>>>> ---
> >>>>>>>>>  drivers/media/v4l2-core/videobuf2-core.c | 8 ++++++++
> >>>>>>>>>  drivers/media/v4l2-core/videobuf2-v4l2.c | 8 --------
> >>>>>>>>>  2 files changed, 8 insertions(+), 8 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> >>>>>>>>> index 5d016f496e0e..199c65dbe330 100644
> >>>>>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >>>>>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >>>>>>>>> @@ -2298,6 +2298,14 @@ unsigned int vb2_core_poll(struct vb2_queue *q, struct file *file,
> >>>>>>>>>  		return POLLERR;
> >>>>>>>>>  
> >>>>>>>>>  	/*
> >>>>>>>>> +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
> >>>>>>>>> +	 * return POLLERR as well. This only affects capture queues, output
> >>>>>>>>> +	 * queues will always initialize waiting_for_buffers to false.
> >>>>>>>>> +	 */
> >>>>>>>>> +	if (q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))
> >>>>>>>>> +		return POLLERR;          
> >>>>>>>>
> >>>>>>>> The problem I have with this is that this should be specific to V4L2. The only
> >>>>>>>> reason we do this is that we had to stay backwards compatible with vb1.
> >>>>>>>>
> >>>>>>>> This is the reason this code was placed in videobuf2-v4l2.c. But you are correct
> >>>>>>>> that this causes a regression, and I see no other choice but to put it in core.c.
> >>>>>>>>
> >>>>>>>> That said, I would still only honor this when called from v4l2, so I suggest that
> >>>>>>>> a new flag 'check_waiting_for_buffers' is added that is only set in vb2_queue_init
> >>>>>>>> in videobuf2-v4l2.c.
> >>>>>>>>
> >>>>>>>> So the test above becomes:
> >>>>>>>>
> >>>>>>>> 	if (q->check_waiting_for_buffers && q->waiting_for_buffers &&
> >>>>>>>> 	    (req_events & (POLLIN | POLLRDNORM)))
> >>>>>>>>
> >>>>>>>> It's not ideal, but at least this keeps this v4l2 specific.        
> >>>>>>>
> >>>>>>> I don't like the above approach, for two reasons:
> >>>>>>>
> >>>>>>> 1) it is not obvious that this is V4L2 specific from the code;        
> >>>>>>
> >>>>>> s/check_waiting_for_buffers/v4l2_needs_to_wait_for_buffers/      
> >>>>>
> >>>>> Better, but still hell of a hack. Maybe we could add a quirks
> >>>>> flag and add a flag like:
> >>>>> 	VB2_FLAG_ENABLE_POLLERR_IF_WAITING_BUFFERS_AND_NO_QBUF
> >>>>> (or some better naming, I'm not inspired today...)
> >>>>>
> >>>>> Of course, such quirk should be properly documented.      
> >>>>
> >>>> How about 'quirk_poll_must_check_waiting_for_buffers'? Something with 'quirk' in the
> >>>> name is a good idea.    
> >>>
> >>> works for me, provided that we add the field as a flag. So it would be like:
> >>>
> >>> #define QUIRK_POLL_MUST_CHECK_WAITING_FOR_BUFFERS 0
> >>>
> >>>  	if (test_bit(q->quirk, QUIRK_POLL_MUST_CHECK_WAITING_FOR_BUFFERS) &&
> >>> 	    q->waiting_for_buffers && (req_events & (POLLIN | POLLRDNORM)))    
> >>
> >> Why should it be a flag? What is wrong with a bitfield?
> >>
> >> Just curious what the reasoning is for that. I don't see any obvious
> >> advantage of a flag over a bitfield.  
> > 
> > Huh? Flags are implemented as bitfields. See the above code: it is
> > using test_bit() for the new q->quirk flags/bitfield.  
> 
> I mean C bitfields like this:
> 
>         unsigned                        fileio_read_once:1;
>         unsigned                        fileio_write_immediately:1;
>         unsigned                        allow_zero_bytesused:1;
> 
> This is already used in struct vb2_queue, so my proposal would be to add:
> 
> 	unsigned			quirk_poll_must_check_waiting_for_buffers:1;

Works for me.

Regards,
Mauro.