linux-kernel - Re: [PATCH v8 01/16] media: subdev: Export get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3a5a3d5a-f2f1-f84b-f189-ad56ff77e431@ideasonboard.com>
Date:   Tue, 1 Aug 2023 17:09:22 +0300
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Jai Luthra <j-luthra@...com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        niklas.soderlund+renesas@...natech.se,
        Benoit Parrot <bparrot@...com>,
        Vaishnav Achath <vaishnav.a@...com>,
        Vignesh Raghavendra <vigneshr@...com>, nm@...com,
        devarsht@...com
Subject: Re: [PATCH v8 01/16] media: subdev: Export get_format helper for link
 validation

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@...asonboard.com>
> Signed-off-by: Jai Luthra <j-luthra@...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