[RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API

[RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API

[Hans Verkuil]

This patch selects MEDIA_CONTROLLER for all camera, analog TV and
digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically.

This will allow us to simplify drivers that currently have to add
#ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API
to their code, since now this will always be available.

The original intent of allowing these to be configured by the
user was (I think) to save a bit of memory. But as more and more
drivers have a media controller and all regular distros already
enable one or more of those drivers, the memory for the MC code is
there anyway.

Complexity has always been the bane of media drivers, so reducing
complexity at the expense of a bit more memory (which is a rounding
error compared to the amount of video buffer memory needed) is IMHO
a good thing.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
index 8add62a18293..56eb01cc8bb4 100644
--- a/drivers/media/Kconfig
+++ b/drivers/media/Kconfig
@@ -31,6 +31,7 @@ comment "Multimedia core support"
 #
 config MEDIA_CAMERA_SUPPORT
 	bool "Cameras/video grabbers support"
+	select MEDIA_CONTROLLER
 	---help---
 	  Enable support for webcams and video grabbers.

@@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT

 config MEDIA_ANALOG_TV_SUPPORT
 	bool "Analog TV support"
+	select MEDIA_CONTROLLER
 	---help---
 	  Enable analog TV support.

@@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT

 config MEDIA_DIGITAL_TV_SUPPORT
 	bool "Digital TV support"
+	select MEDIA_CONTROLLER
 	---help---
 	  Enable digital TV support.

@@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig"

 config MEDIA_CONTROLLER
 	bool "Media Controller API"
-	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT
 	---help---
 	  Enable the media controller API used to query media devices internal
 	  topology and configure it dynamically.
@@ -119,16 +121,11 @@ config VIDEO_DEV
 	tristate
 	depends on MEDIA_SUPPORT
 	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT
+	select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER
 	default y

 config VIDEO_V4L2_SUBDEV_API
-	bool "V4L2 sub-device userspace API"
-	depends on VIDEO_DEV && MEDIA_CONTROLLER
-	---help---
-	  Enables the V4L2 sub-device pad-level userspace API used to configure
-	  video format, size and frame rate between hardware blocks.
-
-	  This API is mostly used by camera interfaces in embedded platforms.
+	bool

 source "drivers/media/v4l2-core/Kconfig"

Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API

[Mauro Carvalho Chehab]

Em Fri, 7 Dec 2018 10:09:04 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> This patch selects MEDIA_CONTROLLER for all camera, analog TV and
> digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically.
> 
> This will allow us to simplify drivers that currently have to add
> #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API
> to their code, since now this will always be available.
> 
> The original intent of allowing these to be configured by the
> user was (I think) to save a bit of memory. 

No. The original intent was/is to be sure that adding the media
controller support won't be breaking existing working drivers.

> But as more and more
> drivers have a media controller and all regular distros already
> enable one or more of those drivers, the memory for the MC code is
> there anyway.
> 
> Complexity has always been the bane of media drivers, so reducing
> complexity at the expense of a bit more memory (which is a rounding
> error compared to the amount of video buffer memory needed) is IMHO
> a good thing.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> index 8add62a18293..56eb01cc8bb4 100644
> --- a/drivers/media/Kconfig
> +++ b/drivers/media/Kconfig
> @@ -31,6 +31,7 @@ comment "Multimedia core support"
>  #
>  config MEDIA_CAMERA_SUPPORT
>  	bool "Cameras/video grabbers support"
> +	select MEDIA_CONTROLLER
>  	---help---
>  	  Enable support for webcams and video grabbers.
> 
> @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT
> 
>  config MEDIA_ANALOG_TV_SUPPORT
>  	bool "Analog TV support"
> +	select MEDIA_CONTROLLER
>  	---help---
>  	  Enable analog TV support.
> 
> @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT
> 
>  config MEDIA_DIGITAL_TV_SUPPORT
>  	bool "Digital TV support"
> +	select MEDIA_CONTROLLER
>  	---help---
>  	  Enable digital TV support.

See my comments below.

> 
> @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig"
> 
>  config MEDIA_CONTROLLER
>  	bool "Media Controller API"
> -	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT
>  	---help---
>  	  Enable the media controller API used to query media devices internal
>  	  topology and configure it dynamically.

I have split comments with regards to it. Yeah, nowadays media controller
has becoming more important. Still, a lot of media drivers work fine
without them.

Anyway, if we're willing to make it mandatory, better to just remove the
entire config option or to make it a silent one. 

> @@ -119,16 +121,11 @@ config VIDEO_DEV
>  	tristate
>  	depends on MEDIA_SUPPORT
>  	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT
> +	select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER
>  	default y
> 
>  config VIDEO_V4L2_SUBDEV_API
> -	bool "V4L2 sub-device userspace API"
> -	depends on VIDEO_DEV && MEDIA_CONTROLLER
> -	---help---
> -	  Enables the V4L2 sub-device pad-level userspace API used to configure
> -	  video format, size and frame rate between hardware blocks.
> -
> -	  This API is mostly used by camera interfaces in embedded platforms.
> +	bool

NACK. 

There is a very good reason why the subdev API is optional: there
are drivers that use camera sensors but are not MC-centric. On those,
the USB bridge driver is responsible to setup the subdevice. 

This options helps to ensure that camera sensors used by such drivers
won't stop working because of the lack of the subdev-API.

> 
>  source "drivers/media/v4l2-core/Kconfig"
> 



Thanks,
Mauro

Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API

[Hans Verkuil]

On 12/07/2018 12:26 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 7 Dec 2018 10:09:04 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> This patch selects MEDIA_CONTROLLER for all camera, analog TV and
>> digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically.
>>
>> This will allow us to simplify drivers that currently have to add
>> #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API
>> to their code, since now this will always be available.
>>
>> The original intent of allowing these to be configured by the
>> user was (I think) to save a bit of memory. 
> 
> No. The original intent was/is to be sure that adding the media
> controller support won't be breaking existing working drivers.

That doesn't make sense. If enabling this option would break existing
drivers, then something is really wrong, isn't it?

> 
>> But as more and more
>> drivers have a media controller and all regular distros already
>> enable one or more of those drivers, the memory for the MC code is
>> there anyway.
>>
>> Complexity has always been the bane of media drivers, so reducing
>> complexity at the expense of a bit more memory (which is a rounding
>> error compared to the amount of video buffer memory needed) is IMHO
>> a good thing.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
>> index 8add62a18293..56eb01cc8bb4 100644
>> --- a/drivers/media/Kconfig
>> +++ b/drivers/media/Kconfig
>> @@ -31,6 +31,7 @@ comment "Multimedia core support"
>>  #
>>  config MEDIA_CAMERA_SUPPORT
>>  	bool "Cameras/video grabbers support"
>> +	select MEDIA_CONTROLLER
>>  	---help---
>>  	  Enable support for webcams and video grabbers.
>>
>> @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT
>>
>>  config MEDIA_ANALOG_TV_SUPPORT
>>  	bool "Analog TV support"
>> +	select MEDIA_CONTROLLER
>>  	---help---
>>  	  Enable analog TV support.
>>
>> @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT
>>
>>  config MEDIA_DIGITAL_TV_SUPPORT
>>  	bool "Digital TV support"
>> +	select MEDIA_CONTROLLER
>>  	---help---
>>  	  Enable digital TV support.
> 
> See my comments below.
> 
>>
>> @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig"
>>
>>  config MEDIA_CONTROLLER
>>  	bool "Media Controller API"
>> -	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT
>>  	---help---
>>  	  Enable the media controller API used to query media devices internal
>>  	  topology and configure it dynamically.
> 
> I have split comments with regards to it. Yeah, nowadays media controller
> has becoming more important. Still, a lot of media drivers work fine
> without them.
> 
> Anyway, if we're willing to make it mandatory, better to just remove the
> entire config option or to make it a silent one. 

Well, that assumes that the media controller will only be used by media
drivers, and not alsa or anyone else who wants to experiment with the MC.

I won't object to making it silent (since it does reflect the current
situation), but since this functionality is not actually specific to media
drivers I think that is a good case to be made to allow it to be selected
manually.

> 
>> @@ -119,16 +121,11 @@ config VIDEO_DEV
>>  	tristate
>>  	depends on MEDIA_SUPPORT
>>  	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT
>> +	select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER
>>  	default y
>>
>>  config VIDEO_V4L2_SUBDEV_API
>> -	bool "V4L2 sub-device userspace API"
>> -	depends on VIDEO_DEV && MEDIA_CONTROLLER
>> -	---help---
>> -	  Enables the V4L2 sub-device pad-level userspace API used to configure
>> -	  video format, size and frame rate between hardware blocks.
>> -
>> -	  This API is mostly used by camera interfaces in embedded platforms.
>> +	bool
> 
> NACK. 
> 
> There is a very good reason why the subdev API is optional: there
> are drivers that use camera sensors but are not MC-centric. On those,
> the USB bridge driver is responsible to setup the subdevice. 
> 
> This options helps to ensure that camera sensors used by such drivers
> won't stop working because of the lack of the subdev-API.

But they won't stop working if this is enabled. This option is used as
a dependency by drivers that require this functionality, but enabling
it will never break other drivers that don't need this. Those drivers
simply won't use it.

Also note that it is the bridge driver that controls whether or not
the v4l-subdevX devices are created. If the bridge driver doesn't
explicitly enable it AND the subdev driver explicitly supports it,
those devices will not be created.

Regards,

	Hans

> 
>>
>>  source "drivers/media/v4l2-core/Kconfig"
>>
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API

[Mauro Carvalho Chehab]

Em Fri, 7 Dec 2018 12:47:24 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 12/07/2018 12:26 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 7 Dec 2018 10:09:04 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> This patch selects MEDIA_CONTROLLER for all camera, analog TV and
> >> digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically.
> >>
> >> This will allow us to simplify drivers that currently have to add
> >> #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API
> >> to their code, since now this will always be available.
> >>
> >> The original intent of allowing these to be configured by the
> >> user was (I think) to save a bit of memory.   
> > 
> > No. The original intent was/is to be sure that adding the media
> > controller support won't be breaking existing working drivers.  
> 
> That doesn't make sense. If enabling this option would break existing
> drivers, then something is really wrong, isn't it?

It is the opposite: disabling it should not break any driver that don't
depend on them to work.

> 
> >   
> >> But as more and more
> >> drivers have a media controller and all regular distros already
> >> enable one or more of those drivers, the memory for the MC code is
> >> there anyway.
> >>
> >> Complexity has always been the bane of media drivers, so reducing
> >> complexity at the expense of a bit more memory (which is a rounding
> >> error compared to the amount of video buffer memory needed) is IMHO
> >> a good thing.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> ---
> >> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> >> index 8add62a18293..56eb01cc8bb4 100644
> >> --- a/drivers/media/Kconfig
> >> +++ b/drivers/media/Kconfig
> >> @@ -31,6 +31,7 @@ comment "Multimedia core support"
> >>  #
> >>  config MEDIA_CAMERA_SUPPORT
> >>  	bool "Cameras/video grabbers support"
> >> +	select MEDIA_CONTROLLER
> >>  	---help---
> >>  	  Enable support for webcams and video grabbers.
> >>
> >> @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT
> >>
> >>  config MEDIA_ANALOG_TV_SUPPORT
> >>  	bool "Analog TV support"
> >> +	select MEDIA_CONTROLLER
> >>  	---help---
> >>  	  Enable analog TV support.
> >>
> >> @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT
> >>
> >>  config MEDIA_DIGITAL_TV_SUPPORT
> >>  	bool "Digital TV support"
> >> +	select MEDIA_CONTROLLER
> >>  	---help---
> >>  	  Enable digital TV support.  
> > 
> > See my comments below.
> >   
> >>
> >> @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig"
> >>
> >>  config MEDIA_CONTROLLER
> >>  	bool "Media Controller API"
> >> -	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT
> >>  	---help---
> >>  	  Enable the media controller API used to query media devices internal
> >>  	  topology and configure it dynamically.  
> > 
> > I have split comments with regards to it. Yeah, nowadays media controller
> > has becoming more important. Still, a lot of media drivers work fine
> > without them.
> > 
> > Anyway, if we're willing to make it mandatory, better to just remove the
> > entire config option or to make it a silent one.   
> 
> Well, that assumes that the media controller will only be used by media
> drivers, and not alsa or anyone else who wants to experiment with the MC.
> 
> I won't object to making it silent (since it does reflect the current
> situation), but since this functionality is not actually specific to media
> drivers I think that is a good case to be made to allow it to be selected
> manually.
> 
> >   
> >> @@ -119,16 +121,11 @@ config VIDEO_DEV
> >>  	tristate
> >>  	depends on MEDIA_SUPPORT
> >>  	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT
> >> +	select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER
> >>  	default y
> >>
> >>  config VIDEO_V4L2_SUBDEV_API
> >> -	bool "V4L2 sub-device userspace API"
> >> -	depends on VIDEO_DEV && MEDIA_CONTROLLER
> >> -	---help---
> >> -	  Enables the V4L2 sub-device pad-level userspace API used to configure
> >> -	  video format, size and frame rate between hardware blocks.
> >> -
> >> -	  This API is mostly used by camera interfaces in embedded platforms.
> >> +	bool  
> > 
> > NACK. 
> > 
> > There is a very good reason why the subdev API is optional: there
> > are drivers that use camera sensors but are not MC-centric. On those,
> > the USB bridge driver is responsible to setup the subdevice. 
> > 
> > This options helps to ensure that camera sensors used by such drivers
> > won't stop working because of the lack of the subdev-API.  
> 
> But they won't stop working if this is enabled.

