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

[Nicolas Dufresne]

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@xxxxxxxxxxxxx> wrote:
> > > > 
> > > > From: Sebastian Fricke <sebastian.fricke@xxxxxxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxxxxxx>
> > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
> > > > Reviewed-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >  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
> > 
>