[PATCH v2 0/2] Document s_stream video op calling (MC only) and CSI-2 stream stopping

[PATCH v2 0/2] Document s_stream video op calling (MC only) and CSI-2 stream stopping

[Sakari Ailus]

Hi folks,

I've updated the patch documenting the s_stream() video op calling for MC
enabled devices based on the review comments.

since v1:

- Split "stopping the transmitter" documentation to a separate patch and move it to
  csi2.rst which is a better place for it.

- Precise that the added s_stream() video op documentation only applies to
  Media controller enabled devices.

- Better wording for the note which discourages deep recursion in pipeline
  start / stop.

Sakari Ailus (2):
  docs-rst: media: Document s_stream() video op usage for MC enabled
    devices
  docs-rst: media: Document broken frame handling in stream stop for
    CSI-2

 Documentation/media/kapi/csi2.rst        | 10 ++++++++++
 Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

-- 
2.7.4

[PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Sakari Ailus]

As we begin to add support for systems with Media controller pipelines
controlled by more than one device driver, it is essential that we
precisely define the responsibilities of each component in the stream
control and common practices.

Specifically, streaming control is done per sub-device and sub-device
drivers themselves are responsible for streaming setup in upstream
sub-devices.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
index e1f0b72..45088ad 100644
--- a/Documentation/media/kapi/v4l2-subdev.rst
+++ b/Documentation/media/kapi/v4l2-subdev.rst
@@ -262,6 +262,35 @@ is called. After all subdevices have been located the .complete() callback is
 called. When a subdevice is removed from the system the .unbind() method is
 called. All three callbacks are optional.
 
+Streaming control on Media controller enabled devices
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Starting and stopping the stream are somewhat complex operations that
+often require walking the media graph to enable streaming on
+sub-devices which the pipeline consists of. This involves interaction
+between multiple drivers, sometimes more than two.
+
+The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
+for starting and stopping the stream on the sub-device it is called
+on. A device driver is only responsible for calling the ``.s_stream()`` ops
+of the adjacent sub-devices that are connected to its sink pads
+through an enabled link. A driver may not call ``.s_stream()`` op
+of any other sub-device further up in the pipeline, for instance.
+
+This means that a sub-device driver is thus in direct control of
+whether the upstream sub-devices start (or stop) streaming before or
+after the sub-device itself is set up for streaming.
+
+.. note::
+
+   As the ``.s_stream()`` callback is called recursively through the
+   sub-devices along the pipeline, it is important to keep the
+   recursion as short as possible. To this end, drivers are encouraged
+   to avoid recursively calling ``.s_stream()`` internally to reduce
+   stack usage. Instead, the ``.s_stream()`` op of the directly
+   connected sub-devices should come from the callback through which
+   the driver was first called.
+
 V4L2 sub-device userspace API
 -----------------------------
 
-- 
2.7.4

[PATCH v2 2/2] docs-rst: media: Document broken frame handling in stream stop for CSI-2

[Sakari Ailus]

Some CSI-2 transmitters will finish an ongoing frame whereas others will
not. Document that receiver drivers should not assume a particular
behaviour but to work in both cases.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/media/kapi/csi2.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/media/kapi/csi2.rst b/Documentation/media/kapi/csi2.rst
index e33fcb9..0560100 100644
--- a/Documentation/media/kapi/csi2.rst
+++ b/Documentation/media/kapi/csi2.rst
@@ -51,6 +51,16 @@ not active. Some transmitters do this automatically but some have to
 be explicitly programmed to do so, and some are unable to do so
 altogether due to hardware constraints.
 
+Stopping the transmitter
+^^^^^^^^^^^^^^^^^^^^^^^^
+
+A transmitter stops sending the stream of images as a result of
+calling the ``.s_stream()`` callback. Some transmitters may stop the
+stream at a frame boundary whereas others stop immediately,
+effectively leaving the current frame unfinished. The receiver driver
+should not make assumptions either way, but function properly in both
+cases.
+
 Receiver drivers
 ----------------
 
-- 
2.7.4

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Mauro Carvalho Chehab]

Hi Sakari,

Em Wed, 16 Aug 2017 15:20:17 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> As we begin to add support for systems with Media controller pipelines
> controlled by more than one device driver, it is essential that we
> precisely define the responsibilities of each component in the stream
> control and common practices.
> 
> Specifically, streaming control is done per sub-device and sub-device
> drivers themselves are responsible for streaming setup in upstream
> sub-devices.

IMO, before this patch, we need something like this:
	https://patchwork.linuxtv.org/patch/43325/

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
> index e1f0b72..45088ad 100644
> --- a/Documentation/media/kapi/v4l2-subdev.rst
> +++ b/Documentation/media/kapi/v4l2-subdev.rst
> @@ -262,6 +262,35 @@ is called. After all subdevices have been located the .complete() callback is
>  called. When a subdevice is removed from the system the .unbind() method is
>  called. All three callbacks are optional.
>  
> +Streaming control on Media controller enabled devices
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Starting and stopping the stream are somewhat complex operations that
> +often require walking the media graph to enable streaming on
> +sub-devices which the pipeline consists of. This involves interaction
> +between multiple drivers, sometimes more than two.

That's still not ok, as it creates a black hole for devnode-based
devices.

I would change it to something like:

	Streaming control
	^^^^^^^^^^^^^^^^^

	Starting and stopping the stream are somewhat complex operations that
	often require to enable streaming on sub-devices which the pipeline 
	consists of. This involves interaction between multiple drivers, sometimes
	more than two. 

	The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
	for starting and stopping the stream on the sub-device it is called
	on. 

	Streaming control on devnode-centric devices
	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

	On **devnode-centric** devices, the main driver is responsible enable
	stream all all sub-devices. On most cases, all the main driver need
	to do is to broadcast s_stream to all connected sub-devices by calling
	:c:func:`v4l2_device_call_all`, e. g.::

		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 1);

	Streaming control on mc-centric devices
	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

	On **mc-centric** devices, it usually requires walking the media graph
	to enable streaming only at the sub-devices which the pipeline consists
	of.

	(place here the details for such scenario)

> +The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> +for starting and stopping the stream on the sub-device it is called
> +on. A device driver is only responsible for calling the ``.s_stream()`` ops
> +of the adjacent sub-devices that are connected to its sink pads
> +through an enabled link. A driver may not call ``.s_stream()`` op
> +of any other sub-device further up in the pipeline, for instance.
> +
> +This means that a sub-device driver is thus in direct control of
> +whether the upstream sub-devices start (or stop) streaming before or
> +after the sub-device itself is set up for streaming.
> +
> +.. note::
> +
> +   As the ``.s_stream()`` callback is called recursively through the
> +   sub-devices along the pipeline, it is important to keep the
> +   recursion as short as possible. To this end, drivers are encouraged
> +   to avoid recursively calling ``.s_stream()`` internally to reduce
> +   stack usage. Instead, the ``.s_stream()`` op of the directly
> +   connected sub-devices should come from the callback through which
> +   the driver was first called.
> +

That sounds too complex, and can lead into troubles, if the same
sub-device driver is used on completely different devices.

IMHO, it should be up to the main driver to navigate at the MC
pipeline and call s_stream(), and not to the sub-drivers.

Thanks,
Mauro

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Mauro Carvalho Chehab]

Em Sat, 19 Aug 2017 07:35:52 -0300
Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:

> Hi Sakari,
> 
> Em Wed, 16 Aug 2017 15:20:17 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > As we begin to add support for systems with Media controller pipelines
> > controlled by more than one device driver, it is essential that we
> > precisely define the responsibilities of each component in the stream
> > control and common practices.
> > 
> > Specifically, streaming control is done per sub-device and sub-device
> > drivers themselves are responsible for streaming setup in upstream
> > sub-devices.  
> 
> IMO, before this patch, we need something like this:
> 	https://patchwork.linuxtv.org/patch/43325/
> 
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
> > index e1f0b72..45088ad 100644
> > --- a/Documentation/media/kapi/v4l2-subdev.rst
> > +++ b/Documentation/media/kapi/v4l2-subdev.rst
> > @@ -262,6 +262,35 @@ is called. After all subdevices have been located the .complete() callback is
> >  called. When a subdevice is removed from the system the .unbind() method is
> >  called. All three callbacks are optional.
> >  
> > +Streaming control on Media controller enabled devices
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +Starting and stopping the stream are somewhat complex operations that
> > +often require walking the media graph to enable streaming on
> > +sub-devices which the pipeline consists of. This involves interaction
> > +between multiple drivers, sometimes more than two.  
> 
> That's still not ok, as it creates a black hole for devnode-based
> devices.
> 
> I would change it to something like:
> 
> 	Streaming control
> 	^^^^^^^^^^^^^^^^^
> 
> 	Starting and stopping the stream are somewhat complex operations that
> 	often require to enable streaming on sub-devices which the pipeline 
> 	consists of. This involves interaction between multiple drivers, sometimes
> 	more than two. 
> 
> 	The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> 	for starting and stopping the stream on the sub-device it is called
> 	on. 
> 
> 	Streaming control on devnode-centric devices
> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> 	On **devnode-centric** devices, the main driver is responsible enable
> 	stream all all sub-devices. On most cases, all the main driver need
> 	to do is to broadcast s_stream to all connected sub-devices by calling
> 	:c:func:`v4l2_device_call_all`, e. g.::
> 
> 		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 1);
> 
> 	Streaming control on mc-centric devices
> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> 	On **mc-centric** devices, it usually requires walking the media graph
> 	to enable streaming only at the sub-devices which the pipeline consists
> 	of.
> 
> 	(place here the details for such scenario)
> 
> > +The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> > +for starting and stopping the stream on the sub-device it is called
> > +on. A device driver is only responsible for calling the ``.s_stream()`` ops
> > +of the adjacent sub-devices that are connected to its sink pads
> > +through an enabled link. A driver may not call ``.s_stream()`` op
> > +of any other sub-device further up in the pipeline, for instance.
> > +
> > +This means that a sub-device driver is thus in direct control of
> > +whether the upstream sub-devices start (or stop) streaming before or
> > +after the sub-device itself is set up for streaming.
> > +
> > +.. note::
> > +
> > +   As the ``.s_stream()`` callback is called recursively through the
> > +   sub-devices along the pipeline, it is important to keep the
> > +   recursion as short as possible. To this end, drivers are encouraged
> > +   to avoid recursively calling ``.s_stream()`` internally to reduce
> > +   stack usage. Instead, the ``.s_stream()`` op of the directly
> > +   connected sub-devices should come from the callback through which
> > +   the driver was first called.
> > +  
> 
> That sounds too complex, and can lead into troubles, if the same
> sub-device driver is used on completely different devices.
> 
> IMHO, it should be up to the main driver to navigate at the MC
> pipeline and call s_stream(), and not to the sub-drivers.

Forgot to mention, but, IMHO, we should also have a macro like 
v4l2_device_call_all() that would implement the "multicast" logic
to send a command from the main driver through all sub-devices
that are currently part of the streaming pipeline.

> 
> Thanks,
> Mauro



Thanks,
Mauro

Re: [PATCH v2 2/2] docs-rst: media: Document broken frame handling in stream stop for CSI-2

[Mauro Carvalho Chehab]

Em Wed, 16 Aug 2017 15:20:18 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Some CSI-2 transmitters will finish an ongoing frame whereas others will
> not. Document that receiver drivers should not assume a particular
> behaviour but to work in both cases.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/media/kapi/csi2.rst | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/media/kapi/csi2.rst b/Documentation/media/kapi/csi2.rst
> index e33fcb9..0560100 100644
> --- a/Documentation/media/kapi/csi2.rst
> +++ b/Documentation/media/kapi/csi2.rst
> @@ -51,6 +51,16 @@ not active. Some transmitters do this automatically but some have to
>  be explicitly programmed to do so, and some are unable to do so
>  altogether due to hardware constraints.
>  
> +Stopping the transmitter
> +^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +A transmitter stops sending the stream of images as a result of
> +calling the ``.s_stream()`` callback. Some transmitters may stop the
> +stream at a frame boundary whereas others stop immediately,
> +effectively leaving the current frame unfinished. The receiver driver
> +should not make assumptions either way, but function properly in both
> +cases.
> +
>  Receiver drivers
>  ----------------
>  

Reviewed-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Thanks,
Mauro

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Sakari Ailus]

Hi Mauro,

Mauro Carvalho Chehab wrote:
> Hi Sakari,
>
> Em Wed, 16 Aug 2017 15:20:17 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
>
>> As we begin to add support for systems with Media controller pipelines
>> controlled by more than one device driver, it is essential that we
>> precisely define the responsibilities of each component in the stream
>> control and common practices.
>>
>> Specifically, streaming control is done per sub-device and sub-device
>> drivers themselves are responsible for streaming setup in upstream
>> sub-devices.
>
> IMO, before this patch, we need something like this:
> 	https://patchwork.linuxtv.org/patch/43325/

Thanks. I'll reply separately to that thread.

>
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> ---
>>  Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
>> index e1f0b72..45088ad 100644
>> --- a/Documentation/media/kapi/v4l2-subdev.rst
>> +++ b/Documentation/media/kapi/v4l2-subdev.rst
>> @@ -262,6 +262,35 @@ is called. After all subdevices have been located the .complete() callback is
>>  called. When a subdevice is removed from the system the .unbind() method is
>>  called. All three callbacks are optional.
>>
>> +Streaming control on Media controller enabled devices
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +Starting and stopping the stream are somewhat complex operations that
>> +often require walking the media graph to enable streaming on
>> +sub-devices which the pipeline consists of. This involves interaction
>> +between multiple drivers, sometimes more than two.
>
> That's still not ok, as it creates a black hole for devnode-based
> devices.
>
> I would change it to something like:
>
> 	Streaming control
> 	^^^^^^^^^^^^^^^^^
>
> 	Starting and stopping the stream are somewhat complex operations that
> 	often require to enable streaming on sub-devices which the pipeline
> 	consists of. This involves interaction between multiple drivers, sometimes
> 	more than two.
>
> 	The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> 	for starting and stopping the stream on the sub-device it is called
> 	on.
>
> 	Streaming control on devnode-centric devices
> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> 	On **devnode-centric** devices, the main driver is responsible enable
> 	stream all all sub-devices. On most cases, all the main driver need
> 	to do is to broadcast s_stream to all connected sub-devices by calling
> 	:c:func:`v4l2_device_call_all`, e. g.::
>
> 		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 1);

Looks good to me.

>
> 	Streaming control on mc-centric devices
> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> 	On **mc-centric** devices, it usually requires walking the media graph
> 	to enable streaming only at the sub-devices which the pipeline consists
> 	of.
>
> 	(place here the details for such scenario)

