Re: [PATCH v15 10/19] media: subdev: add stream based configuration

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 09/10/2022 09:24, Dafna Hirschfeld wrote:
> On 03.10.2022 15:18, 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@xxxxxxxxxxxxxxxx>
> > ---
> > .../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         | 154 +++++++++++++++++-
> > include/media/v4l2-subdev.h                   |  79 +++++++++
> > include/uapi/linux/v4l2-subdev.h              |  28 +++-
> > 10 files changed, 275 insertions(+), 21 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]
> >       - 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 1049c07d2e49..be778e619694 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -159,8 +159,22 @@ 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(struct v4l2_subdev *sd, struct 
> > v4l2_subdev_state *state,
> > +               u32 which, u32 pad, u32 stream)
> > {
> > +    if (sd->flags & V4L2_SUBDEV_FL_STREAMS) {
> > +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > +        if (!v4l2_subdev_state_get_stream_format(state, pad, stream))
> > +            return -EINVAL;
> > +        return 0;
> > +#else
> > +        return -EINVAL;
> > +#endif
> > +    }
> > +
> > +    if (stream != 0)
> > +        return -EINVAL;
> > +
> >     if (which == V4L2_SUBDEV_FORMAT_TRY && (!state || !state->pads))
> >         return -EINVAL;
> >
> > @@ -175,7 +189,7 @@ 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(sd, state, format->which, format->pad, 
> > format->stream);
> > }
> >
> > static int call_get_fmt(struct v4l2_subdev *sd,
> > @@ -202,7 +216,7 @@ 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(sd, state, code->which, code->pad, 
> > code->stream) ? :
> >            sd->ops->pad->enum_mbus_code(sd, state, code);
> > }
> >
> > @@ -214,7 +228,7 @@ 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(sd, state, fse->which, fse->pad, fse->stream) ? :
> >            sd->ops->pad->enum_frame_size(sd, state, fse);
> > }
> >
> > @@ -249,7 +263,7 @@ 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(sd, state, fie->which, fie->pad, fie->stream) ? :
> >            sd->ops->pad->enum_frame_interval(sd, state, fie);
> > }
> >
> > @@ -261,7 +275,7 @@ 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(sd, state, sel->which, sel->pad, sel->stream);
> > }
> >
> > static int call_get_selection(struct v4l2_subdev *sd,
> > @@ -1095,7 +1109,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_STREAMS) && sd->entity.num_pads) {
> >         state->pads = kvcalloc(sd->entity.num_pads,
> >                        sizeof(*state->pads), GFP_KERNEL);
> >         if (!state->pads) {
> > @@ -1135,6 +1150,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);
> > }
> > @@ -1164,6 +1180,59 @@ EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
> >
> > #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> >
> > +static int
> > +v4l2_subdev_init_stream_configs(struct v4l2_subdev_stream_configs 
> > *stream_configs,
> > +                const struct v4l2_subdev_krouting *routing)
> > +{
> > +    struct v4l2_subdev_stream_configs new_configs = { 0 };
> > +    struct v4l2_subdev_route *route;
> > +    u32 format_idx = 0;
> I think you don't need both format_idx and idx,
> also, it can be 'int'

Yes, we can do with just a single variable.

I'll keep it as unsigned, as it's an index to an array. Signed doesn't 
make sense there.

 Tomi