| Message ID | 20231010102458.111227-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: subdev: Don't report V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled | expand |
Hi Hans, On Tue, Oct 10, 2023 at 12:24:58PM +0200, Hans de Goede wrote: > Since the stream API is still experimental it is currently locked away > behind the internal, default disabled, v4l2_subdev_enable_streams_api flag. > > Advertising V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled > confuses userspace. E.g. it causes the following libcamera error: > > ERROR SimplePipeline simple.cpp:1497 Failed to reset routes for > /dev/v4l-subdev1: Inappropriate ioctl for device > > Don't report V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled > to avoid problems like this. > > Reported-by: Dennis Bonke <admin@dennisbonke.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > -Clearing the V4L2_SUBDEV_FL_STREAMS flag from sd.flags might seem > appealing as an alternative fix. But this causes various v4l2-core bits > to enter different code paths which confuses drivers which set > V4L2_SUBDEV_FL_STREAMS, so this is a bad idea. Thanks, this apparently had been missed while disabling the API. Probably also should be added: Fixes: 9a6b5bf4c1bb ("media: add V4L2_SUBDEV_CAP_STREAMS") Cc: stable@vger.kernel.org # for >= 6.3 Also cc'd Tomi. > -No Closes: for the Reported-by since this was reported by private email > --- > drivers/media/v4l2-core/v4l2-subdev.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index b92348ad61f6..31752c06d1f0 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -502,6 +502,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > V4L2_SUBDEV_CLIENT_CAP_STREAMS; > int rval; > > + /* > + * If the streams API is not enabled, remove V4L2_SUBDEV_CAP_STREAMS. > + * Remove this when the API is no longer experimental. > + */ > + if (!v4l2_subdev_enable_streams_api) > + streams_subdev = false; > + > switch (cmd) { > case VIDIOC_SUBDEV_QUERYCAP: { > struct v4l2_subdev_capability *cap = arg;
On 10/10/23 13:49, Sakari Ailus wrote: > Hi Hans, > > On Tue, Oct 10, 2023 at 12:24:58PM +0200, Hans de Goede wrote: >> Since the stream API is still experimental it is currently locked away >> behind the internal, default disabled, v4l2_subdev_enable_streams_api flag. >> >> Advertising V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled >> confuses userspace. E.g. it causes the following libcamera error: >> >> ERROR SimplePipeline simple.cpp:1497 Failed to reset routes for >> /dev/v4l-subdev1: Inappropriate ioctl for device >> >> Don't report V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled >> to avoid problems like this. >> >> Reported-by: Dennis Bonke <admin@dennisbonke.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> -Clearing the V4L2_SUBDEV_FL_STREAMS flag from sd.flags might seem >> appealing as an alternative fix. But this causes various v4l2-core bits >> to enter different code paths which confuses drivers which set >> V4L2_SUBDEV_FL_STREAMS, so this is a bad idea. > > Thanks, this apparently had been missed while disabling the API. > > Probably also should be added: > > Fixes: 9a6b5bf4c1bb ("media: add V4L2_SUBDEV_CAP_STREAMS") > Cc: stable@vger.kernel.org # for >= 6.3 > > Also cc'd Tomi. Should this be queued up as a 6.6 fix? Regards, Hans V. > >> -No Closes: for the Reported-by since this was reported by private email >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index b92348ad61f6..31752c06d1f0 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -502,6 +502,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, >> V4L2_SUBDEV_CLIENT_CAP_STREAMS; >> int rval; >> >> + /* >> + * If the streams API is not enabled, remove V4L2_SUBDEV_CAP_STREAMS. >> + * Remove this when the API is no longer experimental. >> + */ >> + if (!v4l2_subdev_enable_streams_api) >> + streams_subdev = false; >> + >> switch (cmd) { >> case VIDIOC_SUBDEV_QUERYCAP: { >> struct v4l2_subdev_capability *cap = arg; >
Hi Hans, On Tue, Oct 10, 2023 at 01:52:21PM +0200, Hans Verkuil wrote: > On 10/10/23 13:49, Sakari Ailus wrote: > > Hi Hans, > > > > On Tue, Oct 10, 2023 at 12:24:58PM +0200, Hans de Goede wrote: > >> Since the stream API is still experimental it is currently locked away > >> behind the internal, default disabled, v4l2_subdev_enable_streams_api flag. > >> > >> Advertising V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled > >> confuses userspace. E.g. it causes the following libcamera error: > >> > >> ERROR SimplePipeline simple.cpp:1497 Failed to reset routes for > >> /dev/v4l-subdev1: Inappropriate ioctl for device > >> > >> Don't report V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled > >> to avoid problems like this. > >> > >> Reported-by: Dennis Bonke <admin@dennisbonke.com> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> -Clearing the V4L2_SUBDEV_FL_STREAMS flag from sd.flags might seem > >> appealing as an alternative fix. But this causes various v4l2-core bits > >> to enter different code paths which confuses drivers which set > >> V4L2_SUBDEV_FL_STREAMS, so this is a bad idea. > > > > Thanks, this apparently had been missed while disabling the API. > > > > Probably also should be added: > > > > Fixes: 9a6b5bf4c1bb ("media: add V4L2_SUBDEV_CAP_STREAMS") > > Cc: stable@vger.kernel.org # for >= 6.3 > > > > Also cc'd Tomi. > > Should this be queued up as a 6.6 fix? I wonder what the criteria is these days. I'd think it's unlikely anything is or will be broken by this in practice. The further this exists, though, increases the likelihood for that to happen.
On 10/10/23 14:24, Sakari Ailus wrote: > Hi Hans, > > On Tue, Oct 10, 2023 at 01:52:21PM +0200, Hans Verkuil wrote: >> On 10/10/23 13:49, Sakari Ailus wrote: >>> Hi Hans, >>> >>> On Tue, Oct 10, 2023 at 12:24:58PM +0200, Hans de Goede wrote: >>>> Since the stream API is still experimental it is currently locked away >>>> behind the internal, default disabled, v4l2_subdev_enable_streams_api flag. >>>> >>>> Advertising V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled >>>> confuses userspace. E.g. it causes the following libcamera error: >>>> >>>> ERROR SimplePipeline simple.cpp:1497 Failed to reset routes for >>>> /dev/v4l-subdev1: Inappropriate ioctl for device >>>> >>>> Don't report V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled >>>> to avoid problems like this. >>>> >>>> Reported-by: Dennis Bonke <admin@dennisbonke.com> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> -Clearing the V4L2_SUBDEV_FL_STREAMS flag from sd.flags might seem >>>> appealing as an alternative fix. But this causes various v4l2-core bits >>>> to enter different code paths which confuses drivers which set >>>> V4L2_SUBDEV_FL_STREAMS, so this is a bad idea. >>> >>> Thanks, this apparently had been missed while disabling the API. >>> >>> Probably also should be added: >>> >>> Fixes: 9a6b5bf4c1bb ("media: add V4L2_SUBDEV_CAP_STREAMS") >>> Cc: stable@vger.kernel.org # for >= 6.3 >>> >>> Also cc'd Tomi. >> >> Should this be queued up as a 6.6 fix? > > I wonder what the criteria is these days. > > I'd think it's unlikely anything is or will be broken by this in practice. > The further this exists, though, increases the likelihood for that to > happen. > 1) Regressions: i.e. it worked before, but no longer in v6.6. 2) Compilation errors, typically due to Kconfig problems. 3) For new code that appeared in v6.6: serious bugs causing kernel oopses or other bad behavior during normal use. (I.e., the 'Oh shit, I never tested that!' bugs) Generally a lot of these fixes deal with error paths etc., those can often wait for the next kernel. There are no doubt more reasons for considering patches for v6.6, but those three are no-brainers... And there is of course a gray area where you could lean either way. For this particular patch it would affect imx8-isi-crossbar.c in 6.4 onwards, and starting with 6.6 it is also used in the ds90ub9xx drivers according to git grep. Regards, Hans
On Tue, Oct 10, 2023 at 03:01:53PM +0200, Hans Verkuil wrote: > On 10/10/23 14:24, Sakari Ailus wrote: > > Hi Hans, > > > > On Tue, Oct 10, 2023 at 01:52:21PM +0200, Hans Verkuil wrote: > >> On 10/10/23 13:49, Sakari Ailus wrote: > >>> Hi Hans, > >>> > >>> On Tue, Oct 10, 2023 at 12:24:58PM +0200, Hans de Goede wrote: > >>>> Since the stream API is still experimental it is currently locked away > >>>> behind the internal, default disabled, v4l2_subdev_enable_streams_api flag. > >>>> > >>>> Advertising V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled > >>>> confuses userspace. E.g. it causes the following libcamera error: > >>>> > >>>> ERROR SimplePipeline simple.cpp:1497 Failed to reset routes for > >>>> /dev/v4l-subdev1: Inappropriate ioctl for device > >>>> > >>>> Don't report V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled > >>>> to avoid problems like this. > >>>> > >>>> Reported-by: Dennis Bonke <admin@dennisbonke.com> > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>> --- > >>>> -Clearing the V4L2_SUBDEV_FL_STREAMS flag from sd.flags might seem > >>>> appealing as an alternative fix. But this causes various v4l2-core bits > >>>> to enter different code paths which confuses drivers which set > >>>> V4L2_SUBDEV_FL_STREAMS, so this is a bad idea. > >>> > >>> Thanks, this apparently had been missed while disabling the API. > >>> > >>> Probably also should be added: > >>> > >>> Fixes: 9a6b5bf4c1bb ("media: add V4L2_SUBDEV_CAP_STREAMS") > >>> Cc: stable@vger.kernel.org # for >= 6.3 > >>> > >>> Also cc'd Tomi. > >> > >> Should this be queued up as a 6.6 fix? > > > > I wonder what the criteria is these days. > > > > I'd think it's unlikely anything is or will be broken by this in practice. > > The further this exists, though, increases the likelihood for that to > > happen. > > > > 1) Regressions: i.e. it worked before, but no longer in v6.6. > 2) Compilation errors, typically due to Kconfig problems. > 3) For new code that appeared in v6.6: serious bugs causing kernel oopses > or other bad behavior during normal use. (I.e., the 'Oh shit, I never > tested that!' bugs) > > Generally a lot of these fixes deal with error paths etc., those can > often wait for the next kernel. > > There are no doubt more reasons for considering patches for v6.6, but those > three are no-brainers... > > And there is of course a gray area where you could lean either way. > > For this particular patch it would affect imx8-isi-crossbar.c in 6.4 onwards, > and starting with 6.6 it is also used in the ds90ub9xx drivers according to > git grep. I think it'd be better to get it to 6.6 right away. If you take this, please add: Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Hi Hans, Thank you for the patch. On Tue, Oct 10, 2023 at 12:24:58PM +0200, Hans de Goede wrote: > Since the stream API is still experimental it is currently locked away > behind the internal, default disabled, v4l2_subdev_enable_streams_api flag. > > Advertising V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled > confuses userspace. E.g. it causes the following libcamera error: > > ERROR SimplePipeline simple.cpp:1497 Failed to reset routes for > /dev/v4l-subdev1: Inappropriate ioctl for device > > Don't report V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled > to avoid problems like this. > > Reported-by: Dennis Bonke <admin@dennisbonke.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > -Clearing the V4L2_SUBDEV_FL_STREAMS flag from sd.flags might seem > appealing as an alternative fix. But this causes various v4l2-core bits > to enter different code paths which confuses drivers which set > V4L2_SUBDEV_FL_STREAMS, so this is a bad idea. > -No Closes: for the Reported-by since this was reported by private email > --- > drivers/media/v4l2-core/v4l2-subdev.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index b92348ad61f6..31752c06d1f0 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -502,6 +502,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > V4L2_SUBDEV_CLIENT_CAP_STREAMS; > int rval; > > + /* > + * If the streams API is not enabled, remove V4L2_SUBDEV_CAP_STREAMS. > + * Remove this when the API is no longer experimental. > + */ > + if (!v4l2_subdev_enable_streams_api) > + streams_subdev = false; > + > switch (cmd) { > case VIDIOC_SUBDEV_QUERYCAP: { > struct v4l2_subdev_capability *cap = arg;
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index b92348ad61f6..31752c06d1f0 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -502,6 +502,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, V4L2_SUBDEV_CLIENT_CAP_STREAMS; int rval; + /* + * If the streams API is not enabled, remove V4L2_SUBDEV_CAP_STREAMS. + * Remove this when the API is no longer experimental. + */ + if (!v4l2_subdev_enable_streams_api) + streams_subdev = false; + switch (cmd) { case VIDIOC_SUBDEV_QUERYCAP: { struct v4l2_subdev_capability *cap = arg;
Since the stream API is still experimental it is currently locked away behind the internal, default disabled, v4l2_subdev_enable_streams_api flag. Advertising V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled confuses userspace. E.g. it causes the following libcamera error: ERROR SimplePipeline simple.cpp:1497 Failed to reset routes for /dev/v4l-subdev1: Inappropriate ioctl for device Don't report V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled to avoid problems like this. Reported-by: Dennis Bonke <admin@dennisbonke.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- -Clearing the V4L2_SUBDEV_FL_STREAMS flag from sd.flags might seem appealing as an alternative fix. But this causes various v4l2-core bits to enter different code paths which confuses drivers which set V4L2_SUBDEV_FL_STREAMS, so this is a bad idea. -No Closes: for the Reported-by since this was reported by private email --- drivers/media/v4l2-core/v4l2-subdev.c | 7 +++++++ 1 file changed, 7 insertions(+)