[PATCH 1/2] media: Documentation: Deprecate s_stream video op, update docs

[PATCH 1/2] media: Documentation: Deprecate s_stream video op, update docs

[Sakari Ailus]

The scope of the s_stream video operation is now fully supported by
{enable,disable}_straems. Explicitly document the s_stream() op as
deprecated and update the related documentation.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/driver-api/media/camera-sensor.rst |  8 ++++----
 Documentation/driver-api/media/tx-rx.rst         | 11 ++++++-----
 include/media/v4l2-subdev.h                      |  5 +++--
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
index b4920b34cebc..c290833165e6 100644
--- a/Documentation/driver-api/media/camera-sensor.rst
+++ b/Documentation/driver-api/media/camera-sensor.rst
@@ -81,10 +81,10 @@ restart when the system is resumed. This requires coordination between the
 camera sensor and the rest of the camera pipeline. Bridge drivers are
 responsible for this coordination, and instruct camera sensors to stop and
 restart streaming by calling the appropriate subdev operations
-(``.s_stream()``, ``.enable_streams()`` or ``.disable_streams()``). Camera
-sensor drivers shall therefore **not** keep track of the streaming state to
-stop streaming in the PM suspend handler and restart it in the resume handler.
-Drivers should in general not implement the system PM handlers.
+(``.enable_streams()`` or ``.disable_streams()``). Camera sensor drivers shall
+therefore **not** keep track of the streaming state to stop streaming in the PM
+suspend handler and restart it in the resume handler. Drivers should in general
+not implement the system PM handlers.
 
 Camera sensor drivers shall **not** implement the subdev ``.s_power()``
 operation, as it is deprecated. While this operation is implemented in some
diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
index 29d66a47b56e..a339df61fca8 100644
--- a/Documentation/driver-api/media/tx-rx.rst
+++ b/Documentation/driver-api/media/tx-rx.rst
@@ -49,11 +49,12 @@ Link frequency
 The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
 receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
 
-``.s_stream()`` callback
-^^^^^^^^^^^^^^^^^^^^^^^^
+``.enable_streams()`` and ``.disable_streams()`` callbacks
+^^^^^^^^^^^^^^^^^^^^^^^^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-The struct struct v4l2_subdev_video_ops->s_stream() callback is used by the
-receiver driver to control the transmitter driver's streaming state.
+The struct v4l2_subdev_pad_ops->enable_streams() and struct
+v4l2_subdev_pad_ops->disable_streams() callbacks are used by the receiver driver
+to control the transmitter driver's streaming state.
 
 
 CSI-2 transmitter drivers
