Re: [PATCH v8 01/16] media: subdev: Export get_format helper for link validation

[Tomi Valkeinen]

On 31/07/2023 11:29, Jai Luthra wrote:
> For link validation on video device drivers, it may be required to
> match the formats set on the source subdev with the formats set on the
> video device.
>
> Export the existing v4l2_subdev_link_validate_get_format() helper so it
> can be reused by such drivers.
>
> Suggested-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jai Luthra <j-luthra@xxxxxx>
> ---
> New in v8
>
>   drivers/media/v4l2-core/v4l2-subdev.c |  8 ++++----
>   include/media/v4l2-subdev.h           | 12 ++++++++++++
>   2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 217b8019fb9b..0d3b5ff5cacc 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1130,10 +1130,9 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
>   }
>   EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
>   
> -static int
> -v4l2_subdev_link_validate_get_format(struct media_pad *pad, u32 stream,
> -				     struct v4l2_subdev_format *fmt,
> -				     bool states_locked)
> +int v4l2_subdev_link_validate_get_format(struct media_pad *pad, u32 stream,
> +					 struct v4l2_subdev_format *fmt,
> +					 bool states_locked)
>   {
>   	struct v4l2_subdev_state *state;
>   	struct v4l2_subdev *sd;
> @@ -1165,6 +1164,7 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad, u32 stream,
>   
>   	return ret;
>   }
> +EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_get_format);
>   
>   #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>   
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a012741cc876..ef7007f46889 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1301,6 +1301,18 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
>   				      struct v4l2_subdev_format *source_fmt,
>   				      struct v4l2_subdev_format *sink_fmt);
>   
> +/**
> + * v4l2_subdev_link_validate_get_format - get format for media link validation
> + *
> + * @pad: pad id
> + * @stream: stream id
> + * @fmt: pointer to &struct v4l2_subdev_format
> + * @states_locked: is the subdev state already locked
> + */
> +int v4l2_subdev_link_validate_get_format(struct media_pad *pad, u32 stream,
> +					 struct v4l2_subdev_format *fmt,
> +					 bool states_locked);
> +
>   /**
>    * v4l2_subdev_link_validate - validates a media link
>    *

I don't know about this one... The 
v4l2_subdev_link_validate_get_format() is a bit of an internal function, 
especially with the relatively-new "states_locked" parameter. If it's 
made public, at least the "states_locked" should be renamed to 
"state_locked" (well, it could perhaps be renamed anyway).

But just looking at the function doc above, I think the developer's 
question would be "what does this do?". What does "get format for media 
link validation" mean? Is it similar to 
v4l2_subdev_state_get_stream_format() or v4l2_subdev_get_pad_format()? 
What does states_locked do?

How do you use this with multi-stream support? Do you lock the source, 
then validate all streams (with the help of this function), and then 
unlock? If yes, then all the states_locked stuff is extra, and all the 
function really does is v4l2_subdev_call(sd, pad, get_fmt, state, fmt). 
And if so, would it be clearer to just do that, instead of hiding it 
behind v4l2_subdev_link_validate_get_format()?

 Tomi