Re: [PATCH v4 7/7] media: open.rst: add a notice about subdev-API on vdev-centric

[Mauro Carvalho Chehab]

Em Tue, 29 Aug 2017 10:39:52 +0200
Hans Verkuil  escreveu:

> On 29/08/17 10:31, Ramesh Shanmugasundaram wrote:
> > Hi Hans,
> >   
> >> On 28/08/17 12:30, Mauro Carvalho Chehab wrote:  
> >>> Em Mon, 28 Aug 2017 12:05:06 +0200
> >>> Hans Verkuil  escreveu:
> >>>  
>  On 26/08/17 13:53, Mauro Carvalho Chehab wrote:  
> > The documentation doesn't mention if vdev-centric hardware control
> > would have subdev API or not.
> >
> > Add a notice about that, reflecting the current status, where three
> > drivers use it, in order to support some subdev-specific controls.  
> 
>  I posted a patch removing v4l-subdevX support for cobalt. It's only
>  used within Cisco, so this is safe to do and won't break any userspace  
> >> support.  
> >>>
> >>> OK.
> >>>  
>  atmel-isc is another driver that creates subdev nodes. Like cobalt,
>  this is unnecessary. There are no sensors that use private controls.  
> >>>
> >>> The question is not if the driver has private controls. Private
> >>> controls can be V4L2 device node oriented.
> >>>
> >>> The real question is if userspace applications use subdevs or not in
> >>> order to set something specific to a subdev, on a pipeline where
> >>> multiple subdevs could use the same control.
> >>>
> >>> E. g. even on a simple case where the driver would have something like:
> >>>
> >>> sensor -> processing -> DMA
> >>>
> >>> both "sensor" and "processing" could provide the same control (bright,
> >>> contrast, gain, or whatever). Only by exposing such control via subdev
> >>> is possible to pinpoint what part of the hardware pipeline would be
> >>> affected when such control is changed.  
> >>
> >> In theory, yes. In practice this does not happen for any of the V4L2-
> >> centric drivers. Including for the three drivers under discussion.
> >>  
> >>>  
>  This driver is not referenced anywhere (dts or board file) in the  
> >> kernel.  
>  It is highly unlikely anyone would use v4l-subdevX nodes when there
>  is no need to do so. My suggestion is to add a kernel option for this
>  driver to enable v4l-subdevX support, but set it to 'default n'.
>  Perhaps with a note in the Kconfig description and a message in the
>  kernel log that this will be removed in the future.
> 
>  The final driver is rcar_drif that uses this to set the "I2S Enable"
>  private control of the max2175 driver.
> 
>  I remember that there was a long discussion over this control. I
>  still think that there is no need to mark this private.  
> >>>
> >>> The problem with I2S is that a device may have multiple places where
> >>> I2S could be used. I don't know how the rcar-drif driver uses it, but
> >>> there are several vdev-centric boards that use I2S for audio.
> >>>
> >>> On several of the devices I worked with, the I2S can be enabled, in
> >>> runtime, if the audio signal would be directed to some digital output,
> >>> or it can be disabled if the audio signal would be directed to some
> >>> analog output. Thankfully, on those devices, I2S can be indirectly
> >>> controlled via either an ALSA mixer or via VIDIOC A/V routing ioctls.
> >>> Also, there's just one I2S bus on them.
> >>>
> >>> However, on a device that have multiple I2S bus, userspace should be
> >>> able to control each of them individually, as some parts of the
> >>> pipeline may require it enabled while others may require it disabled.
> >>> So, I strongly believe that this should be a subdev control on such
> >>> hardware.
> >>>
> >>> That's said, I don't know how rcar_drif uses it. If it has just one
> >>> I2S bus and it is used only for audio, then VIDIOC A/V routing ioctls
> >>> and/or an ALSA mixer could replace it. If not, then it should be kept
> >>> as-is and the driver would need to add support for MC, in order for
> >>> applications to identify the right sub-devices that are associated
> >>> with the pipelines where I2S will be controlled.  
> >>
> >> Ramesh, do applications using rcar_drif + max2175 have to manually enable
> >> the i2s? Shouldn't this be part of the device tree description instead?
> >>  
> > 
> > Yes, applications have to control this explicitly. It is not only enable 
> > but also disable control is used at run time and hence DT is not 
> > applicable. 
> > 
> > rcar_drif has two registers to write to enable rx on two data pins. It 
> > expects a sequence where the master stops output (in this max2175 i2s 
> > output - disable) - enable rcar_drif rx and then the master starts output 
> > (max2175 i2s output - enable). The application ensures this sequence today. 
> > It is one I2S bus and it is not used for audio but raw I/Q samples from 
> > max2175 tuner. 
> > 
> > The v4l2_subdev_tuner_ops does not have .s_stream api as in 
> > v4l2_subdev_video_ops and v4l2_subdev_audio_ops. If we plan to have one 
> > this functionality may be hidden inside it and no need for an explicit 
> > 