This part requires a more detailed description of the problem area. What 
makes a difference here is that there's a pipeline this pipeline may be 
controlled more than one driver. (More elaborate discussion below.)

>
>> +The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
>> +for starting and stopping the stream on the sub-device it is called
>> +on. A device driver is only responsible for calling the ``.s_stream()`` ops
>> +of the adjacent sub-devices that are connected to its sink pads
>> +through an enabled link. A driver may not call ``.s_stream()`` op
>> +of any other sub-device further up in the pipeline, for instance.
>> +
>> +This means that a sub-device driver is thus in direct control of
>> +whether the upstream sub-devices start (or stop) streaming before or
>> +after the sub-device itself is set up for streaming.
>> +
>> +.. note::
>> +
>> +   As the ``.s_stream()`` callback is called recursively through the
>> +   sub-devices along the pipeline, it is important to keep the
>> +   recursion as short as possible. To this end, drivers are encouraged
>> +   to avoid recursively calling ``.s_stream()`` internally to reduce
>> +   stack usage. Instead, the ``.s_stream()`` op of the directly
>> +   connected sub-devices should come from the callback through which
>> +   the driver was first called.
>> +
>
> That sounds too complex, and can lead into troubles, if the same
> sub-device driver is used on completely different devices.
>
> IMHO, it should be up to the main driver to navigate at the MC
> pipeline and call s_stream(), and not to the sub-drivers.

I would agree with the above statement *if* we had no devices that 
require doing this in a different way.

Consider the following case:

	sensor   -> CSI-2 receiver -> ISP (DMA)
	subdev A -> subdev B	   -> video node

Assume that the CSI-2 receiver requires hardware setup both *before and 
after* streaming has been enabled on the sensor.

In previous cases the CSI-2 receiver and the ISP have been part of the 
same device. This is not universally the case anymore. You'll also get 
the same when you add adv748x to the pipeline, and the upstream device 
in the pipeline before the adv748x is represented as a sub-device (and 
is thus controlled by its sub-device driver).

This is addressable by moving the control of the upstream sub-device 
streaming state to the driver which is next to it, and what the patch 
also documents. Note that there is no difference if you have a 
sub-device without sink pads (or in general case, a sub-device that only 
has one kind of pads).

What comes to compatibility between MC-centric and devnode-centric 
drivers --- on an MC-centric device you have a pipeline and you 
typically have multiple pads on the sub-devices along the pipeline. 
Drivers that are devnode-centric generally aren't aware of pads, and so 
cannot configure sub-devices with multiple pads to begin with.

You could do that in principle, but you'll start running into the same 
problems which were addressed by introducing the Media controller interface.

Note that this is not a commonplace. The difference will be only be 
there *if and only if* you have a sub-device with sink pads in a 
pipeline. There are not many of those, and that difference is not 
introduced by this patch: it is a property of hardware.

I'm definitely open to improving the wording as well as other solutions 
that can address this.

Cc Niklas.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Mauro Carvalho Chehab]

Em Mon, 21 Aug 2017 09:36:49 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> Mauro Carvalho Chehab wrote:
> > Hi Sakari,
> >
> > Em Wed, 16 Aug 2017 15:20:17 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >  
> >> As we begin to add support for systems with Media controller pipelines
> >> controlled by more than one device driver, it is essential that we
> >> precisely define the responsibilities of each component in the stream
> >> control and common practices.
> >>
> >> Specifically, streaming control is done per sub-device and sub-device
> >> drivers themselves are responsible for streaming setup in upstream
> >> sub-devices.  
> >
> > IMO, before this patch, we need something like this:
> > 	https://patchwork.linuxtv.org/patch/43325/  
> 
> Thanks. I'll reply separately to that thread.
> 
> >  
> >>
> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> ---
> >>  Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
> >>  1 file changed, 29 insertions(+)
> >>
> >> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
> >> index e1f0b72..45088ad 100644
> >> --- a/Documentation/media/kapi/v4l2-subdev.rst
> >> +++ b/Documentation/media/kapi/v4l2-subdev.rst
> >> @@ -262,6 +262,35 @@ is called. After all subdevices have been located the .complete() callback is
> >>  called. When a subdevice is removed from the system the .unbind() method is
> >>  called. All three callbacks are optional.
> >>
> >> +Streaming control on Media controller enabled devices
> >> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> +
> >> +Starting and stopping the stream are somewhat complex operations that
> >> +often require walking the media graph to enable streaming on
> >> +sub-devices which the pipeline consists of. This involves interaction
> >> +between multiple drivers, sometimes more than two.  
> >
> > That's still not ok, as it creates a black hole for devnode-based
> > devices.
> >
> > I would change it to something like:
> >
> > 	Streaming control
> > 	^^^^^^^^^^^^^^^^^
> >
> > 	Starting and stopping the stream are somewhat complex operations that
> > 	often require to enable streaming on sub-devices which the pipeline
> > 	consists of. This involves interaction between multiple drivers, sometimes
> > 	more than two.
> >
> > 	The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> > 	for starting and stopping the stream on the sub-device it is called
> > 	on.
> >
> > 	Streaming control on devnode-centric devices
> > 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > 	On **devnode-centric** devices, the main driver is responsible enable
> > 	stream all all sub-devices. On most cases, all the main driver need
> > 	to do is to broadcast s_stream to all connected sub-devices by calling
> > 	:c:func:`v4l2_device_call_all`, e. g.::
> >
> > 		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 1);  
> 
> Looks good to me.
> 
> >
> > 	Streaming control on mc-centric devices
> > 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > 	On **mc-centric** devices, it usually requires walking the media graph
> > 	to enable streaming only at the sub-devices which the pipeline consists
> > 	of.
> >
> > 	(place here the details for such scenario)  
> 
> This part requires a more detailed description of the problem area. What 
> makes a difference here is that there's a pipeline this pipeline may be 
> controlled more than one driver. (More elaborate discussion below.)
> 
> >  
> >> +The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> >> +for starting and stopping the stream on the sub-device it is called
> >> +on. A device driver is only responsible for calling the ``.s_stream()`` ops
> >> +of the adjacent sub-devices that are connected to its sink pads
> >> +through an enabled link. A driver may not call ``.s_stream()`` op
> >> +of any other sub-device further up in the pipeline, for instance.
> >> +
> >> +This means that a sub-device driver is thus in direct control of
> >> +whether the upstream sub-devices start (or stop) streaming before or
> >> +after the sub-device itself is set up for streaming.
> >> +
> >> +.. note::
> >> +
> >> +   As the ``.s_stream()`` callback is called recursively through the
> >> +   sub-devices along the pipeline, it is important to keep the
> >> +   recursion as short as possible. To this end, drivers are encouraged
> >> +   to avoid recursively calling ``.s_stream()`` internally to reduce
> >> +   stack usage. Instead, the ``.s_stream()`` op of the directly
> >> +   connected sub-devices should come from the callback through which
> >> +   the driver was first called.
> >> +  
> >
> > That sounds too complex, and can lead into troubles, if the same
> > sub-device driver is used on completely different devices.
> >
> > IMHO, it should be up to the main driver to navigate at the MC
> > pipeline and call s_stream(), and not to the sub-drivers.  
> 
> I would agree with the above statement *if* we had no devices that 
> require doing this in a different way.
> 
> Consider the following case:
> 
> 	sensor   -> CSI-2 receiver -> ISP (DMA)
> 	subdev A -> subdev B	   -> video node

Let me be clearer about the issue I see.

In the above example, what subdevs are supposed to multicast the
s_stream() to their neighbors, and how they will know that they
need to multicast it.

Let's say, that, in the first pipeline, it would be the sensor
and subdev A. How "sensor" and "subdev A" will know that they're
meant to broadcast s_stream(), and the other entities know they
won't?

Also, the same sensor may be used on a device whose CSI-2 is
integrated at the ISP driver (the main driver). That's why
I think that such logic should be started by the main driver, as
it is the only part of the pipeline that it is aware about
what it is needed. Also, as the DMA engines are controlled by
the main driver (via its associated video devnodes), it is the only
part of the pipeline that knows when a stream starts.

> Assume that the CSI-2 receiver requires hardware setup both *before and 
> after* streaming has been enabled on the sensor.

calling s_stream() before and after seems to be an abuse of it.

This callback is meant to be called just once, and there's no
requirement if it should be called before or after: both should
work.

> 
> In previous cases the CSI-2 receiver and the ISP have been part of the 
> same device. This is not universally the case anymore. You'll also get 
> the same when you add adv748x to the pipeline, and the upstream device 
> in the pipeline before the adv748x is represented as a sub-device (and 
> is thus controlled by its sub-device driver).
> 
> This is addressable by moving the control of the upstream sub-device 

what is the "upstream sub-device"? This term is new for me.

> streaming state to the driver which is next to it, and what the patch 
> also documents. Note that there is no difference if you have a 
> sub-device without sink pads (or in general case, a sub-device that only 
> has one kind of pads).


> What comes to compatibility between MC-centric and devnode-centric 
> drivers --- on an MC-centric device you have a pipeline and you 
> typically have multiple pads on the sub-devices along the pipeline. 
> Drivers that are devnode-centric generally aren't aware of pads, and so 
> cannot configure sub-devices with multiple pads to begin with.
> 
> You could do that in principle, but you'll start running into the same 
> problems which were addressed by introducing the Media controller interface.

I'm not concerned about compatibility. Yet, the same sub-device may
be there on a pipeline that it is mc-centric or devnode-centric.

I'm mainly concerned with introducing hacks on some entities due to some
specific arrangements between them that are required for an specific 
board layout to work.

> Note that this is not a commonplace. The difference will be only be 
> there *if and only if* you have a sub-device with sink pads in a 
> pipeline. There are not many of those, and that difference is not 
> introduced by this patch: it is a property of hardware.

What do you mean? most pipelines have sub-devices with sink pads.

> 
> I'm definitely open to improving the wording as well as other solutions 
> that can address this.
> 
> Cc Niklas.
> 



Thanks,
Mauro

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Hans Verkuil]

On 08/21/2017 11:08 AM, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Aug 2017 09:36:49 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
>> Hi Mauro,
>>
>> Mauro Carvalho Chehab wrote:
>>> Hi Sakari,
>>>
>>> Em Wed, 16 Aug 2017 15:20:17 +0300
>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
>>>  
>>>> As we begin to add support for systems with Media controller pipelines
>>>> controlled by more than one device driver, it is essential that we
>>>> precisely define the responsibilities of each component in the stream
>>>> control and common practices.
>>>>
>>>> Specifically, streaming control is done per sub-device and sub-device
>>>> drivers themselves are responsible for streaming setup in upstream
>>>> sub-devices.  
>>>
>>> IMO, before this patch, we need something like this:
>>> 	https://patchwork.linuxtv.org/patch/43325/  
>>
>> Thanks. I'll reply separately to that thread.
>>
>>>  
>>>>
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>> ---
>>>>  Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
>>>>  1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
>>>> index e1f0b72..45088ad 100644
>>>> --- a/Documentation/media/kapi/v4l2-subdev.rst
>>>> +++ b/Documentation/media/kapi/v4l2-subdev.rst
>>>> @@ -262,6 +262,35 @@ is called. After all subdevices have been located the .complete() callback is
>>>>  called. When a subdevice is removed from the system the .unbind() method is
>>>>  called. All three callbacks are optional.
>>>>
>>>> +Streaming control on Media controller enabled devices
>>>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>> +
>>>> +Starting and stopping the stream are somewhat complex operations that
>>>> +often require walking the media graph to enable streaming on
>>>> +sub-devices which the pipeline consists of. This involves interaction
>>>> +between multiple drivers, sometimes more than two.  
>>>
>>> That's still not ok, as it creates a black hole for devnode-based
>>> devices.
>>>
>>> I would change it to something like:
>>>
>>> 	Streaming control
>>> 	^^^^^^^^^^^^^^^^^
>>>
>>> 	Starting and stopping the stream are somewhat complex operations that
>>> 	often require to enable streaming on sub-devices which the pipeline
>>> 	consists of. This involves interaction between multiple drivers, sometimes
>>> 	more than two.
>>>
>>> 	The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
>>> 	for starting and stopping the stream on the sub-device it is called
>>> 	on.
>>>
>>> 	Streaming control on devnode-centric devices
>>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> 	On **devnode-centric** devices, the main driver is responsible enable
>>> 	stream all all sub-devices. On most cases, all the main driver need
>>> 	to do is to broadcast s_stream to all connected sub-devices by calling
>>> 	:c:func:`v4l2_device_call_all`, e. g.::
>>>
>>> 		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 1);  
>>
>> Looks good to me.
>>
>>>
>>> 	Streaming control on mc-centric devices
>>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> 	On **mc-centric** devices, it usually requires walking the media graph
>>> 	to enable streaming only at the sub-devices which the pipeline consists
>>> 	of.
>>>
>>> 	(place here the details for such scenario)  
>>
>> This part requires a more detailed description of the problem area. What 
>> makes a difference here is that there's a pipeline this pipeline may be 
>> controlled more than one driver. (More elaborate discussion below.)
>>
>>>  
>>>> +The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
>>>> +for starting and stopping the stream on the sub-device it is called
>>>> +on. A device driver is only responsible for calling the ``.s_stream()`` ops
>>>> +of the adjacent sub-devices that are connected to its sink pads
>>>> +through an enabled link. A driver may not call ``.s_stream()`` op
>>>> +of any other sub-device further up in the pipeline, for instance.
>>>> +
>>>> +This means that a sub-device driver is thus in direct control of
>>>> +whether the upstream sub-devices start (or stop) streaming before or
>>>> +after the sub-device itself is set up for streaming.
>>>> +
>>>> +.. note::
>>>> +
>>>> +   As the ``.s_stream()`` callback is called recursively through the
>>>> +   sub-devices along the pipeline, it is important to keep the
>>>> +   recursion as short as possible. To this end, drivers are encouraged
>>>> +   to avoid recursively calling ``.s_stream()`` internally to reduce
>>>> +   stack usage. Instead, the ``.s_stream()`` op of the directly
>>>> +   connected sub-devices should come from the callback through which
>>>> +   the driver was first called.
>>>> +  
>>>
>>> That sounds too complex, and can lead into troubles, if the same
>>> sub-device driver is used on completely different devices.
>>>
>>> IMHO, it should be up to the main driver to navigate at the MC
>>> pipeline and call s_stream(), and not to the sub-drivers.  
>>
>> I would agree with the above statement *if* we had no devices that 
>> require doing this in a different way.
>>
>> Consider the following case:
>>
>> 	sensor   -> CSI-2 receiver -> ISP (DMA)
>> 	subdev A -> subdev B	   -> video node
> 
> Let me be clearer about the issue I see.
> 
> In the above example, what subdevs are supposed to multicast the
> s_stream() to their neighbors, and how they will know that they
> need to multicast it.
> 
> Let's say, that, in the first pipeline, it would be the sensor
> and subdev A. How "sensor" and "subdev A" will know that they're
> meant to broadcast s_stream(), and the other entities know they
> won't?