That's not the issue. I've seen (and nacked) several patches breaking
drivers by assuming that all init would happen via subdev API.

By having this as an optional feature that can be disabled, developers
need to ensure that either the driver won't be built as a hole, if
no subdev API suport is enabled, or need to add the needed logic inside
the sub-device in order to support both cases.

> This option is used as
> a dependency by drivers that require this functionality, but enabling
> it will never break other drivers that don't need this. Those drivers
> simply won't use it.

Not a 100% sure about that. There are some parts of the logic that seems
to assume that the device has subdev API and MC initialized.

See, for example:

	static inline struct v4l2_mbus_framefmt
	*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
				    struct v4l2_subdev_pad_config *cfg,
				    unsigned int pad)
	{
		if (WARN_ON(pad >= sd->entity.num_pads))
			pad = 0;
		return &cfg[pad].try_fmt;
	}

If the USB bridge driver doesn't use the media controller, the above
code will OOPS. See, for example, ov2659_get_fmt() logic.

Ok, this particular driver (AFAIKT) is only used on platform drivers,
but if the same sensor would be used by another driver that don't
expose subdev API, VIDIOC_GET_FMT won't work. Also, if
CONFIG_VIDEO_V4L2_SUBDEV_API is disabled, the ioctl will just return
an error, but if it is enabled, it will OOPS.