Re: [PATCH v4 7/7] media: open.rst: add a notice about subdev-API on vdev-centric

[Hans Verkuil]

On 29/08/17 10:31, Ramesh Shanmugasundaram wrote:
> Hi Hans,
> 
>> On 28/08/17 12:30, Mauro Carvalho Chehab wrote:
>>> Em Mon, 28 Aug 2017 12:05:06 +0200
>>> Hans Verkuil  escreveu:
>>>
 On 26/08/17 13:53, Mauro Carvalho Chehab wrote:
> The documentation doesn't mention if vdev-centric hardware control
> would have subdev API or not.
>
> Add a notice about that, reflecting the current status, where three
> drivers use it, in order to support some subdev-specific controls.

 I posted a patch removing v4l-subdevX support for cobalt. It's only
 used within Cisco, so this is safe to do and won't break any userspace
>> support.
>>>
>>> OK.
>>>
 atmel-isc is another driver that creates subdev nodes. Like cobalt,
 this is unnecessary. There are no sensors that use private controls.
>>>
>>> The question is not if the driver has private controls. Private
>>> controls can be V4L2 device node oriented.
>>>
>>> The real question is if userspace applications use subdevs or not in
>>> order to set something specific to a subdev, on a pipeline where
>>> multiple subdevs could use the same control.
>>>
>>> E. g. even on a simple case where the driver would have something like:
>>>
>>> sensor -> processing -> DMA
>>>
>>> both "sensor" and "processing" could provide the same control (bright,
>>> contrast, gain, or whatever). Only by exposing such control via subdev
>>> is possible to pinpoint what part of the hardware pipeline would be
>>> affected when such control is changed.
>>
>> In theory, yes. In practice this does not happen for any of the V4L2-
>> centric drivers. Including for the three drivers under discussion.
>>
>>>
 This driver is not referenced anywhere (dts or board file) in the
>> kernel.
 It is highly unlikely anyone would use v4l-subdevX nodes when there
 is no need to do so. My suggestion is to add a kernel option for this
 driver to enable v4l-subdevX support, but set it to 'default n'.
 Perhaps with a note in the Kconfig description and a message in the
 kernel log that this will be removed in the future.

 The final driver is rcar_drif that uses this to set the "I2S Enable"
 private control of the max2175 driver.

 I remember that there was a long discussion over this control. I
 still think that there is no need to mark this private.
