| Message ID | 20220301161156.1119557-1-tomi.valkeinen@ideasonboard.com |
|---|---|
| Headers | show |
| Series | v4l: routing and streams support | expand |
Hi Tomi On Tue, Mar 01, 2022 at 06:11:45PM +0200, Tomi Valkeinen wrote: > Add documentation related to multiplexed streams. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../driver-api/media/v4l2-subdev.rst | 8 + > .../userspace-api/media/v4l/dev-subdev.rst | 165 ++++++++++++++++++ > 2 files changed, 173 insertions(+) > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > index 115211cef4d1..80654f1bcac9 100644 > --- a/Documentation/driver-api/media/v4l2-subdev.rst > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > @@ -593,6 +593,14 @@ before calling v4l2_subdev_init_finalize(): > > This shares the driver's private mutex between the controls and the states. > > +Streams, multiplexed media pads and internal routing > +---------------------------------------------------- > + > +A subdevice driver can implement support for multiplexed streams by setting Let me start by being picky with a minor thing: the rest of the documentation seems to use "sub-device". Here you have "sub-device", "subdevice" and "subdev". I think "sub-device" should be used everywhere > +the V4L2_SUBDEV_FL_MULTIPLEXED subdev flag and implementing support for > +centrally managed subdev active state, routing and stream based > +configuration. > + > V4L2 sub-device functions and data structures > --------------------------------------------- > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > index a67c2749089a..fd042afeddd6 100644 > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > @@ -503,3 +503,168 @@ source pads. > :maxdepth: 1 > > subdev-formats > + > +Streams, multiplexed media pads and internal routing > +---------------------------------------------------- > + > +Commonly V4L2 subdevices support only separate video streams, that is, each > +link in the media graph and each pad in a subdevice pass through a single > +video stream. Thus each pad contains a format configuration for that single Isn't it the other way around ? A single video stream passes through a media link. > +stream. In some cases a subdev can do stream processing and split a stream > +into two or compose two streams into one, but the inputs and outputs for the > +subdev are still a single stream per pad. > + > +Some hardware, e.g. MIPI CSI-2, support multiplexed streams, that is, a single > +bus carries multiple streams. Thus a camera could output two streams, a pixel > +stream and a metadata stream, and a bridge subdev could route the streams > +from multiple input pads into a single output pad. I would use sink/source and not input/output and slightly rephrase this, because I understand your "bridge subdev" in this example is a CSI-2 transmitter but "bridge" usually refers to receiver drivers on the SoC. What about Some hardware, e.g. MIPI CSI-2, support multiplexed streams, that is, multiple data streams are transmitted on the same bus, which is represented by a media link connecting a transmitter source pad with a sink pad on the receiver. In example, a camera sensor can produce two distinct streams, a pixel stream and a metadata stream, which are transmitted on the multiplexed data bus, represented by a media link which connects the single sensor's source pad with the receiver sink pad. The stream-aware receiver will de-multiplex the streams received on the its sink pad and allows to route them individually to one of its source pads. Sensor Receiver +----------+ +--------------+ | Pixel | | ___________(1) | \_(0) --------(0)/ | | Meta / | | \___________(2) | | | | +----------+ +--------------- (not sure if makes sense to have a drawing here, but... ) > + > +Subdevice drivers that support multiplexed streams are compatible with > +non-multiplexed subdev drivers, but, of course, require a routing configuration > +where the link between those two types of drivers contains only a single > +stream. > + > +Understanding streams > +^^^^^^^^^^^^^^^^^^^^^ > + > +A stream is a stream of content (e.g. pixel data or metadata) flowing through > +the media pipeline from a source (e.g. a sensor) towards the final sink(e.g. a space between "sink(" > +receiver and demultiplexer in a SoC). Each media link carries all the streams the enabled streams ? > +from one end of the link to the other, and subdevices have routing tables which > +describe how the incoming streams from sink pads are routed to the source > +pads. "... from sink pad", singular.. I would rather say "how the incoming streams from the multiplexed sink pad are routed to source pads". > + > +A stream ID (often just "stream") is a media link-local identifier for a > +stream. In other words, a configuration for a particular stream ID must exist s/In other words// Should the configuration on both ends of the link also be identical ? > +on both sides of a media link, but another stream ID can be used for the same > +stream at the other side of the subdevice. I would A stream ID (often just "stream") is a media link-local identifier for a stream. The configuration for a stream ID must exist and be identical on both ends of a media link connecting two multiplexed pads. and leave the bits about routing out ? > + > +A stream at a specific point in the media pipeline is identified with the > +subdev and a (pad, stream) pair. For subdevices that do not support > +multiplexed streams the 'stream' is always 0. > + > +Configuring streams > +^^^^^^^^^^^^^^^^^^^ > + > +The configuration of the streams is done individually for each subdevice and > +the validity of the streams between subdevices is validated when the pipeline > +is started. > + > +There are three steps in configuring the streams: > + > +1) Set up links. Connect the pads between subdevices using the :ref:`Media > +Controller API <media_controller>` > + > +2) Routing. The routing table for the subdevice must be set with > +:ref:`VIDIOC_SUBDEV_S_ROUTING <VIDIOC_SUBDEV_G_ROUTING>` ioctl. Note that > +setting the routing table will reset all the stream configurations. "stream configurations in a media entity". > + > +3) Configure streams. Each route endpoint must be configured > +with :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>`. > + > +Multiplexed streams setup example > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +A simple example of a multiplexed stream setup might be as follows: > + > +- Two identical sensors (Sensor A and Sensor B). Each sensor has a single > + source pad (pad 0), and outputs two streams, pixel data and metadata. a single source pad (pad 0) which carries two streams: a pixel data stream and a metadata stream. > + > +- Multiplexer bridge (Bridge). The bridge has two sink pads, connected to the > + sensors (pads 0, 1), and one source pad (pad 2), which outputs all 4 > + streams. > + > +- Receiver in the SoC (Receiver). The receiver has a single sink pad (pad 0), > + connected to the bridge, and four source pads (pads 1-4), going to the DMA > + engine. The receiver demultiplexes the incoming streams to the four source > + pads. > + > +- Four DMA Engines in the SoC (DMA Engine). Each DMA engine is connected to a > + single source pad in the receiver. > + > +The sensors, the bridge and the receiver are modeled as V4L2 subdevices, > +exposed to userspace via /dev/v4l-subdevX device nodes. The DMA engines are > +modeled as V4L2 devices, exposed to userspace via /dev/videoX nodes. > + > +To configure this pipeline, the userspace must take the following steps: > + > +1) Set up media links between entities: connect the sensors to the bridge, > +bridge to the receiver, and the receiver to the DMA engines. This step does > +not differ from normal non-multiplexed media controller setup. > + > +2) Configure routing. > + > +.. flat-table:: Sensor routing table (identical on both sensors) > + :header-rows: 1 > + > + * - Sink Pad/Stream > + - Source Pad/Stream > + - Routing Flags > + - Comments > + * - 0/0 (unused) > + - 0/0 > + - V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_SOURCE > + - Pixel data stream. Source route, i.e. the sink fields are unused. > + * - 0/0 (unused) > + - 0/1 > + - V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_SOURCE > + - Metadata stream. Source route, i.e. the sink fields are unused. > + > +.. flat-table:: Bridge routing table > + :header-rows: 1 > + > + * - Sink Pad/Stream > + - Source Pad/Stream > + - Routing Flags > + - Comments > + * - 0/0 > + - 2/0 > + - V4L2_SUBDEV_ROUTE_FL_ACTIVE > + - Pixel data stream from Sensor A > + * - 0/1 > + - 2/1 > + - V4L2_SUBDEV_ROUTE_FL_ACTIVE > + - Metadata stream from Sensor A > + * - 1/0 > + - 2/2 > + - V4L2_SUBDEV_ROUTE_FL_ACTIVE > + - Pixel data stream from Sensor B > + * - 1/1 > + - 2/3 > + - V4L2_SUBDEV_ROUTE_FL_ACTIVE > + - Metadata stream from Sensor B > + > +.. flat-table:: Receiver routing table > + :header-rows: 1 > + > + * - Sink Pad/Stream > + - Source Pad/Stream > + - Routing Flags > + - Comments > + * - 0/0 > + - 1/0 > + - V4L2_SUBDEV_ROUTE_FL_ACTIVE > + - Pixel data stream from Sensor A > + * - 0/1 > + - 2/0 > + - V4L2_SUBDEV_ROUTE_FL_ACTIVE > + - Metadata stream from Sensor A > + * - 0/2 > + - 3/0 > + - V4L2_SUBDEV_ROUTE_FL_ACTIVE > + - Pixel data stream from Sensor B > + * - 0/3 > + - 4/0 > + - V4L2_SUBDEV_ROUTE_FL_ACTIVE > + - Metadata stream from Sensor B > + > +3) Configure streams > + > +After configuring the routing table, the next step is configuring the streams. > +This step is similar to configuring the pads in a non-multiplexed streams > +setup, with the difference that we need to configure each (pad, stream) pair > +(i.e. route endpoint) instead of just a pad. > + > +A common way to accomplish this is to start from the sensors and propagate the > +configurations along the stream towards the receiver, using VIDIOC_SUBDEV_S_FMT using the :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls > +to configure each stream endpoint in each subdev. All minors or just suggestions: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > -- > 2.25.1 >
Hi Tomi On Tue, Mar 01, 2022 at 06:11:46PM +0200, Tomi Valkeinen wrote: > Add support to manage configurations (format, crop, compose) per stream, > instead of per pad. This is accomplished with data structures that hold > an array of all subdev's stream configurations. > > The number of streams can vary at runtime based on routing. Every time > the routing is changed, the stream configurations need to be > re-initialized. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../v4l/vidioc-subdev-enum-frame-interval.rst | 5 +- > .../v4l/vidioc-subdev-enum-frame-size.rst | 5 +- > .../v4l/vidioc-subdev-enum-mbus-code.rst | 5 +- > .../media/v4l/vidioc-subdev-g-crop.rst | 5 +- > .../media/v4l/vidioc-subdev-g-fmt.rst | 5 +- > .../v4l/vidioc-subdev-g-frame-interval.rst | 5 +- > .../media/v4l/vidioc-subdev-g-selection.rst | 5 +- > drivers/media/v4l2-core/v4l2-subdev.c | 129 ++++++++++++++++-- > include/media/v4l2-subdev.h | 48 +++++++ > include/uapi/linux/v4l2-subdev.h | 28 +++- > 10 files changed, 218 insertions(+), 22 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst > index 3703943b412f..8def4c05d3da 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst > @@ -92,7 +92,10 @@ multiple pads of the same sub-device is not defined. > - Frame intervals to be enumerated, from enum > :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. > * - __u32 > - - ``reserved``\ [8] > + - ``stream`` > + - Stream identifier. > + * - __u32 > + - ``reserved``\ [7] Does VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL (as well as VIDIOC_SUBDEV_G_FRAME_INTERVAL) need to be stream-aware ? What is the semantic of the stream identifiers for IOCTLs that seem to control a paramter which is global to the subdev ? Isn't the stream semantic required to be specified in the IOCTL documentation and not just added to the list of fields ? > - Reserved for future extensions. Applications and drivers must set > the array to zero. > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst > index c25a9896df0e..3ef361c0dca7 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst > @@ -97,7 +97,10 @@ information about try formats. > - Frame sizes to be enumerated, from enum > :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. > * - __u32 > - - ``reserved``\ [8] > + - ``stream`` > + - Stream identifier. > + * - __u32 > + - ``reserved``\ [7] > - Reserved for future extensions. Applications and drivers must set > the array to zero. > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst > index 417f1a19bcc4..248f6f9ee7c5 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst > @@ -73,7 +73,10 @@ information about the try formats. > - ``flags`` > - See :ref:`v4l2-subdev-mbus-code-flags` > * - __u32 > - - ``reserved``\ [7] > + - ``stream`` > + - Stream identifier. > + * - __u32 > + - ``reserved``\ [6] > - Reserved for future extensions. Applications and drivers must set > the array to zero. > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst > index bd15c0a5a66b..1d267f7e7991 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst > @@ -96,7 +96,10 @@ modified format should be as close as possible to the original request. > - ``rect`` > - Crop rectangle boundaries, in pixels. > * - __u32 > - - ``reserved``\ [8] > + - ``stream`` > + - Stream identifier. > + * - __u32 > + - ``reserved``\ [7] > - Reserved for future extensions. Applications and drivers must set > the array to zero. > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst > index 7acdbb939d89..ed253a1e44b7 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst > @@ -102,7 +102,10 @@ should be as close as possible to the original request. > - Definition of an image format, see :c:type:`v4l2_mbus_framefmt` for > details. > * - __u32 > - - ``reserved``\ [8] > + - ``stream`` > + - Stream identifier. > + * - __u32 > + - ``reserved``\ [7] > - Reserved for future extensions. Applications and drivers must set > the array to zero. > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > index d7fe7543c506..842f962d2aea 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > @@ -90,7 +90,10 @@ the same sub-device is not defined. > - ``interval`` > - Period, in seconds, between consecutive video frames. > * - __u32 > - - ``reserved``\ [9] > + - ``stream`` > + - Stream identifier. > + * - __u32 > + - ``reserved``\ [8] > - Reserved for future extensions. Applications and drivers must set > the array to zero. > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst > index f9172a42f036..6b629c19168c 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst > @@ -94,7 +94,10 @@ Selection targets and flags are documented in > - ``r`` > - Selection rectangle, in pixels. > * - __u32 > - - ``reserved``\ [8] > + - ``stream`` > + - Stream identifier. > + * - __u32 > + - ``reserved``\ [7] > - Reserved for future extensions. Applications and drivers must set > the array to zero. > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 1c836c2de86e..339d7b15e26c 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -149,14 +149,34 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad) > return 0; > } To be honest, the only IOCTL for which I have a clear idea of the stream paramter semantic is s/g_format. > > -static int check_state_pads(u32 which, struct v4l2_subdev_state *state) > +static int check_state_pads(struct v4l2_subdev *sd, u32 which, > + struct v4l2_subdev_state *state) > { > + if (sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED) > + return 0; > + > if (which == V4L2_SUBDEV_FORMAT_TRY && (!state || !state->pads)) > return -EINVAL; > > return 0; > } > > +static int check_state_pad_stream(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, u32 pad, > + u32 stream) > +{ > + struct v4l2_mbus_framefmt *fmt; > + > + if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED)) > + return 0; > + > + fmt = v4l2_subdev_state_get_stream_format(state, pad, stream); > + if (!fmt) > + return -EINVAL; > + > + return 0; > +} > + check_state_pads() is always called in conjunction with check_state_pad_stream(). I would have made a check_state() that handles the multiplexed and non-multiplexed case. But that's an implementation detail, so up to you. > static inline int check_format(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format) > @@ -165,7 +185,8 @@ static inline int check_format(struct v4l2_subdev *sd, > return -EINVAL; > > return check_which(format->which) ? : check_pad(sd, format->pad) ? : > - check_state_pads(format->which, state); > + check_state_pads(sd, format->which, state) ? : > + check_state_pad_stream(sd, state, format->pad, format->stream); > } > > static int call_get_fmt(struct v4l2_subdev *sd, > @@ -192,7 +213,8 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, > return -EINVAL; > > return check_which(code->which) ? : check_pad(sd, code->pad) ? : > - check_state_pads(code->which, state) ? : > + check_state_pads(sd, code->which, state) ? : > + check_state_pad_stream(sd, state, code->pad, code->stream) ? : > sd->ops->pad->enum_mbus_code(sd, state, code); > } > > @@ -204,7 +226,8 @@ static int call_enum_frame_size(struct v4l2_subdev *sd, > return -EINVAL; > > return check_which(fse->which) ? : check_pad(sd, fse->pad) ? : > - check_state_pads(fse->which, state) ? : > + check_state_pads(sd, fse->which, state) ? : > + check_state_pad_stream(sd, state, fse->pad, fse->stream) ? : > sd->ops->pad->enum_frame_size(sd, state, fse); > } > > @@ -239,7 +262,8 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, > return -EINVAL; > > return check_which(fie->which) ? : check_pad(sd, fie->pad) ? : > - check_state_pads(fie->which, state) ? : > + check_state_pads(sd, fie->which, state) ? : > + check_state_pad_stream(sd, state, fie->pad, fie->stream) ? : > sd->ops->pad->enum_frame_interval(sd, state, fie); call_g_frame_interval and call_s_frame_interval do accept a struct v4l2_subdev_frame_interval paramter now. Should the validity of streams be checked there too ? > } > > @@ -251,7 +275,8 @@ static inline int check_selection(struct v4l2_subdev *sd, > return -EINVAL; > > return check_which(sel->which) ? : check_pad(sd, sel->pad) ? : > - check_state_pads(sel->which, state); > + check_state_pads(sd, sel->which, state) ? : > + check_state_pad_stream(sd, state, sel->pad, sel->stream); > } > > static int call_get_selection(struct v4l2_subdev *sd, > @@ -865,6 +890,71 @@ const struct v4l2_file_operations v4l2_subdev_fops = { > > #ifdef CONFIG_MEDIA_CONTROLLER > > +static int > +v4l2_subdev_init_stream_configs(struct v4l2_subdev_stream_configs *stream_configs, > + const struct v4l2_subdev_krouting *routing) > +{ > + u32 num_configs = 0; > + unsigned int i; > + u32 format_idx = 0; > + > + kvfree(stream_configs->configs); > + stream_configs->configs = NULL; > + stream_configs->num_configs = 0; > + > + /* Count number of formats needed */ > + for (i = 0; i < routing->num_routes; ++i) { > + struct v4l2_subdev_route *route = &routing->routes[i]; This is a good candidate for for_each_active_route() > + > + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)) > + continue; > + > + /* > + * Each route needs a format on both ends of the route, except > + * for source streams which only need one format. > + */ > + num_configs += > + (route->flags & V4L2_SUBDEV_ROUTE_FL_SOURCE) ? 1 : 2; > + } > + > + if (num_configs) { > + stream_configs->configs = > + kvcalloc(num_configs, sizeof(*stream_configs->configs), > + GFP_KERNEL); > + > + if (!stream_configs->configs) > + return -ENOMEM; > + > + stream_configs->num_configs = num_configs; > + } > + > + /* > + * Fill in the 'pad' and stream' value for each item in the array from > + * the routing table > + */ > + for (i = 0; i < routing->num_routes; ++i) { > + struct v4l2_subdev_route *route = &routing->routes[i]; > + u32 idx; > + > + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)) > + continue; you iterate only active routes again. Which makes me think that you could return after the first loop if (!num_configs) ? > + > + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_SOURCE)) { > + idx = format_idx++; > + > + stream_configs->configs[idx].pad = route->sink_pad; > + stream_configs->configs[idx].stream = route->sink_stream; > + } > + > + idx = format_idx++; > + > + stream_configs->configs[idx].pad = route->source_pad; > + stream_configs->configs[idx].stream = route->source_stream; > + } > + > + return 0; > +} > + > int v4l2_subdev_get_fwnode_pad_1_to_1(struct media_entity *entity, > struct fwnode_endpoint *endpoint) > { > @@ -1042,7 +1132,8 @@ __v4l2_subdev_state_alloc(struct v4l2_subdev *sd, const char *lock_name, > else > state->lock = &state->_lock; > > - if (sd->entity.num_pads) { > + /* Drivers that support streams do not need the legacy pad config */ > + if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED) && sd->entity.num_pads) { > state->pads = kvmalloc_array(sd->entity.num_pads, > sizeof(*state->pads), > GFP_KERNEL | __GFP_ZERO); > @@ -1083,6 +1174,7 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state) > mutex_destroy(&state->_lock); > > kfree(state->routing.routes); > + kvfree(state->stream_configs.configs); > kvfree(state->pads); > kfree(state); > } > @@ -1133,10 +1225,31 @@ int v4l2_subdev_set_routing(struct v4l2_subdev *sd, > dst->num_routes = src->num_routes; > } > > - return 0; > + return v4l2_subdev_init_stream_configs(&state->stream_configs, dst); > } > EXPORT_SYMBOL_GPL(v4l2_subdev_set_routing); > > +struct v4l2_mbus_framefmt * > +v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > + unsigned int pad, u32 stream) > +{ > + struct v4l2_subdev_stream_configs *stream_configs; > + unsigned int i; > + > + lockdep_assert_held(state->lock); This function is only called by check_state_pad_stream(). Does it locks the state ? I understand this is temporary only as other users introduced later will lock the state ? Thanks j > + > + stream_configs = &state->stream_configs; > + > + for (i = 0; i < stream_configs->num_configs; ++i) { > + if (stream_configs->configs[i].pad == pad && > + stream_configs->configs[i].stream == stream) > + return &stream_configs->configs[i].fmt; > + } > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_stream_format); > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 6323bae3860e..2a40ad273cf8 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -697,6 +697,37 @@ struct v4l2_subdev_pad_config { > struct v4l2_rect try_compose; > }; > > +/** > + * struct v4l2_subdev_stream_config - Used for storing stream configuration. > + * > + * @pad: pad number > + * @stream: stream number > + * @fmt: &struct v4l2_mbus_framefmt > + * @crop: &struct v4l2_rect to be used for crop > + * @compose: &struct v4l2_rect to be used for compose > + * > + * This structure stores configuration for a stream. > + */ > +struct v4l2_subdev_stream_config { > + u32 pad; > + u32 stream; > + > + struct v4l2_mbus_framefmt fmt; > + struct v4l2_rect crop; > + struct v4l2_rect compose; > +}; > + > +/** > + * struct v4l2_subdev_stream_configs - A collection of stream configs. > + * > + * @num_configs: number of entries in @config. > + * @configs: an array of &struct v4l2_subdev_stream_configs. > + */ > +struct v4l2_subdev_stream_configs { > + u32 num_configs; > + struct v4l2_subdev_stream_config *configs; > +}; > + > /** > * struct v4l2_subdev_krouting - subdev routing table > * > @@ -717,6 +748,7 @@ struct v4l2_subdev_krouting { > * @lock: mutex for the state. May be replaced by the user. > * @pads: &struct v4l2_subdev_pad_config array > * @routing: routing table for the subdev > + * @stream_configs: stream configurations (only for V4L2_SUBDEV_FL_MULTIPLEXED) > * > * This structure only needs to be passed to the pad op if the 'which' field > * of the main argument is set to %V4L2_SUBDEV_FORMAT_TRY. For > @@ -728,6 +760,7 @@ struct v4l2_subdev_state { > struct mutex *lock; > struct v4l2_subdev_pad_config *pads; > struct v4l2_subdev_krouting routing; > + struct v4l2_subdev_stream_configs stream_configs; > }; > > /** > @@ -1397,6 +1430,21 @@ int v4l2_subdev_set_routing(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state, > struct v4l2_subdev_krouting *routing); > > +/** > + * v4l2_subdev_state_get_stream_format() - Get pointer to a stream format > + * @state: subdevice state > + * @pad: pad id > + * @stream: stream id > + * > + * This returns a pointer to &struct v4l2_mbus_framefmt for the given pad + > + * stream in the subdev state. for a given (pad, stream) ? > + * > + * If the state does not contain the given pad + stream, NULL is returned. > + */ > +struct v4l2_mbus_framefmt * > +v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > + unsigned int pad, u32 stream); > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > /** > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > index 1ec3141bf860..480891dba193 100644 > --- a/include/uapi/linux/v4l2-subdev.h > +++ b/include/uapi/linux/v4l2-subdev.h > @@ -44,13 +44,15 @@ enum v4l2_subdev_format_whence { > * @which: format type (from enum v4l2_subdev_format_whence) > * @pad: pad number, as reported by the media API > * @format: media bus format (format code and frame size) > + * @stream: stream number, defined in subdev routing @stream: stream identifier what does "defined in subdev routing" mean ? > * @reserved: drivers and applications must zero this array > */ > struct v4l2_subdev_format { > __u32 which; > __u32 pad; > struct v4l2_mbus_framefmt format; > - __u32 reserved[8]; > + __u32 stream; > + __u32 reserved[7]; > }; > > /** > @@ -58,13 +60,15 @@ struct v4l2_subdev_format { > * @which: format type (from enum v4l2_subdev_format_whence) > * @pad: pad number, as reported by the media API > * @rect: pad crop rectangle boundaries > + * @stream: stream number, defined in subdev routing > * @reserved: drivers and applications must zero this array > */ > struct v4l2_subdev_crop { > __u32 which; > __u32 pad; > struct v4l2_rect rect; > - __u32 reserved[8]; > + __u32 stream; > + __u32 reserved[7]; > }; > > #define V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE 0x00000001 > @@ -80,6 +84,7 @@ struct v4l2_subdev_crop { > * @code: format code (MEDIA_BUS_FMT_ definitions) > * @which: format type (from enum v4l2_subdev_format_whence) > * @flags: flags set by the driver, (V4L2_SUBDEV_MBUS_CODE_*) > + * @stream: stream number, defined in subdev routing > * @reserved: drivers and applications must zero this array > */ > struct v4l2_subdev_mbus_code_enum { > @@ -88,7 +93,8 @@ struct v4l2_subdev_mbus_code_enum { > __u32 code; > __u32 which; > __u32 flags; > - __u32 reserved[7]; > + __u32 stream; > + __u32 reserved[6]; > }; > > /** > @@ -101,6 +107,7 @@ struct v4l2_subdev_mbus_code_enum { > * @min_height: minimum frame height, in pixels > * @max_height: maximum frame height, in pixels > * @which: format type (from enum v4l2_subdev_format_whence) > + * @stream: stream number, defined in subdev routing > * @reserved: drivers and applications must zero this array > */ > struct v4l2_subdev_frame_size_enum { > @@ -112,19 +119,22 @@ struct v4l2_subdev_frame_size_enum { > __u32 min_height; > __u32 max_height; > __u32 which; > - __u32 reserved[8]; > + __u32 stream; > + __u32 reserved[7]; > }; > > /** > * struct v4l2_subdev_frame_interval - Pad-level frame rate > * @pad: pad number, as reported by the media API > * @interval: frame interval in seconds > + * @stream: stream number, defined in subdev routing > * @reserved: drivers and applications must zero this array > */ > struct v4l2_subdev_frame_interval { > __u32 pad; > struct v4l2_fract interval; > - __u32 reserved[9]; > + __u32 stream; > + __u32 reserved[8]; > }; > > /** > @@ -136,6 +146,7 @@ struct v4l2_subdev_frame_interval { > * @height: frame height in pixels > * @interval: frame interval in seconds > * @which: format type (from enum v4l2_subdev_format_whence) > + * @stream: stream number, defined in subdev routing > * @reserved: drivers and applications must zero this array > */ > struct v4l2_subdev_frame_interval_enum { > @@ -146,7 +157,8 @@ struct v4l2_subdev_frame_interval_enum { > __u32 height; > struct v4l2_fract interval; > __u32 which; > - __u32 reserved[8]; > + __u32 stream; > + __u32 reserved[7]; > }; > > /** > @@ -158,6 +170,7 @@ struct v4l2_subdev_frame_interval_enum { > * defined in v4l2-common.h; V4L2_SEL_TGT_* . > * @flags: constraint flags, defined in v4l2-common.h; V4L2_SEL_FLAG_*. > * @r: coordinates of the selection window > + * @stream: stream number, defined in subdev routing > * @reserved: for future use, set to zero for now > * > * Hardware may use multiple helper windows to process a video stream. > @@ -170,7 +183,8 @@ struct v4l2_subdev_selection { > __u32 target; > __u32 flags; > struct v4l2_rect r; > - __u32 reserved[8]; > + __u32 stream; > + __u32 reserved[7]; > }; > > /** > -- > 2.25.1 >
Hi Tomi On Tue, Mar 01, 2022 at 06:11:49PM +0200, Tomi Valkeinen wrote: > Add v4l2_subdev_get_fmt() helper function which implements > v4l2_subdev_pad_ops.get_fmt using streams. Subdev drivers that do not > need to do anything special in their get_fmt op can use this helper > directly for v4l2_subdev_pad_ops.get_fmt. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++++++++ > include/media/v4l2-subdev.h | 17 +++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index e65f802fe2aa..c1cc9b91dba7 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1507,6 +1507,22 @@ v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, > } > EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_opposite_stream_format); > > +int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *format) > +{ > + struct v4l2_mbus_framefmt *fmt; > + > + fmt = v4l2_subdev_state_get_stream_format(state, format->pad, > + format->stream); > + if (!fmt) > + return -EINVAL; > + > + format->format = *fmt; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(v4l2_subdev_get_fmt); > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index ed3fe21637e6..a80830801a7f 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -1481,6 +1481,23 @@ struct v4l2_mbus_framefmt * > v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state *state, > u32 pad, u32 stream); > > +/** > + * v4l2_subdev_get_fmt() - Fill format based on state > + * @sd: subdevice > + * @state: subdevice state > + * @format: pointer to &struct v4l2_subdev_format > + * > + * Fill @format based on the pad and stream given in the @format struct. > + * > + * This function can be used by the subdev drivers to implement > + * v4l2_subdev_pad_ops.get_fmt if the subdev driver does not need to do > + * anything special in their get_fmt op. > + * > + * Returns 0 on success, error value otherwise. > + */ > +int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *format); > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > /** > -- > 2.25.1 >
Hi Tomi On Tue, Mar 01, 2022 at 06:11:51PM +0200, Tomi Valkeinen wrote: > v4l2_subdev_set_routing_with_fmt() is the same as > v4l2_subdev_set_routing(), but additionally initializes all the streams > with the given format. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++++ > include/media/v4l2-subdev.h | 16 ++++++++++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 7f50871054cd..1ceee8313246 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1438,6 +1438,28 @@ int v4l2_subdev_set_routing(struct v4l2_subdev *sd, > } > EXPORT_SYMBOL_GPL(v4l2_subdev_set_routing); > > +int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_krouting *routing, > + const struct v4l2_mbus_framefmt *fmt) > +{ > + struct v4l2_subdev_stream_configs *stream_configs; > + unsigned int i; > + int ret; > + > + ret = v4l2_subdev_set_routing(sd, state, routing); > + if (ret) > + return ret; > + > + stream_configs = &state->stream_configs; > + > + for (i = 0; i < stream_configs->num_configs; ++i) > + stream_configs->configs[i].fmt = *fmt; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(v4l2_subdev_set_routing_with_fmt); > + > struct v4l2_mbus_framefmt * > v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > unsigned int pad, u32 stream) > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index a80830801a7f..97db6dfc0b7a 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -1430,6 +1430,22 @@ int v4l2_subdev_set_routing(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state, > struct v4l2_subdev_krouting *routing); > > +/** > + * v4l2_subdev_set_routing_with_fmt() - Set given routing and format to subdev > + * state > + * @sd: The subdevice > + * @state: The subdevice state > + * @routing: Routing that will be copied to subdev state > + * @fmt: Format used to initialize all the streams > + * > + * This is the same as v4l2_subdev_set_routing, but additionally initializes > + * all the streams using the given format. > + */ > +int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_krouting *routing, > + const struct v4l2_mbus_framefmt *fmt); > + > /** > * v4l2_subdev_state_get_stream_format() - Get pointer to a stream format > * @state: subdevice state > -- > 2.25.1 >
Hi Tomi, On Tue, Mar 01, 2022 at 06:11:55PM +0200, Tomi Valkeinen wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Add two new subdev pad operations, .enable_streams() and > .disable_streams(), to allow control of individual streams per pad. This > is a superset of what the video .s_stream() operation implements. > > To help with handling of backward compatibility, add two wrapper > functions around those operations, and require their usage in drivers. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 216 ++++++++++++++++++++++++++ > include/media/v4l2-subdev.h | 85 ++++++++++ > 2 files changed, 301 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 6a9fc62dacbf..f75a1995a70b 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1698,6 +1698,222 @@ __v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting *routing, > } > EXPORT_SYMBOL_GPL(__v4l2_subdev_next_active_route); > > +static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad, > + u64 streams_mask) > +{ > + unsigned int i; > + int ret; > + > + /* > + * The subdev doesn't implement pad-based stream enable, fall back > + * on the .s_stream() operation. This can only be done for subdevs that > + * have a single source pad, as sd->enabled_streams is global to the > + * subdev. > + */ > + if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)) > + return -EOPNOTSUPP; > + > + for (i = 0; i < sd->entity.num_pads; ++i) { > + if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE) > + return -EOPNOTSUPP; > + } > + > + if (sd->enabled_streams & streams_mask) > + return -EALREADY; I wonder if a few dev_dbg on errors might save someone an headache > + > + /* Start streaming when the first streams are enabled. */ > + if (!sd->enabled_streams) { > + ret = v4l2_subdev_call(sd, video, s_stream, 1); > + if (ret) > + return ret; > + } > + > + sd->enabled_streams |= streams_mask; > + > + return 0; > +} > + > +int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad, > + u64 streams_mask) > +{ > + struct device *dev = sd->entity.graph_obj.mdev->dev; > + struct v4l2_subdev_state *state; > + u64 found_streams = 0; > + unsigned int i; > + int ret; > + > + /* A few basic sanity checks first. */ > + if (pad >= sd->entity.num_pads) > + return -EINVAL; Should we make sure pad is a SOURCE (and remove the same check in the _fallback version) ? > + > + if (!streams_mask) > + return 0; > + > + /* Fallback on .s_stream() if .enable_streams() isn't available. */ > + if (!sd->ops->pad || !sd->ops->pad->enable_streams) > + return v4l2_subdev_enable_streams_fallback(sd, pad, > + streams_mask); > + > + state = v4l2_subdev_lock_and_get_active_state(sd); > + > + /* > + * Verify that the requested streams exist and that they are not > + * already enabled. > + */ > + for (i = 0; i < state->stream_configs.num_configs; ++i) { > + struct v4l2_subdev_stream_config *cfg = > + &state->stream_configs.configs[i]; > + > + if (cfg->pad != pad || !(streams_mask & BIT(cfg->stream))) > + continue; > + > + found_streams |= BIT(cfg->stream); > + > + if (cfg->enabled) { > + dev_dbg(dev, "stream %u already enabled on %s/%u\n", > + cfg->stream, sd->entity.name, pad); > + ret = -EALREADY; > + goto done; > + } > + } > + > + if (found_streams != streams_mask) { > + dev_dbg(dev, "streams 0x%llx not found on %s/%u\n", nit: I would use the more usual form of entity:pad in the error message I like the idea :) Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + streams_mask & ~found_streams, sd->entity.name, pad); > + ret = -EINVAL; > + goto done; > + } > + > + /* Call the .enable_streams() operation. */ > + ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad, > + streams_mask); > + if (ret) > + goto done; > + > + /* Mark the streams as enabled. */ > + for (i = 0; i < state->stream_configs.num_configs; ++i) { > + struct v4l2_subdev_stream_config *cfg = > + &state->stream_configs.configs[i]; > + > + if (cfg->pad == pad && (streams_mask & BIT(cfg->stream))) > + cfg->enabled = true; > + } > + > +done: > + v4l2_subdev_unlock_state(state); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(v4l2_subdev_enable_streams); > + > +static int v4l2_subdev_disable_streams_fallback(struct v4l2_subdev *sd, u32 pad, > + u64 streams_mask) > +{ > + unsigned int i; > + int ret; > + > + /* > + * If the subdev doesn't implement pad-based stream enable, fall back > + * on the .s_stream() operation. This can only be done for subdevs that > + * have a single source pad, as sd->enabled_streams is global to the > + * subdev. > + */ > + if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)) > + return -EOPNOTSUPP; > + > + for (i = 0; i < sd->entity.num_pads; ++i) { > + if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE) > + return -EOPNOTSUPP; > + } > + > + if ((sd->enabled_streams & streams_mask) != streams_mask) > + return -EALREADY; > + > + /* Stop streaming when the last streams are disabled. */ > + if (!(sd->enabled_streams & ~streams_mask)) { > + ret = v4l2_subdev_call(sd, video, s_stream, 0); > + if (ret) > + return ret; > + } > + > + sd->enabled_streams &= ~streams_mask; > + > + return 0; > +} > + > +int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad, > + u64 streams_mask) > +{ > + struct device *dev = sd->entity.graph_obj.mdev->dev; > + struct v4l2_subdev_state *state; > + u64 found_streams = 0; > + unsigned int i; > + int ret; > + > + /* A few basic sanity checks first. */ > + if (pad >= sd->entity.num_pads) > + return -EINVAL; > + > + if (!streams_mask) > + return 0; > + > + /* Fallback on .s_stream() if .disable_streams() isn't available. */ > + if (!sd->ops->pad || !sd->ops->pad->disable_streams) > + return v4l2_subdev_disable_streams_fallback(sd, pad, > + streams_mask); > + > + state = v4l2_subdev_lock_and_get_active_state(sd); > + > + /* > + * Verify that the requested streams exist and that they are not > + * already disabled. > + */ > + for (i = 0; i < state->stream_configs.num_configs; ++i) { > + struct v4l2_subdev_stream_config *cfg = > + &state->stream_configs.configs[i]; > + > + if (cfg->pad != pad || !(streams_mask & BIT(cfg->stream))) > + continue; > + > + found_streams |= BIT(cfg->stream); > + > + if (!cfg->enabled) { > + dev_dbg(dev, "stream %u already disabled on %s/%u\n", > + cfg->stream, sd->entity.name, pad); > + ret = -EALREADY; > + goto done; > + } > + } > + > + if (found_streams != streams_mask) { > + dev_dbg(dev, "streams 0x%llx not found on %s/%u\n", > + streams_mask & ~found_streams, sd->entity.name, pad); > + ret = -EINVAL; > + goto done; > + } > + > + /* Call the .disable_streams() operation. */ > + ret = v4l2_subdev_call(sd, pad, disable_streams, state, pad, > + streams_mask); > + if (ret) > + goto done; > + > + /* Mark the streams as disabled. */ > + for (i = 0; i < state->stream_configs.num_configs; ++i) { > + struct v4l2_subdev_stream_config *cfg = > + &state->stream_configs.configs[i]; > + > + if (cfg->pad == pad && (streams_mask & BIT(cfg->stream))) > + cfg->enabled = false; > + } > + > +done: > + v4l2_subdev_unlock_state(state); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(v4l2_subdev_disable_streams); > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 992debe116ac..bb1713863973 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -702,6 +702,7 @@ struct v4l2_subdev_pad_config { > * > * @pad: pad number > * @stream: stream number > + * @enabled: has the stream been enabled with v4l2_subdev_enable_stream() > * @fmt: &struct v4l2_mbus_framefmt > * @crop: &struct v4l2_rect to be used for crop > * @compose: &struct v4l2_rect to be used for compose > @@ -711,6 +712,7 @@ struct v4l2_subdev_pad_config { > struct v4l2_subdev_stream_config { > u32 pad; > u32 stream; > + bool enabled; > > struct v4l2_mbus_framefmt fmt; > struct v4l2_rect crop; > @@ -816,6 +818,18 @@ struct v4l2_subdev_state { > * > * @set_routing: enable or disable data connection routes described in the > * subdevice routing table. > + * > + * @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. > + * > + * @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. > */ > struct v4l2_subdev_pad_ops { > int (*init_cfg)(struct v4l2_subdev *sd, > @@ -862,6 +876,12 @@ struct v4l2_subdev_pad_ops { > struct v4l2_subdev_state *state, > enum v4l2_subdev_format_whence which, > struct v4l2_subdev_krouting *route); > + int (*enable_streams)(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, u32 pad, > + u64 streams_mask); > + int (*disable_streams)(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, u32 pad, > + u64 streams_mask); > }; > > /** > @@ -1007,6 +1027,10 @@ struct v4l2_subdev_platform_data { > * @active_state: Active state for the subdev (NULL for subdevs tracking the > * state internally). Initialized by calling > * v4l2_subdev_init_finalize(). > + * @enabled_streams: Bitmask of enabled streams used by > + * v4l2_subdev_enable_streams() and > + * v4l2_subdev_disable_streams() helper functions for fallback > + * cases. > * > * Each instance of a subdev driver should create this struct, either > * stand-alone or embedded in a larger struct. > @@ -1052,6 +1076,7 @@ struct v4l2_subdev { > * doesn't support it. > */ > struct v4l2_subdev_state *active_state; > + u64 enabled_streams; > }; > > > @@ -1589,6 +1614,66 @@ __v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting *routing, > for ((route) = NULL; \ > ((route) = __v4l2_subdev_next_active_route((routing), (route)));) > > +/** > + * v4l2_subdev_enable_streams() - Enable streams on a pad > + * @sd: The subdevice > + * @pad: The pad > + * @streams_mask: Bitmask of streams to enable > + * > + * This function enables streams on a source @pad of a subdevice. The pad is > + * identified by its index, while the streams are identified by the > + * @streams_mask bitmask. This allows enabling multiple streams on a pad at > + * once. > + * > + * Enabling a stream that is already enabled isn't allowed. If @streams_mask > + * contains an already enabled stream, this function returns -EALREADY without > + * performing any operation. > + * > + * Per-stream enable is only available for subdevs that implement the > + * .enable_streams() and .disable_streams() operations. For other subdevs, this > + * function implements a best-effort compatibility by calling the .s_stream() > + * operation, limited to subdevs that have a single source pad. > + * > + * Return: > + * * 0: Success > + * * -EALREADY: One of the streams in streams_mask is already enabled > + * * -EINVAL: The pad index is invalid, or doesn't correspond to a source pad > + * * -EOPNOTSUPP: Falling back to the legacy .s_stream() operation is > + * impossible because the subdev has multiple source pads > + */ > +int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad, > + u64 streams_mask); > + > +/** > + * v4l2_subdev_disable_streams() - Disable streams on a pad > + * @sd: The subdevice > + * @pad: The pad > + * @streams_mask: Bitmask of streams to disable > + * > + * This function disables streams on a source @pad of a subdevice. The pad is > + * identified by its index, while the streams are identified by the > + * @streams_mask bitmask. This allows disabling multiple streams on a pad at > + * once. > + * > + * Disabling a streams that is not enabled isn't allowed. If @streams_mask > + * contains a disabled stream, this function returns -EALREADY without > + * performing any operation. > + * > + * Per-stream disable is only available for subdevs that implement the > + * .enable_streams() and .disable_streams() operations. For other subdevs, this > + * function implements a best-effort compatibility by calling the .s_stream() > + * operation, limited to subdevs that have a single source pad. > + * > + * Return: > + * * 0: Success > + * * -EALREADY: One of the streams in streams_mask is not enabled > + * * -EINVAL: The pad index is invalid, or doesn't correspond to a source pad > + * * -EOPNOTSUPP: Falling back to the legacy .s_stream() operation is > + * impossible because the subdev has multiple source pads > + */ > +int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad, > + u64 streams_mask); > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > /** > -- > 2.25.1 >
On 16/03/2022 11:04, Jacopo Mondi wrote: > Hi Tomi > > On Tue, Mar 01, 2022 at 06:11:45PM +0200, Tomi Valkeinen wrote: >> Add documentation related to multiplexed streams. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> .../driver-api/media/v4l2-subdev.rst | 8 + >> .../userspace-api/media/v4l/dev-subdev.rst | 165 ++++++++++++++++++ >> 2 files changed, 173 insertions(+) >> >> diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst >> index 115211cef4d1..80654f1bcac9 100644 >> --- a/Documentation/driver-api/media/v4l2-subdev.rst >> +++ b/Documentation/driver-api/media/v4l2-subdev.rst >> @@ -593,6 +593,14 @@ before calling v4l2_subdev_init_finalize(): >> >> This shares the driver's private mutex between the controls and the states. >> >> +Streams, multiplexed media pads and internal routing >> +---------------------------------------------------- >> + >> +A subdevice driver can implement support for multiplexed streams by setting > > Let me start by being picky with a minor thing: the rest of the > documentation seems to use "sub-device". Here you have "sub-device", > "subdevice" and "subdev". I think "sub-device" should be used > everywhere I can see lots of all those three in the docs, so I don't really know which one to pick... >> +the V4L2_SUBDEV_FL_MULTIPLEXED subdev flag and implementing support for >> +centrally managed subdev active state, routing and stream based >> +configuration. >> + >> V4L2 sub-device functions and data structures >> --------------------------------------------- >> >> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst >> index a67c2749089a..fd042afeddd6 100644 >> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst >> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst >> @@ -503,3 +503,168 @@ source pads. >> :maxdepth: 1 >> >> subdev-formats >> + >> +Streams, multiplexed media pads and internal routing >> +---------------------------------------------------- >> + >> +Commonly V4L2 subdevices support only separate video streams, that is, each >> +link in the media graph and each pad in a subdevice pass through a single >> +video stream. Thus each pad contains a format configuration for that single > > Isn't it the other way around ? A single video stream passes through a > media link. It's a bit confusing. I meant "pass through" as in "each link and pad let a single stream pass through". How about: Commonly V4L2 subdevices support only separate video streams, that is, only a single stream can pass through a media link and a media pad. >> +stream. In some cases a subdev can do stream processing and split a stream >> +into two or compose two streams into one, but the inputs and outputs for the >> +subdev are still a single stream per pad. >> + >> +Some hardware, e.g. MIPI CSI-2, support multiplexed streams, that is, a single >> +bus carries multiple streams. Thus a camera could output two streams, a pixel >> +stream and a metadata stream, and a bridge subdev could route the streams >> +from multiple input pads into a single output pad. > > I would use sink/source and not input/output and slightly rephrase > this, because I understand your "bridge subdev" in this example is a > CSI-2 transmitter but "bridge" usually refers to receiver drivers on > the SoC. Ok. In DRM, bridges are not the display controllers in the SoC, but the "bridges" between the SoC and the panel. > What about > > Some hardware, e.g. MIPI CSI-2, support multiplexed streams, that is, > multiple data streams are transmitted on the same bus, which is > represented by a media link connecting a transmitter source pad with a > sink pad on the receiver. In example, a camera sensor can produce two > distinct streams, a pixel stream and a metadata stream, which are > transmitted on the multiplexed data bus, represented by a media link > which connects the single sensor's source pad with the receiver sink > pad. The stream-aware receiver will de-multiplex the streams received > on the its sink pad and allows to route them individually to one of > its source pads. This sounds fine. > Sensor Receiver > +----------+ +--------------+ > | Pixel | | ___________(1) > | \_(0) --------(0)/ | > | Meta / | | \___________(2) > | | | | > +----------+ +--------------- > > (not sure if makes sense to have a drawing here, but... ) > >> + >> +Subdevice drivers that support multiplexed streams are compatible with >> +non-multiplexed subdev drivers, but, of course, require a routing configuration >> +where the link between those two types of drivers contains only a single >> +stream. >> + >> +Understanding streams >> +^^^^^^^^^^^^^^^^^^^^^ >> + >> +A stream is a stream of content (e.g. pixel data or metadata) flowing through >> +the media pipeline from a source (e.g. a sensor) towards the final sink(e.g. a > > space between "sink(" > >> +receiver and demultiplexer in a SoC). Each media link carries all the streams > > the enabled streams ? > >> +from one end of the link to the other, and subdevices have routing tables which >> +describe how the incoming streams from sink pads are routed to the source >> +pads. > > "... from sink pad", singular.. No, the routing table describes how the streams from the sink pads are routed to the source pads. Why would it be singular? > I would rather say "how the incoming streams from the multiplexed sink > pad are routed to source pads". > >> + >> +A stream ID (often just "stream") is a media link-local identifier for a >> +stream. In other words, a configuration for a particular stream ID must exist > > s/In other words// Why would you remove it? The sentence explains the previous sentence in a more verbose way. > Should the configuration on both ends of the link also be identical ? If you refer to the format etc. config, the same rules apply here as for non-multiplexed case. I'm not sure if they need to be identical, but they have to match. But I'm not really talking about configuration here. >> +on both sides of a media link, but another stream ID can be used for the same >> +stream at the other side of the subdevice. > > I would > > A stream ID (often just "stream") is a media link-local identifier for a > stream. The configuration for a stream ID must exist and be identical > on both ends of a media link connecting two multiplexed pads. You're describing a different thing there. I'm talking about the stream IDs. I think it becomes clearer if I remove "a configuration for". So: > A stream ID (often just "stream") is a media link-local identifier for a stream. > In other words, a particular stream ID must exist on both sides of a media > link, but another stream ID can be used for the same stream at the other side > of the subdevice. So it's explaining the stream IDs, how a single stream can be represented by multiple stream IDs. > and leave the bits about routing out ? > >> + >> +A stream at a specific point in the media pipeline is identified with the >> +subdev and a (pad, stream) pair. For subdevices that do not support >> +multiplexed streams the 'stream' is always 0. >> + >> +Configuring streams >> +^^^^^^^^^^^^^^^^^^^ >> + >> +The configuration of the streams is done individually for each subdevice and >> +the validity of the streams between subdevices is validated when the pipeline >> +is started. >> + >> +There are three steps in configuring the streams: >> + >> +1) Set up links. Connect the pads between subdevices using the :ref:`Media >> +Controller API <media_controller>` >> + >> +2) Routing. The routing table for the subdevice must be set with >> +:ref:`VIDIOC_SUBDEV_S_ROUTING <VIDIOC_SUBDEV_G_ROUTING>` ioctl. Note that >> +setting the routing table will reset all the stream configurations. > > "stream configurations in a media entity". Ok. >> + >> +3) Configure streams. Each route endpoint must be configured >> +with :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>`. >> + >> +Multiplexed streams setup example >> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> + >> +A simple example of a multiplexed stream setup might be as follows: >> + >> +- Two identical sensors (Sensor A and Sensor B). Each sensor has a single >> + source pad (pad 0), and outputs two streams, pixel data and metadata. > > a single source pad (pad 0) which carries two streams: a pixel data > stream and a metadata stream. Ok. >> + >> +- Multiplexer bridge (Bridge). The bridge has two sink pads, connected to the >> + sensors (pads 0, 1), and one source pad (pad 2), which outputs all 4 >> + streams. >> + >> +- Receiver in the SoC (Receiver). The receiver has a single sink pad (pad 0), >> + connected to the bridge, and four source pads (pads 1-4), going to the DMA >> + engine. The receiver demultiplexes the incoming streams to the four source >> + pads. >> + >> +- Four DMA Engines in the SoC (DMA Engine). Each DMA engine is connected to a >> + single source pad in the receiver. >> + >> +The sensors, the bridge and the receiver are modeled as V4L2 subdevices, >> +exposed to userspace via /dev/v4l-subdevX device nodes. The DMA engines are >> +modeled as V4L2 devices, exposed to userspace via /dev/videoX nodes. >> + >> +To configure this pipeline, the userspace must take the following steps: >> + >> +1) Set up media links between entities: connect the sensors to the bridge, >> +bridge to the receiver, and the receiver to the DMA engines. This step does >> +not differ from normal non-multiplexed media controller setup. >> + >> +2) Configure routing. >> + >> +.. flat-table:: Sensor routing table (identical on both sensors) >> + :header-rows: 1 >> + >> + * - Sink Pad/Stream >> + - Source Pad/Stream >> + - Routing Flags >> + - Comments >> + * - 0/0 (unused) >> + - 0/0 >> + - V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_SOURCE >> + - Pixel data stream. Source route, i.e. the sink fields are unused. >> + * - 0/0 (unused) >> + - 0/1 >> + - V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_SOURCE >> + - Metadata stream. Source route, i.e. the sink fields are unused. >> + >> +.. flat-table:: Bridge routing table >> + :header-rows: 1 >> + >> + * - Sink Pad/Stream >> + - Source Pad/Stream >> + - Routing Flags >> + - Comments >> + * - 0/0 >> + - 2/0 >> + - V4L2_SUBDEV_ROUTE_FL_ACTIVE >> + - Pixel data stream from Sensor A >> + * - 0/1 >> + - 2/1 >> + - V4L2_SUBDEV_ROUTE_FL_ACTIVE >> + - Metadata stream from Sensor A >> + * - 1/0 >> + - 2/2 >> + - V4L2_SUBDEV_ROUTE_FL_ACTIVE >> + - Pixel data stream from Sensor B >> + * - 1/1 >> + - 2/3 >> + - V4L2_SUBDEV_ROUTE_FL_ACTIVE >> + - Metadata stream from Sensor B >> + >> +.. flat-table:: Receiver routing table >> + :header-rows: 1 >> + >> + * - Sink Pad/Stream >> + - Source Pad/Stream >> + - Routing Flags >> + - Comments >> + * - 0/0 >> + - 1/0 >> + - V4L2_SUBDEV_ROUTE_FL_ACTIVE >> + - Pixel data stream from Sensor A >> + * - 0/1 >> + - 2/0 >> + - V4L2_SUBDEV_ROUTE_FL_ACTIVE >> + - Metadata stream from Sensor A >> + * - 0/2 >> + - 3/0 >> + - V4L2_SUBDEV_ROUTE_FL_ACTIVE >> + - Pixel data stream from Sensor B >> + * - 0/3 >> + - 4/0 >> + - V4L2_SUBDEV_ROUTE_FL_ACTIVE >> + - Metadata stream from Sensor B >> + >> +3) Configure streams >> + >> +After configuring the routing table, the next step is configuring the streams. >> +This step is similar to configuring the pads in a non-multiplexed streams >> +setup, with the difference that we need to configure each (pad, stream) pair >> +(i.e. route endpoint) instead of just a pad. >> + >> +A common way to accomplish this is to start from the sensors and propagate the >> +configurations along the stream towards the receiver, using VIDIOC_SUBDEV_S_FMT > > using the :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls > >> +to configure each stream endpoint in each subdev. > > All minors or just suggestions: > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks! Tomi
On 16/03/2022 11:59, Jacopo Mondi wrote: > Hi Tomi > > On Tue, Mar 01, 2022 at 06:11:46PM +0200, Tomi Valkeinen wrote: >> Add support to manage configurations (format, crop, compose) per stream, >> instead of per pad. This is accomplished with data structures that hold >> an array of all subdev's stream configurations. >> >> The number of streams can vary at runtime based on routing. Every time >> the routing is changed, the stream configurations need to be >> re-initialized. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> .../v4l/vidioc-subdev-enum-frame-interval.rst | 5 +- >> .../v4l/vidioc-subdev-enum-frame-size.rst | 5 +- >> .../v4l/vidioc-subdev-enum-mbus-code.rst | 5 +- >> .../media/v4l/vidioc-subdev-g-crop.rst | 5 +- >> .../media/v4l/vidioc-subdev-g-fmt.rst | 5 +- >> .../v4l/vidioc-subdev-g-frame-interval.rst | 5 +- >> .../media/v4l/vidioc-subdev-g-selection.rst | 5 +- >> drivers/media/v4l2-core/v4l2-subdev.c | 129 ++++++++++++++++-- >> include/media/v4l2-subdev.h | 48 +++++++ >> include/uapi/linux/v4l2-subdev.h | 28 +++- >> 10 files changed, 218 insertions(+), 22 deletions(-) >> >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst >> index 3703943b412f..8def4c05d3da 100644 >> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst >> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst >> @@ -92,7 +92,10 @@ multiple pads of the same sub-device is not defined. >> - Frame intervals to be enumerated, from enum >> :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. >> * - __u32 >> - - ``reserved``\ [8] >> + - ``stream`` >> + - Stream identifier. >> + * - __u32 >> + - ``reserved``\ [7] > > Does VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL (as well as > VIDIOC_SUBDEV_G_FRAME_INTERVAL) need to be stream-aware ? > > What is the semantic of the stream identifiers for IOCTLs that seem to > control a paramter which is global to the subdev ? Isn't the stream semantic > required to be specified in the IOCTL documentation and not just added > to the list of fields ? Why would it be global to the subdev? struct v4l2_subdev_frame_interval_enum already has 'pad' field, so it operates on that pad. With streams, each stream in a pad may have different characteristics, (similarly to different pads in non-stream case), so it feels logical to me to add the 'stream' field. >> - Reserved for future extensions. Applications and drivers must set >> the array to zero. >> >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst >> index c25a9896df0e..3ef361c0dca7 100644 >> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst >> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst >> @@ -97,7 +97,10 @@ information about try formats. >> - Frame sizes to be enumerated, from enum >> :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. >> * - __u32 >> - - ``reserved``\ [8] >> + - ``stream`` >> + - Stream identifier. >> + * - __u32 >> + - ``reserved``\ [7] >> - Reserved for future extensions. Applications and drivers must set >> the array to zero. >> >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst >> index 417f1a19bcc4..248f6f9ee7c5 100644 >> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst >> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst >> @@ -73,7 +73,10 @@ information about the try formats. >> - ``flags`` >> - See :ref:`v4l2-subdev-mbus-code-flags` >> * - __u32 >> - - ``reserved``\ [7] >> + - ``stream`` >> + - Stream identifier. >> + * - __u32 >> + - ``reserved``\ [6] >> - Reserved for future extensions. Applications and drivers must set >> the array to zero. >> >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst >> index bd15c0a5a66b..1d267f7e7991 100644 >> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst >> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst >> @@ -96,7 +96,10 @@ modified format should be as close as possible to the original request. >> - ``rect`` >> - Crop rectangle boundaries, in pixels. >> * - __u32 >> - - ``reserved``\ [8] >> + - ``stream`` >> + - Stream identifier. >> + * - __u32 >> + - ``reserved``\ [7] >> - Reserved for future extensions. Applications and drivers must set >> the array to zero. >> >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst >> index 7acdbb939d89..ed253a1e44b7 100644 >> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst >> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst >> @@ -102,7 +102,10 @@ should be as close as possible to the original request. >> - Definition of an image format, see :c:type:`v4l2_mbus_framefmt` for >> details. >> * - __u32 >> - - ``reserved``\ [8] >> + - ``stream`` >> + - Stream identifier. >> + * - __u32 >> + - ``reserved``\ [7] >> - Reserved for future extensions. Applications and drivers must set >> the array to zero. >> >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst >> index d7fe7543c506..842f962d2aea 100644 >> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst >> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst >> @@ -90,7 +90,10 @@ the same sub-device is not defined. >> - ``interval`` >> - Period, in seconds, between consecutive video frames. >> * - __u32 >> - - ``reserved``\ [9] >> + - ``stream`` >> + - Stream identifier. >> + * - __u32 >> + - ``reserved``\ [8] >> - Reserved for future extensions. Applications and drivers must set >> the array to zero. >> >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst >> index f9172a42f036..6b629c19168c 100644 >> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst >> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst >> @@ -94,7 +94,10 @@ Selection targets and flags are documented in >> - ``r`` >> - Selection rectangle, in pixels. >> * - __u32 >> - - ``reserved``\ [8] >> + - ``stream`` >> + - Stream identifier. >> + * - __u32 >> + - ``reserved``\ [7] >> - Reserved for future extensions. Applications and drivers must set >> the array to zero. >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index 1c836c2de86e..339d7b15e26c 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -149,14 +149,34 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad) >> return 0; >> } > > To be honest, the only IOCTL for which I have a clear idea of the > stream paramter semantic is s/g_format. I admit I don't have much experience with some of these ioctls. But the idea is simple: in non-stream case ioctls operate on a subdev pad, in stream case those ioctls operate on a subdev pad + stream tuple. If there's an ioctl that truly operates on a pad only, then we need to drop the stream parameter. But the ioctls don't look like that to me. >> -static int check_state_pads(u32 which, struct v4l2_subdev_state *state) >> +static int check_state_pads(struct v4l2_subdev *sd, u32 which, >> + struct v4l2_subdev_state *state) >> { >> + if (sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED) >> + return 0; >> + >> if (which == V4L2_SUBDEV_FORMAT_TRY && (!state || !state->pads)) >> return -EINVAL; >> >> return 0; >> } >> >> +static int check_state_pad_stream(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, u32 pad, >> + u32 stream) >> +{ >> + struct v4l2_mbus_framefmt *fmt; >> + >> + if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED)) >> + return 0; >> + >> + fmt = v4l2_subdev_state_get_stream_format(state, pad, stream); >> + if (!fmt) >> + return -EINVAL; >> + >> + return 0; >> +} >> + > > check_state_pads() is always called in conjunction with > check_state_pad_stream(). I would have made a check_state() that > handles the multiplexed and non-multiplexed case. But that's an > implementation detail, so up to you. That's true. I think it looked a bit different in earlier versions, but looking at it now, combining those two makes sense. >> static inline int check_format(struct v4l2_subdev *sd, >> struct v4l2_subdev_state *state, >> struct v4l2_subdev_format *format) >> @@ -165,7 +185,8 @@ static inline int check_format(struct v4l2_subdev *sd, >> return -EINVAL; >> >> return check_which(format->which) ? : check_pad(sd, format->pad) ? : >> - check_state_pads(format->which, state); >> + check_state_pads(sd, format->which, state) ? : >> + check_state_pad_stream(sd, state, format->pad, format->stream); >> } >> >> static int call_get_fmt(struct v4l2_subdev *sd, >> @@ -192,7 +213,8 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, >> return -EINVAL; >> >> return check_which(code->which) ? : check_pad(sd, code->pad) ? : >> - check_state_pads(code->which, state) ? : >> + check_state_pads(sd, code->which, state) ? : >> + check_state_pad_stream(sd, state, code->pad, code->stream) ? : >> sd->ops->pad->enum_mbus_code(sd, state, code); >> } >> >> @@ -204,7 +226,8 @@ static int call_enum_frame_size(struct v4l2_subdev *sd, >> return -EINVAL; >> >> return check_which(fse->which) ? : check_pad(sd, fse->pad) ? : >> - check_state_pads(fse->which, state) ? : >> + check_state_pads(sd, fse->which, state) ? : >> + check_state_pad_stream(sd, state, fse->pad, fse->stream) ? : >> sd->ops->pad->enum_frame_size(sd, state, fse); >> } >> >> @@ -239,7 +262,8 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, >> return -EINVAL; >> >> return check_which(fie->which) ? : check_pad(sd, fie->pad) ? : >> - check_state_pads(fie->which, state) ? : >> + check_state_pads(sd, fie->which, state) ? : >> + check_state_pad_stream(sd, state, fie->pad, fie->stream) ? : >> sd->ops->pad->enum_frame_interval(sd, state, fie); > > call_g_frame_interval and call_s_frame_interval do accept a struct > v4l2_subdev_frame_interval paramter now. Should the validity of > streams be checked there too ? Yes, I need to check this. >> } >> >> @@ -251,7 +275,8 @@ static inline int check_selection(struct v4l2_subdev *sd, >> return -EINVAL; >> >> return check_which(sel->which) ? : check_pad(sd, sel->pad) ? : >> - check_state_pads(sel->which, state); >> + check_state_pads(sd, sel->which, state) ? : >> + check_state_pad_stream(sd, state, sel->pad, sel->stream); >> } >> >> static int call_get_selection(struct v4l2_subdev *sd, >> @@ -865,6 +890,71 @@ const struct v4l2_file_operations v4l2_subdev_fops = { >> >> #ifdef CONFIG_MEDIA_CONTROLLER >> >> +static int >> +v4l2_subdev_init_stream_configs(struct v4l2_subdev_stream_configs *stream_configs, >> + const struct v4l2_subdev_krouting *routing) >> +{ >> + u32 num_configs = 0; >> + unsigned int i; >> + u32 format_idx = 0; >> + >> + kvfree(stream_configs->configs); >> + stream_configs->configs = NULL; >> + stream_configs->num_configs = 0; >> + >> + /* Count number of formats needed */ >> + for (i = 0; i < routing->num_routes; ++i) { >> + struct v4l2_subdev_route *route = &routing->routes[i]; > > This is a good candidate for for_each_active_route() Indeed. >> + >> + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)) >> + continue; >> + >> + /* >> + * Each route needs a format on both ends of the route, except >> + * for source streams which only need one format. >> + */ >> + num_configs += >> + (route->flags & V4L2_SUBDEV_ROUTE_FL_SOURCE) ? 1 : 2; >> + } >> + >> + if (num_configs) { >> + stream_configs->configs = >> + kvcalloc(num_configs, sizeof(*stream_configs->configs), >> + GFP_KERNEL); >> + >> + if (!stream_configs->configs) >> + return -ENOMEM; >> + >> + stream_configs->num_configs = num_configs; >> + } >> + >> + /* >> + * Fill in the 'pad' and stream' value for each item in the array from >> + * the routing table >> + */ >> + for (i = 0; i < routing->num_routes; ++i) { >> + struct v4l2_subdev_route *route = &routing->routes[i]; >> + u32 idx; >> + >> + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)) >> + continue; > > you iterate only active routes again. Which makes me think that you > could return after the first loop if (!num_configs) ? Yes, I think you're right. >> + >> + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_SOURCE)) { >> + idx = format_idx++; >> + >> + stream_configs->configs[idx].pad = route->sink_pad; >> + stream_configs->configs[idx].stream = route->sink_stream; >> + } >> + >> + idx = format_idx++; >> + >> + stream_configs->configs[idx].pad = route->source_pad; >> + stream_configs->configs[idx].stream = route->source_stream; >> + } >> + >> + return 0; >> +} >> + >> int v4l2_subdev_get_fwnode_pad_1_to_1(struct media_entity *entity, >> struct fwnode_endpoint *endpoint) >> { >> @@ -1042,7 +1132,8 @@ __v4l2_subdev_state_alloc(struct v4l2_subdev *sd, const char *lock_name, >> else >> state->lock = &state->_lock; >> >> - if (sd->entity.num_pads) { >> + /* Drivers that support streams do not need the legacy pad config */ >> + if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED) && sd->entity.num_pads) { >> state->pads = kvmalloc_array(sd->entity.num_pads, >> sizeof(*state->pads), >> GFP_KERNEL | __GFP_ZERO); >> @@ -1083,6 +1174,7 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state) >> mutex_destroy(&state->_lock); >> >> kfree(state->routing.routes); >> + kvfree(state->stream_configs.configs); >> kvfree(state->pads); >> kfree(state); >> } >> @@ -1133,10 +1225,31 @@ int v4l2_subdev_set_routing(struct v4l2_subdev *sd, >> dst->num_routes = src->num_routes; >> } >> >> - return 0; >> + return v4l2_subdev_init_stream_configs(&state->stream_configs, dst); >> } >> EXPORT_SYMBOL_GPL(v4l2_subdev_set_routing); >> >> +struct v4l2_mbus_framefmt * >> +v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, >> + unsigned int pad, u32 stream) >> +{ >> + struct v4l2_subdev_stream_configs *stream_configs; >> + unsigned int i; >> + >> + lockdep_assert_held(state->lock); > > This function is only called by check_state_pad_stream(). Does it It is used in multiple places later. > locks the state ? I understand this is temporary only as other users > introduced later will lock the state ? The caller is supposed to lock the state. subdev_do_ioctl_lock() does it here. Do you mean the call wrappers could be called via some other route? Tomi
Hi Tomi On Thu, Mar 17, 2022 at 09:18:42AM +0200, Tomi Valkeinen wrote: > On 16/03/2022 11:04, Jacopo Mondi wrote: > > Hi Tomi > > > > On Tue, Mar 01, 2022 at 06:11:45PM +0200, Tomi Valkeinen wrote: > > > Add documentation related to multiplexed streams. > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > --- > > > .../driver-api/media/v4l2-subdev.rst | 8 + > > > .../userspace-api/media/v4l/dev-subdev.rst | 165 ++++++++++++++++++ > > > 2 files changed, 173 insertions(+) > > > > > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > > > index 115211cef4d1..80654f1bcac9 100644 > > > --- a/Documentation/driver-api/media/v4l2-subdev.rst > > > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > > > @@ -593,6 +593,14 @@ before calling v4l2_subdev_init_finalize(): > > > > > > This shares the driver's private mutex between the controls and the states. > > > > > > +Streams, multiplexed media pads and internal routing > > > +---------------------------------------------------- > > > + > > > +A subdevice driver can implement support for multiplexed streams by setting > > > > Let me start by being picky with a minor thing: the rest of the > > documentation seems to use "sub-device". Here you have "sub-device", > > "subdevice" and "subdev". I think "sub-device" should be used > > everywhere > > I can see lots of all those three in the docs, so I don't really know which > one to pick... > You're right, the number of "subdev" entries is larger that what I first thought. Let's keep using whatever then :)
Hi Tomi On Thu, Mar 17, 2022 at 10:01:47AM +0200, Tomi Valkeinen wrote: > On 16/03/2022 11:59, Jacopo Mondi wrote: > > Hi Tomi > > > > On Tue, Mar 01, 2022 at 06:11:46PM +0200, Tomi Valkeinen wrote: > > > Add support to manage configurations (format, crop, compose) per stream, > > > instead of per pad. This is accomplished with data structures that hold > > > an array of all subdev's stream configurations. > > > > > > The number of streams can vary at runtime based on routing. Every time > > > the routing is changed, the stream configurations need to be > > > re-initialized. > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > --- > > > .../v4l/vidioc-subdev-enum-frame-interval.rst | 5 +- > > > .../v4l/vidioc-subdev-enum-frame-size.rst | 5 +- > > > .../v4l/vidioc-subdev-enum-mbus-code.rst | 5 +- > > > .../media/v4l/vidioc-subdev-g-crop.rst | 5 +- > > > .../media/v4l/vidioc-subdev-g-fmt.rst | 5 +- > > > .../v4l/vidioc-subdev-g-frame-interval.rst | 5 +- > > > .../media/v4l/vidioc-subdev-g-selection.rst | 5 +- > > > drivers/media/v4l2-core/v4l2-subdev.c | 129 ++++++++++++++++-- > > > include/media/v4l2-subdev.h | 48 +++++++ > > > include/uapi/linux/v4l2-subdev.h | 28 +++- > > > 10 files changed, 218 insertions(+), 22 deletions(-) > > > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst > > > index 3703943b412f..8def4c05d3da 100644 > > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst > > > @@ -92,7 +92,10 @@ multiple pads of the same sub-device is not defined. > > > - Frame intervals to be enumerated, from enum > > > :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. > > > * - __u32 > > > - - ``reserved``\ [8] > > > + - ``stream`` > > > + - Stream identifier. > > > + * - __u32 > > > + - ``reserved``\ [7] > > > > Does VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL (as well as > > VIDIOC_SUBDEV_G_FRAME_INTERVAL) need to be stream-aware ? > > > > What is the semantic of the stream identifiers for IOCTLs that seem to > > control a paramter which is global to the subdev ? Isn't the stream semantic > > required to be specified in the IOCTL documentation and not just added > > to the list of fields ? > > Why would it be global to the subdev? struct v4l2_subdev_frame_interval_enum > already has 'pad' field, so it operates on that pad. With streams, each > stream in a pad may have different characteristics, (similarly to different > pads in non-stream case), so it feels logical to me to add the 'stream' > field. > I understand a device with multiple output interfaces can decide to clock out frames at different speeds due to some bus configuration parameters. But I have an hard time immagine a device that can clock out frames at different rates on the same bus. > > > - Reserved for future extensions. Applications and drivers must set > > > the array to zero. > > > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst > > > index c25a9896df0e..3ef361c0dca7 100644 > > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-size.rst > > > @@ -97,7 +97,10 @@ information about try formats. > > > - Frame sizes to be enumerated, from enum > > > :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. > > > * - __u32 > > > - - ``reserved``\ [8] > > > + - ``stream`` > > > + - Stream identifier. > > > + * - __u32 > > > + - ``reserved``\ [7] > > > - Reserved for future extensions. Applications and drivers must set > > > the array to zero. > > > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst > > > index 417f1a19bcc4..248f6f9ee7c5 100644 > > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-mbus-code.rst > > > @@ -73,7 +73,10 @@ information about the try formats. > > > - ``flags`` > > > - See :ref:`v4l2-subdev-mbus-code-flags` > > > * - __u32 > > > - - ``reserved``\ [7] > > > + - ``stream`` > > > + - Stream identifier. > > > + * - __u32 > > > + - ``reserved``\ [6] > > > - Reserved for future extensions. Applications and drivers must set > > > the array to zero. > > > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst > > > index bd15c0a5a66b..1d267f7e7991 100644 > > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst > > > @@ -96,7 +96,10 @@ modified format should be as close as possible to the original request. > > > - ``rect`` > > > - Crop rectangle boundaries, in pixels. > > > * - __u32 > > > - - ``reserved``\ [8] > > > + - ``stream`` > > > + - Stream identifier. > > > + * - __u32 > > > + - ``reserved``\ [7] > > > - Reserved for future extensions. Applications and drivers must set > > > the array to zero. > > > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst > > > index 7acdbb939d89..ed253a1e44b7 100644 > > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst > > > @@ -102,7 +102,10 @@ should be as close as possible to the original request. > > > - Definition of an image format, see :c:type:`v4l2_mbus_framefmt` for > > > details. > > > * - __u32 > > > - - ``reserved``\ [8] > > > + - ``stream`` > > > + - Stream identifier. > > > + * - __u32 > > > + - ``reserved``\ [7] > > > - Reserved for future extensions. Applications and drivers must set > > > the array to zero. > > > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > > > index d7fe7543c506..842f962d2aea 100644 > > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst > > > @@ -90,7 +90,10 @@ the same sub-device is not defined. > > > - ``interval`` > > > - Period, in seconds, between consecutive video frames. > > > * - __u32 > > > - - ``reserved``\ [9] > > > + - ``stream`` > > > + - Stream identifier. > > > + * - __u32 > > > + - ``reserved``\ [8] > > > - Reserved for future extensions. Applications and drivers must set > > > the array to zero. > > > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst > > > index f9172a42f036..6b629c19168c 100644 > > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst > > > @@ -94,7 +94,10 @@ Selection targets and flags are documented in > > > - ``r`` > > > - Selection rectangle, in pixels. > > > * - __u32 > > > - - ``reserved``\ [8] > > > + - ``stream`` > > > + - Stream identifier. > > > + * - __u32 > > > + - ``reserved``\ [7] > > > - Reserved for future extensions. Applications and drivers must set > > > the array to zero. > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > index 1c836c2de86e..339d7b15e26c 100644 > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > @@ -149,14 +149,34 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad) > > > return 0; > > > } > > > > To be honest, the only IOCTL for which I have a clear idea of the > > stream paramter semantic is s/g_format. > > I admit I don't have much experience with some of these ioctls. But the idea > is simple: in non-stream case ioctls operate on a subdev pad, in stream case > those ioctls operate on a subdev pad + stream tuple. > > If there's an ioctl that truly operates on a pad only, then we need to drop > the stream parameter. But the ioctls don't look like that to me. > As above mentioned I have an hard time reconciling streams and frame rate handling. Thinking about image sensors, changing the rate at which frames are clocked out for one stream modifies the frame rate globally for all of them. Other devices might behave differently ? > > > -static int check_state_pads(u32 which, struct v4l2_subdev_state *state) > > > +static int check_state_pads(struct v4l2_subdev *sd, u32 which, > > > + struct v4l2_subdev_state *state) > > > { > > > + if (sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED) > > > + return 0; > > > + > > > if (which == V4L2_SUBDEV_FORMAT_TRY && (!state || !state->pads)) > > > return -EINVAL; > > > > > > return 0; > > > } > > > > > > +static int check_state_pad_stream(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state, u32 pad, > > > + u32 stream) > > > +{ > > > + struct v4l2_mbus_framefmt *fmt; > > > + > > > + if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED)) > > > + return 0; > > > + > > > + fmt = v4l2_subdev_state_get_stream_format(state, pad, stream); > > > + if (!fmt) > > > + return -EINVAL; > > > + > > > + return 0; > > > +} > > > + > > > > check_state_pads() is always called in conjunction with > > check_state_pad_stream(). I would have made a check_state() that > > handles the multiplexed and non-multiplexed case. But that's an > > implementation detail, so up to you. > > That's true. I think it looked a bit different in earlier versions, but > looking at it now, combining those two makes sense. > > > > static inline int check_format(struct v4l2_subdev *sd, > > > struct v4l2_subdev_state *state, > > > struct v4l2_subdev_format *format) > > > @@ -165,7 +185,8 @@ static inline int check_format(struct v4l2_subdev *sd, > > > return -EINVAL; > > > > > > return check_which(format->which) ? : check_pad(sd, format->pad) ? : > > > - check_state_pads(format->which, state); > > > + check_state_pads(sd, format->which, state) ? : > > > + check_state_pad_stream(sd, state, format->pad, format->stream); > > > } > > > > > > static int call_get_fmt(struct v4l2_subdev *sd, > > > @@ -192,7 +213,8 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, > > > return -EINVAL; > > > > > > return check_which(code->which) ? : check_pad(sd, code->pad) ? : > > > - check_state_pads(code->which, state) ? : > > > + check_state_pads(sd, code->which, state) ? : > > > + check_state_pad_stream(sd, state, code->pad, code->stream) ? : > > > sd->ops->pad->enum_mbus_code(sd, state, code); > > > } > > > > > > @@ -204,7 +226,8 @@ static int call_enum_frame_size(struct v4l2_subdev *sd, > > > return -EINVAL; > > > > > > return check_which(fse->which) ? : check_pad(sd, fse->pad) ? : > > > - check_state_pads(fse->which, state) ? : > > > + check_state_pads(sd, fse->which, state) ? : > > > + check_state_pad_stream(sd, state, fse->pad, fse->stream) ? : > > > sd->ops->pad->enum_frame_size(sd, state, fse); > > > } > > > > > > @@ -239,7 +262,8 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, > > > return -EINVAL; > > > > > > return check_which(fie->which) ? : check_pad(sd, fie->pad) ? : > > > - check_state_pads(fie->which, state) ? : > > > + check_state_pads(sd, fie->which, state) ? : > > > + check_state_pad_stream(sd, state, fie->pad, fie->stream) ? : > > > sd->ops->pad->enum_frame_interval(sd, state, fie); > > > > call_g_frame_interval and call_s_frame_interval do accept a struct > > v4l2_subdev_frame_interval paramter now. Should the validity of > > streams be checked there too ? > > Yes, I need to check this. > > > > } > > > > > > @@ -251,7 +275,8 @@ static inline int check_selection(struct v4l2_subdev *sd, > > > return -EINVAL; > > > > > > return check_which(sel->which) ? : check_pad(sd, sel->pad) ? : > > > - check_state_pads(sel->which, state); > > > + check_state_pads(sd, sel->which, state) ? : > > > + check_state_pad_stream(sd, state, sel->pad, sel->stream); > > > } > > > > > > static int call_get_selection(struct v4l2_subdev *sd, > > > @@ -865,6 +890,71 @@ const struct v4l2_file_operations v4l2_subdev_fops = { > > > > > > #ifdef CONFIG_MEDIA_CONTROLLER > > > > > > +static int > > > +v4l2_subdev_init_stream_configs(struct v4l2_subdev_stream_configs *stream_configs, > > > + const struct v4l2_subdev_krouting *routing) > > > +{ > > > + u32 num_configs = 0; > > > + unsigned int i; > > > + u32 format_idx = 0; > > > + > > > + kvfree(stream_configs->configs); > > > + stream_configs->configs = NULL; > > > + stream_configs->num_configs = 0; > > > + > > > + /* Count number of formats needed */ > > > + for (i = 0; i < routing->num_routes; ++i) { > > > + struct v4l2_subdev_route *route = &routing->routes[i]; > > > > This is a good candidate for for_each_active_route() > > Indeed. > > > > + > > > + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)) > > > + continue; > > > + > > > + /* > > > + * Each route needs a format on both ends of the route, except > > > + * for source streams which only need one format. > > > + */ > > > + num_configs += > > > + (route->flags & V4L2_SUBDEV_ROUTE_FL_SOURCE) ? 1 : 2; > > > + } > > > + > > > + if (num_configs) { > > > + stream_configs->configs = > > > + kvcalloc(num_configs, sizeof(*stream_configs->configs), > > > + GFP_KERNEL); > > > + > > > + if (!stream_configs->configs) > > > + return -ENOMEM; > > > + > > > + stream_configs->num_configs = num_configs; > > > + } > > > + > > > + /* > > > + * Fill in the 'pad' and stream' value for each item in the array from > > > + * the routing table > > > + */ > > > + for (i = 0; i < routing->num_routes; ++i) { > > > + struct v4l2_subdev_route *route = &routing->routes[i]; > > > + u32 idx; > > > + > > > + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)) > > > + continue; > > > > you iterate only active routes again. Which makes me think that you > > could return after the first loop if (!num_configs) ? > > Yes, I think you're right. > > > > + > > > + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_SOURCE)) { > > > + idx = format_idx++; > > > + > > > + stream_configs->configs[idx].pad = route->sink_pad; > > > + stream_configs->configs[idx].stream = route->sink_stream; > > > + } > > > + > > > + idx = format_idx++; > > > + > > > + stream_configs->configs[idx].pad = route->source_pad; > > > + stream_configs->configs[idx].stream = route->source_stream; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > int v4l2_subdev_get_fwnode_pad_1_to_1(struct media_entity *entity, > > > struct fwnode_endpoint *endpoint) > > > { > > > @@ -1042,7 +1132,8 @@ __v4l2_subdev_state_alloc(struct v4l2_subdev *sd, const char *lock_name, > > > else > > > state->lock = &state->_lock; > > > > > > - if (sd->entity.num_pads) { > > > + /* Drivers that support streams do not need the legacy pad config */ > > > + if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED) && sd->entity.num_pads) { > > > state->pads = kvmalloc_array(sd->entity.num_pads, > > > sizeof(*state->pads), > > > GFP_KERNEL | __GFP_ZERO); > > > @@ -1083,6 +1174,7 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state) > > > mutex_destroy(&state->_lock); > > > > > > kfree(state->routing.routes); > > > + kvfree(state->stream_configs.configs); > > > kvfree(state->pads); > > > kfree(state); > > > } > > > @@ -1133,10 +1225,31 @@ int v4l2_subdev_set_routing(struct v4l2_subdev *sd, > > > dst->num_routes = src->num_routes; > > > } > > > > > > - return 0; > > > + return v4l2_subdev_init_stream_configs(&state->stream_configs, dst); > > > } > > > EXPORT_SYMBOL_GPL(v4l2_subdev_set_routing); > > > > > > +struct v4l2_mbus_framefmt * > > > +v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > > > + unsigned int pad, u32 stream) > > > +{ > > > + struct v4l2_subdev_stream_configs *stream_configs; > > > + unsigned int i; > > > + > > > + lockdep_assert_held(state->lock); > > > > This function is only called by check_state_pad_stream(). Does it > > It is used in multiple places later. > > > locks the state ? I understand this is temporary only as other users > > introduced later will lock the state ? > > The caller is supposed to lock the state. subdev_do_ioctl_lock() does it > here. Do you mean the call wrappers could be called via some other route? I was concerned calling v4l2_subdev_state_get_stream_format() from check_state_pad_stream() would trigger a lockdep warning. But I had missed subdev_do_ioctl_lock() in the call path. Thanks j > > Tomi
On 17/03/2022 10:38, Jacopo Mondi wrote: > Hi Tomi > > On Thu, Mar 17, 2022 at 10:01:47AM +0200, Tomi Valkeinen wrote: >> On 16/03/2022 11:59, Jacopo Mondi wrote: >>> Hi Tomi >>> >>> On Tue, Mar 01, 2022 at 06:11:46PM +0200, Tomi Valkeinen wrote: >>>> Add support to manage configurations (format, crop, compose) per stream, >>>> instead of per pad. This is accomplished with data structures that hold >>>> an array of all subdev's stream configurations. >>>> >>>> The number of streams can vary at runtime based on routing. Every time >>>> the routing is changed, the stream configurations need to be >>>> re-initialized. >>>> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>> --- >>>> .../v4l/vidioc-subdev-enum-frame-interval.rst | 5 +- >>>> .../v4l/vidioc-subdev-enum-frame-size.rst | 5 +- >>>> .../v4l/vidioc-subdev-enum-mbus-code.rst | 5 +- >>>> .../media/v4l/vidioc-subdev-g-crop.rst | 5 +- >>>> .../media/v4l/vidioc-subdev-g-fmt.rst | 5 +- >>>> .../v4l/vidioc-subdev-g-frame-interval.rst | 5 +- >>>> .../media/v4l/vidioc-subdev-g-selection.rst | 5 +- >>>> drivers/media/v4l2-core/v4l2-subdev.c | 129 ++++++++++++++++-- >>>> include/media/v4l2-subdev.h | 48 +++++++ >>>> include/uapi/linux/v4l2-subdev.h | 28 +++- >>>> 10 files changed, 218 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst >>>> index 3703943b412f..8def4c05d3da 100644 >>>> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst >>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-enum-frame-interval.rst >>>> @@ -92,7 +92,10 @@ multiple pads of the same sub-device is not defined. >>>> - Frame intervals to be enumerated, from enum >>>> :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. >>>> * - __u32 >>>> - - ``reserved``\ [8] >>>> + - ``stream`` >>>> + - Stream identifier. >>>> + * - __u32 >>>> + - ``reserved``\ [7] >>> >>> Does VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL (as well as >>> VIDIOC_SUBDEV_G_FRAME_INTERVAL) need to be stream-aware ? >>> >>> What is the semantic of the stream identifiers for IOCTLs that seem to >>> control a paramter which is global to the subdev ? Isn't the stream semantic >>> required to be specified in the IOCTL documentation and not just added >>> to the list of fields ? >> >> Why would it be global to the subdev? struct v4l2_subdev_frame_interval_enum >> already has 'pad' field, so it operates on that pad. With streams, each >> stream in a pad may have different characteristics, (similarly to different >> pads in non-stream case), so it feels logical to me to add the 'stream' >> field. >> > > I understand a device with multiple output interfaces can decide to > clock out frames at different speeds due to some bus configuration > parameters. But I have an hard time immagine a device that can clock > out frames at different rates on the same bus. I see your point, but doesn't the viability depend on the bus and protocol? A sensor could send pixel frames 60 fps, but metadata only at 10 fps. Or a HD picture 10 fps, and a low res picture 60 fps. All it needs is a bus that can interleave the streams seamlessly. Tomi
On 16/03/2022 14:05, Jacopo Mondi wrote: > Hi Tomi, > > On Tue, Mar 01, 2022 at 06:11:55PM +0200, Tomi Valkeinen wrote: >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Add two new subdev pad operations, .enable_streams() and >> .disable_streams(), to allow control of individual streams per pad. This >> is a superset of what the video .s_stream() operation implements. >> >> To help with handling of backward compatibility, add two wrapper >> functions around those operations, and require their usage in drivers. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 216 ++++++++++++++++++++++++++ >> include/media/v4l2-subdev.h | 85 ++++++++++ >> 2 files changed, 301 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index 6a9fc62dacbf..f75a1995a70b 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -1698,6 +1698,222 @@ __v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting *routing, >> } >> EXPORT_SYMBOL_GPL(__v4l2_subdev_next_active_route); >> >> +static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad, >> + u64 streams_mask) >> +{ >> + unsigned int i; >> + int ret; >> + >> + /* >> + * The subdev doesn't implement pad-based stream enable, fall back >> + * on the .s_stream() operation. This can only be done for subdevs that >> + * have a single source pad, as sd->enabled_streams is global to the >> + * subdev. >> + */ >> + if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)) >> + return -EOPNOTSUPP; >> + >> + for (i = 0; i < sd->entity.num_pads; ++i) { >> + if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE) >> + return -EOPNOTSUPP; >> + } >> + >> + if (sd->enabled_streams & streams_mask) >> + return -EALREADY; > > I wonder if a few dev_dbg on errors might save someone an headache Yep, I'll add a few. >> + >> + /* Start streaming when the first streams are enabled. */ >> + if (!sd->enabled_streams) { >> + ret = v4l2_subdev_call(sd, video, s_stream, 1); >> + if (ret) >> + return ret; >> + } >> + >> + sd->enabled_streams |= streams_mask; >> + >> + return 0; >> +} >> + >> +int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad, >> + u64 streams_mask) >> +{ >> + struct device *dev = sd->entity.graph_obj.mdev->dev; >> + struct v4l2_subdev_state *state; >> + u64 found_streams = 0; >> + unsigned int i; >> + int ret; >> + >> + /* A few basic sanity checks first. */ >> + if (pad >= sd->entity.num_pads) >> + return -EINVAL; > > Should we make sure pad is a SOURCE (and remove the same check in the > _fallback version) ? I tried that, but according to Laurent: no. Enable streams may work on both directions, i.e. for output drivers it is called on a sink pad. >> + >> + if (!streams_mask) >> + return 0; >> + >> + /* Fallback on .s_stream() if .enable_streams() isn't available. */ >> + if (!sd->ops->pad || !sd->ops->pad->enable_streams) >> + return v4l2_subdev_enable_streams_fallback(sd, pad, >> + streams_mask); >> + >> + state = v4l2_subdev_lock_and_get_active_state(sd); >> + >> + /* >> + * Verify that the requested streams exist and that they are not >> + * already enabled. >> + */ >> + for (i = 0; i < state->stream_configs.num_configs; ++i) { >> + struct v4l2_subdev_stream_config *cfg = >> + &state->stream_configs.configs[i]; >> + >> + if (cfg->pad != pad || !(streams_mask & BIT(cfg->stream))) >> + continue; >> + >> + found_streams |= BIT(cfg->stream); >> + >> + if (cfg->enabled) { >> + dev_dbg(dev, "stream %u already enabled on %s/%u\n", >> + cfg->stream, sd->entity.name, pad); >> + ret = -EALREADY; >> + goto done; >> + } >> + } >> + >> + if (found_streams != streams_mask) { >> + dev_dbg(dev, "streams 0x%llx not found on %s/%u\n", > > nit: I would use the more usual form of entity:pad in the error > message Ok. > I like the idea :) > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > >> + streams_mask & ~found_streams, sd->entity.name, pad); >> + ret = -EINVAL; >> + goto done; >> + } >> + >> + /* Call the .enable_streams() operation. */ >> + ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad, >> + streams_mask); >> + if (ret) >> + goto done; >> + >> + /* Mark the streams as enabled. */ >> + for (i = 0; i < state->stream_configs.num_configs; ++i) { >> + struct v4l2_subdev_stream_config *cfg = >> + &state->stream_configs.configs[i]; >> + >> + if (cfg->pad == pad && (streams_mask & BIT(cfg->stream))) >> + cfg->enabled = true; >> + } >> + >> +done: >> + v4l2_subdev_unlock_state(state); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(v4l2_subdev_enable_streams); >> + >> +static int v4l2_subdev_disable_streams_fallback(struct v4l2_subdev *sd, u32 pad, >> + u64 streams_mask) >> +{ >> + unsigned int i; >> + int ret; >> + >> + /* >> + * If the subdev doesn't implement pad-based stream enable, fall back >> + * on the .s_stream() operation. This can only be done for subdevs that >> + * have a single source pad, as sd->enabled_streams is global to the >> + * subdev. >> + */ >> + if (!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)) >> + return -EOPNOTSUPP; >> + >> + for (i = 0; i < sd->entity.num_pads; ++i) { >> + if (i != pad && sd->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE) >> + return -EOPNOTSUPP; >> + } >> + >> + if ((sd->enabled_streams & streams_mask) != streams_mask) >> + return -EALREADY; >> + >> + /* Stop streaming when the last streams are disabled. */ >> + if (!(sd->enabled_streams & ~streams_mask)) { >> + ret = v4l2_subdev_call(sd, video, s_stream, 0); >> + if (ret) >> + return ret; >> + } >> + >> + sd->enabled_streams &= ~streams_mask; >> + >> + return 0; >> +} >> + >> +int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad, >> + u64 streams_mask) >> +{ >> + struct device *dev = sd->entity.graph_obj.mdev->dev; >> + struct v4l2_subdev_state *state; >> + u64 found_streams = 0; >> + unsigned int i; >> + int ret; >> + >> + /* A few basic sanity checks first. */ >> + if (pad >= sd->entity.num_pads) >> + return -EINVAL; >> + >> + if (!streams_mask) >> + return 0; >> + >> + /* Fallback on .s_stream() if .disable_streams() isn't available. */ >> + if (!sd->ops->pad || !sd->ops->pad->disable_streams) >> + return v4l2_subdev_disable_streams_fallback(sd, pad, >> + streams_mask); >> + >> + state = v4l2_subdev_lock_and_get_active_state(sd); >> + >> + /* >> + * Verify that the requested streams exist and that they are not >> + * already disabled. >> + */ >> + for (i = 0; i < state->stream_configs.num_configs; ++i) { >> + struct v4l2_subdev_stream_config *cfg = >> + &state->stream_configs.configs[i]; >> + >> + if (cfg->pad != pad || !(streams_mask & BIT(cfg->stream))) >> + continue; >> + >> + found_streams |= BIT(cfg->stream); >> + >> + if (!cfg->enabled) { >> + dev_dbg(dev, "stream %u already disabled on %s/%u\n", >> + cfg->stream, sd->entity.name, pad); >> + ret = -EALREADY; >> + goto done; >> + } >> + } >> + >> + if (found_streams != streams_mask) { >> + dev_dbg(dev, "streams 0x%llx not found on %s/%u\n", >> + streams_mask & ~found_streams, sd->entity.name, pad); >> + ret = -EINVAL; >> + goto done; >> + } >> + >> + /* Call the .disable_streams() operation. */ >> + ret = v4l2_subdev_call(sd, pad, disable_streams, state, pad, >> + streams_mask); >> + if (ret) >> + goto done; >> + >> + /* Mark the streams as disabled. */ >> + for (i = 0; i < state->stream_configs.num_configs; ++i) { >> + struct v4l2_subdev_stream_config *cfg = >> + &state->stream_configs.configs[i]; >> + >> + if (cfg->pad == pad && (streams_mask & BIT(cfg->stream))) >> + cfg->enabled = false; >> + } >> + >> +done: >> + v4l2_subdev_unlock_state(state); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(v4l2_subdev_disable_streams); >> + >> #endif /* CONFIG_MEDIA_CONTROLLER */ >> >> void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >> index 992debe116ac..bb1713863973 100644 >> --- a/include/media/v4l2-subdev.h >> +++ b/include/media/v4l2-subdev.h >> @@ -702,6 +702,7 @@ struct v4l2_subdev_pad_config { >> * >> * @pad: pad number >> * @stream: stream number >> + * @enabled: has the stream been enabled with v4l2_subdev_enable_stream() >> * @fmt: &struct v4l2_mbus_framefmt >> * @crop: &struct v4l2_rect to be used for crop >> * @compose: &struct v4l2_rect to be used for compose >> @@ -711,6 +712,7 @@ struct v4l2_subdev_pad_config { >> struct v4l2_subdev_stream_config { >> u32 pad; >> u32 stream; >> + bool enabled; >> >> struct v4l2_mbus_framefmt fmt; >> struct v4l2_rect crop; >> @@ -816,6 +818,18 @@ struct v4l2_subdev_state { >> * >> * @set_routing: enable or disable data connection routes described in the >> * subdevice routing table. >> + * >> + * @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. >> + * >> + * @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. >> */ >> struct v4l2_subdev_pad_ops { >> int (*init_cfg)(struct v4l2_subdev *sd, >> @@ -862,6 +876,12 @@ struct v4l2_subdev_pad_ops { >> struct v4l2_subdev_state *state, >> enum v4l2_subdev_format_whence which, >> struct v4l2_subdev_krouting *route); >> + int (*enable_streams)(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, u32 pad, >> + u64 streams_mask); >> + int (*disable_streams)(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, u32 pad, >> + u64 streams_mask); >> }; >> >> /** >> @@ -1007,6 +1027,10 @@ struct v4l2_subdev_platform_data { >> * @active_state: Active state for the subdev (NULL for subdevs tracking the >> * state internally). Initialized by calling >> * v4l2_subdev_init_finalize(). >> + * @enabled_streams: Bitmask of enabled streams used by >> + * v4l2_subdev_enable_streams() and >> + * v4l2_subdev_disable_streams() helper functions for fallback >> + * cases. >> * >> * Each instance of a subdev driver should create this struct, either >> * stand-alone or embedded in a larger struct. >> @@ -1052,6 +1076,7 @@ struct v4l2_subdev { >> * doesn't support it. >> */ >> struct v4l2_subdev_state *active_state; >> + u64 enabled_streams; >> }; >> >> >> @@ -1589,6 +1614,66 @@ __v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting *routing, >> for ((route) = NULL; \ >> ((route) = __v4l2_subdev_next_active_route((routing), (route)));) >> >> +/** >> + * v4l2_subdev_enable_streams() - Enable streams on a pad >> + * @sd: The subdevice >> + * @pad: The pad >> + * @streams_mask: Bitmask of streams to enable >> + * >> + * This function enables streams on a source @pad of a subdevice. The pad is >> + * identified by its index, while the streams are identified by the >> + * @streams_mask bitmask. This allows enabling multiple streams on a pad at >> + * once. >> + * >> + * Enabling a stream that is already enabled isn't allowed. If @streams_mask >> + * contains an already enabled stream, this function returns -EALREADY without >> + * performing any operation. >> + * >> + * Per-stream enable is only available for subdevs that implement the >> + * .enable_streams() and .disable_streams() operations. For other subdevs, this >> + * function implements a best-effort compatibility by calling the .s_stream() >> + * operation, limited to subdevs that have a single source pad. >> + * >> + * Return: >> + * * 0: Success >> + * * -EALREADY: One of the streams in streams_mask is already enabled >> + * * -EINVAL: The pad index is invalid, or doesn't correspond to a source pad >> + * * -EOPNOTSUPP: Falling back to the legacy .s_stream() operation is >> + * impossible because the subdev has multiple source pads >> + */ >> +int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad, >> + u64 streams_mask); >> + >> +/** >> + * v4l2_subdev_disable_streams() - Disable streams on a pad >> + * @sd: The subdevice >> + * @pad: The pad >> + * @streams_mask: Bitmask of streams to disable >> + * >> + * This function disables streams on a source @pad of a subdevice. The pad is >> + * identified by its index, while the streams are identified by the >> + * @streams_mask bitmask. This allows disabling multiple streams on a pad at >> + * once. >> + * >> + * Disabling a streams that is not enabled isn't allowed. If @streams_mask >> + * contains a disabled stream, this function returns -EALREADY without >> + * performing any operation. >> + * >> + * Per-stream disable is only available for subdevs that implement the >> + * .enable_streams() and .disable_streams() operations. For other subdevs, this >> + * function implements a best-effort compatibility by calling the .s_stream() >> + * operation, limited to subdevs that have a single source pad. >> + * >> + * Return: >> + * * 0: Success >> + * * -EALREADY: One of the streams in streams_mask is not enabled >> + * * -EINVAL: The pad index is invalid, or doesn't correspond to a source pad >> + * * -EOPNOTSUPP: Falling back to the legacy .s_stream() operation is >> + * impossible because the subdev has multiple source pads >> + */ >> +int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad, >> + u64 streams_mask); >> + >> #endif /* CONFIG_MEDIA_CONTROLLER */ >> >> /** >> -- >> 2.25.1 >>
Hi, Here's v11 of the streams series (used to be "multiplexed streams"). v10 can be found from: https://lore.kernel.org/all/20211130141536.891878-1-tomi.valkeinen@ideasonboard.com/ This series is based on the v5 of the subdev active state: https://lore.kernel.org/all/20220301105548.305191-1-tomi.valkeinen@ideasonboard.com/ My work branch with additional drivers can be found from: git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git streams/work-v11 And there's also the v4l-utils series to add support to v4l2-ctl and media-ctl: https://lore.kernel.org/all/20211130141815.892354-1-tomi.valkeinen@ideasonboard.com/ The main changes compared to v10: - Rebased on the v5 of the active state series, which has different locking system than what we had in v10 of this series. - Moved code around in v4l2-subdev.[ch] so that the functions are inside the correct ifdefs - A better route validation helper v4l2_subdev_routing_validate - Subdev enable/disable_streams ops (see "media: v4l2-subdev: Add subdev .(enable|disable)_streams() operations") There are still some comments I haven't addressed from the v10 review, and I'd like to change the route flags a bit. We've also thought about adding more support for drivers that don't need multiple streams but would still use the new streams APIs to simplify the drivers. However, as the v10 is already quite old, and I posted the v5 for the active state, I thought it's better to also post the current version of the streams series. Tomi Jacopo Mondi (3): media: entity: Add iterator helper for entity pads media: Documentation: Add GS_ROUTING documentation media: subdev: Add for_each_active_route() macro Laurent Pinchart (9): media: entity: Add has_route entity operation media: entity: Add media_entity_has_route() function media: entity: Use routing information during graph traversal media: subdev: Add [GS]_ROUTING subdev ioctls and operations media: subdev: Fallback to pad config in v4l2_subdev_get_fmt() media: subdev: add v4l2_subdev_routing_validate() helper media: v4l2-subdev: Add v4l2_subdev_state_xlate_streams() helper media: v4l2-subdev: Add subdev .(enable|disable)_streams() operations media: v4l2-subdev: Add v4l2_subdev_s_stream_helper() function Sakari Ailus (13): media: entity: Use pad as a starting point for graph walk media: entity: Use pads instead of entities in the media graph walk stack media: entity: Walk the graph based on pads media: mc: Start walk from a specific pad in use count calculation media: entity: Move the pipeline from entity to pads media: entity: Use pad as the starting point for a pipeline media: entity: Skip link validation for pads to which there is no route media: entity: Add an iterator helper for connected pads media: entity: Add only connected pads to the pipeline media: entity: Add debug information in graph walk route check media: Add bus type to frame descriptors media: Add CSI-2 bus configuration to frame descriptors media: Add stream to frame descriptor Tomi Valkeinen (11): media: subdev: increase V4L2_FRAME_DESC_ENTRY_MAX to 8 media: add V4L2_SUBDEV_FL_MULTIPLEXED media: add V4L2_SUBDEV_CAP_MPLEXED media: subdev: add v4l2_subdev_has_route() media: subdev: add v4l2_subdev_set_routing helper() media: Documentation: add multiplexed streams documentation media: subdev: add stream based configuration media: subdev: use streams in v4l2_subdev_link_validate() media: subdev: add "opposite" stream helper funcs media: subdev: add v4l2_subdev_get_fmt() helper function media: subdev: add v4l2_subdev_set_routing_with_fmt() helper .clang-format | 1 + Documentation/driver-api/media/mc-core.rst | 18 +- .../driver-api/media/v4l2-subdev.rst | 8 + .../userspace-api/media/v4l/dev-subdev.rst | 167 +++ .../userspace-api/media/v4l/user-func.rst | 1 + .../v4l/vidioc-subdev-enum-frame-interval.rst | 5 +- .../v4l/vidioc-subdev-enum-frame-size.rst | 5 +- .../v4l/vidioc-subdev-enum-mbus-code.rst | 5 +- .../media/v4l/vidioc-subdev-g-crop.rst | 5 +- .../media/v4l/vidioc-subdev-g-fmt.rst | 5 +- .../v4l/vidioc-subdev-g-frame-interval.rst | 5 +- .../media/v4l/vidioc-subdev-g-routing.rst | 150 +++ .../media/v4l/vidioc-subdev-g-selection.rst | 5 +- drivers/media/mc/mc-device.c | 13 +- drivers/media/mc/mc-entity.c | 270 +++-- drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 6 +- .../media/platform/exynos4-is/fimc-capture.c | 8 +- .../platform/exynos4-is/fimc-isp-video.c | 8 +- drivers/media/platform/exynos4-is/fimc-isp.c | 2 +- drivers/media/platform/exynos4-is/fimc-lite.c | 10 +- drivers/media/platform/exynos4-is/media-dev.c | 20 +- drivers/media/platform/omap3isp/isp.c | 2 +- drivers/media/platform/omap3isp/ispvideo.c | 25 +- drivers/media/platform/omap3isp/ispvideo.h | 2 +- .../media/platform/qcom/camss/camss-video.c | 6 +- drivers/media/platform/rcar-vin/rcar-core.c | 16 +- drivers/media/platform/rcar-vin/rcar-dma.c | 8 +- .../platform/rockchip/rkisp1/rkisp1-capture.c | 6 +- .../media/platform/s3c-camif/camif-capture.c | 6 +- drivers/media/platform/stm32/stm32-dcmi.c | 6 +- .../platform/sunxi/sun4i-csi/sun4i_dma.c | 6 +- .../platform/sunxi/sun6i-csi/sun6i_video.c | 6 +- drivers/media/platform/ti-vpe/cal-video.c | 6 +- drivers/media/platform/vsp1/vsp1_video.c | 18 +- drivers/media/platform/xilinx/xilinx-dma.c | 20 +- drivers/media/platform/xilinx/xilinx-dma.h | 2 +- .../media/test-drivers/vimc/vimc-capture.c | 6 +- drivers/media/usb/au0828/au0828-core.c | 8 +- drivers/media/v4l2-core/v4l2-ioctl.c | 25 +- drivers/media/v4l2-core/v4l2-mc.c | 43 +- drivers/media/v4l2-core/v4l2-subdev.c | 1002 ++++++++++++++++- drivers/staging/media/imx/imx-media-utils.c | 8 +- drivers/staging/media/ipu3/ipu3-v4l2.c | 6 +- drivers/staging/media/omap4iss/iss.c | 2 +- drivers/staging/media/omap4iss/iss_video.c | 40 +- drivers/staging/media/omap4iss/iss_video.h | 2 +- drivers/staging/media/tegra-video/tegra210.c | 6 +- include/media/media-entity.h | 125 +- include/media/v4l2-subdev.h | 403 ++++++- include/uapi/linux/v4l2-subdev.h | 88 +- 50 files changed, 2273 insertions(+), 343 deletions(-) create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst