Re: [PATCH v3 2/8] media: v4l: subdev: Also return pads array information on stream functions

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

Thank you for the patch.

On Mon, Oct 23, 2023 at 08:44:02PM +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.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++++++++
>  include/media/v4l2-subdev.h           |  9 ++++---
>  2 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index ee4fe8f33a41..955ee9a6c91f 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->sd->entity.num_pads))
> +			pad = 0;

Do we want to carry this forward from the existing pad config accessors,
can't we return NULL instead ?

Either way,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +		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->sd->entity.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->sd->entity.num_pads))
> +			pad = 0;
> +
> +		return &state->pads[pad].try_compose;
> +	}
> +
>  	lockdep_assert_held(state->lock);
>  
>  	stream_configs = &state->stream_configs;
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 6a02a565035c..0ba1cd8c3ac7 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1550,7 +1550,8 @@ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd,
>   * 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.
> + * For stream-unaware drivers the format for the corresponding pad is returned.
> + * If the pad does not exist, NULL is returned.
>   */
>  struct v4l2_mbus_framefmt *
>  v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state,
> @@ -1565,7 +1566,8 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state,
>   * This returns a pointer to crop rectangle for the given pad + stream in the
>   * subdev state.
>   *
> - * If the state does not contain the given pad + stream, NULL is returned.
> + * For stream-unaware drivers the crop rectangle for the corresponding pad is
> + * returned. If the pad does not exist, NULL is returned.
>   */
>  struct v4l2_rect *
>  v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state,
> @@ -1581,7 +1583,8 @@ v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state,
>   * This returns a pointer to compose rectangle for the given pad + stream in the
>   * subdev state.
>   *
> - * If the state does not contain the given pad + stream, NULL is returned.
> + * For stream-unaware drivers the compose rectangle for the corresponding pad is
> + * returned. If the pad does not exist, NULL is returned.
>   */
>  struct v4l2_rect *
>  v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state,

-- 
Regards,

Laurent Pinchart