>>>
>>> The problem with I2S is that a device may have multiple places where
>>> I2S could be used. I don't know how the rcar-drif driver uses it, but
>>> there are several vdev-centric boards that use I2S for audio.
>>>
>>> On several of the devices I worked with, the I2S can be enabled, in
>>> runtime, if the audio signal would be directed to some digital output,
>>> or it can be disabled if the audio signal would be directed to some
>>> analog output. Thankfully, on those devices, I2S can be indirectly
>>> controlled via either an ALSA mixer or via VIDIOC A/V routing ioctls.
>>> Also, there's just one I2S bus on them.
>>>
>>> However, on a device that have multiple I2S bus, userspace should be
>>> able to control each of them individually, as some parts of the
>>> pipeline may require it enabled while others may require it disabled.
>>> So, I strongly believe that this should be a subdev control on such
>>> hardware.
>>>
>>> That's said, I don't know how rcar_drif uses it. If it has just one
>>> I2S bus and it is used only for audio, then VIDIOC A/V routing ioctls
>>> and/or an ALSA mixer could replace it. If not, then it should be kept
>>> as-is and the driver would need to add support for MC, in order for
>>> applications to identify the right sub-devices that are associated
>>> with the pipelines where I2S will be controlled.
>>
>> Ramesh, do applications using rcar_drif + max2175 have to manually enable
>> the i2s? Shouldn't this be part of the device tree description instead?
>>
> 
> Yes, applications have to control this explicitly. It is not only enable but 
> also disable control is used at run time and hence DT is not applicable. 
> 
> rcar_drif has two registers to write to enable rx on two data pins. It 
> expects a sequence where the master stops output (in this max2175 i2s output 
> - disable) - enable rcar_drif rx and then the master starts output (max2175 
> i2s output - enable). The application ensures this sequence today. It is one 
> I2S bus and it is not used for audio but raw I/Q samples from max2175 tuner. 
> 
> The v4l2_subdev_tuner_ops does not have .s_stream api as in 
> v4l2_subdev_video_ops and v4l2_subdev_audio_ops. If we plan to have one this 
> functionality may be hidden inside it and no need for an explicit control. I 
> too do not like a private control option.

I think it would be reasonable to use the audio ops s_stream for this. We're
streaming data after all. The audio ops most closely fits what we want to do.

All this is an internal API, so can be changed in the future if needed.

I like that a lot better than this weird 

RE: [PATCH v4 7/7] media: open.rst: add a notice about subdev-API on vdev-centric

[Ramesh Shanmugasundaram]

Hi Hans,

> On 28/08/17 12:30, Mauro Carvalho Chehab wrote:
> > Em Mon, 28 Aug 2017 12:05:06 +0200
> > Hans Verkuil  escreveu:
> >
> >> On 26/08/17 13:53, Mauro Carvalho Chehab wrote:
> >>> The documentation doesn't mention if vdev-centric hardware control
> >>> would have subdev API or not.
> >>>
> >>> Add a notice about that, reflecting the current status, where three
> >>> drivers use it, in order to support some subdev-specific controls.
> >>
> >> I posted a patch removing v4l-subdevX support for cobalt. It's only
> >> used within Cisco, so this is safe to do and won't break any userspace
> support.
> >
> > OK.
> >
> >> atmel-isc is another driver that creates subdev nodes. Like cobalt,
> >> this is unnecessary. There are no sensors that use private controls.
> >
> > The question is not if the driver has private controls. Private
> > controls can be V4L2 device node oriented.
> >
> > The real question is if userspace applications use subdevs or not in
> > order to set something specific to a subdev, on a pipeline where
> > multiple subdevs could use the same control.
> >
> > E. g. even on a simple case where the driver would have something like:
> >
> > sensor -> processing -> DMA
> >
> > both "sensor" and "processing" could provide the same control (bright,
> > contrast, gain, or whatever). Only by exposing such control via subdev
> > is possible to pinpoint what part of the hardware pipeline would be
> > affected when such control is changed.
> 
> In theory, yes. In practice this does not happen for any of the V4L2-
> centric drivers. Including for the three drivers under discussion.
> 
> >
> >> This driver is not referenced anywhere (dts or board file) in the
> kernel.
> >> It is highly unlikely anyone would use v4l-subdevX nodes when there
> >> is no need to do so. My suggestion is to add a kernel option for this
> >> driver to enable v4l-subdevX support, but set it to 'default n'.
> >> Perhaps with a note in the Kconfig description and a message in the
> >> kernel log that this will be removed in the future.
> >>
> >> The final driver is rcar_drif that uses this to set the "I2S Enable"
> >> private control of the max2175 driver.
> >>
> >> I remember that there was a long discussion over this control. I
> >> still think that there is no need to mark this private.
> >
> > The problem with I2S is that a device may have multiple places where
> > I2S could be used. I don't know how the rcar-drif driver uses it, but
> > there are several vdev-centric boards that use I2S for audio.
> >
> > On several of the devices I worked with, the I2S can be enabled, in
> > runtime, if the audio signal would be directed to some digital output,
> > or it can be disabled if the audio signal would be directed to some
> > analog output. Thankfully, on those devices, I2S can be indirectly
> > controlled via either an ALSA mixer or via VIDIOC A/V routing ioctls.
> > Also, there's just one I2S bus on them.
> >
> > However, on a device that have multiple I2S bus, userspace should be
> > able to control each of them individually, as some parts of the
> > pipeline may require it enabled while others may require it disabled.
> > So, I strongly believe that this should be a subdev control on such
> > hardware.
> >
> > That's said, I don't know how rcar_drif uses it. If it has just one
> > I2S bus and it is used only for audio, then VIDIOC A/V routing ioctls
> > and/or an ALSA mixer could replace it. If not, then it should be kept
> > as-is and the driver would need to add support for MC, in order for
> > applications to identify the right sub-devices that are associated
> > with the pipelines where I2S will be controlled.
> 
> Ramesh, do applications using rcar_drif + max2175 have to manually enable
> the i2s? Shouldn't this be part of the device tree description instead?
> 

