Re: [RFC PATCH v1 3/4] media: v4l2-subdev: Store frame interval in subdev state

Tomi Valkeinen
2023-10-24T07:21:26Z

On 24/10/2023 03:51, Laurent Pinchart wrote:
> Subdev states store all standard pad configuration data, except for
> frame intervals. Fix it by adding interval fields in the
> v4l2_subdev_pad_config and v4l2_subdev_stream_config structures, with
> corresponding accessor functions and a helper function to implement the
> .get_frame_interval() operation.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 44 +++++++++++++++++++++
> include/media/v4l2-subdev.h | 57 +++++++++++++++++++++++++++
> 2 files changed, 101 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index c45d60a87701..518f2f35c68d 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1618,6 +1618,29 @@ int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
> }
> EXPORT_SYMBOL_GPL(v4l2_subdev_get_fmt);
>
> +int v4l2_subdev_get_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_frame_interval *fi)
> +{
> + struct v4l2_fract *interval;
> +
> + if (sd->flags & V4L2_SUBDEV_FL_STREAMS)
> + interval = v4l2_subdev_state_get_stream_interval(state, fi->pad,
> + fi->stream);
> + else if (fi->pad < sd->entity.num_pads && fi->stream == 0)
> + interval = v4l2_subdev_get_pad_interval(sd, state, fi->pad);
> + else
> + interval = NULL;
> +
> + if (!interval)
> + return -EINVAL;
> +
> + fi->interval = *interval;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_frame_interval);
> +
> int v4l2_subdev_set_routing(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *state,
> const struct v4l2_subdev_krouting *routing)
> @@ -1761,6 +1784,27 @@ v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state,
> }
> EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_stream_compose);
>
> +struct v4l2_fract *
> +v4l2_subdev_state_get_stream_interval(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].interval;
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_stream_interval);
> +
> int v4l2_subdev_routing_find_opposite_end(const struct v4l2_subdev_krouting *routing,
> u32 pad, u32 stream, u32 *other_pad,
> u32 *other_stream)
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 962b546d0e3b..363f9a8a084c 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -681,11 +681,13 @@ struct v4l2_subdev_ir_ops {
> * @format: &struct v4l2_mbus_framefmt
> * @crop: &struct v4l2_rect to be used for crop
> * @compose: &struct v4l2_rect to be used for compose
> + * @interval: frame interval
> */
> struct v4l2_subdev_pad_config {
> struct v4l2_mbus_framefmt format;
> struct v4l2_rect crop;
> struct v4l2_rect compose;
> + struct v4l2_fract interval;
> };
>
> /**
> @@ -697,6 +699,7 @@ struct v4l2_subdev_pad_config {
> * @fmt: &struct v4l2_mbus_framefmt
> * @crop: &struct v4l2_rect to be used for crop
> * @compose: &struct v4l2_rect to be used for compose
> + * @interval: frame interval
> *
> * This structure stores configuration for a stream.
> */
> @@ -708,6 +711,7 @@ struct v4l2_subdev_stream_config {
> struct v4l2_mbus_framefmt fmt;
> struct v4l2_rect crop;
> struct v4l2_rect compose;
> + struct v4l2_fract interval;
> };
>
> /**
> @@ -1199,6 +1203,26 @@ v4l2_subdev_get_pad_compose(struct v4l2_subdev *sd,
> return &state->pads[pad].compose;
> }
>
> +/**
> + * v4l2_subdev_get_pad_interval - ancillary routine to get
> + * &struct v4l2_subdev_pad_config->interval
> + *
> + * @sd: pointer to &struct v4l2_subdev
> + * @state: pointer to &struct v4l2_subdev_state.
> + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array.
> + */
> +static inline struct v4l2_fract *
> +v4l2_subdev_get_pad_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + unsigned int pad)
> +{
> + if (WARN_ON(!state))
> + return NULL;
> + if (WARN_ON(pad >= sd->entity.num_pads))
> + pad = 0;
> + return &state->pads[pad].interval;
> +}
> +
> /*
> * Temprary helpers until uses of v4l2_subdev_get_try_* functions have been
> * renamed
> @@ -1489,6 +1513,24 @@ v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
> int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
> struct v4l2_subdev_format *format);
>
> +/**
> + * v4l2_subdev_get_frame_interval() - Fill frame interval based on state
> + * @sd: subdevice
> + * @state: subdevice state
> + * @fi: pointer to &struct v4l2_subdev_frame_interval
> + *
> + * Fill @fi->interval field based on the information in the @fi struct.
> + *
> + * This function can be used by the subdev drivers which support active state to
> + * implement v4l2_subdev_pad_ops.get_frame_interval if the subdev driver does
> + * not need to do anything special in their get_frame_interval op.

What would the "something special" be? Can we instead just say that the
framework will return the frame interval from the state for drivers that
support active state (similar to get-routing)? Or perhaps that's not a
wise thing to do for old operations...

Tomi