| Message ID | 20220426125751.108293-4-nicolas.dufresne@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,01/24] media: doc: Document dual use of H.264 pic_num/frame_num | expand |
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
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
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 >
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 > > >
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 --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;