Message ID | 20220426125751.108293-4-nicolas.dufresne@collabora.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Hans Verkuil |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1njKlp-002d5Q-8l; Tue, 26 Apr 2022 12:58:21 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1350361AbiDZNB0 (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 26 Apr 2022 09:01:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45340 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1350322AbiDZNBP (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 26 Apr 2022 09:01:15 -0400 Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DC5C317F112; Tue, 26 Apr 2022 05:58:04 -0700 (PDT) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: nicolas) with ESMTPSA id 986A21F43991 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1650977883; bh=BQPoxOAZCWnxNCOeZo2D/Nd5xmmLaOzP4aXydtQmwEk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=lCMq/XYI1rT//EaxIr4tzjZMIi9SfAN1J+rxbo0nX1o4jG4rFJlV21qrb532VgoPi EEausR1Wl1NSrzYd+jfzayy0oZQjE5BI7jV38EemoScgGvtutOjtfDOghZfLlOdrFD a/4lzfJCPQH8WFiU/NWsOdcSzvcXMBcGxuLi/QUOTJUzgjNzTm1kbRZ/5/Z6gp4imz Z4JHsvPFxFNCK/NvXo6GVxqx5P3XPJMDaiRVBjtiaqHoH+0YGlzIrtBfU4nyLOF0Xo KRJ+jm1IBdnAFnMDJups+8ryQ8pIHfokEdpjIqk7b4h59bl8YB0PQw7s6Q6lqWjX9q wMAE8nt1ra+QQ== From: Nicolas Dufresne <nicolas.dufresne@collabora.com> To: Tomasz Figa <tfiga@chromium.org>, Marek Szyprowski <m.szyprowski@samsung.com>, Mauro Carvalho Chehab <mchehab@kernel.org> Cc: nicolas@ndufresne.ca, Sebastian Fricke <sebastian.fricke@collabora.com>, linux-media@vger.kernel.org, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>, linux-kernel@vger.kernel.org Subject: [PATCH v4 03/24] media: videobuf2-v4l2: Warn on holding buffers without support Date: Tue, 26 Apr 2022 08:57:29 -0400 Message-Id: <20220426125751.108293-4-nicolas.dufresne@collabora.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20220426125751.108293-1-nicolas.dufresne@collabora.com> References: <20220426125751.108293-1-nicolas.dufresne@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_PASS,SPF_PASS, UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,UNPARSEABLE_RELAY=0.001 autolearn=ham autolearn_force=no |
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
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;