diff mbox series

[v4,03/24] media: videobuf2-v4l2: Warn on holding buffers without support

Message ID 20220426125751.108293-4-nicolas.dufresne@collabora.com (mailing list archive)
State Changes Requested
Delegated to: Hans Verkuil
Headers
Series [v4,01/24] media: doc: Document dual use of H.264 pic_num/frame_num |

Commit Message

Nicolas Dufresne April 26, 2022, 12:57 p.m. UTC 
  From: Sebastian Fricke <sebastian.fricke@collabora.com>

Using V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag without specifying the
subsystem flag VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, results in
silently ignoring it.
Warn the user via a debug print when the flag is requested but ignored
by the videobuf2 framework.

Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Tomasz Figa April 27, 2022, 4:31 a.m. UTC | #1 
Hi Nicolas, Sebastian,

On Tue, Apr 26, 2022 at 9:58 PM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> From: Sebastian Fricke <sebastian.fricke@collabora.com>
>
> Using V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag without specifying the
> subsystem flag VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, results in
> silently ignoring it.
> Warn the user via a debug print when the flag is requested but ignored
> by the videobuf2 framework.
>
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>

Thanks for the patch. Please see my comments inline.

> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 6edf4508c636..812c8d1962e0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -329,8 +329,13 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
>                  */
>                 vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
>                 vbuf->field = b->field;
> -               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
> +               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)) {
> +                       if (vbuf->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
> +                               dprintk(q, 1,
> +                                       "Request holding buffer (%d), unsupported on output queue\n",
> +                                       b->index);

I wonder if we shouldn't just fail such a QBUF operation. Otherwise
the application would get unexpected behavior from the kernel.
Although it might be too late to do it now if there are applications
that rely on this implicit ignore...

Best regards,
Tomasz
Nicolas Dufresne April 27, 2022, 3:08 p.m. UTC | #2 
Le mercredi 27 avril 2022 à 13:31 +0900, Tomasz Figa a écrit :
> Hi Nicolas, Sebastian,
> 
> On Tue, Apr 26, 2022 at 9:58 PM Nicolas Dufresne
> <nicolas.dufresne@collabora.com> wrote:
> > 
> > From: Sebastian Fricke <sebastian.fricke@collabora.com>
> > 
> > Using V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag without specifying the
> > subsystem flag VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, results in
> > silently ignoring it.
> > Warn the user via a debug print when the flag is requested but ignored
> > by the videobuf2 framework.
> > 
> > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > ---
> >  drivers/media/common/videobuf2/videobuf2-v4l2.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> 
> Thanks for the patch. Please see my comments inline.
> 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > index 6edf4508c636..812c8d1962e0 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > @@ -329,8 +329,13 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
> >                  */
> >                 vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
> >                 vbuf->field = b->field;
> > -               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
> > +               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)) {
> > +                       if (vbuf->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
> > +                               dprintk(q, 1,
> > +                                       "Request holding buffer (%d), unsupported on output queue\n",
> > +                                       b->index);
> 
> I wonder if we shouldn't just fail such a QBUF operation. Otherwise
> the application would get unexpected behavior from the kernel.
> Although it might be too late to do it now if there are applications
> that rely on this implicit ignore...

In the context of this patchset, the statu quo seems to be the logical thing to
do. We can raise this up in a separate thread. The side effect is of course
confusing for developers, but it is hard for me to tell if a hard failure may
break an existing software.

regards,
Nicolas

> 
> Best regards,
> Tomasz
Hans Verkuil April 28, 2022, 6:12 a.m. UTC | #3 
On 27/04/2022 17:08, Nicolas Dufresne wrote:
> Le mercredi 27 avril 2022 à 13:31 +0900, Tomasz Figa a écrit :
>> Hi Nicolas, Sebastian,
>>
>> On Tue, Apr 26, 2022 at 9:58 PM Nicolas Dufresne
>> <nicolas.dufresne@collabora.com> wrote:
>>>
>>> From: Sebastian Fricke <sebastian.fricke@collabora.com>
>>>
>>> Using V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag without specifying the
>>> subsystem flag VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, results in
>>> silently ignoring it.
>>> Warn the user via a debug print when the flag is requested but ignored
>>> by the videobuf2 framework.
>>>
>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>> ---
>>>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>
>> Thanks for the patch. Please see my comments inline.
>>
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> index 6edf4508c636..812c8d1962e0 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> @@ -329,8 +329,13 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
>>>                  */
>>>                 vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
>>>                 vbuf->field = b->field;
>>> -               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
>>> +               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)) {
>>> +                       if (vbuf->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
>>> +                               dprintk(q, 1,
>>> +                                       "Request holding buffer (%d), unsupported on output queue\n",
>>> +                                       b->index);
>>
>> I wonder if we shouldn't just fail such a QBUF operation. Otherwise
>> the application would get unexpected behavior from the kernel.
>> Although it might be too late to do it now if there are applications
>> that rely on this implicit ignore...
> 
> In the context of this patchset, the statu quo seems to be the logical thing to
> do. We can raise this up in a separate thread. The side effect is of course
> confusing for developers, but it is hard for me to tell if a hard failure may
> break an existing software.

I am leaning towards returning an error as well. It makes no sense to try
to hold on to a buffer when this is not supported.

I also thought that it should be enough to rely on the core to clear the
flag upon return if it isn't supported, but looking through the vb2 core code
it looks like we're not clearing unknown flags at all, so running this for
older kernels that do not support holding at all will not clear the flag
either.

The handling for flags in vb2 can be improved, I think I'll take a look at
that myself.

I plan to merge this series soon, but will skip this patch for now.

Regards,

	Hans

> 
> regards,
> Nicolas
> 
>>
>> Best regards,
>> Tomasz
>
Nicolas Dufresne April 28, 2022, 1:09 p.m. UTC | #4 
Le jeudi 28 avril 2022 à 08:12 +0200, Hans Verkuil a écrit :
> On 27/04/2022 17:08, Nicolas Dufresne wrote:
> > Le mercredi 27 avril 2022 à 13:31 +0900, Tomasz Figa a écrit :
> > > Hi Nicolas, Sebastian,
> > > 
> > > On Tue, Apr 26, 2022 at 9:58 PM Nicolas Dufresne
> > > <nicolas.dufresne@collabora.com> wrote:
> > > > 
> > > > From: Sebastian Fricke <sebastian.fricke@collabora.com>
> > > > 
> > > > Using V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag without specifying the
> > > > subsystem flag VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, results in
> > > > silently ignoring it.
> > > > Warn the user via a debug print when the flag is requested but ignored
> > > > by the videobuf2 framework.
> > > > 
> > > > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > > > ---
> > > >  drivers/media/common/videobuf2/videobuf2-v4l2.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > 
> > > Thanks for the patch. Please see my comments inline.
> > > 
> > > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > > index 6edf4508c636..812c8d1962e0 100644
> > > > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > > @@ -329,8 +329,13 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
> > > >                  */
> > > >                 vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
> > > >                 vbuf->field = b->field;
> > > > -               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
> > > > +               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)) {
> > > > +                       if (vbuf->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
> > > > +                               dprintk(q, 1,
> > > > +                                       "Request holding buffer (%d), unsupported on output queue\n",
> > > > +                                       b->index);
> > > 
> > > I wonder if we shouldn't just fail such a QBUF operation. Otherwise
> > > the application would get unexpected behavior from the kernel.
> > > Although it might be too late to do it now if there are applications
> > > that rely on this implicit ignore...
> > 
> > In the context of this patchset, the statu quo seems to be the logical thing to
> > do. We can raise this up in a separate thread. The side effect is of course
> > confusing for developers, but it is hard for me to tell if a hard failure may
> > break an existing software.
> 
> I am leaning towards returning an error as well. It makes no sense to try
> to hold on to a buffer when this is not supported.
> 
> I also thought that it should be enough to rely on the core to clear the
> flag upon return if it isn't supported, but looking through the vb2 core code
> it looks like we're not clearing unknown flags at all, so running this for
> older kernels that do not support holding at all will not clear the flag
> either.
> 
> The handling for flags in vb2 can be improved, I think I'll take a look at
> that myself.
> 
> I plan to merge this series soon, but will skip this patch for now.

Ok, no problem. For me, as long as we do something about it, since it was not
obvious and time consuming to debug.

regards,
Nicolas

> 
> Regards,
> 
> 	Hans
> 
> > 
> > regards,
> > Nicolas
> > 
> > > 
> > > Best regards,
> > > Tomasz
> > 
>
Hans Verkuil Nov. 24, 2022, 8:57 a.m. UTC | #5 
On 28/04/2022 15:09, Nicolas Dufresne wrote:
> Le jeudi 28 avril 2022 à 08:12 +0200, Hans Verkuil a écrit :
>> On 27/04/2022 17:08, Nicolas Dufresne wrote:
>>> Le mercredi 27 avril 2022 à 13:31 +0900, Tomasz Figa a écrit :
>>>> Hi Nicolas, Sebastian,
>>>>
>>>> On Tue, Apr 26, 2022 at 9:58 PM Nicolas Dufresne
>>>> <nicolas.dufresne@collabora.com> wrote:
>>>>>
>>>>> From: Sebastian Fricke <sebastian.fricke@collabora.com>
>>>>>
>>>>> Using V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag without specifying the
>>>>> subsystem flag VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, results in
>>>>> silently ignoring it.
>>>>> Warn the user via a debug print when the flag is requested but ignored
>>>>> by the videobuf2 framework.
>>>>>
>>>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>>>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>>>> ---
>>>>>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 7 ++++++-
>>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>>> Thanks for the patch. Please see my comments inline.
>>>>
>>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>>> index 6edf4508c636..812c8d1962e0 100644
>>>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>>> @@ -329,8 +329,13 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
>>>>>                  */
>>>>>                 vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
>>>>>                 vbuf->field = b->field;
>>>>> -               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
>>>>> +               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)) {
>>>>> +                       if (vbuf->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
>>>>> +                               dprintk(q, 1,
>>>>> +                                       "Request holding buffer (%d), unsupported on output queue\n",
>>>>> +                                       b->index);
>>>>
>>>> I wonder if we shouldn't just fail such a QBUF operation. Otherwise
>>>> the application would get unexpected behavior from the kernel.
>>>> Although it might be too late to do it now if there are applications
>>>> that rely on this implicit ignore...
>>>
>>> In the context of this patchset, the statu quo seems to be the logical thing to
>>> do. We can raise this up in a separate thread. The side effect is of course
>>> confusing for developers, but it is hard for me to tell if a hard failure may
>>> break an existing software.
>>
>> I am leaning towards returning an error as well. It makes no sense to try
>> to hold on to a buffer when this is not supported.
>>
>> I also thought that it should be enough to rely on the core to clear the
>> flag upon return if it isn't supported, but looking through the vb2 core code
>> it looks like we're not clearing unknown flags at all, so running this for
>> older kernels that do not support holding at all will not clear the flag
>> either.
>>
>> The handling for flags in vb2 can be improved, I think I'll take a look at
>> that myself.
>>
>> I plan to merge this series soon, but will skip this patch for now.
> 
> Ok, no problem. For me, as long as we do something about it, since it was not
> obvious and time consuming to debug.

After thinking some more (it only took me 7 months :-) ) I believe that an error
should be returned and the documentation for V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF
should be updated to mention that it will return EINVAL if set for a queue that
doesn't support this. vb2_queue_or_prepare_buf() is probably the best place for
this check.

Regards,

	Hans

> 
> regards,
> Nicolas
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> regards,
>>> Nicolas
>>>
>>>>
>>>> Best regards,
>>>> Tomasz
>>>
>>
>
diff mbox series

Patch

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 6edf4508c636..812c8d1962e0 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -329,8 +329,13 @@  static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
 		 */
 		vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
 		vbuf->field = b->field;
-		if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
+		if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)) {
+			if (vbuf->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
+				dprintk(q, 1,
+					"Request holding buffer (%d), unsupported on output queue\n",
+					b->index);
 			vbuf->flags &= ~V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF;
+		}
 	} else {
 		/* Zero any output buffer flags as this is a capture buffer */
 		vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS;