Messages in this thread | | | Date | Tue, 1 Aug 2023 17:09:22 +0300 | Subject | Re: [PATCH v8 01/16] media: subdev: Export get_format helper for link validation | From | 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@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
| |