> Also note that it is the bridge driver that controls whether or not
> the v4l-subdevX devices are created. If the bridge driver doesn't
> explicitly enable it AND the subdev driver explicitly supports it,
> those devices will not be created.

The problem is not related to subdev creation. It is related to
having support for being fully set without using the subdev API
(or DT).

I'm not saying that it is not doable to solve this issue, but, right
now, some parts at the V4L2 core assumes that subdev API is
used if CONFIG_VIDEO_V4L2_SUBDEV_API is enabled.

See, for example, the drivers/media/i2c/mt9v011.c driver, with is 
used by a USB bridge driver that doesn't expose subdev API.

On this driver, even the probe logic had to be different, as it has 
to explicitly support platform data, as otherwise the sensor won't be
properly initialized, and it won't work.

Frankly, I don't see an easy way to make a sensor driver that would
be fully independent, as we would need to move all DT-specific
stuff to be handled outside the sensors and have a common way for
the V4L2 core to handle it, either as platform data or as DT,
and calling subdev-specific logic to handle it depending on the
case.

While we don't have the V4L2 fully abstracting the logic
if a device has subdev API or not, we can't get rid of
VIDEO_V4L2_SUBDEV_API.


Thanks,
Mauro

Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API

[Hans Verkuil]

On 12/07/2018 01:42 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 7 Dec 2018 12:47:24 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 12/07/2018 12:26 PM, Mauro Carvalho Chehab wrote:
>>> Em Fri, 7 Dec 2018 10:09:04 +0100
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>   
>>>> This patch selects MEDIA_CONTROLLER for all camera, analog TV and
>>>> digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically.
>>>>
>>>> This will allow us to simplify drivers that currently have to add
>>>> #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API
>>>> to their code, since now this will always be available.
>>>>
>>>> The original intent of allowing these to be configured by the
>>>> user was (I think) to save a bit of memory.   
>>>
>>> No. The original intent was/is to be sure that adding the media
>>> controller support won't be breaking existing working drivers.  
>>
>> That doesn't make sense. If enabling this option would break existing
>> drivers, then something is really wrong, isn't it?
> 
> It is the opposite: disabling it should not break any driver that don't
> depend on them to work.
> 
>>
>>>   
>>>> But as more and more
>>>> drivers have a media controller and all regular distros already
>>>> enable one or more of those drivers, the memory for the MC code is
>>>> there anyway.
>>>>
>>>> Complexity has always been the bane of media drivers, so reducing
>>>> complexity at the expense of a bit more memory (which is a rounding
>>>> error compared to the amount of video buffer memory needed) is IMHO
>>>> a good thing.
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>> ---
>>>> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
>>>> index 8add62a18293..56eb01cc8bb4 100644
>>>> --- a/drivers/media/Kconfig
>>>> +++ b/drivers/media/Kconfig
>>>> @@ -31,6 +31,7 @@ comment "Multimedia core support"
>>>>  #
>>>>  config MEDIA_CAMERA_SUPPORT
>>>>  	bool "Cameras/video grabbers support"
>>>> +	select MEDIA_CONTROLLER
>>>>  	---help---
>>>>  	  Enable support for webcams and video grabbers.
>>>>
>>>> @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT
>>>>
>>>>  config MEDIA_ANALOG_TV_SUPPORT
>>>>  	bool "Analog TV support"
>>>> +	select MEDIA_CONTROLLER
>>>>  	---help---
>>>>  	  Enable analog TV support.
>>>>
>>>> @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT
>>>>
>>>>  config MEDIA_DIGITAL_TV_SUPPORT
>>>>  	bool "Digital TV support"
>>>> +	select MEDIA_CONTROLLER
>>>>  	---help---
>>>>  	  Enable digital TV support.  
>>>
>>> See my comments below.
>>>   
>>>>
>>>> @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig"
>>>>
>>>>  config MEDIA_CONTROLLER
>>>>  	bool "Media Controller API"
>>>> -	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT
>>>>  	---help---
>>>>  	  Enable the media controller API used to query media devices internal
>>>>  	  topology and configure it dynamically.  
>>>
>>> I have split comments with regards to it. Yeah, nowadays media controller
>>> has becoming more important. Still, a lot of media drivers work fine
>>> without them.
>>>
>>> Anyway, if we're willing to make it mandatory, better to just remove the
>>> entire config option or to make it a silent one.   
>>
>> Well, that assumes that the media controller will only be used by media
>> drivers, and not alsa or anyone else who wants to experiment with the MC.
>>
>> I won't object to making it silent (since it does reflect the current
>> situation), but since this functionality is not actually specific to media
>> drivers I think that is a good case to be made to allow it to be selected
>> manually.
>>
>>>   
>>>> @@ -119,16 +121,11 @@ config VIDEO_DEV
>>>>  	tristate
>>>>  	depends on MEDIA_SUPPORT
>>>>  	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT
>>>> +	select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER
>>>>  	default y
>>>>
>>>>  config VIDEO_V4L2_SUBDEV_API
>>>> -	bool "V4L2 sub-device userspace API"
>>>> -	depends on VIDEO_DEV && MEDIA_CONTROLLER
>>>> -	---help---
>>>> -	  Enables the V4L2 sub-device pad-level userspace API used to configure
>>>> -	  video format, size and frame rate between hardware blocks.
>>>> -
>>>> -	  This API is mostly used by camera interfaces in embedded platforms.
>>>> +	bool  
>>>
>>> NACK. 
>>>
>>> There is a very good reason why the subdev API is optional: there
>>> are drivers that use camera sensors but are not MC-centric. On those,
>>> the USB bridge driver is responsible to setup the subdevice. 
>>>
>>> This options helps to ensure that camera sensors used by such drivers
>>> won't stop working because of the lack of the subdev-API.  
>>
>> But they won't stop working if this is enabled.
> 
> That's not the issue. I've seen (and nacked) several patches breaking
> drivers by assuming that all init would happen via subdev API.
> 
> By having this as an optional feature that can be disabled, developers
> need to ensure that either the driver won't be built as a hole, if
> no subdev API suport is enabled, or need to add the needed logic inside
> the sub-device in order to support both cases.
> 
>> This option is used as
>> a dependency by drivers that require this functionality, but enabling
>> it will never break other drivers that don't need this. Those drivers
>> simply won't use it.
> 
> Not a 100% sure about that. There are some parts of the logic that seems
> to assume that the device has subdev API and MC initialized.
> 
> See, for example:
> 
> 	static inline struct v4l2_mbus_framefmt
> 	*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> 				    struct v4l2_subdev_pad_config *cfg,
> 				    unsigned int pad)
> 	{
> 		if (WARN_ON(pad >= sd->entity.num_pads))
> 			pad = 0;
> 		return &cfg[pad].try_fmt;
> 	}
> 
> If the USB bridge driver doesn't use the media controller, the above
> code will OOPS. See, for example, ov2659_get_fmt() logic.