So my understanding is that the bridge driver (ISP) will call s_stream
for the CSI-2 receiver, and that in turn calls s_stream of the sensor.

This should only be done for mc-centric devices, so we need a clear
property telling a subdev whether it is part of an mc-centric pipeline
or a devnode-centric pipeline. Since in the latter case it should not
call s_stream in this way. For devnode-centric pipelines the bridge
driver broadcasts s_stream to all subdevs.

For the record, I am not aware of any subdevs that are used by both
mc and devnode-centric scenarios AND that can sit in the middle of a
pipeline. Sensors/video receiver subdevs can certainly be used in both
scenarios, but they don't have to propagate a s_stream call.

It would be very helpful if we have a good description of these two
scenarios in our documentation, and a capability indicating mc-centric
behavior for devnodes. And also for v4l2-subdevs internally (i.e.
am I used in a mc-centric scenario or not?).

Then this documentation will start to make more sense as well.

> Also, the same sensor may be used on a device whose CSI-2 is
> integrated at the ISP driver (the main driver). That's why
> I think that such logic should be started by the main driver, as
> it is the only part of the pipeline that it is aware about
> what it is needed. Also, as the DMA engines are controlled by
> the main driver (via its associated video devnodes), it is the only
> part of the pipeline that knows when a stream starts.

Yes, and this driver is the one that calls s_stream on the
adjacent subdevs. But just those and not all.

> 
>> Assume that the CSI-2 receiver requires hardware setup both *before and 
>> after* streaming has been enabled on the sensor.
> 
> calling s_stream() before and after seems to be an abuse of it.

I think you misunderstand what Sakari tries to say.

In the scenario above the bridge driver calls s_stream for the
CSI receiver. That in turn has code like this:

s_stream(bool enable)
{
	... initialize CSI ...
	if error initializing CSI
		return error
	call s_stream for adjacent source subdev (i.e. sensor)
	if success
		return 0
	... de-initialize CSI
	return error
}

This makes a lot of sense for mc-centric devices and is also much more
precise than the broadcast that a devnode-centric device would do.

In the very unlikely case that this CSI subdev would also be used in
a devnode-centric scenario the s_stream implementation would just
return 0 after it initializes the CSI hardware. It will depend on
the hardware whether that works or not.

subdevs used in devnode-centric scenarios tend to be pretty simple
and are able to handle this.

Regards,

	Hans

> 
> This callback is meant to be called just once, and there's no
> requirement if it should be called before or after: both should
> work.
> 
>>
>> In previous cases the CSI-2 receiver and the ISP have been part of the 
>> same device. This is not universally the case anymore. You'll also get 
>> the same when you add adv748x to the pipeline, and the upstream device 
>> in the pipeline before the adv748x is represented as a sub-device (and 
>> is thus controlled by its sub-device driver).
>>
>> This is addressable by moving the control of the upstream sub-device 
> 
> what is the "upstream sub-device"? This term is new for me.
> 
>> streaming state to the driver which is next to it, and what the patch 
>> also documents. Note that there is no difference if you have a 
>> sub-device without sink pads (or in general case, a sub-device that only 
>> has one kind of pads).
> 
> 
>> What comes to compatibility between MC-centric and devnode-centric 
>> drivers --- on an MC-centric device you have a pipeline and you 
>> typically have multiple pads on the sub-devices along the pipeline. 
>> Drivers that are devnode-centric generally aren't aware of pads, and so 
>> cannot configure sub-devices with multiple pads to begin with.
>>
>> You could do that in principle, but you'll start running into the same 
>> problems which were addressed by introducing the Media controller interface.
> 
> I'm not concerned about compatibility. Yet, the same sub-device may
> be there on a pipeline that it is mc-centric or devnode-centric.
> 
> I'm mainly concerned with introducing hacks on some entities due to some
> specific arrangements between them that are required for an specific 
> board layout to work.
> 
>> Note that this is not a commonplace. The difference will be only be 
>> there *if and only if* you have a sub-device with sink pads in a 
>> pipeline. There are not many of those, and that difference is not 
>> introduced by this patch: it is a property of hardware.
> 
> What do you mean? most pipelines have sub-devices with sink pads.
> 
>>
>> I'm definitely open to improving the wording as well as other solutions 
>> that can address this.
>>
>> Cc Niklas.
>>
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCH v2 2/2] docs-rst: media: Document broken frame handling in stream stop for CSI-2

[Hans Verkuil]

On 08/16/2017 02:20 PM, Sakari Ailus wrote:
> Some CSI-2 transmitters will finish an ongoing frame whereas others will
> not. Document that receiver drivers should not assume a particular
> behaviour but to work in both cases.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/media/kapi/csi2.rst | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/media/kapi/csi2.rst b/Documentation/media/kapi/csi2.rst
> index e33fcb9..0560100 100644
> --- a/Documentation/media/kapi/csi2.rst
> +++ b/Documentation/media/kapi/csi2.rst
> @@ -51,6 +51,16 @@ not active. Some transmitters do this automatically but some have to
>  be explicitly programmed to do so, and some are unable to do so
>  altogether due to hardware constraints.
>  
> +Stopping the transmitter
> +^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +A transmitter stops sending the stream of images as a result of
> +calling the ``.s_stream()`` callback. Some transmitters may stop the
> +stream at a frame boundary whereas others stop immediately,
> +effectively leaving the current frame unfinished. The receiver driver
> +should not make assumptions either way, but function properly in both
> +cases.
> +
>  Receiver drivers
>  ----------------
>  
> 

Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks,

	Hans

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Mauro Carvalho Chehab]

Em Mon, 21 Aug 2017 12:14:18 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 08/21/2017 11:08 AM, Mauro Carvalho Chehab wrote:
> > Em Mon, 21 Aug 2017 09:36:49 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >   
> >> Hi Mauro,
> >>
> >> Mauro Carvalho Chehab wrote:  
> >>> Hi Sakari,
> >>>
> >>> Em Wed, 16 Aug 2017 15:20:17 +0300
> >>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >>>    
> >>>> As we begin to add support for systems with Media controller pipelines
> >>>> controlled by more than one device driver, it is essential that we
> >>>> precisely define the responsibilities of each component in the stream
> >>>> control and common practices.
> >>>>
> >>>> Specifically, streaming control is done per sub-device and sub-device
> >>>> drivers themselves are responsible for streaming setup in upstream
> >>>> sub-devices.    
> >>>
> >>> IMO, before this patch, we need something like this:
> >>> 	https://patchwork.linuxtv.org/patch/43325/    
> >>
> >> Thanks. I'll reply separately to that thread.
> >>  
> >>>    
> >>>>
> >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>>> ---
> >>>>  Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
> >>>>  1 file changed, 29 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
> >>>> index e1f0b72..45088ad 100644
> >>>> --- a/Documentation/media/kapi/v4l2-subdev.rst
> >>>> +++ b/Documentation/media/kapi/v4l2-subdev.rst
> >>>> @@ -262,6 +262,35 @@ is called. After all subdevices have been located the .complete() callback is
> >>>>  called. When a subdevice is removed from the system the .unbind() method is
> >>>>  called. All three callbacks are optional.
> >>>>
> >>>> +Streaming control on Media controller enabled devices
> >>>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>>> +
> >>>> +Starting and stopping the stream are somewhat complex operations that
> >>>> +often require walking the media graph to enable streaming on
> >>>> +sub-devices which the pipeline consists of. This involves interaction
> >>>> +between multiple drivers, sometimes more than two.    
> >>>
> >>> That's still not ok, as it creates a black hole for devnode-based
> >>> devices.
> >>>
> >>> I would change it to something like:
> >>>
> >>> 	Streaming control
> >>> 	^^^^^^^^^^^^^^^^^
> >>>
> >>> 	Starting and stopping the stream are somewhat complex operations that
> >>> 	often require to enable streaming on sub-devices which the pipeline
> >>> 	consists of. This involves interaction between multiple drivers, sometimes
> >>> 	more than two.
> >>>
> >>> 	The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> >>> 	for starting and stopping the stream on the sub-device it is called
> >>> 	on.
> >>>
> >>> 	Streaming control on devnode-centric devices
> >>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>
> >>> 	On **devnode-centric** devices, the main driver is responsible enable
> >>> 	stream all all sub-devices. On most cases, all the main driver need
> >>> 	to do is to broadcast s_stream to all connected sub-devices by calling
> >>> 	:c:func:`v4l2_device_call_all`, e. g.::
> >>>
> >>> 		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 1);    
> >>
> >> Looks good to me.
> >>  
> >>>
> >>> 	Streaming control on mc-centric devices
> >>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>
> >>> 	On **mc-centric** devices, it usually requires walking the media graph
> >>> 	to enable streaming only at the sub-devices which the pipeline consists
> >>> 	of.
> >>>
> >>> 	(place here the details for such scenario)    
> >>
> >> This part requires a more detailed description of the problem area. What 
> >> makes a difference here is that there's a pipeline this pipeline may be 
> >> controlled more than one driver. (More elaborate discussion below.)
> >>  
> >>>    
> >>>> +The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> >>>> +for starting and stopping the stream on the sub-device it is called
> >>>> +on. A device driver is only responsible for calling the ``.s_stream()`` ops
> >>>> +of the adjacent sub-devices that are connected to its sink pads
> >>>> +through an enabled link. A driver may not call ``.s_stream()`` op
> >>>> +of any other sub-device further up in the pipeline, for instance.
> >>>> +
> >>>> +This means that a sub-device driver is thus in direct control of
> >>>> +whether the upstream sub-devices start (or stop) streaming before or
> >>>> +after the sub-device itself is set up for streaming.
> >>>> +
> >>>> +.. note::
> >>>> +
> >>>> +   As the ``.s_stream()`` callback is called recursively through the
> >>>> +   sub-devices along the pipeline, it is important to keep the
> >>>> +   recursion as short as possible. To this end, drivers are encouraged
> >>>> +   to avoid recursively calling ``.s_stream()`` internally to reduce
> >>>> +   stack usage. Instead, the ``.s_stream()`` op of the directly
> >>>> +   connected sub-devices should come from the callback through which
> >>>> +   the driver was first called.
> >>>> +    
> >>>
> >>> That sounds too complex, and can lead into troubles, if the same
> >>> sub-device driver is used on completely different devices.
> >>>
> >>> IMHO, it should be up to the main driver to navigate at the MC
> >>> pipeline and call s_stream(), and not to the sub-drivers.    
> >>
> >> I would agree with the above statement *if* we had no devices that 
> >> require doing this in a different way.
> >>
> >> Consider the following case:
> >>
> >> 	sensor   -> CSI-2 receiver -> ISP (DMA)
> >> 	subdev A -> subdev B	   -> video node  
> > 
> > Let me be clearer about the issue I see.
> > 
> > In the above example, what subdevs are supposed to multicast the
> > s_stream() to their neighbors, and how they will know that they
> > need to multicast it.
> > 
> > Let's say, that, in the first pipeline, it would be the sensor
> > and subdev A. How "sensor" and "subdev A" will know that they're
> > meant to broadcast s_stream(), and the other entities know they
> > won't?  
> 
> So my understanding is that the bridge driver (ISP) will call s_stream
> for the CSI-2 receiver, and that in turn calls s_stream of the sensor.

Alternatively, the ISP driver could call s_stream for both CSI-2 and
sensor.

> This should only be done for mc-centric devices, so we need a clear
> property telling a subdev whether it is part of an mc-centric pipeline
> or a devnode-centric pipeline. Since in the latter case it should not
> call s_stream in this way. For devnode-centric pipelines the bridge
> driver broadcasts s_stream to all subdevs.

It would be easier to have a logic called by the ISP driver that would
get all elements from an active pipeline and call s_stream() there.

That would keep the flexibility that would be needed by devices with a
separate CSI-2 receiver, while preventing the addition of an extra
logic on every sub-device to teach them that s_stream() should be
called to communicate with some specific sub-device.

The thing is that, if you have a pipeline like:

source subdev -> subdev A -> subdev B -> subdev C -> subdev D -> DMA

due to PM constraints, all subdevs on such pipeline may require s_stream()
to control its power consumption. So, it is not just a CSI-2 device that
would need to enable power at the sensor, but an entire pipeline that
would need to receive s_stream() calls.

IMO, the best would be a logic similar to media_entity_pipeline_start() that,
for each entity at the pipeline, s_stream() would be called, e. g. something
like:

	v4l2_subdev_pipeline_call()

> For the record, I am not aware of any subdevs that are used by both
> mc and devnode-centric scenarios AND that can sit in the middle of a
> pipeline. Sensors/video receiver subdevs can certainly be used in both
> scenarios, but they don't have to propagate a s_stream call.

em28xx has a bunch of sensors that are also used on embedded drivers.
It also has a tvp5150, with is also used by omap3.

On the only OMAP3 board whose has a DT upstream, the tvp5150 has both source
pads connected to S-Video connectors, but nothing prevents that someone
would add a tuner before it. I remember someone mentioned that such device
exists (although its DT is not upstream).

On an embedded device with both tvp5150 and a tuner, s_stream() should be
called by both.

> 
> It would be very helpful if we have a good description of these two
> scenarios in our documentation, and a capability indicating mc-centric
> behavior for devnodes. And also for v4l2-subdevs internally (i.e.
> am I used in a mc-centric scenario or not?).
> 
> Then this documentation will start to make more sense as well.
> 
> > Also, the same sensor may be used on a device whose CSI-2 is
> > integrated at the ISP driver (the main driver). That's why
> > I think that such logic should be started by the main driver, as
> > it is the only part of the pipeline that it is aware about
> > what it is needed. Also, as the DMA engines are controlled by
> > the main driver (via its associated video devnodes), it is the only
> > part of the pipeline that knows when a stream starts.  
> 
> Yes, and this driver is the one that calls s_stream on the
> adjacent subdevs. But just those and not all.
> 
> >   
> >> Assume that the CSI-2 receiver requires hardware setup both *before and 
> >> after* streaming has been enabled on the sensor.  
> > 
> > calling s_stream() before and after seems to be an abuse of it.  
> 
> I think you misunderstand what Sakari tries to say.
> 
> In the scenario above the bridge driver calls s_stream for the
> CSI receiver. That in turn has code like this:
> 
> s_stream(bool enable)
> {
> 	... initialize CSI ...
> 	if error initializing CSI
> 		return error
> 	call s_stream for adjacent source subdev (i.e. sensor)
> 	if success
> 		return 0
> 	... de-initialize CSI
> 	return error
> }
> 
> This makes a lot of sense for mc-centric devices and is also much more
> precise than the broadcast that a devnode-centric device would do.
> 
> In the very unlikely case that this CSI subdev would also be used in
> a devnode-centric scenario the s_stream implementation would just
> return 0 after it initializes the CSI hardware. It will depend on
> the hardware whether that works or not.
> 
> subdevs used in devnode-centric scenarios tend to be pretty simple
> and are able to handle this.

