Hi Sakari,
Thank you for the patch.
On Fri, Oct 13, 2023 at 01:44:20PM +0300, Sakari Ailus wrote:
> There are two sets of functions that return information from sub-device
> state, one for stream-unaware users and another for stream-aware users.
> Add support for stream-aware functions to return format, crop and compose
> information from pad-based array that are functionally equivalent to the
> old, stream-unaware ones.
>
> Also check state is non-NULL, in order to guard against old drivers
> potentially calling this with NULL state for active formats or selection
> rectangles.
I'm not too keen on this I'm afraid :-( I think it gets confusing for
drivers that are not stream-aware to have to call a function that takes
a stream number. I don't see a problem with keeping two different sets
of functions, one for stream-aware drivers, and one for other drivers.
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 52a8043ab556..7d0ce8c8aab4 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1684,6 +1684,19 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state,
> struct v4l2_subdev_stream_configs *stream_configs;
> unsigned int i;
>
> + if (WARN_ON(!state))
> + return NULL;
> +
> + if (state->pads) {
> + if (stream)
> + return NULL;
> +
> + if (WARN_ON(pad >= state->num_pads))
> + pad = 0;
> +
> + return &state->pads[pad].try_fmt;
> + }
> +
> lockdep_assert_held(state->lock);
>
> stream_configs = &state->stream_configs;
> @@ -1705,6 +1718,19 @@ v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state,
> struct v4l2_subdev_stream_configs *stream_configs;
> unsigned int i;
>
> + if (WARN_ON(!state))
> + return NULL;
> +
> + if (state->pads) {
> + if (stream)
> + return NULL;
> +
> + if (WARN_ON(pad >= state->num_pads))
> + pad = 0;
> +
> + return &state->pads[pad].try_crop;
> + }
> +
> lockdep_assert_held(state->lock);
>
> stream_configs = &state->stream_configs;
> @@ -1726,6 +1752,19 @@ v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state,
> struct v4l2_subdev_stream_configs *stream_configs;
> unsigned int i;
>
> + if (WARN_ON(!state))
> + return NULL;
> +
> + if (state->pads) {
> + if (stream)
> + return NULL;
> +
> + if (WARN_ON(pad >= state->num_pads))
> + pad = 0;
> +
> + return &state->pads[pad].try_compose;
> + }
> +
> lockdep_assert_held(state->lock);
>
> stream_configs = &state->stream_configs;
--
Regards,
Laurent Pinchart