So if I have that USB bridge driver, and I also enable the V4L2_SUBDEV_API
for another driver, then the USB bridge driver would crash?!

If that's the case, then this is really, really broken. I always enable
this option whenever I build the media drivers, and I have never seen
anything break because of this. And if a driver would break then that
is an enormous bug in that driver or the subdev driver.

Please note that bridge drivers that do not rely on this config option
will never call these subdev ops with V4L2_SUBDEV_FORMAT_TRY.

So it will also never crash on this.

Basically what you want is a way to check that bridge drivers that do
not support the media controller or the subdev API (i.e. V4L2_SUBDEV_FORMAT_TRY)
do not attempt to use features that rely on subdevs supporting it.

I'm not sure that's possible, but let me think about it.

Regards,

	Hans

> 
> Ok, this particular driver (AFAIKT) is only used on platform drivers,
> but if the same sensor would be used by another driver that don't
> expose subdev API, VIDIOC_GET_FMT won't work. Also, if
> CONFIG_VIDEO_V4L2_SUBDEV_API is disabled, the ioctl will just return
> an error, but if it is enabled, it will OOPS.
> 
>> Also note that it is the bridge driver that controls whether or not
>> the v4l-subdevX devices are created. If the bridge driver doesn't
>> explicitly enable it AND the subdev driver explicitly supports it,
>> those devices will not be created.
> 
> The problem is not related to subdev creation. It is related to
> having support for being fully set without using the subdev API
> (or DT).
> 
> I'm not saying that it is not doable to solve this issue, but, right
> now, some parts at the V4L2 core assumes that subdev API is
> used if CONFIG_VIDEO_V4L2_SUBDEV_API is enabled.
> 
> See, for example, the drivers/media/i2c/mt9v011.c driver, with is 
> used by a USB bridge driver that doesn't expose subdev API.
> 
> On this driver, even the probe logic had to be different, as it has 
> to explicitly support platform data, as otherwise the sensor won't be
> properly initialized, and it won't work.
> 
> Frankly, I don't see an easy way to make a sensor driver that would
> be fully independent, as we would need to move all DT-specific
> stuff to be handled outside the sensors and have a common way for
> the V4L2 core to handle it, either as platform data or as DT,
> and calling subdev-specific logic to handle it depending on the
> case.
> 
> While we don't have the V4L2 fully abstracting the logic
> if a device has subdev API or not, we can't get rid of
> VIDEO_V4L2_SUBDEV_API.
> 
> 
> Thanks,
> Mauro
> 

Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API