If I understand well, you're basically concerned about error handling, right?

That could easily be handled with something like:

	ret = v4l2_subdev_pipeline_call(source_entity, video, s_stream, 1);
	if (ret < 0) {
		v4l2_subdev_pipeline_call(source_entity, video, s_stream, 0);
		return ret;
	}


> 
> Regards,
> 
> 	Hans
> 
> > 
> > This callback is meant to be called just once, and there's no
> > requirement if it should be called before or after: both should
> > work.
> >   
> >>
> >> In previous cases the CSI-2 receiver and the ISP have been part of the 
> >> same device. This is not universally the case anymore. You'll also get 
> >> the same when you add adv748x to the pipeline, and the upstream device 
> >> in the pipeline before the adv748x is represented as a sub-device (and 
> >> is thus controlled by its sub-device driver).
> >>
> >> This is addressable by moving the control of the upstream sub-device   
> > 
> > what is the "upstream sub-device"? This term is new for me.
> >   
> >> streaming state to the driver which is next to it, and what the patch 
> >> also documents. Note that there is no difference if you have a 
> >> sub-device without sink pads (or in general case, a sub-device that only 
> >> has one kind of pads).  
> > 
> >   
> >> What comes to compatibility between MC-centric and devnode-centric 
> >> drivers --- on an MC-centric device you have a pipeline and you 
> >> typically have multiple pads on the sub-devices along the pipeline. 
> >> Drivers that are devnode-centric generally aren't aware of pads, and so 
> >> cannot configure sub-devices with multiple pads to begin with.
> >>
> >> You could do that in principle, but you'll start running into the same 
> >> problems which were addressed by introducing the Media controller interface.  
> > 
> > I'm not concerned about compatibility. Yet, the same sub-device may
> > be there on a pipeline that it is mc-centric or devnode-centric.
> > 
> > I'm mainly concerned with introducing hacks on some entities due to some
> > specific arrangements between them that are required for an specific 
> > board layout to work.
> >   
> >> Note that this is not a commonplace. The difference will be only be 
> >> there *if and only if* you have a sub-device with sink pads in a 
> >> pipeline. There are not many of those, and that difference is not 
> >> introduced by this patch: it is a property of hardware.  
> > 
> > What do you mean? most pipelines have sub-devices with sink pads.
> >   
> >>
> >> I'm definitely open to improving the wording as well as other solutions 
> >> that can address this.
> >>
> >> Cc Niklas.
> >>  
> > 
> > 
> > 
> > Thanks,
> > Mauro
> >   
> 

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Hans Verkuil]

On 08/21/2017 02:07 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Aug 2017 12:14:18 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 08/21/2017 11:08 AM, Mauro Carvalho Chehab wrote:
>>> Em Mon, 21 Aug 2017 09:36:49 +0300
>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
>>>   
>>>> Hi Mauro,
>>>>
>>>> Mauro Carvalho Chehab wrote:  
>>>>> Hi Sakari,
>>>>>
>>>>> Em Wed, 16 Aug 2017 15:20:17 +0300
>>>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
>>>>>    
>>>>>> As we begin to add support for systems with Media controller pipelines
>>>>>> controlled by more than one device driver, it is essential that we
>>>>>> precisely define the responsibilities of each component in the stream
>>>>>> control and common practices.
>>>>>>
>>>>>> Specifically, streaming control is done per sub-device and sub-device
>>>>>> drivers themselves are responsible for streaming setup in upstream
>>>>>> sub-devices.    
>>>>>
>>>>> IMO, before this patch, we need something like this:
>>>>> 	https://patchwork.linuxtv.org/patch/43325/    
>>>>
>>>> Thanks. I'll reply separately to that thread.
>>>>  
>>>>>    
>>>>>>
>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>>> ---
>>>>>>  Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
>>>>>>  1 file changed, 29 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
>>>>>> index e1f0b72..45088ad 100644
>>>>>> --- a/Documentation/media/kapi/v4l2-subdev.rst
>>>>>> +++ b/Documentation/media/kapi/v4l2-subdev.rst
>>>>>> @@ -262,6 +262,35 @@ is called. After all subdevices have been located the .complete() callback is
>>>>>>  called. When a subdevice is removed from the system the .unbind() method is
>>>>>>  called. All three callbacks are optional.
>>>>>>
>>>>>> +Streaming control on Media controller enabled devices
>>>>>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>>> +
>>>>>> +Starting and stopping the stream are somewhat complex operations that
>>>>>> +often require walking the media graph to enable streaming on
>>>>>> +sub-devices which the pipeline consists of. This involves interaction
>>>>>> +between multiple drivers, sometimes more than two.    
>>>>>
>>>>> That's still not ok, as it creates a black hole for devnode-based
>>>>> devices.
>>>>>
>>>>> I would change it to something like:
>>>>>
>>>>> 	Streaming control
>>>>> 	^^^^^^^^^^^^^^^^^
>>>>>
>>>>> 	Starting and stopping the stream are somewhat complex operations that
>>>>> 	often require to enable streaming on sub-devices which the pipeline
>>>>> 	consists of. This involves interaction between multiple drivers, sometimes
>>>>> 	more than two.
>>>>>
>>>>> 	The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
>>>>> 	for starting and stopping the stream on the sub-device it is called
>>>>> 	on.
>>>>>
>>>>> 	Streaming control on devnode-centric devices
>>>>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>
>>>>> 	On **devnode-centric** devices, the main driver is responsible enable
>>>>> 	stream all all sub-devices. On most cases, all the main driver need
>>>>> 	to do is to broadcast s_stream to all connected sub-devices by calling
>>>>> 	:c:func:`v4l2_device_call_all`, e. g.::
>>>>>
>>>>> 		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 1);    
>>>>
>>>> Looks good to me.
>>>>  
>>>>>
>>>>> 	Streaming control on mc-centric devices
>>>>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>
>>>>> 	On **mc-centric** devices, it usually requires walking the media graph
>>>>> 	to enable streaming only at the sub-devices which the pipeline consists
>>>>> 	of.
>>>>>
>>>>> 	(place here the details for such scenario)    
>>>>
>>>> This part requires a more detailed description of the problem area. What 
>>>> makes a difference here is that there's a pipeline this pipeline may be 
>>>> controlled more than one driver. (More elaborate discussion below.)
>>>>  
>>>>>    
>>>>>> +The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
>>>>>> +for starting and stopping the stream on the sub-device it is called
>>>>>> +on. A device driver is only responsible for calling the ``.s_stream()`` ops
>>>>>> +of the adjacent sub-devices that are connected to its sink pads
>>>>>> +through an enabled link. A driver may not call ``.s_stream()`` op
>>>>>> +of any other sub-device further up in the pipeline, for instance.
>>>>>> +
>>>>>> +This means that a sub-device driver is thus in direct control of
>>>>>> +whether the upstream sub-devices start (or stop) streaming before or
>>>>>> +after the sub-device itself is set up for streaming.
>>>>>> +
>>>>>> +.. note::
>>>>>> +
>>>>>> +   As the ``.s_stream()`` callback is called recursively through the
>>>>>> +   sub-devices along the pipeline, it is important to keep the
>>>>>> +   recursion as short as possible. To this end, drivers are encouraged
>>>>>> +   to avoid recursively calling ``.s_stream()`` internally to reduce
>>>>>> +   stack usage. Instead, the ``.s_stream()`` op of the directly
>>>>>> +   connected sub-devices should come from the callback through which
>>>>>> +   the driver was first called.
>>>>>> +    
>>>>>
>>>>> That sounds too complex, and can lead into troubles, if the same
>>>>> sub-device driver is used on completely different devices.
>>>>>
>>>>> IMHO, it should be up to the main driver to navigate at the MC
>>>>> pipeline and call s_stream(), and not to the sub-drivers.    
>>>>
>>>> I would agree with the above statement *if* we had no devices that 
>>>> require doing this in a different way.
>>>>
>>>> Consider the following case:
>>>>
>>>> 	sensor   -> CSI-2 receiver -> ISP (DMA)
>>>> 	subdev A -> subdev B	   -> video node  
>>>
>>> Let me be clearer about the issue I see.
>>>
>>> In the above example, what subdevs are supposed to multicast the
>>> s_stream() to their neighbors, and how they will know that they
>>> need to multicast it.
>>>
>>> Let's say, that, in the first pipeline, it would be the sensor
>>> and subdev A. How "sensor" and "subdev A" will know that they're
>>> meant to broadcast s_stream(), and the other entities know they
>>> won't?  
>>
>> So my understanding is that the bridge driver (ISP) will call s_stream
>> for the CSI-2 receiver, and that in turn calls s_stream of the sensor.
> 
> Alternatively, the ISP driver could call s_stream for both CSI-2 and
> sensor.
> 
>> This should only be done for mc-centric devices, so we need a clear
>> property telling a subdev whether it is part of an mc-centric pipeline
>> or a devnode-centric pipeline. Since in the latter case it should not
>> call s_stream in this way. For devnode-centric pipelines the bridge
>> driver broadcasts s_stream to all subdevs.
> 
> It would be easier to have a logic called by the ISP driver that would
> get all elements from an active pipeline and call s_stream() there.
> 
> That would keep the flexibility that would be needed by devices with a
> separate CSI-2 receiver, while preventing the addition of an extra
> logic on every sub-device to teach them that s_stream() should be
> called to communicate with some specific sub-device.
> 
> The thing is that, if you have a pipeline like:
> 
> source subdev -> subdev A -> subdev B -> subdev C -> subdev D -> DMA
> 
> due to PM constraints, all subdevs on such pipeline may require s_stream()
> to control its power consumption. So, it is not just a CSI-2 device that
> would need to enable power at the sensor, but an entire pipeline that
> would need to receive s_stream() calls.

So? If DMA calls s_stream for subdev D, which calls s_stream for subdev C, etc.
until the source subdev's s_stream is called, then they are all powered up,
aren't they?

I don't see the difference between this and the DMA calling s_stream for all
subdevs.

One clear difference is (as Sakari mentioned) stack usage. There having the
DMA call s_stream is clearly better. See below for more comments...

> IMO, the best would be a logic similar to media_entity_pipeline_start() that,
> for each entity at the pipeline, s_stream() would be called, e. g. something
> like:
> 
> 	v4l2_subdev_pipeline_call()
> 
>> For the record, I am not aware of any subdevs that are used by both
>> mc and devnode-centric scenarios AND that can sit in the middle of a
>> pipeline. Sensors/video receiver subdevs can certainly be used in both
>> scenarios, but they don't have to propagate a s_stream call.
> 
> em28xx has a bunch of sensors that are also used on embedded drivers.
> It also has a tvp5150, with is also used by omap3.
> 
> On the only OMAP3 board whose has a DT upstream, the tvp5150 has both source
> pads connected to S-Video connectors, but nothing prevents that someone
> would add a tuner before it. I remember someone mentioned that such device
> exists (although its DT is not upstream).
> 
> On an embedded device with both tvp5150 and a tuner, s_stream() should be
> called by both.
> 
>>
>> It would be very helpful if we have a good description of these two
>> scenarios in our documentation, and a capability indicating mc-centric
>> behavior for devnodes. And also for v4l2-subdevs internally (i.e.
>> am I used in a mc-centric scenario or not?).
>>
>> Then this documentation will start to make more sense as well.
>>
>>> Also, the same sensor may be used on a device whose CSI-2 is
>>> integrated at the ISP driver (the main driver). That's why
>>> I think that such logic should be started by the main driver, as
>>> it is the only part of the pipeline that it is aware about
>>> what it is needed. Also, as the DMA engines are controlled by
>>> the main driver (via its associated video devnodes), it is the only
>>> part of the pipeline that knows when a stream starts.  
>>
>> Yes, and this driver is the one that calls s_stream on the
>> adjacent subdevs. But just those and not all.
>>
>>>   
>>>> Assume that the CSI-2 receiver requires hardware setup both *before and 
>>>> after* streaming has been enabled on the sensor.  
>>>
>>> calling s_stream() before and after seems to be an abuse of it.  
>>
>> I think you misunderstand what Sakari tries to say.
>>
>> In the scenario above the bridge driver calls s_stream for the
>> CSI receiver. That in turn has code like this:
>>
>> s_stream(bool enable)
>> {
>> 	... initialize CSI ...
>> 	if error initializing CSI
>> 		return error
>> 	call s_stream for adjacent source subdev (i.e. sensor)
>> 	if success
>> 		return 0
>> 	... de-initialize CSI
>> 	return error
>> }
>>
>> This makes a lot of sense for mc-centric devices and is also much more
>> precise than the broadcast that a devnode-centric device would do.
>>
>> In the very unlikely case that this CSI subdev would also be used in
>> a devnode-centric scenario the s_stream implementation would just
>> return 0 after it initializes the CSI hardware. It will depend on
>> the hardware whether that works or not.
>>
>> subdevs used in devnode-centric scenarios tend to be pretty simple
>> and are able to handle this.
> 
> If I understand well, you're basically concerned about error handling, right?
> 
> That could easily be handled with something like:
> 
> 	ret = v4l2_subdev_pipeline_call(source_entity, video, s_stream, 1);
> 	if (ret < 0) {
> 		v4l2_subdev_pipeline_call(source_entity, video, s_stream, 0);
> 		return ret;
> 	}

That's not it. It is that some subdevs need to execute initialization code
first before you can call s_stream on the sensor.

If you want to use a pipeline_call you would need to introduce stream_prepare
and stream_unprepare ops to achieve the same (idea stolen from the clk API).

I'm not sure if this would fully resolve Sakari's issue.

Sakari, what is your opinion on having the DMA call a stream_prepare followed
by s_stream(1)? It avoids the stack recursion issue, and it allows for HW
initialization.

Regards,

	Hans

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Mauro Carvalho Chehab]