Yes, applications have to control this explicitly. It is not only enable but 
also disable control is used at run time and hence DT is not applicable. 

rcar_drif has two registers to write to enable rx on two data pins. It expects 
a sequence where the master stops output (in this max2175 i2s output - disable) 
- enable rcar_drif rx and then the master starts output (max2175 i2s output - 
enable). The application ensures this sequence today. It is one I2S bus and it 
is not used for audio but raw I/Q samples from max2175 tuner. 

The v4l2_subdev_tuner_ops does not have .s_stream api as in 
v4l2_subdev_video_ops and v4l2_subdev_audio_ops. If we plan to have one this 
functionality may be hidden inside it and no need for an explicit control. I 
too do not like a private control option.

Thanks,
Ramesh

Re: [PATCH v4 7/7] media: open.rst: add a notice about subdev-API on vdev-centric

[Hans Verkuil]

On 28/08/17 12:30, Mauro Carvalho Chehab wrote:
> Em Mon, 28 Aug 2017 12:05:06 +0200
> Hans Verkuil  escreveu:
> 
>> On 26/08/17 13:53, Mauro Carvalho Chehab wrote:
>>> The documentation doesn't mention if vdev-centric hardware
>>> control would have subdev API or not.
>>>
>>> Add a notice about that, reflecting the current status, where
>>> three drivers use it, in order to support some subdev-specific
>>> controls.
>>
>> I posted a patch removing v4l-subdevX support for cobalt. It's only used
>> within Cisco, so this is safe to do and won't break any userspace support.
> 
> OK.
> 
>> atmel-isc is another driver that creates subdev nodes. Like cobalt, this
>> is unnecessary. There are no sensors that use private controls.
> 
> The question is not if the driver has private controls. Private controls
> can be V4L2 device node oriented.
> 
> The real question is if userspace applications use subdevs or not in
> order to set something specific to a subdev, on a pipeline where
> multiple subdevs could use the same control.
> 
> E. g. even on a simple case where the driver would have something like:
> 
> sensor -> processing -> DMA
> 
> both "sensor" and "processing" could provide the same control
> (bright, contrast, gain, or whatever). Only by exposing such 
> control via subdev is possible to pinpoint what part of the 
> hardware pipeline would be affected when such control is changed.