@@ -127,7 +128,7 @@ 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
+calling the ``.disable_streams()`` 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
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 8daa0929865c..3cc6b4a5935f 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -450,8 +450,9 @@ enum v4l2_subdev_pre_streamon_flags {
  *	already started or stopped subdev. Also see call_s_stream wrapper in
  *	v4l2-subdev.c.
  *
- *	New drivers should instead implement &v4l2_subdev_pad_ops.enable_streams
- *	and &v4l2_subdev_pad_ops.disable_streams operations, and use
+ *	This callback is DEPRECATED. New drivers should instead implement
+ *	&v4l2_subdev_pad_ops.enable_streams and
+ *	&v4l2_subdev_pad_ops.disable_streams operations, and use
  *	v4l2_subdev_s_stream_helper for the &v4l2_subdev_video_ops.s_stream
  *	operation to support legacy users.
  *
-- 
2.39.5

[PATCH 2/2] media: Documentation: Update {enable,disable}_streams documentation

[Sakari Ailus]

Document the expected {enable,disable}_streams callback behaviour for
drivers that are stream-unaware i.e. don't specify the
V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
the mask argument can be ignored.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/v4l2-subdev.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 3cc6b4a5935f..67a6e6ec58b8 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -834,11 +834,19 @@ struct v4l2_subdev_state {
  *	v4l2_subdev_init_finalize() at initialization time). Do not call
  *	directly, use v4l2_subdev_enable_streams() instead.
  *
+ *	Drivers that support only a single stream without setting the
+ *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
+ *	be concerned with the mask argument.
+ *
  * @disable_streams: Disable the streams defined in streams_mask on the given
  *	source pad. Subdevs that implement this operation must use the active
  *	state management provided by the subdev core (enabled through a call to
  *	v4l2_subdev_init_finalize() at initialization time). Do not call
  *	directly, use v4l2_subdev_disable_streams() instead.
+ *
+ *	Drivers that support only a single stream without setting the
+ *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
+ *	be concerned with the mask argument.
  */
 struct v4l2_subdev_pad_ops {
 	int (*enum_mbus_code)(struct v4l2_subdev *sd,
-- 
2.39.5

Re: [PATCH 2/2] media: Documentation: Update {enable,disable}_streams documentation

[Laurent Pinchart]

On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
> Document the expected {enable,disable}_streams callback behaviour for
> drivers that are stream-unaware i.e. don't specify the
> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
> the mask argument can be ignored.

Wouldn't it be better to use BIT(0) in that case to simplifiy
interoperability with stream-aware devices ?

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/v4l2-subdev.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 3cc6b4a5935f..67a6e6ec58b8 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -834,11 +834,19 @@ struct v4l2_subdev_state {
>   *	v4l2_subdev_init_finalize() at initialization time). Do not call
>   *	directly, use v4l2_subdev_enable_streams() instead.
>   *
> + *	Drivers that support only a single stream without setting the
> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> + *	be concerned with the mask argument.
> + *
>   * @disable_streams: Disable the streams defined in streams_mask on the given
>   *	source pad. Subdevs that implement this operation must use the active
>   *	state management provided by the subdev core (enabled through a call to
>   *	v4l2_subdev_init_finalize() at initialization time). Do not call
>   *	directly, use v4l2_subdev_disable_streams() instead.
> + *
> + *	Drivers that support only a single stream without setting the
> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> + *	be concerned with the mask argument.
>   */
>  struct v4l2_subdev_pad_ops {
>  	int (*enum_mbus_code)(struct v4l2_subdev *sd,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: Documentation: Update {enable,disable}_streams documentation

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 17, 2024 at 04:00:47PM +0300, Laurent Pinchart wrote:
> On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
> > Document the expected {enable,disable}_streams callback behaviour for
> > drivers that are stream-unaware i.e. don't specify the
> > V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
> > the mask argument can be ignored.
> 
> Wouldn't it be better to use BIT(0) in that case to simplifiy
> interoperability with stream-aware devices ?

That's indeed the current implementation.

-- 
Regards,

Sakari Ailus

Re: [PATCH 2/2] media: Documentation: Update {enable,disable}_streams documentation

[Tomi Valkeinen]

On 17/09/2024 16:00, Laurent Pinchart wrote:
> On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
>> Document the expected {enable,disable}_streams callback behaviour for
>> drivers that are stream-unaware i.e. don't specify the
>> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
>> the mask argument can be ignored.
> 
> Wouldn't it be better to use BIT(0) in that case to simplifiy
> interoperability with stream-aware devices ?

The caller has to set BIT(0), but I think here the documentation is 
about the callee.

If the driver is not stream aware and implements the callbacks, it will 
get BIT(0) as the mask parameter (do we enforce this?), but as there's 
nothing it can do with the parameter it "does not need to be concerned 
with the mask argument".

  Tomi

>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>   include/media/v4l2-subdev.h | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 3cc6b4a5935f..67a6e6ec58b8 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -834,11 +834,19 @@ struct v4l2_subdev_state {
>>    *	v4l2_subdev_init_finalize() at initialization time). Do not call
>>    *	directly, use v4l2_subdev_enable_streams() instead.
>>    *
>> + *	Drivers that support only a single stream without setting the
>> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
>> + *	be concerned with the mask argument.
>> + *
>>    * @disable_streams: Disable the streams defined in streams_mask on the given
>>    *	source pad. Subdevs that implement this operation must use the active
>>    *	state management provided by the subdev core (enabled through a call to
>>    *	v4l2_subdev_init_finalize() at initialization time). Do not call
>>    *	directly, use v4l2_subdev_disable_streams() instead.
>> + *
>> + *	Drivers that support only a single stream without setting the
>> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
>> + *	be concerned with the mask argument.
>>    */
>>   struct v4l2_subdev_pad_ops {
>>   	int (*enum_mbus_code)(struct v4l2_subdev *sd,
> 

Re: [PATCH 2/2] media: Documentation: Update {enable,disable}_streams documentation

[Laurent Pinchart]

On Tue, Sep 17, 2024 at 05:16:25PM +0300, Tomi Valkeinen wrote:
> On 17/09/2024 16:00, Laurent Pinchart wrote:
> > On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
> >> Document the expected {enable,disable}_streams callback behaviour for
> >> drivers that are stream-unaware i.e. don't specify the
> >> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
> >> the mask argument can be ignored.
> > 
> > Wouldn't it be better to use BIT(0) in that case to simplifiy
> > interoperability with stream-aware devices ?
> 
> The caller has to set BIT(0), but I think here the documentation is 
> about the callee.
> 
> If the driver is not stream aware and implements the callbacks, it will 
> get BIT(0) as the mask parameter (do we enforce this?), but as there's 
> nothing it can do with the parameter it "does not need to be concerned 
> with the mask argument".

Right. I had misunderstood the patch.

> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> ---
> >>   include/media/v4l2-subdev.h | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> index 3cc6b4a5935f..67a6e6ec58b8 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -834,11 +834,19 @@ struct v4l2_subdev_state {
> >>    *	v4l2_subdev_init_finalize() at initialization time). Do not call
> >>    *	directly, use v4l2_subdev_enable_streams() instead.
> >>    *
> >> + *	Drivers that support only a single stream without setting the
> >> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to

s/capatility/capability/

Same below.

> >> + *	be concerned with the mask argument.

How about "can ignore the mask argument" instead ? I interpreted as "not
need to be concerned with" from the point of view of the caller.

> >> + *
> >>    * @disable_streams: Disable the streams defined in streams_mask on the given
> >>    *	source pad. Subdevs that implement this operation must use the active
> >>    *	state management provided by the subdev core (enabled through a call to
> >>    *	v4l2_subdev_init_finalize() at initialization time). Do not call
> >>    *	directly, use v4l2_subdev_disable_streams() instead.
> >> + *
> >> + *	Drivers that support only a single stream without setting the
> >> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> >> + *	be concerned with the mask argument.
> >>    */
> >>   struct v4l2_subdev_pad_ops {
> >>   	int (*enum_mbus_code)(struct v4l2_subdev *sd,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: Documentation: Update {enable,disable}_streams documentation

[Tomi Valkeinen]

On 17/09/2024 17:57, Laurent Pinchart wrote:
> On Tue, Sep 17, 2024 at 05:16:25PM +0300, Tomi Valkeinen wrote:
>> On 17/09/2024 16:00, Laurent Pinchart wrote:
>>> On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
>>>> Document the expected {enable,disable}_streams callback behaviour for
>>>> drivers that are stream-unaware i.e. don't specify the
>>>> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
>>>> the mask argument can be ignored.
>>>
>>> Wouldn't it be better to use BIT(0) in that case to simplifiy
>>> interoperability with stream-aware devices ?
>>
>> The caller has to set BIT(0), but I think here the documentation is
>> about the callee.
>>
>> If the driver is not stream aware and implements the callbacks, it will
>> get BIT(0) as the mask parameter (do we enforce this?), but as there's
>> nothing it can do with the parameter it "does not need to be concerned
>> with the mask argument".
> 
> Right. I had misunderstood the patch.
> 
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> ---
>>>>    include/media/v4l2-subdev.h | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>>> index 3cc6b4a5935f..67a6e6ec58b8 100644
>>>> --- a/include/media/v4l2-subdev.h
>>>> +++ b/include/media/v4l2-subdev.h
>>>> @@ -834,11 +834,19 @@ struct v4l2_subdev_state {
>>>>     *	v4l2_subdev_init_finalize() at initialization time). Do not call
>>>>     *	directly, use v4l2_subdev_enable_streams() instead.
>>>>     *
>>>> + *	Drivers that support only a single stream without setting the
>>>> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> 
> s/capatility/capability/
> 
> Same below.
> 
>>>> + *	be concerned with the mask argument.
> 
> How about "can ignore the mask argument" instead ? I interpreted as "not
> need to be concerned with" from the point of view of the caller.

Or maybe, to be transparent, "can ignore the mask argument as it is 
always 1".

  Tomi

Re: [PATCH 2/2] media: Documentation: Update {enable,disable}_streams documentation

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 17, 2024 at 05:57:35PM +0300, Laurent Pinchart wrote:
> On Tue, Sep 17, 2024 at 05:16:25PM +0300, Tomi Valkeinen wrote:
> > On 17/09/2024 16:00, Laurent Pinchart wrote:
> > > On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote:
> > >> Document the expected {enable,disable}_streams callback behaviour for
> > >> drivers that are stream-unaware i.e. don't specify the
> > >> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case,
> > >> the mask argument can be ignored.
> > > 
> > > Wouldn't it be better to use BIT(0) in that case to simplifiy
> > > interoperability with stream-aware devices ?
> > 
> > The caller has to set BIT(0), but I think here the documentation is 
> > about the callee.
> > 
> > If the driver is not stream aware and implements the callbacks, it will 
> > get BIT(0) as the mask parameter (do we enforce this?), but as there's 
> > nothing it can do with the parameter it "does not need to be concerned 
> > with the mask argument".
> 
> Right. I had misunderstood the patch.
> 
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >> ---
> > >>   include/media/v4l2-subdev.h | 8 ++++++++
> > >>   1 file changed, 8 insertions(+)
> > >>
> > >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > >> index 3cc6b4a5935f..67a6e6ec58b8 100644
> > >> --- a/include/media/v4l2-subdev.h
> > >> +++ b/include/media/v4l2-subdev.h
> > >> @@ -834,11 +834,19 @@ struct v4l2_subdev_state {
> > >>    *	v4l2_subdev_init_finalize() at initialization time). Do not call
> > >>    *	directly, use v4l2_subdev_enable_streams() instead.
> > >>    *
> > >> + *	Drivers that support only a single stream without setting the
> > >> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> 
> s/capatility/capability/
> 
> Same below.
> 
> > >> + *	be concerned with the mask argument.
> 
> How about "can ignore the mask argument" instead ? I interpreted as "not
> need to be concerned with" from the point of view of the caller.

Sounds good. I'll address these in v3, after waiting a bit for further
comments.

> 
> > >> + *
> > >>    * @disable_streams: Disable the streams defined in streams_mask on the given
> > >>    *	source pad. Subdevs that implement this operation must use the active
> > >>    *	state management provided by the subdev core (enabled through a call to
> > >>    *	v4l2_subdev_init_finalize() at initialization time). Do not call
> > >>    *	directly, use v4l2_subdev_disable_streams() instead.
> > >> + *
> > >> + *	Drivers that support only a single stream without setting the
> > >> + *	V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to
> > >> + *	be concerned with the mask argument.
> > >>    */
> > >>   struct v4l2_subdev_pad_ops {
> > >>   	int (*enum_mbus_code)(struct v4l2_subdev *sd,
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/2] media: Documentation: Deprecate s_stream video op, update docs

[Tomi Valkeinen]

On 17/09/2024 15:43, Sakari Ailus wrote:
> The scope of the s_stream video operation is now fully supported by
> {enable,disable}_straems. Explicitly document the s_stream() op as
> deprecated and update the related documentation.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>   Documentation/driver-api/media/camera-sensor.rst |  8 ++++----
>   Documentation/driver-api/media/tx-rx.rst         | 11 ++++++-----
>   include/media/v4l2-subdev.h                      |  5 +++--
>   3 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> index b4920b34cebc..c290833165e6 100644
> --- a/Documentation/driver-api/media/camera-sensor.rst
> +++ b/Documentation/driver-api/media/camera-sensor.rst
> @@ -81,10 +81,10 @@ restart when the system is resumed. This requires coordination between the
>   camera sensor and the rest of the camera pipeline. Bridge drivers are
>   responsible for this coordination, and instruct camera sensors to stop and
>   restart streaming by calling the appropriate subdev operations
> -(``.s_stream()``, ``.enable_streams()`` or ``.disable_streams()``). Camera
> -sensor drivers shall therefore **not** keep track of the streaming state to
> -stop streaming in the PM suspend handler and restart it in the resume handler.
> -Drivers should in general not implement the system PM handlers.
> +(``.enable_streams()`` or ``.disable_streams()``). Camera sensor drivers shall

I didn't go through the docs, but I think we need to make it clear 
somewhere that v4l2_subdev_enable_streams() and 
v4l2_subdev_disable_streams() _must_ be used to enable/disable the 
streaming in the source subdevice, and the related subdev callbacks must 
not be called directly.

Also, v4l2_subdev_enable_streams() and v4l2_subdev_disable_streams() 
kernel doc doesn't seem to clarify that if the driver using those 
functions does not support streams, it should use BIT_ULL(1) as the 
streams_mask parameter.

  Tomi

> +therefore **not** keep track of the streaming state to stop streaming in the PM
> +suspend handler and restart it in the resume handler. Drivers should in general
> +not implement the system PM handlers.
>   
>   Camera sensor drivers shall **not** implement the subdev ``.s_power()``
>   operation, as it is deprecated. While this operation is implemented in some
> diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> index 29d66a47b56e..a339df61fca8 100644
> --- a/Documentation/driver-api/media/tx-rx.rst
> +++ b/Documentation/driver-api/media/tx-rx.rst
> @@ -49,11 +49,12 @@ Link frequency
>   The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
>   receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
>   
> -``.s_stream()`` callback
> -^^^^^^^^^^^^^^^^^^^^^^^^
> +``.enable_streams()`` and ``.disable_streams()`` callbacks
> +^^^^^^^^^^^^^^^^^^^^^^^^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   
> -The struct struct v4l2_subdev_video_ops->s_stream() callback is used by the
> -receiver driver to control the transmitter driver's streaming state.
> +The struct v4l2_subdev_pad_ops->enable_streams() and struct
> +v4l2_subdev_pad_ops->disable_streams() callbacks are used by the receiver driver
> +to control the transmitter driver's streaming state.
>   
>   
>   CSI-2 transmitter drivers
> @@ -127,7 +128,7 @@ 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
> +calling the ``.disable_streams()`` 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
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 8daa0929865c..3cc6b4a5935f 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -450,8 +450,9 @@ enum v4l2_subdev_pre_streamon_flags {
>    *	already started or stopped subdev. Also see call_s_stream wrapper in
>    *	v4l2-subdev.c.
>    *
> - *	New drivers should instead implement &v4l2_subdev_pad_ops.enable_streams
> - *	and &v4l2_subdev_pad_ops.disable_streams operations, and use
> + *	This callback is DEPRECATED. New drivers should instead implement
> + *	&v4l2_subdev_pad_ops.enable_streams and
> + *	&v4l2_subdev_pad_ops.disable_streams operations, and use
>    *	v4l2_subdev_s_stream_helper for the &v4l2_subdev_video_ops.s_stream
>    *	operation to support legacy users.
>    *

Re: [PATCH 1/2] media: Documentation: Deprecate s_stream video op, update docs

[Sakari Ailus]

Moi,

On Tue, Sep 17, 2024 at 05:22:29PM +0300, Tomi Valkeinen wrote:
> On 17/09/2024 15:43, Sakari Ailus wrote:
> > The scope of the s_stream video operation is now fully supported by
> > {enable,disable}_straems. Explicitly document the s_stream() op as
> > deprecated and update the related documentation.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >   Documentation/driver-api/media/camera-sensor.rst |  8 ++++----
> >   Documentation/driver-api/media/tx-rx.rst         | 11 ++++++-----
> >   include/media/v4l2-subdev.h                      |  5 +++--
> >   3 files changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > index b4920b34cebc..c290833165e6 100644
> > --- a/Documentation/driver-api/media/camera-sensor.rst
> > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > @@ -81,10 +81,10 @@ restart when the system is resumed. This requires coordination between the
> >   camera sensor and the rest of the camera pipeline. Bridge drivers are
> >   responsible for this coordination, and instruct camera sensors to stop and
> >   restart streaming by calling the appropriate subdev operations
> > -(``.s_stream()``, ``.enable_streams()`` or ``.disable_streams()``). Camera
> > -sensor drivers shall therefore **not** keep track of the streaming state to
> > -stop streaming in the PM suspend handler and restart it in the resume handler.
> > -Drivers should in general not implement the system PM handlers.
> > +(``.enable_streams()`` or ``.disable_streams()``). Camera sensor drivers shall
> 
> I didn't go through the docs, but I think we need to make it clear somewhere
> that v4l2_subdev_enable_streams() and v4l2_subdev_disable_streams() _must_
> be used to enable/disable the streaming in the source subdevice, and the
> related subdev callbacks must not be called directly.

It was in the header but not in tx-rx.rst. I added it there for v2.

> 
> Also, v4l2_subdev_enable_streams() and v4l2_subdev_disable_streams() kernel
> doc doesn't seem to clarify that if the driver using those functions does
> not support streams, it should use BIT_ULL(1) as the streams_mask parameter.

BIT_ULL(0)?

I'll add that, too.

-- 
Terveisin,

Sakari Ailus

Re: [PATCH 1/2] media: Documentation: Deprecate s_stream video op, update docs

[Laurent Pinchart]

On Tue, Sep 17, 2024 at 05:22:29PM +0300, Tomi Valkeinen wrote:
> On 17/09/2024 15:43, Sakari Ailus wrote:
> > The scope of the s_stream video operation is now fully supported by
> > {enable,disable}_straems. Explicitly document the s_stream() op as
> > deprecated and update the related documentation.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >   Documentation/driver-api/media/camera-sensor.rst |  8 ++++----
> >   Documentation/driver-api/media/tx-rx.rst         | 11 ++++++-----
> >   include/media/v4l2-subdev.h                      |  5 +++--
> >   3 files changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > index b4920b34cebc..c290833165e6 100644
> > --- a/Documentation/driver-api/media/camera-sensor.rst
> > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > @@ -81,10 +81,10 @@ restart when the system is resumed. This requires coordination between the
> >   camera sensor and the rest of the camera pipeline. Bridge drivers are
> >   responsible for this coordination, and instruct camera sensors to stop and
> >   restart streaming by calling the appropriate subdev operations
> > -(``.s_stream()``, ``.enable_streams()`` or ``.disable_streams()``). Camera
> > -sensor drivers shall therefore **not** keep track of the streaming state to
> > -stop streaming in the PM suspend handler and restart it in the resume handler.
> > -Drivers should in general not implement the system PM handlers.
> > +(``.enable_streams()`` or ``.disable_streams()``). Camera sensor drivers shall
> 
> I didn't go through the docs, but I think we need to make it clear 
> somewhere that v4l2_subdev_enable_streams() and 
> v4l2_subdev_disable_streams() _must_ be used to enable/disable the 
> streaming in the source subdevice, and the related subdev callbacks must 
> not be called directly.

include/media/v4l2-subdev.h states

 * @enable_streams: Enable the streams defined in streams_mask on the given
 *      source pad. Subdevs that implement this operation must use the active
 *      state management provided by the subdev core (enabled through a call to
 *      v4l2_subdev_init_finalize() at initialization time). Do not call
 *      directly, use v4l2_subdev_enable_streams() instead.

This patch looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> Also, v4l2_subdev_enable_streams() and v4l2_subdev_disable_streams() 
> kernel doc doesn't seem to clarify that if the driver using those 
> functions does not support streams, it should use BIT_ULL(1) as the 
> streams_mask parameter.
> 
> > +therefore **not** keep track of the streaming state to stop streaming in the PM
> > +suspend handler and restart it in the resume handler. Drivers should in general
> > +not implement the system PM handlers.
> >   
> >   Camera sensor drivers shall **not** implement the subdev ``.s_power()``
> >   operation, as it is deprecated. While this operation is implemented in some
> > diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> > index 29d66a47b56e..a339df61fca8 100644
> > --- a/Documentation/driver-api/media/tx-rx.rst
> > +++ b/Documentation/driver-api/media/tx-rx.rst
> > @@ -49,11 +49,12 @@ Link frequency
> >   The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> >   receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> >   
> > -``.s_stream()`` callback
> > -^^^^^^^^^^^^^^^^^^^^^^^^
> > +``.enable_streams()`` and ``.disable_streams()`` callbacks
> > +^^^^^^^^^^^^^^^^^^^^^^^^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   
> > -The struct struct v4l2_subdev_video_ops->s_stream() callback is used by the
> > -receiver driver to control the transmitter driver's streaming state.
> > +The struct v4l2_subdev_pad_ops->enable_streams() and struct
> > +v4l2_subdev_pad_ops->disable_streams() callbacks are used by the receiver driver
> > +to control the transmitter driver's streaming state.
> >   
> >   
> >   CSI-2 transmitter drivers
> > @@ -127,7 +128,7 @@ 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
> > +calling the ``.disable_streams()`` 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
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 8daa0929865c..3cc6b4a5935f 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -450,8 +450,9 @@ enum v4l2_subdev_pre_streamon_flags {
> >    *	already started or stopped subdev. Also see call_s_stream wrapper in
> >    *	v4l2-subdev.c.
> >    *
> > - *	New drivers should instead implement &v4l2_subdev_pad_ops.enable_streams
> > - *	and &v4l2_subdev_pad_ops.disable_streams operations, and use
> > + *	This callback is DEPRECATED. New drivers should instead implement
> > + *	&v4l2_subdev_pad_ops.enable_streams and
> > + *	&v4l2_subdev_pad_ops.disable_streams operations, and use
> >    *	v4l2_subdev_s_stream_helper for the &v4l2_subdev_video_ops.s_stream
> >    *	operation to support legacy users.
> >    *

-- 
Regards,

Laurent Pinchart