[Mauro Carvalho Chehab]

Em Fri, 7 Dec 2018 14:27:48 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 12/07/2018 01:42 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 7 Dec 2018 12:47:24 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> On 12/07/2018 12:26 PM, Mauro Carvalho Chehab wrote:  
> >>> Em Fri, 7 Dec 2018 10:09:04 +0100
> >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>     
> >>>> This patch selects MEDIA_CONTROLLER for all camera, analog TV and
> >>>> digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically.
> >>>>
> >>>> This will allow us to simplify drivers that currently have to add
> >>>> #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API
> >>>> to their code, since now this will always be available.
> >>>>
> >>>> The original intent of allowing these to be configured by the
> >>>> user was (I think) to save a bit of memory.     
> >>>
> >>> No. The original intent was/is to be sure that adding the media
> >>> controller support won't be breaking existing working drivers.    
> >>
> >> That doesn't make sense. If enabling this option would break existing
> >> drivers, then something is really wrong, isn't it?  
> > 
> > It is the opposite: disabling it should not break any driver that don't
> > depend on them to work.
> >   
> >>  
> >>>     
> >>>> But as more and more
> >>>> drivers have a media controller and all regular distros already
> >>>> enable one or more of those drivers, the memory for the MC code is
> >>>> there anyway.
> >>>>
> >>>> Complexity has always been the bane of media drivers, so reducing
> >>>> complexity at the expense of a bit more memory (which is a rounding
> >>>> error compared to the amount of video buffer memory needed) is IMHO
> >>>> a good thing.
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>>> ---
> >>>> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> >>>> index 8add62a18293..56eb01cc8bb4 100644
> >>>> --- a/drivers/media/Kconfig
> >>>> +++ b/drivers/media/Kconfig
> >>>> @@ -31,6 +31,7 @@ comment "Multimedia core support"
> >>>>  #
> >>>>  config MEDIA_CAMERA_SUPPORT
> >>>>  	bool "Cameras/video grabbers support"
> >>>> +	select MEDIA_CONTROLLER
> >>>>  	---help---
> >>>>  	  Enable support for webcams and video grabbers.
> >>>>
> >>>> @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT
> >>>>
> >>>>  config MEDIA_ANALOG_TV_SUPPORT
> >>>>  	bool "Analog TV support"
> >>>> +	select MEDIA_CONTROLLER
> >>>>  	---help---
> >>>>  	  Enable analog TV support.
> >>>>
> >>>> @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT
> >>>>
> >>>>  config MEDIA_DIGITAL_TV_SUPPORT
> >>>>  	bool "Digital TV support"
> >>>> +	select MEDIA_CONTROLLER
> >>>>  	---help---
> >>>>  	  Enable digital TV support.    
> >>>
> >>> See my comments below.
> >>>     
> >>>>
> >>>> @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig"
> >>>>
> >>>>  config MEDIA_CONTROLLER
> >>>>  	bool "Media Controller API"
> >>>> -	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT
> >>>>  	---help---
> >>>>  	  Enable the media controller API used to query media devices internal
> >>>>  	  topology and configure it dynamically.    
> >>>
> >>> I have split comments with regards to it. Yeah, nowadays media controller
> >>> has becoming more important. Still, a lot of media drivers work fine
> >>> without them.
> >>>
> >>> Anyway, if we're willing to make it mandatory, better to just remove the
> >>> entire config option or to make it a silent one.     
> >>
> >> Well, that assumes that the media controller will only be used by media
> >> drivers, and not alsa or anyone else who wants to experiment with the MC.
> >>
> >> I won't object to making it silent (since it does reflect the current
> >> situation), but since this functionality is not actually specific to media
> >> drivers I think that is a good case to be made to allow it to be selected
> >> manually.
> >>  
> >>>     
> >>>> @@ -119,16 +121,11 @@ config VIDEO_DEV
> >>>>  	tristate
> >>>>  	depends on MEDIA_SUPPORT
> >>>>  	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT
> >>>> +	select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER
> >>>>  	default y
> >>>>
> >>>>  config VIDEO_V4L2_SUBDEV_API
> >>>> -	bool "V4L2 sub-device userspace API"
> >>>> -	depends on VIDEO_DEV && MEDIA_CONTROLLER
> >>>> -	---help---
> >>>> -	  Enables the V4L2 sub-device pad-level userspace API used to configure
> >>>> -	  video format, size and frame rate between hardware blocks.
> >>>> -
> >>>> -	  This API is mostly used by camera interfaces in embedded platforms.
> >>>> +	bool    
> >>>
> >>> NACK. 
> >>>
> >>> There is a very good reason why the subdev API is optional: there
> >>> are drivers that use camera sensors but are not MC-centric. On those,
> >>> the USB bridge driver is responsible to setup the subdevice. 
> >>>
> >>> This options helps to ensure that camera sensors used by such drivers
> >>> won't stop working because of the lack of the subdev-API.    
> >>
> >> But they won't stop working if this is enabled.  
> > 
> > That's not the issue. I've seen (and nacked) several patches breaking
> > drivers by assuming that all init would happen via subdev API.
> > 
> > By having this as an optional feature that can be disabled, developers
> > need to ensure that either the driver won't be built as a hole, if
> > no subdev API suport is enabled, or need to add the needed logic inside
> > the sub-device in order to support both cases.
> >   
> >> This option is used as
> >> a dependency by drivers that require this functionality, but enabling
> >> it will never break other drivers that don't need this. Those drivers
> >> simply won't use it.  
> > 
> > Not a 100% sure about that. There are some parts of the logic that seems
> > to assume that the device has subdev API and MC initialized.
> > 
> > See, for example:
> > 
> > 	static inline struct v4l2_mbus_framefmt
> > 	*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> > 				    struct v4l2_subdev_pad_config *cfg,
> > 				    unsigned int pad)
> > 	{
> > 		if (WARN_ON(pad >= sd->entity.num_pads))
> > 			pad = 0;
> > 		return &cfg[pad].try_fmt;
> > 	}
> > 
> > If the USB bridge driver doesn't use the media controller, the above
> > code will OOPS. See, for example, ov2659_get_fmt() logic.  
> 
> So if I have that USB bridge driver, and I also enable the V4L2_SUBDEV_API
> for another driver, then the USB bridge driver would crash?!
> 
> If that's the case, then this is really, really broken.

