| Message ID | 20220301161156.1119557-27-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v4l: routing and streams support | expand |
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 >
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 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
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] - 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; } -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; +} + 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); } @@ -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]; + + 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; + + 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); + + 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. + * + * 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 * @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]; }; /**
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(-)