diff mbox series

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

Message ID 20230731-upstream_csi-v8-1-fb7d3661c2c9@ti.com (mailing list archive)
State Changes Requested
Headers
Series CSI2RX support on J721E and AM62 |

Commit Message

Jai Luthra July 31, 2023, 8:29 a.m. UTC 
  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(-)

Comments

Tomi Valkeinen Aug. 1, 2023, 2:09 p.m. UTC | #1 
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
diff mbox series

Patch

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
  *