Yes.

> I always enable
> this option whenever I build the media drivers, and I have never seen
> anything break because of this. And if a driver would break then that
> is an enormous bug in that driver or the subdev driver.

Last time I checked, PC distros usually disable it. Not sure how many
devices are out there that use it. I carefully review the patches for the
devices I have myself and that I know it would be affected by this
issue.

> Please note that bridge drivers that do not rely on this config option
> will never call these subdev ops with V4L2_SUBDEV_FORMAT_TRY.
> 
> So it will also never crash on this.

Yes, USB bridges typically handles it on another way. That could be
a reason why we never received a bug report (the other reason is
because PC distros may not be enabling subdev API).

> Basically what you want is a way to check that bridge drivers that do
> not support the media controller or the subdev API (i.e. V4L2_SUBDEV_FORMAT_TRY)
> do not attempt to use features that rely on subdevs supporting it.

Yes. I also want sensor drivers to either be written considering that
they can be called by bridges that don't export subdev API or to
be explicitly tagged as dependent of V4L2_SUBDEV_API.

This way, if someone ever need to use those on a bridge driver
that doesn't export the subdev API, he will also be aware that
the sensor driver will require changes in order to work.

> I'm not sure that's possible, but let me think about it.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Ok, this particular driver (AFAIKT) is only used on platform drivers,
> > but if the same sensor would be used by another driver that don't
> > expose subdev API, VIDIOC_GET_FMT won't work. Also, if
> > CONFIG_VIDEO_V4L2_SUBDEV_API is disabled, the ioctl will just return
> > an error, but if it is enabled, it will OOPS.
> >   
> >> Also note that it is the bridge driver that controls whether or not
> >> the v4l-subdevX devices are created. If the bridge driver doesn't
> >> explicitly enable it AND the subdev driver explicitly supports it,
> >> those devices will not be created.  
> > 
> > The problem is not related to subdev creation. It is related to
> > having support for being fully set without using the subdev API
> > (or DT).
> > 
> > I'm not saying that it is not doable to solve this issue, but, right
> > now, some parts at the V4L2 core assumes that subdev API is
> > used if CONFIG_VIDEO_V4L2_SUBDEV_API is enabled.
> > 
> > See, for example, the drivers/media/i2c/mt9v011.c driver, with is 
> > used by a USB bridge driver that doesn't expose subdev API.
> > 
> > On this driver, even the probe logic had to be different, as it has 
> > to explicitly support platform data, as otherwise the sensor won't be
> > properly initialized, and it won't work.
> > 
> > Frankly, I don't see an easy way to make a sensor driver that would
> > be fully independent, as we would need to move all DT-specific
> > stuff to be handled outside the sensors and have a common way for
> > the V4L2 core to handle it, either as platform data or as DT,
> > and calling subdev-specific logic to handle it depending on the
> > case.
> > 
> > While we don't have the V4L2 fully abstracting the logic
> > if a device has subdev API or not, we can't get rid of
> > VIDEO_V4L2_SUBDEV_API.
> > 
> > 
> > Thanks,
> > Mauro
> >   
> 