In theory, yes. In practice this does not happen for any of the
V4L2-centric drivers. Including for the three drivers under discussion.

> 
>> This driver is not referenced anywhere (dts or board file) in the kernel.
>> It is highly unlikely anyone would use v4l-subdevX nodes when there is no
>> need to do so. My suggestion is to add a kernel option for this driver
>> to enable v4l-subdevX support, but set it to 'default n'. Perhaps with
>> a note in the Kconfig description and a message in the kernel log that
>> this will be removed in the future.
>>
>> The final driver is rcar_drif that uses this to set the "I2S Enable"
>> private control of the max2175 driver.
>>
>> I remember that there was a long discussion over this control. I still
>> think that there is no need to mark this private. 
> 
> The problem with I2S is that a device may have multiple places
> where I2S could be used. I don't know how the rcar-drif driver uses
> it, but there are several vdev-centric boards that use I2S for audio.
> 
> On several of the devices I worked with, the I2S can be enabled, in
> runtime, if the audio signal would be directed to some digital output,
> or it can be disabled if the audio signal would be directed to some
> analog output. Thankfully, on those devices, I2S can be indirectly
> controlled via either an ALSA mixer or via VIDIOC A/V routing
> ioctls. Also, there's just one I2S bus on them.
> 
> However, on a device that have multiple I2S bus, userspace should
> be able to control each of them individually, as some parts of the
> pipeline may require it enabled while others may require it
> disabled. So, I strongly believe that this should be a subdev
> control on such hardware.
> 
> That's said, I don't know how rcar_drif uses it. If it has just
> one I2S bus and it is used only for audio, then VIDIOC A/V routing
> ioctls and/or an ALSA mixer could replace it. If not, then
> it should be kept as-is and the driver would need to add support
> for MC, in order for applications to identify the right
> sub-devices that are associated with the pipelines where I2S
> will be controlled.

Ramesh, do applications using rcar_drif + max2175 have to manually
enable the i2s? Shouldn't this be part of the device tree description
instead?

Regards,

Hans

Re: [PATCH v4 7/7] media: open.rst: add a notice about subdev-API on vdev-centric

[Mauro Carvalho Chehab]

Em Mon, 28 Aug 2017 12:05:06 +0200
Hans Verkuil  escreveu:

> On 26/08/17 13:53, Mauro Carvalho Chehab wrote:
> > The documentation doesn't mention if vdev-centric hardware
> > control would have subdev API or not.
> > 
> > Add a notice about that, reflecting the current status, where
> > three drivers use it, in order to support some subdev-specific
> > controls.
> 
> I posted a patch removing v4l-subdevX support for cobalt. It's only used
> within Cisco, so this is safe to do and won't break any userspace support.

OK.

> atmel-isc is another driver that creates subdev nodes. Like cobalt, this
> is unnecessary. There are no sensors that use private controls.

The question is not if the driver has private controls. Private controls
can be V4L2 device node oriented.

The real question is if userspace applications use subdevs or not in
order to set something specific to a subdev, on a pipeline where
multiple subdevs could use the same control.

E. g. even on a simple case where the driver would have something like:

sensor -> processing -> DMA

both "sensor" and "processing" could provide the same control
(bright, contrast, gain, or whatever). Only by exposing such 
control via subdev is possible to pinpoint what part of the 
hardware pipeline would be affected when such control is changed.

> This driver is not referenced anywhere (dts or board file) in the kernel.
> It is highly unlikely anyone would use v4l-subdevX nodes when there is no
> need to do so. My suggestion is to add a kernel option for this driver
> to enable v4l-subdevX support, but set it to 'default n'. Perhaps with
> a note in the Kconfig description and a message in the kernel log that
> this will be removed in the future.
> 
> The final driver is rcar_drif that uses this to set the "I2S Enable"
> private control of the max2175 driver.
> 
> I remember that there was a long discussion over this control. I still
> think that there is no need to mark this private. 

