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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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