Thanks,
Mauro

Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API

[Mauro Carvalho Chehab]

Em Fri, 7 Dec 2018 11:53:17 -0200
Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:

> Em Fri, 7 Dec 2018 14:27:48 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On 12/07/2018 01:42 PM, Mauro Carvalho Chehab wrote:
> > > Em Fri, 7 Dec 2018 12:47:24 +0100
> > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > >   
> > >> On 12/07/2018 12:26 PM, Mauro Carvalho Chehab wrote:  
> > >>> Em Fri, 7 Dec 2018 10:09:04 +0100
> > >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > >>>     
> > >>>> This patch selects MEDIA_CONTROLLER for all camera, analog TV and
> > >>>> digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically.
> > >>>>
> > >>>> This will allow us to simplify drivers that currently have to add
> > >>>> #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API
> > >>>> to their code, since now this will always be available.
> > >>>>
> > >>>> The original intent of allowing these to be configured by the
> > >>>> user was (I think) to save a bit of memory.     
> > >>>
> > >>> No. The original intent was/is to be sure that adding the media
> > >>> controller support won't be breaking existing working drivers.    
> > >>
> > >> That doesn't make sense. If enabling this option would break existing
> > >> drivers, then something is really wrong, isn't it?  
> > > 
> > > It is the opposite: disabling it should not break any driver that don't
> > > depend on them to work.
> > >   
> > >>  
> > >>>     
> > >>>> But as more and more
> > >>>> drivers have a media controller and all regular distros already
> > >>>> enable one or more of those drivers, the memory for the MC code is
> > >>>> there anyway.
> > >>>>
> > >>>> Complexity has always been the bane of media drivers, so reducing
> > >>>> complexity at the expense of a bit more memory (which is a rounding
> > >>>> error compared to the amount of video buffer memory needed) is IMHO
> > >>>> a good thing.
> > >>>>
> > >>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > >>>> ---
> > >>>> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> > >>>> index 8add62a18293..56eb01cc8bb4 100644
> > >>>> --- a/drivers/media/Kconfig
> > >>>> +++ b/drivers/media/Kconfig
> > >>>> @@ -31,6 +31,7 @@ comment "Multimedia core support"
> > >>>>  #
> > >>>>  config MEDIA_CAMERA_SUPPORT
> > >>>>  	bool "Cameras/video grabbers support"
> > >>>> +	select MEDIA_CONTROLLER
> > >>>>  	---help---
> > >>>>  	  Enable support for webcams and video grabbers.
> > >>>>
> > >>>> @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT
> > >>>>
> > >>>>  config MEDIA_ANALOG_TV_SUPPORT
> > >>>>  	bool "Analog TV support"
> > >>>> +	select MEDIA_CONTROLLER
> > >>>>  	---help---
> > >>>>  	  Enable analog TV support.
> > >>>>
> > >>>> @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT
> > >>>>
> > >>>>  config MEDIA_DIGITAL_TV_SUPPORT
> > >>>>  	bool "Digital TV support"
> > >>>> +	select MEDIA_CONTROLLER
> > >>>>  	---help---
> > >>>>  	  Enable digital TV support.    
> > >>>
> > >>> See my comments below.
> > >>>     
> > >>>>
> > >>>> @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig"
> > >>>>
> > >>>>  config MEDIA_CONTROLLER
> > >>>>  	bool "Media Controller API"
> > >>>> -	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT
> > >>>>  	---help---
> > >>>>  	  Enable the media controller API used to query media devices internal
> > >>>>  	  topology and configure it dynamically.    
> > >>>
> > >>> I have split comments with regards to it. Yeah, nowadays media controller
> > >>> has becoming more important. Still, a lot of media drivers work fine
> > >>> without them.
> > >>>
> > >>> Anyway, if we're willing to make it mandatory, better to just remove the
> > >>> entire config option or to make it a silent one.     
> > >>
> > >> Well, that assumes that the media controller will only be used by media
> > >> drivers, and not alsa or anyone else who wants to experiment with the MC.
> > >>
> > >> I won't object to making it silent (since it does reflect the current
> > >> situation), but since this functionality is not actually specific to media
> > >> drivers I think that is a good case to be made to allow it to be selected
> > >> manually.
> > >>  
> > >>>     
> > >>>> @@ -119,16 +121,11 @@ config VIDEO_DEV
> > >>>>  	tristate
> > >>>>  	depends on MEDIA_SUPPORT
> > >>>>  	depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT
> > >>>> +	select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER
> > >>>>  	default y
> > >>>>
> > >>>>  config VIDEO_V4L2_SUBDEV_API
> > >>>> -	bool "V4L2 sub-device userspace API"
> > >>>> -	depends on VIDEO_DEV && MEDIA_CONTROLLER
> > >>>> -	---help---
> > >>>> -	  Enables the V4L2 sub-device pad-level userspace API used to configure
> > >>>> -	  video format, size and frame rate between hardware blocks.
> > >>>> -
> > >>>> -	  This API is mostly used by camera interfaces in embedded platforms.
> > >>>> +	bool    
> > >>>
> > >>> NACK. 
> > >>>
> > >>> There is a very good reason why the subdev API is optional: there
> > >>> are drivers that use camera sensors but are not MC-centric. On those,
> > >>> the USB bridge driver is responsible to setup the subdevice. 
> > >>>
> > >>> This options helps to ensure that camera sensors used by such drivers
> > >>> won't stop working because of the lack of the subdev-API.    
> > >>
> > >> But they won't stop working if this is enabled.  
> > > 
> > > That's not the issue. I've seen (and nacked) several patches breaking
> > > drivers by assuming that all init would happen via subdev API.
> > > 
> > > By having this as an optional feature that can be disabled, developers
> > > need to ensure that either the driver won't be built as a hole, if
> > > no subdev API suport is enabled, or need to add the needed logic inside
> > > the sub-device in order to support both cases.
> > >   
> > >> This option is used as
> > >> a dependency by drivers that require this functionality, but enabling
> > >> it will never break other drivers that don't need this. Those drivers
> > >> simply won't use it.  
> > > 
> > > Not a 100% sure about that. There are some parts of the logic that seems
> > > to assume that the device has subdev API and MC initialized.
> > > 
> > > See, for example:
> > > 
> > > 	static inline struct v4l2_mbus_framefmt
> > > 	*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> > > 				    struct v4l2_subdev_pad_config *cfg,
> > > 				    unsigned int pad)
> > > 	{
> > > 		if (WARN_ON(pad >= sd->entity.num_pads))
> > > 			pad = 0;
> > > 		return &cfg[pad].try_fmt;
> > > 	}
> > > 
> > > If the USB bridge driver doesn't use the media controller, the above
> > > code will OOPS. See, for example, ov2659_get_fmt() logic.  
> > 
> > So if I have that USB bridge driver, and I also enable the V4L2_SUBDEV_API
> > for another driver, then the USB bridge driver would crash?!
> > 
> > If that's the case, then this is really, really broken.
> 
> Yes.
> 
> > I always enable
> > this option whenever I build the media drivers, and I have never seen
> > anything break because of this. And if a driver would break then that
> > is an enormous bug in that driver or the subdev driver.
> 
> Last time I checked, PC distros usually disable it. Not sure how many
> devices are out there that use it. I carefully review the patches for the
> devices I have myself and that I know it would be affected by this
> issue.
> 
> > Please note that bridge drivers that do not rely on this config option
> > will never call these subdev ops with V4L2_SUBDEV_FORMAT_TRY.
> > 
> > So it will also never crash on this.
> 
> Yes, USB bridges typically handles it on another way. That could be
> a reason why we never received a bug report (the other reason is
> because PC distros may not be enabling subdev API).
> 
> > Basically what you want is a way to check that bridge drivers that do
> > not support the media controller or the subdev API (i.e. V4L2_SUBDEV_FORMAT_TRY)
> > do not attempt to use features that rely on subdevs supporting it.
> 
> Yes. I also want sensor drivers to either be written considering that
> they can be called by bridges that don't export subdev API or to
> be explicitly tagged as dependent of V4L2_SUBDEV_API.
> 
> This way, if someone ever need to use those on a bridge driver
> that doesn't export the subdev API, he will also be aware that
> the sensor driver will require changes in order to work.