The problem with I2S is that a device may have multiple places
where I2S could be used. I don't know how the rcar-drif driver uses
it, but there are several vdev-centric boards that use I2S for audio.

On several of the devices I worked with, the I2S can be enabled, in
runtime, if the audio signal would be directed to some digital output,
or it can be disabled if the audio signal would be directed to some
analog output. Thankfully, on those devices, I2S can be indirectly
controlled via either an ALSA mixer or via VIDIOC A/V routing
ioctls. Also, there's just one I2S bus on them.

However, on a device that have multiple I2S bus, userspace should
be able to control each of them individually, as some parts of the
pipeline may require it enabled while others may require it
disabled. So, I strongly believe that this should be a subdev
control on such hardware.

That's said, I don't know how rcar_drif uses it. If it has just
one I2S bus and it is used only for audio, then VIDIOC A/V routing
ioctls and/or an ALSA mixer could replace it. If not, then
it should be kept as-is and the driver would need to add support
for MC, in order for applications to identify the right
sub-devices that are associated with the pipelines where I2S
will be controlled.

Thanks,
Mauro

Re: [PATCH v4 7/7] media: open.rst: add a notice about subdev-API on vdev-centric

[Hans Verkuil]

On 26/08/17 13:53, Mauro Carvalho Chehab wrote:
> The documentation doesn't mention if vdev-centric hardware
> control would have subdev API or not.
> 
> Add a notice about that, reflecting the current status, where
> three drivers use it, in order to support some subdev-specific
> controls.

I posted a patch removing v4l-subdevX support for cobalt. It's only used
within Cisco, so this is safe to do and won't break any userspace support.

atmel-isc is another driver that creates subdev nodes. Like cobalt, this
is unnecessary. There are no sensors that use private controls.

This driver is not referenced anywhere (dts or board file) in the kernel.
It is highly unlikely anyone would use v4l-subdevX nodes when there is no
need to do so. My suggestion is to add a kernel option for this driver
to enable v4l-subdevX support, but set it to 'default n'. Perhaps with
a note in the Kconfig description and a message in the kernel log that
this will be removed in the future.

The final driver is rcar_drif that uses this to set the "I2S Enable"
private control of the max2175 driver.

I remember that there was a long discussion over this control. I still
think that there is no need to mark this private. This is a recent
driver addition and we can get away with changing this, possibly using
a Kconfig option as well as discussed for the atmel-isc driver.

This is actually the only driver using a private control. I am of the
opinion that private controls were a mistake. I think it is the
bridge driver that has to decide whether or not to make subdev controls
available through a video node.

So in summary:

- drop is_private controls
- apply the cobalt patch (safe to do)
- add a Kconfig option for atmel-isc & rcar_drif that has to be explicitly
  enabled to support subdev nodes, and add logging that this is deprecated.
- by 4.17 or so remove this altogether.

If we agree to this, then this patch can be dropped.

Regards,

Hans

> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  Documentation/media/uapi/v4l/open.rst | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/open.rst 
> b/Documentation/media/uapi/v4l/open.rst
> index d0930fc170f0..48f628bbabc7 100644
> --- a/Documentation/media/uapi/v4l/open.rst
> +++ b/Documentation/media/uapi/v4l/open.rst
> @@ -46,6 +46,13 @@ the periferal can be used. For such devices, the 
> sub-devices' configuration
>  can be controlled via the :ref:`sub-device API `, which creates one
>  device node per sub-device.
>  
> +.. note::
> +
> +   A **vdev-centric** may also optionally expose V4L2 sub-devices via
> +   :ref:`sub-device API `. In that case, it has to implement
> +   the :ref:`media controller API ` as well.
> +
> +
>  .. attention::
>  
> Devices that require **mc-centric** hardware peripheral control should
>