diff mbox series

media: subdev: Don't report V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled

Message ID 20231010102458.111227-1-hdegoede@redhat.com (mailing list archive)
State Accepted
Delegated to: Hans Verkuil
Headers
Series media: subdev: Don't report V4L2_SUBDEV_CAP_STREAMS when the streams API is disabled |

Commit Message

Hans de Goede Oct. 10, 2023, 10:24 a.m. UTC 
  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(+)

Comments

Sakari Ailus Oct. 10, 2023, 11:49 a.m. UTC | #1 
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;
Hans Verkuil Oct. 10, 2023, 11:52 a.m. UTC | #2 
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;
>
Sakari Ailus Oct. 10, 2023, 12:24 p.m. UTC | #3 
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.
Hans Verkuil Oct. 10, 2023, 1:01 p.m. UTC | #4 
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
Sakari Ailus Oct. 10, 2023, 1:10 p.m. UTC | #5 
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>
Laurent Pinchart Oct. 10, 2023, 1:35 p.m. UTC | #6 
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 mbox series

Patch

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;