Hi Hans,
On Wed, Oct 25, 2023 at 01:20:29PM +0200, Hans Verkuil wrote:
> On 25/10/2023 12:19, Sakari Ailus wrote:
> > Hi Hans,
> >
> > Thank you for the review.
> >
> > On Wed, Oct 25, 2023 at 11:53:35AM +0200, Hans Verkuil wrote:
> >> On 23/10/2023 19:44, Sakari Ailus wrote:
> >>> The sub-device state access functions take three arguments: sub-device
> >>> state, pad and stream. The stream is not relevant for the majority of
> >>> drivers and having to specify 0 for the stream is considered a nuisance.
> >>>
> >>> Provide a two-argument macros for these state access functions to cover
> >>> the needs of stream-unaware users.
> >>>
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >>> ---
> >>> drivers/media/v4l2-core/v4l2-subdev.c | 18 ++++++-------
> >>> include/media/v4l2-subdev.h | 39 ++++++++++++++++++++-------
> >>> 2 files changed, 39 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> index f0f0af48730f..e35226587244 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> @@ -1671,8 +1671,8 @@ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd,
> >>> EXPORT_SYMBOL_GPL(v4l2_subdev_set_routing_with_fmt);
> >>>
> >>> struct v4l2_mbus_framefmt *
> >>> -v4l2_subdev_state_get_format(struct v4l2_subdev_state *state, unsigned int pad,
> >>> - u32 stream)
> >>> +__v4l2_subdev_state_get_format(struct v4l2_subdev_state *state,
> >>> + unsigned int pad, u32 stream)
> >>> {
> >>> struct v4l2_subdev_stream_configs *stream_configs;
> >>> unsigned int i;
> >>> @@ -1702,11 +1702,11 @@ v4l2_subdev_state_get_format(struct v4l2_subdev_state *state, unsigned int pad,
> >>>
> >>> return NULL;
> >>> }
> >>> -EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_format);
> >>> +EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_format);
> >>>
> >>> struct v4l2_rect *
> >>> -v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad,
> >>> - u32 stream)
> >>> +__v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad,
> >>> + u32 stream)
> >>> {
> >>> struct v4l2_subdev_stream_configs *stream_configs;
> >>> unsigned int i;
> >>> @@ -1736,11 +1736,11 @@ v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad,
> >>>
> >>> return NULL;
> >>> }
> >>> -EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_crop);
> >>> +EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_crop);
> >>>
> >>> struct v4l2_rect *
> >>> -v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state, unsigned int pad,
> >>> - u32 stream)
> >>> +__v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state,
> >>> + unsigned int pad, u32 stream)
> >>> {
> >>> struct v4l2_subdev_stream_configs *stream_configs;
> >>> unsigned int i;
> >>> @@ -1770,7 +1770,7 @@ v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state, unsigned int pad,
> >>>
> >>> return NULL;
> >>> }
> >>> -EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_compose);
> >>> +EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_compose);
> >>>
> >>> int v4l2_subdev_routing_find_opposite_end(const struct v4l2_subdev_krouting *routing,
> >>> u32 pad, u32 stream, u32 *other_pad,
> >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>> index 71cb35133b4a..12eb0fe746d7 100644
> >>> --- a/include/media/v4l2-subdev.h
> >>> +++ b/include/media/v4l2-subdev.h
> >>> @@ -1545,7 +1545,7 @@ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd,
> >>> * v4l2_subdev_state_get_format() - Get pointer to a stream format
> >>> * @state: subdevice state
> >>> * @pad: pad id
> >>> - * @stream: stream id
> >>> + * @...: stream id (optional argument)
> >>> *
> >>> * This returns a pointer to &struct v4l2_mbus_framefmt for the given pad +
> >>> * stream in the subdev state.
> >>> @@ -1553,15 +1553,22 @@ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd,
> >>> * For stream-unaware drivers the format for the corresponding pad is returned.
> >>> * If the pad does not exist, NULL is returned.
> >>> */
> >>> +#define v4l2_subdev_state_get_format(state, pad, ...) \
> >>> + __v4l2_subdev_state_get_format_ ## __VA_OPT__(stream) \
> >>> + (state, pad __VA_OPT__(,) __VA_ARGS__)
> >>> +#define __v4l2_subdev_state_get_format_(state, pad) \
> >>> + __v4l2_subdev_state_get_format(state, pad, 0)
> >>> +#define __v4l2_subdev_state_get_format_stream(state, pad, stream) \
> >>> + __v4l2_subdev_state_get_format(state, pad, stream)
> >>
> >> Hmm, 'git grep __VA_OPT__' finds no use of this in the kernel. Are you sure this
> >> is safe to use? Does clang support it? Do all gcc versions that the kernel support
> >> implement it?
> >>
> >> A 10+ for creativity, but I'm a bit concerned about it.
> >
> > :-)
> >
> > I noticed the problem, too, as the lkp bot failed compiling this on GCC
> > 7.5. Indeed only GCC 8 supports __VA_OPT__ but the oldest GCC version
> > supported for kernel compilation is 7.1. So __VA_OPT__ isn't... an option.
> >
> > I thought this could be done using variable arguments, making most error
> > handling take place at runtime (for GCC versions not supporting
> > __VA_OPT__), but then Laurent figured this can be done with just
> > __VA_ARGS__ which is part of C99.
> >
> > The code would look like this, which is only slightly less understandable
> > than with __VA_OPT__:
> >
> > #define v4l2_subdev_state_get_format(...) \
> > __v4l2_subdev_state_get_format_var(__VA_ARGS__, , _2)(__VA_ARGS__)
> > #define __v4l2_subdev_state_get_format_var(_1, _2, _3, NUM, ...) \
> > __v4l2_subdev_state_get_format ## NUM
> > #define __v4l2_subdev_state_get_format_2(state, pad) \
> > __v4l2_subdev_state_get_format(state, pad, 0)
> > struct v4l2_mbus_framefmt *
> > __v4l2_subdev_state_get_format(struct v4l2_subdev_state *state,
> > unsigned int pad, u32 stream);
>
> Wow! OK, I guess that will work.
>
> A strongly suggest adding a comment before this explaining what you are
> trying to achieve. It's not exactly obvious :-)
I'll add:
/*
* Wrap v4l2_subdev_state_get_format(), allowing the function to be called with
* two or three arguments. The purpose of the __v4l2_subdev_state_get_format()
* macro below is to come up with the name of the function or macro to call,
* using the last two arguments (_stream and _pad). The selected function or
* macro is then called using the arguments specified by the caller. A similar
* arrangement is used for v4l2_subdev_state_crop() and
* v4l2_subdev_state_compose() below.
*/
--
Sakari Ailus