Em Mon, 21 Aug 2017 15:52:17 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 08/21/2017 02:07 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 21 Aug 2017 12:14:18 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> On 08/21/2017 11:08 AM, Mauro Carvalho Chehab wrote:  
> >>> Em Mon, 21 Aug 2017 09:36:49 +0300
> >>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >>>     
> >>>> Hi Mauro,
> >>>>
> >>>> Mauro Carvalho Chehab wrote:    
> >>>>> Hi Sakari,
> >>>>>
> >>>>> Em Wed, 16 Aug 2017 15:20:17 +0300
> >>>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >>>>>      
> >>>>>> As we begin to add support for systems with Media controller pipelines
> >>>>>> controlled by more than one device driver, it is essential that we
> >>>>>> precisely define the responsibilities of each component in the stream
> >>>>>> control and common practices.
> >>>>>>
> >>>>>> Specifically, streaming control is done per sub-device and sub-device
> >>>>>> drivers themselves are responsible for streaming setup in upstream
> >>>>>> sub-devices.      
> >>>>>
> >>>>> IMO, before this patch, we need something like this:
> >>>>> 	https://patchwork.linuxtv.org/patch/43325/      
> >>>>
> >>>> Thanks. I'll reply separately to that thread.
> >>>>    
> >>>>>      
> >>>>>>
> >>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>>>> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>>>>> ---
> >>>>>>  Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
> >>>>>>  1 file changed, 29 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
> >>>>>> index e1f0b72..45088ad 100644
> >>>>>> --- a/Documentation/media/kapi/v4l2-subdev.rst
> >>>>>> +++ b/Documentation/media/kapi/v4l2-subdev.rst
> >>>>>> @@ -262,6 +262,35 @@ is called. After all subdevices have been located the .complete() callback is
> >>>>>>  called. When a subdevice is removed from the system the .unbind() method is
> >>>>>>  called. All three callbacks are optional.
> >>>>>>
> >>>>>> +Streaming control on Media controller enabled devices
> >>>>>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>>>>> +
> >>>>>> +Starting and stopping the stream are somewhat complex operations that
> >>>>>> +often require walking the media graph to enable streaming on
> >>>>>> +sub-devices which the pipeline consists of. This involves interaction
> >>>>>> +between multiple drivers, sometimes more than two.      
> >>>>>
> >>>>> That's still not ok, as it creates a black hole for devnode-based
> >>>>> devices.
> >>>>>
> >>>>> I would change it to something like:
> >>>>>
> >>>>> 	Streaming control
> >>>>> 	^^^^^^^^^^^^^^^^^
> >>>>>
> >>>>> 	Starting and stopping the stream are somewhat complex operations that
> >>>>> 	often require to enable streaming on sub-devices which the pipeline
> >>>>> 	consists of. This involves interaction between multiple drivers, sometimes
> >>>>> 	more than two.
> >>>>>
> >>>>> 	The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> >>>>> 	for starting and stopping the stream on the sub-device it is called
> >>>>> 	on.
> >>>>>
> >>>>> 	Streaming control on devnode-centric devices
> >>>>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>>>
> >>>>> 	On **devnode-centric** devices, the main driver is responsible enable
> >>>>> 	stream all all sub-devices. On most cases, all the main driver need
> >>>>> 	to do is to broadcast s_stream to all connected sub-devices by calling
> >>>>> 	:c:func:`v4l2_device_call_all`, e. g.::
> >>>>>
> >>>>> 		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 1);      
> >>>>
> >>>> Looks good to me.
> >>>>    
> >>>>>
> >>>>> 	Streaming control on mc-centric devices
> >>>>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>>>
> >>>>> 	On **mc-centric** devices, it usually requires walking the media graph
> >>>>> 	to enable streaming only at the sub-devices which the pipeline consists
> >>>>> 	of.
> >>>>>
> >>>>> 	(place here the details for such scenario)      
> >>>>
> >>>> This part requires a more detailed description of the problem area. What 
> >>>> makes a difference here is that there's a pipeline this pipeline may be 
> >>>> controlled more than one driver. (More elaborate discussion below.)
> >>>>    
> >>>>>      
> >>>>>> +The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> >>>>>> +for starting and stopping the stream on the sub-device it is called
> >>>>>> +on. A device driver is only responsible for calling the ``.s_stream()`` ops
> >>>>>> +of the adjacent sub-devices that are connected to its sink pads
> >>>>>> +through an enabled link. A driver may not call ``.s_stream()`` op
> >>>>>> +of any other sub-device further up in the pipeline, for instance.
> >>>>>> +
> >>>>>> +This means that a sub-device driver is thus in direct control of
> >>>>>> +whether the upstream sub-devices start (or stop) streaming before or
> >>>>>> +after the sub-device itself is set up for streaming.
> >>>>>> +
> >>>>>> +.. note::
> >>>>>> +
> >>>>>> +   As the ``.s_stream()`` callback is called recursively through the
> >>>>>> +   sub-devices along the pipeline, it is important to keep the
> >>>>>> +   recursion as short as possible. To this end, drivers are encouraged
> >>>>>> +   to avoid recursively calling ``.s_stream()`` internally to reduce
> >>>>>> +   stack usage. Instead, the ``.s_stream()`` op of the directly
> >>>>>> +   connected sub-devices should come from the callback through which
> >>>>>> +   the driver was first called.
> >>>>>> +      
> >>>>>
> >>>>> That sounds too complex, and can lead into troubles, if the same
> >>>>> sub-device driver is used on completely different devices.
> >>>>>
> >>>>> IMHO, it should be up to the main driver to navigate at the MC
> >>>>> pipeline and call s_stream(), and not to the sub-drivers.      
> >>>>
> >>>> I would agree with the above statement *if* we had no devices that 
> >>>> require doing this in a different way.
> >>>>
> >>>> Consider the following case:
> >>>>
> >>>> 	sensor   -> CSI-2 receiver -> ISP (DMA)
> >>>> 	subdev A -> subdev B	   -> video node    
> >>>
> >>> Let me be clearer about the issue I see.
> >>>
> >>> In the above example, what subdevs are supposed to multicast the
> >>> s_stream() to their neighbors, and how they will know that they
> >>> need to multicast it.
> >>>
> >>> Let's say, that, in the first pipeline, it would be the sensor
> >>> and subdev A. How "sensor" and "subdev A" will know that they're
> >>> meant to broadcast s_stream(), and the other entities know they
> >>> won't?    
> >>
> >> So my understanding is that the bridge driver (ISP) will call s_stream
> >> for the CSI-2 receiver, and that in turn calls s_stream of the sensor.  
> > 
> > Alternatively, the ISP driver could call s_stream for both CSI-2 and
> > sensor.
> >   
> >> This should only be done for mc-centric devices, so we need a clear
> >> property telling a subdev whether it is part of an mc-centric pipeline
> >> or a devnode-centric pipeline. Since in the latter case it should not
> >> call s_stream in this way. For devnode-centric pipelines the bridge
> >> driver broadcasts s_stream to all subdevs.  
> > 
> > It would be easier to have a logic called by the ISP driver that would
> > get all elements from an active pipeline and call s_stream() there.
> > 
> > That would keep the flexibility that would be needed by devices with a
> > separate CSI-2 receiver, while preventing the addition of an extra
> > logic on every sub-device to teach them that s_stream() should be
> > called to communicate with some specific sub-device.
> > 
> > The thing is that, if you have a pipeline like:
> > 
> > source subdev -> subdev A -> subdev B -> subdev C -> subdev D -> DMA
> > 
> > due to PM constraints, all subdevs on such pipeline may require s_stream()
> > to control its power consumption. So, it is not just a CSI-2 device that
> > would need to enable power at the sensor, but an entire pipeline that
> > would need to receive s_stream() calls.  
> 
> So? If DMA calls s_stream for subdev D, which calls s_stream for subdev C, etc.
> until the source subdev's s_stream is called, then they are all powered up,
> aren't they?
> 
> I don't see the difference between this and the DMA calling s_stream for all
> subdevs.
> 
> One clear difference is (as Sakari mentioned) stack usage. There having the
> DMA call s_stream is clearly better. See below for more comments...

That's not the only difference. If we pass this to subdevs, they
need to know that they're working on a mc-centric way.

There is another problem, with is more serious, and it is somewhat
related to stack usage: what happens if there's a loop inside a
pipeline? That would cause a Kernel crash. I can't see any easy
way to avoid that (nor stack starving, if the pipeline is too big).

> 
> > IMO, the best would be a logic similar to media_entity_pipeline_start() that,
> > for each entity at the pipeline, s_stream() would be called, e. g. something
> > like:
> > 
> > 	v4l2_subdev_pipeline_call()
> >   
> >> For the record, I am not aware of any subdevs that are used by both
> >> mc and devnode-centric scenarios AND that can sit in the middle of a
> >> pipeline. Sensors/video receiver subdevs can certainly be used in both
> >> scenarios, but they don't have to propagate a s_stream call.  
> > 
> > em28xx has a bunch of sensors that are also used on embedded drivers.
> > It also has a tvp5150, with is also used by omap3.
> > 
> > On the only OMAP3 board whose has a DT upstream, the tvp5150 has both source
> > pads connected to S-Video connectors, but nothing prevents that someone
> > would add a tuner before it. I remember someone mentioned that such device
> > exists (although its DT is not upstream).
> > 
> > On an embedded device with both tvp5150 and a tuner, s_stream() should be
> > called by both.
> >   
> >>
> >> It would be very helpful if we have a good description of these two
> >> scenarios in our documentation, and a capability indicating mc-centric
> >> behavior for devnodes. And also for v4l2-subdevs internally (i.e.
> >> am I used in a mc-centric scenario or not?).
> >>
> >> Then this documentation will start to make more sense as well.
> >>  
> >>> Also, the same sensor may be used on a device whose CSI-2 is
> >>> integrated at the ISP driver (the main driver). That's why
> >>> I think that such logic should be started by the main driver, as
> >>> it is the only part of the pipeline that it is aware about
> >>> what it is needed. Also, as the DMA engines are controlled by
> >>> the main driver (via its associated video devnodes), it is the only
> >>> part of the pipeline that knows when a stream starts.    
> >>
> >> Yes, and this driver is the one that calls s_stream on the
> >> adjacent subdevs. But just those and not all.
> >>  
> >>>     
> >>>> Assume that the CSI-2 receiver requires hardware setup both *before and 
> >>>> after* streaming has been enabled on the sensor.    
> >>>
> >>> calling s_stream() before and after seems to be an abuse of it.    
> >>
> >> I think you misunderstand what Sakari tries to say.
> >>
> >> In the scenario above the bridge driver calls s_stream for the
> >> CSI receiver. That in turn has code like this:
> >>
> >> s_stream(bool enable)
> >> {
> >> 	... initialize CSI ...
> >> 	if error initializing CSI
> >> 		return error
> >> 	call s_stream for adjacent source subdev (i.e. sensor)
> >> 	if success
> >> 		return 0
> >> 	... de-initialize CSI
> >> 	return error
> >> }
> >>
> >> This makes a lot of sense for mc-centric devices and is also much more
> >> precise than the broadcast that a devnode-centric device would do.
> >>
> >> In the very unlikely case that this CSI subdev would also be used in
> >> a devnode-centric scenario the s_stream implementation would just
> >> return 0 after it initializes the CSI hardware. It will depend on
> >> the hardware whether that works or not.
> >>
> >> subdevs used in devnode-centric scenarios tend to be pretty simple
> >> and are able to handle this.  
> > 
> > If I understand well, you're basically concerned about error handling, right?
> > 
> > That could easily be handled with something like:
> > 
> > 	ret = v4l2_subdev_pipeline_call(source_entity, video, s_stream, 1);
> > 	if (ret < 0) {
> > 		v4l2_subdev_pipeline_call(source_entity, video, s_stream, 0);
> > 		return ret;
> > 	}  
> 
> That's not it. It is that some subdevs need to execute initialization code
> first before you can call s_stream on the sensor.
> 
> If you want to use a pipeline_call you would need to introduce stream_prepare
> and stream_unprepare ops to achieve the same (idea stolen from the clk API).
> 
> I'm not sure if this would fully resolve Sakari's issue.

If the problem is with initialization, all we need to enforce is
that the graph traversal will happen at the reverse order, e. g.
from subdev D to subdev A.

But yeah, if it is more complex, we may need a stream_prepare ops.

> 
> Sakari, what is your opinion on having the DMA call a stream_prepare followed
> by s_stream(1)? It avoids the stack recursion issue, and it allows for HW
> initialization.
> 
> Regards,
> 
> 	Hans




Cheers,
Mauro

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Niklas Söderlund]