In time:

An alternative approach would be if the V4L2 core do all the
abstractions, but I don't think this is doable without spending
a lot of efforts on an abstraction that would be painful to write,
and probably won't worth the efforts.

The problem with such approach is that the V4L2 core should
need a way to implement a DT-like platform data handler that
the drivers would use at probing time, parsing the
i2c_driver::of_match_table internally at the core for drivers
that pass it via platform_data.

> 
> > I'm not sure that's possible, but let me think about it.
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > > 
> > > Ok, this particular driver (AFAIKT) is only used on platform drivers,
> > > but if the same sensor would be used by another driver that don't
> > > expose subdev API, VIDIOC_GET_FMT won't work. Also, if
> > > CONFIG_VIDEO_V4L2_SUBDEV_API is disabled, the ioctl will just return
> > > an error, but if it is enabled, it will OOPS.
> > >   
> > >> Also note that it is the bridge driver that controls whether or not
> > >> the v4l-subdevX devices are created. If the bridge driver doesn't
> > >> explicitly enable it AND the subdev driver explicitly supports it,
> > >> those devices will not be created.  
> > > 
> > > The problem is not related to subdev creation. It is related to
> > > having support for being fully set without using the subdev API
> > > (or DT).
> > > 
> > > I'm not saying that it is not doable to solve this issue, but, right
> > > now, some parts at the V4L2 core assumes that subdev API is
> > > used if CONFIG_VIDEO_V4L2_SUBDEV_API is enabled.
> > > 
> > > See, for example, the drivers/media/i2c/mt9v011.c driver, with is 
> > > used by a USB bridge driver that doesn't expose subdev API.
> > > 
> > > On this driver, even the probe logic had to be different, as it has 
> > > to explicitly support platform data, as otherwise the sensor won't be
> > > properly initialized, and it won't work.
> > > 
> > > Frankly, I don't see an easy way to make a sensor driver that would
> > > be fully independent, as we would need to move all DT-specific
> > > stuff to be handled outside the sensors and have a common way for
> > > the V4L2 core to handle it, either as platform data or as DT,
> > > and calling subdev-specific logic to handle it depending on the
> > > case.
> > > 
> > > While we don't have the V4L2 fully abstracting the logic
> > > if a device has subdev API or not, we can't get rid of
> > > VIDEO_V4L2_SUBDEV_API.
> > > 
> > > 
> > > Thanks,
> > > Mauro
> > >   
> > 
> 
> 
> 
> Thanks,
> Mauro



Thanks,
Mauro