lkml.org 
[lkml]   [2023]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v8 01/16] media: subdev: Export get_format helper for link validation
From
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@ideasonboard.com>
> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> ---
> 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

\
 
 \ /
  Last update: 2023-08-01 16:09    [W:0.236 / U:1.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site