On 2017-08-21 11:01:00 -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Aug 2017 15:52:17 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On 08/21/2017 02:07 PM, Mauro Carvalho Chehab wrote:
> > > Em Mon, 21 Aug 2017 12:14:18 +0200
> > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > >   
> > >> On 08/21/2017 11:08 AM, Mauro Carvalho Chehab wrote:  
> > >>> Em Mon, 21 Aug 2017 09:36:49 +0300
> > >>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > >>>     
> > >>>> Hi Mauro,
> > >>>>
> > >>>> Mauro Carvalho Chehab wrote:    
> > >>>>> Hi Sakari,
> > >>>>>
> > >>>>> Em Wed, 16 Aug 2017 15:20:17 +0300
> > >>>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > >>>>>      
> > >>>>>> As we begin to add support for systems with Media controller pipelines
> > >>>>>> controlled by more than one device driver, it is essential that we
> > >>>>>> precisely define the responsibilities of each component in the stream
> > >>>>>> control and common practices.
> > >>>>>>
> > >>>>>> Specifically, streaming control is done per sub-device and sub-device
> > >>>>>> drivers themselves are responsible for streaming setup in upstream
> > >>>>>> sub-devices.      
> > >>>>>
> > >>>>> IMO, before this patch, we need something like this:
> > >>>>> 	https://patchwork.linuxtv.org/patch/43325/      
> > >>>>
> > >>>> Thanks. I'll reply separately to that thread.
> > >>>>    
> > >>>>>      
> > >>>>>>
> > >>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >>>>>> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >>>>>> ---
> > >>>>>>  Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
> > >>>>>>  1 file changed, 29 insertions(+)
> > >>>>>>
> > >>>>>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
> > >>>>>> index e1f0b72..45088ad 100644
> > >>>>>> --- a/Documentation/media/kapi/v4l2-subdev.rst
> > >>>>>> +++ b/Documentation/media/kapi/v4l2-subdev.rst
> > >>>>>> @@ -262,6 +262,35 @@ is called. After all subdevices have been located the .complete() callback is
> > >>>>>>  called. When a subdevice is removed from the system the .unbind() method is
> > >>>>>>  called. All three callbacks are optional.
> > >>>>>>
> > >>>>>> +Streaming control on Media controller enabled devices
> > >>>>>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >>>>>> +
> > >>>>>> +Starting and stopping the stream are somewhat complex operations that
> > >>>>>> +often require walking the media graph to enable streaming on
> > >>>>>> +sub-devices which the pipeline consists of. This involves interaction
> > >>>>>> +between multiple drivers, sometimes more than two.      
> > >>>>>
> > >>>>> That's still not ok, as it creates a black hole for devnode-based
> > >>>>> devices.
> > >>>>>
> > >>>>> I would change it to something like:
> > >>>>>
> > >>>>> 	Streaming control
> > >>>>> 	^^^^^^^^^^^^^^^^^
> > >>>>>
> > >>>>> 	Starting and stopping the stream are somewhat complex operations that
> > >>>>> 	often require to enable streaming on sub-devices which the pipeline
> > >>>>> 	consists of. This involves interaction between multiple drivers, sometimes
> > >>>>> 	more than two.
> > >>>>>
> > >>>>> 	The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> > >>>>> 	for starting and stopping the stream on the sub-device it is called
> > >>>>> 	on.
> > >>>>>
> > >>>>> 	Streaming control on devnode-centric devices
> > >>>>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>>>>
> > >>>>> 	On **devnode-centric** devices, the main driver is responsible enable
> > >>>>> 	stream all all sub-devices. On most cases, all the main driver need
> > >>>>> 	to do is to broadcast s_stream to all connected sub-devices by calling
> > >>>>> 	:c:func:`v4l2_device_call_all`, e. g.::
> > >>>>>
> > >>>>> 		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 1);      
> > >>>>
> > >>>> Looks good to me.
> > >>>>    
> > >>>>>
> > >>>>> 	Streaming control on mc-centric devices
> > >>>>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>>>>
> > >>>>> 	On **mc-centric** devices, it usually requires walking the media graph
> > >>>>> 	to enable streaming only at the sub-devices which the pipeline consists
> > >>>>> 	of.
> > >>>>>
> > >>>>> 	(place here the details for such scenario)      
> > >>>>
> > >>>> This part requires a more detailed description of the problem area. What 
> > >>>> makes a difference here is that there's a pipeline this pipeline may be 
> > >>>> controlled more than one driver. (More elaborate discussion below.)
> > >>>>    
> > >>>>>      
> > >>>>>> +The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> > >>>>>> +for starting and stopping the stream on the sub-device it is called
> > >>>>>> +on. A device driver is only responsible for calling the ``.s_stream()`` ops
> > >>>>>> +of the adjacent sub-devices that are connected to its sink pads
> > >>>>>> +through an enabled link. A driver may not call ``.s_stream()`` op
> > >>>>>> +of any other sub-device further up in the pipeline, for instance.
> > >>>>>> +
> > >>>>>> +This means that a sub-device driver is thus in direct control of
> > >>>>>> +whether the upstream sub-devices start (or stop) streaming before or
> > >>>>>> +after the sub-device itself is set up for streaming.
> > >>>>>> +
> > >>>>>> +.. note::
> > >>>>>> +
> > >>>>>> +   As the ``.s_stream()`` callback is called recursively through the
> > >>>>>> +   sub-devices along the pipeline, it is important to keep the
> > >>>>>> +   recursion as short as possible. To this end, drivers are encouraged
> > >>>>>> +   to avoid recursively calling ``.s_stream()`` internally to reduce
> > >>>>>> +   stack usage. Instead, the ``.s_stream()`` op of the directly
> > >>>>>> +   connected sub-devices should come from the callback through which
> > >>>>>> +   the driver was first called.
> > >>>>>> +      
> > >>>>>
> > >>>>> That sounds too complex, and can lead into troubles, if the same
> > >>>>> sub-device driver is used on completely different devices.
> > >>>>>
> > >>>>> IMHO, it should be up to the main driver to navigate at the MC
> > >>>>> pipeline and call s_stream(), and not to the sub-drivers.      
> > >>>>
> > >>>> I would agree with the above statement *if* we had no devices that 
> > >>>> require doing this in a different way.
> > >>>>
> > >>>> Consider the following case:
> > >>>>
> > >>>> 	sensor   -> CSI-2 receiver -> ISP (DMA)
> > >>>> 	subdev A -> subdev B	   -> video node    
> > >>>
> > >>> Let me be clearer about the issue I see.
> > >>>
> > >>> In the above example, what subdevs are supposed to multicast the
> > >>> s_stream() to their neighbors, and how they will know that they
> > >>> need to multicast it.
> > >>>
> > >>> Let's say, that, in the first pipeline, it would be the sensor
> > >>> and subdev A. How "sensor" and "subdev A" will know that they're
> > >>> meant to broadcast s_stream(), and the other entities know they
> > >>> won't?    
> > >>
> > >> So my understanding is that the bridge driver (ISP) will call s_stream
> > >> for the CSI-2 receiver, and that in turn calls s_stream of the sensor.  
> > > 
> > > Alternatively, the ISP driver could call s_stream for both CSI-2 and
> > > sensor.
> > >   
> > >> This should only be done for mc-centric devices, so we need a clear
> > >> property telling a subdev whether it is part of an mc-centric pipeline
> > >> or a devnode-centric pipeline. Since in the latter case it should not
> > >> call s_stream in this way. For devnode-centric pipelines the bridge
> > >> driver broadcasts s_stream to all subdevs.  
> > > 
> > > It would be easier to have a logic called by the ISP driver that would
> > > get all elements from an active pipeline and call s_stream() there.
> > > 
> > > That would keep the flexibility that would be needed by devices with a
> > > separate CSI-2 receiver, while preventing the addition of an extra
> > > logic on every sub-device to teach them that s_stream() should be
> > > called to communicate with some specific sub-device.
> > > 
> > > The thing is that, if you have a pipeline like:
> > > 
> > > source subdev -> subdev A -> subdev B -> subdev C -> subdev D -> DMA
> > > 
> > > due to PM constraints, all subdevs on such pipeline may require s_stream()
> > > to control its power consumption. So, it is not just a CSI-2 device that
> > > would need to enable power at the sensor, but an entire pipeline that
> > > would need to receive s_stream() calls.  
> > 
> > So? If DMA calls s_stream for subdev D, which calls s_stream for subdev C, etc.
> > until the source subdev's s_stream is called, then they are all powered up,
> > aren't they?
> > 
> > I don't see the difference between this and the DMA calling s_stream for all
> > subdevs.
> > 
> > One clear difference is (as Sakari mentioned) stack usage. There having the
> > DMA call s_stream is clearly better. See below for more comments...
> 
> That's not the only difference. If we pass this to subdevs, they
> need to know that they're working on a mc-centric way.

Another difference is that in a mc-centric driver more information might 
be needed in the s_stream call then currently is available and would be 
harder to do if the DMA driver calls s_stream on all subdevices. One 
example of this is in the patch series '[PATCH 00/20] Add multiplexed 
media pads to support CSI-2 virtual channels' which tries to add 
multiplexed pads which can contain more then one stream.

In that series the s_stream() operation is extended to carry information 
about both which pad and which stream the s_stream() operation intends 
to start. Then each subdevice is responsible to call s_stream() on it's 
neighbor providing which pad and stream of the remote it wants to start 
based on which of its own pads and possibly stream s_stream() as called 
up on itself. This propagation by each subdevice then becomes much more 
useful as it's only the subdevice itself that knows about the internal 
subdevice routing from its sink to source pad(s). Maybe this can also be 
covered by some generic v4l2 helper to start the stream, but it feels 
like such implementations could become complex.

> 
> There is another problem, with is more serious, and it is somewhat
> related to stack usage: what happens if there's a loop inside a
> pipeline? That would cause a Kernel crash. I can't see any easy
> way to avoid that (nor stack starving, if the pipeline is too big).
> 
> > 
> > > IMO, the best would be a logic similar to media_entity_pipeline_start() that,
> > > for each entity at the pipeline, s_stream() would be called, e. g. something
> > > like:
> > > 
> > > 	v4l2_subdev_pipeline_call()
> > >   
> > >> For the record, I am not aware of any subdevs that are used by both
> > >> mc and devnode-centric scenarios AND that can sit in the middle of a
> > >> pipeline. Sensors/video receiver subdevs can certainly be used in both
> > >> scenarios, but they don't have to propagate a s_stream call.  
> > > 
> > > em28xx has a bunch of sensors that are also used on embedded drivers.
> > > It also has a tvp5150, with is also used by omap3.
> > > 
> > > On the only OMAP3 board whose has a DT upstream, the tvp5150 has both source
> > > pads connected to S-Video connectors, but nothing prevents that someone
> > > would add a tuner before it. I remember someone mentioned that such device
> > > exists (although its DT is not upstream).
> > > 
> > > On an embedded device with both tvp5150 and a tuner, s_stream() should be
> > > called by both.
> > >   
> > >>
> > >> It would be very helpful if we have a good description of these two
> > >> scenarios in our documentation, and a capability indicating mc-centric
> > >> behavior for devnodes. And also for v4l2-subdevs internally (i.e.
> > >> am I used in a mc-centric scenario or not?).
> > >>
> > >> Then this documentation will start to make more sense as well.
> > >>  
> > >>> Also, the same sensor may be used on a device whose CSI-2 is
> > >>> integrated at the ISP driver (the main driver). That's why
> > >>> I think that such logic should be started by the main driver, as
> > >>> it is the only part of the pipeline that it is aware about
> > >>> what it is needed. Also, as the DMA engines are controlled by
> > >>> the main driver (via its associated video devnodes), it is the only
> > >>> part of the pipeline that knows when a stream starts.    
> > >>
> > >> Yes, and this driver is the one that calls s_stream on the
> > >> adjacent subdevs. But just those and not all.
> > >>  
> > >>>     
> > >>>> Assume that the CSI-2 receiver requires hardware setup both *before and 
> > >>>> after* streaming has been enabled on the sensor.    
> > >>>
> > >>> calling s_stream() before and after seems to be an abuse of it.    
> > >>
> > >> I think you misunderstand what Sakari tries to say.
> > >>
> > >> In the scenario above the bridge driver calls s_stream for the
> > >> CSI receiver. That in turn has code like this:
> > >>
> > >> s_stream(bool enable)
> > >> {
> > >> 	... initialize CSI ...
> > >> 	if error initializing CSI
> > >> 		return error
> > >> 	call s_stream for adjacent source subdev (i.e. sensor)
> > >> 	if success
> > >> 		return 0
> > >> 	... de-initialize CSI
> > >> 	return error
> > >> }
> > >>
> > >> This makes a lot of sense for mc-centric devices and is also much more
> > >> precise than the broadcast that a devnode-centric device would do.
> > >>
> > >> In the very unlikely case that this CSI subdev would also be used in
> > >> a devnode-centric scenario the s_stream implementation would just
> > >> return 0 after it initializes the CSI hardware. It will depend on
> > >> the hardware whether that works or not.
> > >>
> > >> subdevs used in devnode-centric scenarios tend to be pretty simple
> > >> and are able to handle this.  
> > > 
> > > If I understand well, you're basically concerned about error handling, right?
> > > 
> > > That could easily be handled with something like:
> > > 
> > > 	ret = v4l2_subdev_pipeline_call(source_entity, video, s_stream, 1);
> > > 	if (ret < 0) {
> > > 		v4l2_subdev_pipeline_call(source_entity, video, s_stream, 0);
> > > 		return ret;
> > > 	}  
> > 
> > That's not it. It is that some subdevs need to execute initialization code
> > first before you can call s_stream on the sensor.
> > 
> > If you want to use a pipeline_call you would need to introduce stream_prepare
> > and stream_unprepare ops to achieve the same (idea stolen from the clk API).
> > 
> > I'm not sure if this would fully resolve Sakari's issue.
> 
> If the problem is with initialization, all we need to enforce is
> that the graph traversal will happen at the reverse order, e. g.
> from subdev D to subdev A.
> 
> But yeah, if it is more complex, we may need a stream_prepare ops.
> 
> > 
> > Sakari, what is your opinion on having the DMA call a stream_prepare followed
> > by s_stream(1)? It avoids the stack recursion issue, and it allows for HW
> > initialization.
> > 
> > Regards,
> > 
> > 	Hans
> 
> 
> 
> 
> Cheers,
> Mauro

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Mauro Carvalho Chehab]

Em Tue, 22 Aug 2017 02:44:00 +0200
Niklas Söderlund <niklas.soderlund@ragnatech.se> escreveu:

