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

[Mauro Carvalho Chehab <mchehab@kernel.org>]

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