Re: [PATCH v6 12/28] media: v4l: subdev: Add helpers for format, crop and compose pointers

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 03/10/2023 15:07, Sakari Ailus wrote:
> Add a helper for obtaining format, crop and compose pointers. These are
> convenient for drivers, independently of the driver uses streams or not.

If we go with these, should we deprecate 
v4l2_subdev_state_get_stream_format() and v4l2_subdev_get_pad_format()?

Having three different ways to get the fmt seems a bit excessive.

Should we add 'num_pads' to the state? That would remove the need pass 
the subdevice to these functions.

> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
>   drivers/media/v4l2-core/v4l2-subdev.c | 54 ++++++++++++++++++++++----
>   include/media/v4l2-subdev.h           | 56 +++++++++++++++++++++++++++
>   2 files changed, 102 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index d295a4e87b66..854f9d4db923 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1573,19 +1573,57 @@ v4l2_subdev_init_stream_configs(struct v4l2_subdev_stream_configs *stream_config
>   	return 0;
>   }
>   
> +struct v4l2_mbus_framefmt
> +*v4l2_subdev_get_fmt_ptr(struct v4l2_subdev *sd,
> +			 struct v4l2_subdev_state *state, unsigned int pad,
> +			 unsigned int stream)
> +{
> +	if (sd->flags & V4L2_SUBDEV_FL_STREAMS)
> +		return v4l2_subdev_state_get_stream_format(state, pad, stream);
> +
> +	if (pad < sd->entity.num_pads && stream == 0)
> +		return v4l2_subdev_get_pad_format(sd, state, pad);
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_fmt_ptr);
> +
> +struct v4l2_rect
> +*v4l2_subdev_get_crop_ptr(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_state *state, unsigned int pad,
> +			  unsigned int stream)
> +{
> +	if (sd->flags & V4L2_SUBDEV_FL_STREAMS)
> +		return v4l2_subdev_state_get_stream_crop(state, pad, stream);
> +
> +	if (pad < sd->entity.num_pads && stream == 0)
> +		return v4l2_subdev_get_pad_crop(sd, state, pad);
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_crop_ptr);
> +
> +struct v4l2_rect
> +*v4l2_subdev_get_compose_ptr(struct v4l2_subdev *sd,
> +			     struct v4l2_subdev_state *state, unsigned int pad,
> +			     unsigned int stream)
> +{
> +	if (sd->flags & V4L2_SUBDEV_FL_STREAMS)
> +		return v4l2_subdev_state_get_stream_compose(state, pad, stream);
> +
> +	if (pad < sd->entity.num_pads && stream == 0)
> +		return v4l2_subdev_get_pad_compose(sd, state, pad);
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_compose_ptr);
> +
>   int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
>   			struct v4l2_subdev_format *format)
>   {
>   	struct v4l2_mbus_framefmt *fmt;
>   
> -	if (sd->flags & V4L2_SUBDEV_FL_STREAMS)
> -		fmt = v4l2_subdev_state_get_stream_format(state, format->pad,
> -							  format->stream);
> -	else if (format->pad < sd->entity.num_pads && format->stream == 0)
> -		fmt = v4l2_subdev_get_pad_format(sd, state, format->pad);
> -	else
> -		fmt = NULL;
> -
> +	fmt = v4l2_subdev_get_fmt_ptr(sd, state, format->pad, format->stream);
>   	if (!fmt)
>   		return -EINVAL;
>   
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 5f59ff0796b7..7c34243ffed9 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1479,6 +1479,62 @@ v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
>   
>   #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>   
> +/**
> + * v4l2_subdev_get_fmt_ptr - Obtain a pointer to V4L2 sub-device format for pad
> + *			     and stream
> + * @sd: subdevice
> + * @state: subdevice state
> + * @pad: the pad on the sub-device
> + * @stream: stream in the pad
> + *
> + * For given pad and stream, obtain a pointer to the mbus format from the
> + * sub-device.
> + *
> + * Returns NULL if the format is not found or the parameters are invalid.
> + */
> +struct v4l2_mbus_framefmt *
> +v4l2_subdev_get_fmt_ptr(struct v4l2_subdev *sd,
> +			struct v4l2_subdev_state *state, unsigned int pad,
> +			unsigned int stream);
> +
> +/**
> + * v4l2_subdev_get_crop_ptr - Obtain a pointer to V4L2 sub-device crop
> + *			      rectangle for pad and stream
> + * @sd: subdevice
> + * @state: subdevice state
> + * @pad: the pad on the sub-device
> + * @stream: stream in the pad
> + *
> + * For given pad and stream, obtain a pointer to the crop selection rectangle
> + * from the sub-device.
> + *
> + * Returns NULL if the selection rectangle is not found or the parameters are
> + * invalid.
> + */
> +struct v4l2_rect *
> +v4l2_subdev_get_crop_ptr(struct v4l2_subdev *sd,
> +			 struct v4l2_subdev_state *state, unsigned int pad,
> +			 unsigned int stream);
> +
> +/**
> + * v4l2_subdev_get_compose_ptr - Obtain a pointer to V4L2 sub-device compose
> + *				 rectangle for pad and stream
> + * @sd: subdevice
> + * @state: subdevice state
> + * @pad: the pad on the sub-device
> + * @stream: stream in the pad
> + *
> + * For given pad and stream, obtain a pointer to the compose selection rectangle
> + * from the sub-device.
> + *
> + * Returns NULL if the selection rectangle is not found or the parameters are
> + * invalid.
> + */
> +struct v4l2_rect *
> +v4l2_subdev_get_compose_ptr(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_state *state, unsigned int pad,
> +			    unsigned int stream);
> +
>   /**
>    * v4l2_subdev_get_fmt() - Fill format based on state
>    * @sd: subdevice