> On 2017-08-21 11:01:00 -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 21 Aug 2017 15:52:17 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > 
> > > On 08/21/2017 02:07 PM, Mauro Carvalho Chehab wrote:
> > > > Em Mon, 21 Aug 2017 12:14:18 +0200
> > > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > >   
> > > >> On 08/21/2017 11:08 AM, Mauro Carvalho Chehab wrote:  
> > > >>> Em Mon, 21 Aug 2017 09:36:49 +0300
> > > >>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > > >>>     
> > > >>>> Hi Mauro,
> > > >>>>
> > > >>>> Mauro Carvalho Chehab wrote:    
> > > >>>>> Hi Sakari,
> > > >>>>>
> > > >>>>> Em Wed, 16 Aug 2017 15:20:17 +0300
> > > >>>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > > >>>>>      
> > > >>>>>> As we begin to add support for systems with Media controller pipelines
> > > >>>>>> controlled by more than one device driver, it is essential that we
> > > >>>>>> precisely define the responsibilities of each component in the stream
> > > >>>>>> control and common practices.
> > > >>>>>>
> > > >>>>>> Specifically, streaming control is done per sub-device and sub-device
> > > >>>>>> drivers themselves are responsible for streaming setup in upstream
> > > >>>>>> sub-devices.      
> > > >>>>>
> > > >>>>> IMO, before this patch, we need something like this:
> > > >>>>> 	https://patchwork.linuxtv.org/patch/43325/      
> > > >>>>
> > > >>>> Thanks. I'll reply separately to that thread.
> > > >>>>    
> > > >>>>>      
> > > >>>>>>
> > > >>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > >>>>>> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > >>>>>> ---
> > > >>>>>>  Documentation/media/kapi/v4l2-subdev.rst | 29 +++++++++++++++++++++++++++++
> > > >>>>>>  1 file changed, 29 insertions(+)
> > > >>>>>>
> > > >>>>>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
> > > >>>>>> index e1f0b72..45088ad 100644
> > > >>>>>> --- a/Documentation/media/kapi/v4l2-subdev.rst
> > > >>>>>> +++ b/Documentation/media/kapi/v4l2-subdev.rst
> > > >>>>>> @@ -262,6 +262,35 @@ is called. After all subdevices have been located the .complete() callback is
> > > >>>>>>  called. When a subdevice is removed from the system the .unbind() method is
> > > >>>>>>  called. All three callbacks are optional.
> > > >>>>>>
> > > >>>>>> +Streaming control on Media controller enabled devices
> > > >>>>>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > >>>>>> +
> > > >>>>>> +Starting and stopping the stream are somewhat complex operations that
> > > >>>>>> +often require walking the media graph to enable streaming on
> > > >>>>>> +sub-devices which the pipeline consists of. This involves interaction
> > > >>>>>> +between multiple drivers, sometimes more than two.      
> > > >>>>>
> > > >>>>> That's still not ok, as it creates a black hole for devnode-based
> > > >>>>> devices.
> > > >>>>>
> > > >>>>> I would change it to something like:
> > > >>>>>
> > > >>>>> 	Streaming control
> > > >>>>> 	^^^^^^^^^^^^^^^^^
> > > >>>>>
> > > >>>>> 	Starting and stopping the stream are somewhat complex operations that
> > > >>>>> 	often require to enable streaming on sub-devices which the pipeline
> > > >>>>> 	consists of. This involves interaction between multiple drivers, sometimes
> > > >>>>> 	more than two.
> > > >>>>>
> > > >>>>> 	The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> > > >>>>> 	for starting and stopping the stream on the sub-device it is called
> > > >>>>> 	on.
> > > >>>>>
> > > >>>>> 	Streaming control on devnode-centric devices
> > > >>>>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >>>>>
> > > >>>>> 	On **devnode-centric** devices, the main driver is responsible enable
> > > >>>>> 	stream all all sub-devices. On most cases, all the main driver need
> > > >>>>> 	to do is to broadcast s_stream to all connected sub-devices by calling
> > > >>>>> 	:c:func:`v4l2_device_call_all`, e. g.::
> > > >>>>>
> > > >>>>> 		v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 1);      
> > > >>>>
> > > >>>> Looks good to me.
> > > >>>>    
> > > >>>>>
> > > >>>>> 	Streaming control on mc-centric devices
> > > >>>>> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >>>>>
> > > >>>>> 	On **mc-centric** devices, it usually requires walking the media graph
> > > >>>>> 	to enable streaming only at the sub-devices which the pipeline consists
> > > >>>>> 	of.
> > > >>>>>
> > > >>>>> 	(place here the details for such scenario)      
> > > >>>>
> > > >>>> This part requires a more detailed description of the problem area. What 
> > > >>>> makes a difference here is that there's a pipeline this pipeline may be 
> > > >>>> controlled more than one driver. (More elaborate discussion below.)
> > > >>>>    
> > > >>>>>      
> > > >>>>>> +The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
> > > >>>>>> +for starting and stopping the stream on the sub-device it is called
> > > >>>>>> +on. A device driver is only responsible for calling the ``.s_stream()`` ops
> > > >>>>>> +of the adjacent sub-devices that are connected to its sink pads
> > > >>>>>> +through an enabled link. A driver may not call ``.s_stream()`` op
> > > >>>>>> +of any other sub-device further up in the pipeline, for instance.
> > > >>>>>> +
> > > >>>>>> +This means that a sub-device driver is thus in direct control of
> > > >>>>>> +whether the upstream sub-devices start (or stop) streaming before or
> > > >>>>>> +after the sub-device itself is set up for streaming.
> > > >>>>>> +
> > > >>>>>> +.. note::
> > > >>>>>> +
> > > >>>>>> +   As the ``.s_stream()`` callback is called recursively through the
> > > >>>>>> +   sub-devices along the pipeline, it is important to keep the
> > > >>>>>> +   recursion as short as possible. To this end, drivers are encouraged
> > > >>>>>> +   to avoid recursively calling ``.s_stream()`` internally to reduce
> > > >>>>>> +   stack usage. Instead, the ``.s_stream()`` op of the directly
> > > >>>>>> +   connected sub-devices should come from the callback through which
> > > >>>>>> +   the driver was first called.
> > > >>>>>> +      
> > > >>>>>
> > > >>>>> That sounds too complex, and can lead into troubles, if the same
> > > >>>>> sub-device driver is used on completely different devices.
> > > >>>>>
> > > >>>>> IMHO, it should be up to the main driver to navigate at the MC
> > > >>>>> pipeline and call s_stream(), and not to the sub-drivers.      
> > > >>>>
> > > >>>> I would agree with the above statement *if* we had no devices that 
> > > >>>> require doing this in a different way.
> > > >>>>
> > > >>>> Consider the following case:
> > > >>>>
> > > >>>> 	sensor   -> CSI-2 receiver -> ISP (DMA)
> > > >>>> 	subdev A -> subdev B	   -> video node    
> > > >>>
> > > >>> Let me be clearer about the issue I see.
> > > >>>
> > > >>> In the above example, what subdevs are supposed to multicast the
> > > >>> s_stream() to their neighbors, and how they will know that they
> > > >>> need to multicast it.
> > > >>>
> > > >>> Let's say, that, in the first pipeline, it would be the sensor
> > > >>> and subdev A. How "sensor" and "subdev A" will know that they're
> > > >>> meant to broadcast s_stream(), and the other entities know they
> > > >>> won't?    
> > > >>
> > > >> So my understanding is that the bridge driver (ISP) will call s_stream
> > > >> for the CSI-2 receiver, and that in turn calls s_stream of the sensor.  
> > > > 
> > > > Alternatively, the ISP driver could call s_stream for both CSI-2 and
> > > > sensor.
> > > >   
> > > >> This should only be done for mc-centric devices, so we need a clear
> > > >> property telling a subdev whether it is part of an mc-centric pipeline
> > > >> or a devnode-centric pipeline. Since in the latter case it should not
> > > >> call s_stream in this way. For devnode-centric pipelines the bridge
> > > >> driver broadcasts s_stream to all subdevs.  
> > > > 
> > > > It would be easier to have a logic called by the ISP driver that would
> > > > get all elements from an active pipeline and call s_stream() there.
> > > > 
> > > > That would keep the flexibility that would be needed by devices with a
> > > > separate CSI-2 receiver, while preventing the addition of an extra
> > > > logic on every sub-device to teach them that s_stream() should be
> > > > called to communicate with some specific sub-device.
> > > > 
> > > > The thing is that, if you have a pipeline like:
> > > > 
> > > > source subdev -> subdev A -> subdev B -> subdev C -> subdev D -> DMA
> > > > 
> > > > due to PM constraints, all subdevs on such pipeline may require s_stream()
> > > > to control its power consumption. So, it is not just a CSI-2 device that
> > > > would need to enable power at the sensor, but an entire pipeline that
> > > > would need to receive s_stream() calls.  
> > > 
> > > So? If DMA calls s_stream for subdev D, which calls s_stream for subdev C, etc.
> > > until the source subdev's s_stream is called, then they are all powered up,
> > > aren't they?
> > > 
> > > I don't see the difference between this and the DMA calling s_stream for all
> > > subdevs.
> > > 
> > > One clear difference is (as Sakari mentioned) stack usage. There having the
> > > DMA call s_stream is clearly better. See below for more comments...
> > 
> > That's not the only difference. If we pass this to subdevs, they
> > need to know that they're working on a mc-centric way.
> 
> Another difference is that in a mc-centric driver more information might 
> be needed in the s_stream call then currently is available and would be 
> harder to do if the DMA driver calls s_stream on all subdevices. One 
> example of this is in the patch series '[PATCH 00/20] Add multiplexed 
> media pads to support CSI-2 virtual channels' which tries to add 
> multiplexed pads which can contain more then one stream.

I'll try to take a look on such patch series later this week.

> 
> In that series the s_stream() operation is extended to carry information 
> about both which pad and which stream the s_stream() operation intends 
> to start.

If you change it, it won't likely work anymore for existing devices, as, 
on all currently implemented usecases, s_stream() doesn't care about pads. 
The hole idea behind the original implementations for s_stream() is to
actually power on the device as a hole.

Also, I can't see how this would work on broadcast mode[1].

[1] On devnode-centric devices, s_stream() is just broadcasted to all
audio and video devices, in order to wake them up.

Btw, devnode-centric may even be compiled with MC disabled - so, there
won't be a MC graph with pads information.

So, it sounds that we'll need to have a different callback instead of
changing s_stream() - something like s_stream_subdev() or s_stream_pad().

> Then each subdevice is responsible to call s_stream() on it's 
> neighbor providing which pad and stream of the remote it wants to start 
> based on which of its own pads and possibly stream s_stream() as called 
> up on itself. This propagation by each subdevice then becomes much more 
> useful as it's only the subdevice itself that knows about the internal 
> subdevice routing from its sink to source pad(s). Maybe this can also be 
> covered by some generic v4l2 helper to start the stream, but it feels 
> like such implementations could become complex.

Even with a new callback, recursion can be very bad. The pipeline might
have a loop. Even if it doesn't, some protection may be needed to avoid
too long pipelines that could cause stack overflow.

Thanks,
Mauro

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Philipp Zabel]

