Em Fri, 7 Dec 2018 14:27:48 +0100
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> On 12/07/2018 01:42 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 7 Dec 2018 12:47:24 +0100
> > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> >
> >> On 12/07/2018 12:26 PM, Mauro Carvalho Chehab wrote:
> >>> Em Fri, 7 Dec 2018 10:09:04 +0100
> >>> Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxxxx>
> >>>> ---
> >>>> 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