On Mon, 2017-08-21 at 11:01 -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Aug 2017 15:52:17 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On 08/21/2017 02:07 PM, Mauro Carvalho Chehab wrote:
> > > Em Mon, 21 Aug 2017 12:14:18 +0200
> > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > >   
> > > > On 08/21/2017 11:08 AM, Mauro Carvalho Chehab wrote:  
> > > > > Em Mon, 21 Aug 2017 09:36:49 +0300
> > > > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > > > >     
> > > > > > Hi Mauro,
> > > > > > 
> > > > > > Mauro Carvalho Chehab wrote:    
> > > > > > > Hi Sakari,
> > > > > > > 
> > > > > > > Em Wed, 16 Aug 2017 15:20:17 +0300
> > > > > > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > > > > > >      
> > > > > > > > As we begin to add support for systems with Media
> > > > > > > > controller pipelines
> > > > > > > > controlled by more than one device driver, it is
> > > > > > > > essential that we
> > > > > > > > precisely define the responsibilities of each component
> > > > > > > > in the stream
> > > > > > > > control and common practices.
> > > > > > > > 
> > > > > > > > Specifically, streaming control is done per sub-device
> > > > > > > > and sub-device
> > > > > > > > drivers themselves are responsible for streaming setup
> > > > > > > > in upstream
> > > > > > > > sub-devices.      
> > > > > > > 
> > > > > > > IMO, before this patch, we need something like this:
> > > > > > > 	https://patchwork.linuxtv.org/patch/43325/      
> > > > > > 
> > > > > > Thanks. I'll reply separately to that thread.
> > > > > >    
> > > > > > >      
> > > > > > > > 
> > > > > > > > Signed-off-by: Sakari Ailus
> > > > > > > > <sakari.ailus@linux.intel.com>
> > > > > > > > Acked-by: Niklas Söderlund
> > > > > > > > <niklas.soderlund+renesas@ragnatech.se>
> > > > > > > > ---
> > > > > > > >  Documentation/media/kapi/v4l2-subdev.rst | 29
> > > > > > > > +++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 29 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/Documentation/media/kapi/v4l2-subdev.rst
> > > > > > > > b/Documentation/media/kapi/v4l2-subdev.rst
> > > > > > > > index e1f0b72..45088ad 100644
> > > > > > > > --- a/Documentation/media/kapi/v4l2-subdev.rst
> > > > > > > > +++ b/Documentation/media/kapi/v4l2-subdev.rst
> > > > > > > > @@ -262,6 +262,35 @@ is called. After all subdevices
> > > > > > > > have been located the .complete() callback is
> > > > > > > >  called. When a subdevice is removed from the system
> > > > > > > > the .unbind() method is
> > > > > > > >  called. All three callbacks are optional.
> > > > > > > > 
> > > > > > > > +Streaming control on Media controller enabled devices
> > > > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > > > > +
> > > > > > > > +Starting and stopping the stream are somewhat complex
> > > > > > > > operations that
> > > > > > > > +often require walking the media graph to enable
> > > > > > > > streaming on
> > > > > > > > +sub-devices which the pipeline consists of. This
> > > > > > > > involves interaction
> > > > > > > > +between multiple drivers, sometimes more than
> > > > > > > > two.      
> > > > > > > 
> > > > > > > That's still not ok, as it creates a black hole for
> > > > > > > devnode-based
> > > > > > > devices.
> > > > > > > 
> > > > > > > I would change it to something like:
> > > > > > > 
> > > > > > > 	Streaming control
> > > > > > > 	^^^^^^^^^^^^^^^^^
> > > > > > > 
> > > > > > > 	Starting and stopping the stream are somewhat complex
> > > > > > > operations that
> > > > > > > 	often require to enable streaming on sub-devices which
> > > > > > > the pipeline
> > > > > > > 	consists of. This involves interaction between multiple
> > > > > > > drivers, sometimes
> > > > > > > 	more than two.
> > > > > > > 
> > > > > > > 	The ``.s_stream()`` op in
> > > > > > > :c:type:`v4l2_subdev_video_ops` is responsible
> > > > > > > 	for starting and stopping the stream on the sub-device
> > > > > > > it is called
> > > > > > > 	on.
> > > > > > > 
> > > > > > > 	Streaming control on devnode-centric devices
> > > > > > > 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > > 
> > > > > > > 	On **devnode-centric** devices, the main driver is
> > > > > > > responsible enable
> > > > > > > 	stream all all sub-devices. On most cases, all the main
> > > > > > > driver need
> > > > > > > 	to do is to broadcast s_stream to all connected sub-
> > > > > > > devices by calling
> > > > > > > 	:c:func:`v4l2_device_call_all`, e. g.::
> > > > > > > 
> > > > > > > 		v4l2_device_call_all(&dev->v4l2_dev, 0, video,
> > > > > > > s_stream, 1);      
> > > > > > 
> > > > > > Looks good to me.
> > > > > >    
> > > > > > > 
> > > > > > > 	Streaming control on mc-centric devices
> > > > > > > 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > > 
> > > > > > > 	On **mc-centric** devices, it usually requires walking
> > > > > > > the media graph
> > > > > > > 	to enable streaming only at the sub-devices which the
> > > > > > > pipeline consists
> > > > > > > 	of.
> > > > > > > 
> > > > > > > 	(place here the details for such scenario)      
> > > > > > 
> > > > > > This part requires a more detailed description of the
> > > > > > problem area. What 
> > > > > > makes a difference here is that there's a pipeline this
> > > > > > pipeline may be 
> > > > > > controlled more than one driver. (More elaborate discussion
> > > > > > below.)
> > > > > >    
> > > > > > >      
> > > > > > > > +The ``.s_stream()`` op in
> > > > > > > > :c:type:`v4l2_subdev_video_ops` is responsible
> > > > > > > > +for starting and stopping the stream on the sub-device 
> > > > > > > > it is called
> > > > > > > > +on. A device driver is only responsible for calling
> > > > > > > > the ``.s_stream()`` ops
> > > > > > > > +of the adjacent sub-devices that are connected to its
> > > > > > > > sink pads
> > > > > > > > +through an enabled link. A driver may not call
> > > > > > > > ``.s_stream()`` op
> > > > > > > > +of any other sub-device further up in the pipeline,
> > > > > > > > for instance.
> > > > > > > > +
> > > > > > > > +This means that a sub-device driver is thus in direct
> > > > > > > > control of
> > > > > > > > +whether the upstream sub-devices start (or stop)
> > > > > > > > streaming before or
> > > > > > > > +after the sub-device itself is set up for streaming.
> > > > > > > > +
> > > > > > > > +.. note::
> > > > > > > > +
> > > > > > > > +   As the ``.s_stream()`` callback is called
> > > > > > > > recursively through the
> > > > > > > > +   sub-devices along the pipeline, it is important to
> > > > > > > > keep the
> > > > > > > > +   recursion as short as possible. To this end,
> > > > > > > > drivers are encouraged
> > > > > > > > +   to avoid recursively calling ``.s_stream()``
> > > > > > > > internally to reduce
> > > > > > > > +   stack usage. Instead, the ``.s_stream()`` op of the
> > > > > > > > directly
> > > > > > > > +   connected sub-devices should come from the callback
> > > > > > > > through which
> > > > > > > > +   the driver was first called.
> > > > > > > > +      
> > > > > > > 
> > > > > > > That sounds too complex, and can lead into troubles, if
> > > > > > > the same
> > > > > > > sub-device driver is used on completely different
> > > > > > > devices.
> > > > > > > 
> > > > > > > IMHO, it should be up to the main driver to navigate at
> > > > > > > the MC
> > > > > > > pipeline and call s_stream(), and not to the sub-
> > > > > > > drivers.      
> > > > > > 
> > > > > > I would agree with the above statement *if* we had no
> > > > > > devices that 
> > > > > > require doing this in a different way.
> > > > > > 
> > > > > > Consider the following case:
> > > > > > 
> > > > > > 	sensor   -> CSI-2 receiver -> ISP (DMA)
> > > > > > 	subdev A -> subdev B	   -> video node    
> > > > > 
> > > > > Let me be clearer about the issue I see.
> > > > > 
> > > > > In the above example, what subdevs are supposed to multicast
> > > > > the
> > > > > s_stream() to their neighbors, and how they will know that
> > > > > they
> > > > > need to multicast it.
> > > > > 
> > > > > Let's say, that, in the first pipeline, it would be the
> > > > > sensor
> > > > > and subdev A. How "sensor" and "subdev A" will know that
> > > > > they're
> > > > > meant to broadcast s_stream(), and the other entities know
> > > > > they
> > > > > won't?    
> > > > 
> > > > So my understanding is that the bridge driver (ISP) will call
> > > > s_stream
> > > > for the CSI-2 receiver, and that in turn calls s_stream of the
> > > > sensor.  
> > > 
> > > Alternatively, the ISP driver could call s_stream for both CSI-2
> > > and
> > > sensor.
> > >   
> > > > This should only be done for mc-centric devices, so we need a
> > > > clear
> > > > property telling a subdev whether it is part of an mc-centric
> > > > pipeline
> > > > or a devnode-centric pipeline. Since in the latter case it
> > > > should not
> > > > call s_stream in this way. For devnode-centric pipelines the
> > > > bridge
> > > > driver broadcasts s_stream to all subdevs.  
> > > 
> > > It would be easier to have a logic called by the ISP driver that
> > > would
> > > get all elements from an active pipeline and call s_stream()
> > > there.
> > > 
> > > That would keep the flexibility that would be needed by devices
> > > with a
> > > separate CSI-2 receiver, while preventing the addition of an
> > > extra
> > > logic on every sub-device to teach them that s_stream() should be
> > > called to communicate with some specific sub-device.
> > > 
> > > The thing is that, if you have a pipeline like:
> > > 
> > > source subdev -> subdev A -> subdev B -> subdev C -> subdev D ->
> > > DMA
> > > 
> > > due to PM constraints, all subdevs on such pipeline may require
> > > s_stream()
> > > to control its power consumption. So, it is not just a CSI-2
> > > device that
> > > would need to enable power at the sensor, but an entire pipeline
> > > that
> > > would need to receive s_stream() calls.  
> > 
> > So? If DMA calls s_stream for subdev D, which calls s_stream for
> > subdev C, etc.
> > until the source subdev's s_stream is called, then they are all
> > powered up,
> > aren't they?
> > 
> > I don't see the difference between this and the DMA calling
> > s_stream for all
> > subdevs.
> > 
> > One clear difference is (as Sakari mentioned) stack usage. There
> > having the
> > DMA call s_stream is clearly better. See below for more comments...
> 
> That's not the only difference. If we pass this to subdevs, they
> need to know that they're working on a mc-centric way.
> 
> There is another problem, with is more serious, and it is somewhat
> related to stack usage: what happens if there's a loop inside a
> pipeline? That would cause a Kernel crash. I can't see any easy
> way to avoid that (nor stack starving, if the pipeline is too big).
> 
> > 
> > > IMO, the best would be a logic similar to
> > > media_entity_pipeline_start() that,
> > > for each entity at the pipeline, s_stream() would be called, e.
> > > g. something
> > > like:
> > > 
> > > 	v4l2_subdev_pipeline_call()
> > >   
> > > > For the record, I am not aware of any subdevs that are used by
> > > > both
> > > > mc and devnode-centric scenarios AND that can sit in the middle
> > > > of a
> > > > pipeline. Sensors/video receiver subdevs can certainly be used
> > > > in both
> > > > scenarios, but they don't have to propagate a s_stream call.  
> > > 
> > > em28xx has a bunch of sensors that are also used on embedded
> > > drivers.
> > > It also has a tvp5150, with is also used by omap3.
> > > 
> > > On the only OMAP3 board whose has a DT upstream, the tvp5150 has
> > > both source
> > > pads connected to S-Video connectors, but nothing prevents that
> > > someone
> > > would add a tuner before it. I remember someone mentioned that
> > > such device
> > > exists (although its DT is not upstream).
> > > 
> > > On an embedded device with both tvp5150 and a tuner, s_stream()
> > > should be
> > > called by both.
> > >   
> > > > 
> > > > It would be very helpful if we have a good description of these
> > > > two
> > > > scenarios in our documentation, and a capability indicating mc-
> > > > centric
> > > > behavior for devnodes. And also for v4l2-subdevs internally
> > > > (i.e.
> > > > am I used in a mc-centric scenario or not?).
> > > > 
> > > > Then this documentation will start to make more sense as well.
> > > >  
> > > > > Also, the same sensor may be used on a device whose CSI-2 is
> > > > > integrated at the ISP driver (the main driver). That's why
> > > > > I think that such logic should be started by the main driver,
> > > > > as
> > > > > it is the only part of the pipeline that it is aware about
> > > > > what it is needed. Also, as the DMA engines are controlled by
> > > > > the main driver (via its associated video devnodes), it is
> > > > > the only
> > > > > part of the pipeline that knows when a stream starts.    
> > > > 
> > > > Yes, and this driver is the one that calls s_stream on the
> > > > adjacent subdevs. But just those and not all.
> > > >  
> > > > >     
> > > > > > Assume that the CSI-2 receiver requires hardware setup both
> > > > > > *before and 
> > > > > > after* streaming has been enabled on the sensor.    
> > > > > 
> > > > > calling s_stream() before and after seems to be an abuse of
> > > > > it.    
> > > > 
> > > > I think you misunderstand what Sakari tries to say.
> > > > 
> > > > In the scenario above the bridge driver calls s_stream for the
> > > > CSI receiver. That in turn has code like this:
> > > > 
> > > > s_stream(bool enable)
> > > > {
> > > > 	... initialize CSI ...
> > > > 	if error initializing CSI
> > > > 		return error
> > > > 	call s_stream for adjacent source subdev (i.e. sensor)
> > > > 	if success
> > > > 		return 0
> > > > 	... de-initialize CSI
> > > > 	return error
> > > > }
> > > > 
> > > > This makes a lot of sense for mc-centric devices and is also
> > > > much more
> > > > precise than the broadcast that a devnode-centric device would
> > > > do.
> > > > 
> > > > In the very unlikely case that this CSI subdev would also be
> > > > used in
> > > > a devnode-centric scenario the s_stream implementation would
> > > > just
> > > > return 0 after it initializes the CSI hardware. It will depend
> > > > on
> > > > the hardware whether that works or not.
> > > > 
> > > > subdevs used in devnode-centric scenarios tend to be pretty
> > > > simple
> > > > and are able to handle this.  
> > > 
> > > If I understand well, you're basically concerned about error
> > > handling, right?
> > > 
> > > That could easily be handled with something like:
> > > 
> > > 	ret = v4l2_subdev_pipeline_call(source_entity, video, s_stream,
> > > 1);
> > > 	if (ret < 0) {
> > > 		v4l2_subdev_pipeline_call(source_entity, video,
> > > s_stream, 0);
> > > 		return ret;
> > > 	}  
> > 
> > That's not it. It is that some subdevs need to execute
> > initialization code
> > first before you can call s_stream on the sensor.
> > 
> > If you want to use a pipeline_call you would need to introduce
> > stream_prepare
> > and stream_unprepare ops to achieve the same (idea stolen from the
> > clk API).
> > 
> > I'm not sure if this would fully resolve Sakari's issue.
> 
> If the problem is with initialization, all we need to enforce is
> that the graph traversal will happen at the reverse order, e. g.
> from subdev D to subdev A.
> 
> But yeah, if it is more complex, we may need a stream_prepare ops.

Have we just come full circle [1]?

[1] https://patchwork.linuxtv.org/patch/37321/

I'm not convinced anymore that this would be enough for all possible
use cases, though. What if there are multiple chained CSI-2 links links
in the pipeline, like a CSI-2 merge element between cameras and
receiver that need to be perpared and started one link after the other?

regards
Philipp

Re: [PATCH v2 1/2] docs-rst: media: Document s_stream() video op usage for MC enabled devices

[Sakari Ailus]

Hi Hans,

On 08/21/17 13:14, Hans Verkuil wrote:
...
>>>>> +The ``.s_stream()`` op in :c:type:`v4l2_subdev_video_ops` is responsible
>>>>> +for starting and stopping the stream on the sub-device it is called
>>>>> +on. A device driver is only responsible for calling the ``.s_stream()`` ops
>>>>> +of the adjacent sub-devices that are connected to its sink pads
>>>>> +through an enabled link. A driver may not call ``.s_stream()`` op
>>>>> +of any other sub-device further up in the pipeline, for instance.
>>>>> +
>>>>> +This means that a sub-device driver is thus in direct control of
>>>>> +whether the upstream sub-devices start (or stop) streaming before or
>>>>> +after the sub-device itself is set up for streaming.
>>>>> +
>>>>> +.. note::
>>>>> +
>>>>> +   As the ``.s_stream()`` callback is called recursively through the
>>>>> +   sub-devices along the pipeline, it is important to keep the
>>>>> +   recursion as short as possible. To this end, drivers are encouraged
>>>>> +   to avoid recursively calling ``.s_stream()`` internally to reduce
>>>>> +   stack usage. Instead, the ``.s_stream()`` op of the directly
>>>>> +   connected sub-devices should come from the callback through which
>>>>> +   the driver was first called.
>>>>> +  
>>>>
>>>> That sounds too complex, and can lead into troubles, if the same
>>>> sub-device driver is used on completely different devices.
>>>>
>>>> IMHO, it should be up to the main driver to navigate at the MC
>>>> pipeline and call s_stream(), and not to the sub-drivers.  
>>>
>>> I would agree with the above statement *if* we had no devices that 
>>> require doing this in a different way.
>>>
>>> Consider the following case:
>>>
>>> 	sensor   -> CSI-2 receiver -> ISP (DMA)
>>> 	subdev A -> subdev B	   -> video node
>>
>> Let me be clearer about the issue I see.
>>
>> In the above example, what subdevs are supposed to multicast the
>> s_stream() to their neighbors, and how they will know that they
>> need to multicast it.
>>
>> Let's say, that, in the first pipeline, it would be the sensor
>> and subdev A. How "sensor" and "subdev A" will know that they're
>> meant to broadcast s_stream(), and the other entities know they
>> won't?
> 
> So my understanding is that the bridge driver (ISP) will call s_stream
> for the CSI-2 receiver, and that in turn calls s_stream of the sensor.
> 
> This should only be done for mc-centric devices, so we need a clear
> property telling a subdev whether it is part of an mc-centric pipeline
> or a devnode-centric pipeline. Since in the latter case it should not
> call s_stream in this way. For devnode-centric pipelines the bridge
> driver broadcasts s_stream to all subdevs.
> 
> For the record, I am not aware of any subdevs that are used by both
> mc and devnode-centric scenarios AND that can sit in the middle of a
> pipeline. Sensors/video receiver subdevs can certainly be used in both
> scenarios, but they don't have to propagate a s_stream call.
> 
> It would be very helpful if we have a good description of these two
> scenarios in our documentation, and a capability indicating mc-centric
> behavior for devnodes. And also for v4l2-subdevs internally (i.e.
> am I used in a mc-centric scenario or not?).
> 
> Then this documentation will start to make more sense as well.
> 
>> Also, the same sensor may be used on a device whose CSI-2 is
>> integrated at the ISP driver (the main driver). That's why
>> I think that such logic should be started by the main driver, as
>> it is the only part of the pipeline that it is aware about
>> what it is needed. Also, as the DMA engines are controlled by
>> the main driver (via its associated video devnodes), it is the only
>> part of the pipeline that knows when a stream starts.
> 
> Yes, and this driver is the one that calls s_stream on the
> adjacent subdevs. But just those and not all.
> 
>>
>>> Assume that the CSI-2 receiver requires hardware setup both *before and 
>>> after* streaming has been enabled on the sensor.
>>
>> calling s_stream() before and after seems to be an abuse of it.
> 
> I think you misunderstand what Sakari tries to say.
> 
> In the scenario above the bridge driver calls s_stream for the
> CSI receiver. That in turn has code like this:
> 
> s_stream(bool enable)
> {
> 	... initialize CSI ...
> 	if error initializing CSI
> 		return error
> 	call s_stream for adjacent source subdev (i.e. sensor)
> 	if success
> 		return 0
> 	... de-initialize CSI
> 	return error

This isn't really about error handling: error handling can and is being
done by calling s_stream(0) on subdevs on which streaming had already
been started.

There are two purposes:

1) The knowledge whether the the upstream sub-device should start
streaming before or after is ultimately device specific. A driver for
another device may not know that (or without this information being
explicitly conveyed for which we don't have an API).

2) There may be a need to prepare things before streaming is started on
an upstream sub-device as well as proceed with hardware setup *after*
starting streaming on an upstream sub-device. I.e.

	subdev_s_stream(bool enable)
	{
		do some hardware setup;
		call s_stream on a sensor;
		do more hardware setup;
	}

-- 
Sakari Ailus
sakari.ailus@linux.intel.com