On 24/03/2022 09:00, Tomi Valkeinen wrote:
> It is common that media drivers call subdev ops in source subdevs, and
> pass NULL as the state. This was the way to indicate that the callee
> should use the callee's private active state.
>
> E.g.:
>
> v4l2_subdev_call(priv->source_sd, pad, get_fmt, NULL, &sd_fmt);
>
> Now that we have a real subdev active state in the v4l2_subdev struct,
> we want the caller to pass a proper state (when available). And
> furthermore, the state should be locked.
>
> This would mean changing all the callers, which is the long term goal.
>
> To fix this issue in the short term, let's add an extra wrapper layer to
> all v4l2_subdev_call_pad_wrappers which deal with states. These wrappers
> handle the state == NULL case by using the locked active state instead
> (when available).
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
Regards,
Hans
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 42 ++++++++++++++++++++++-----
> 1 file changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 824424f0a741..d8d1c9ef4dc4 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -319,14 +319,42 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
> sd->ops->pad->get_mbus_config(sd, pad, config);
> }
>
> +/*
> + * Create state-management wrapper for pad ops dealing with subdev state. The
> + * wrapper handles the case where the caller does not provide the called
> + * subdev's state. This should be removed when all the callers are fixed.
> + */
> +#define DEFINE_STATE_WRAPPER(f, arg_type) \
> + static int call_##f##_state(struct v4l2_subdev *sd, \
> + struct v4l2_subdev_state *_state, \
> + arg_type *format) \
> + { \
> + struct v4l2_subdev_state *state = _state; \
> + int ret; \
> + if (!_state) \
> + state = v4l2_subdev_lock_and_get_active_state(sd); \
> + ret = call_##f(sd, state, format); \
> + if (!_state && state) \
> + v4l2_subdev_unlock_state(state); \
> + return ret; \
> + }
> +
> +DEFINE_STATE_WRAPPER(get_fmt, struct v4l2_subdev_format);
> +DEFINE_STATE_WRAPPER(set_fmt, struct v4l2_subdev_format);
> +DEFINE_STATE_WRAPPER(enum_mbus_code, struct v4l2_subdev_mbus_code_enum);
> +DEFINE_STATE_WRAPPER(enum_frame_size, struct v4l2_subdev_frame_size_enum);
> +DEFINE_STATE_WRAPPER(enum_frame_interval, struct v4l2_subdev_frame_interval_enum);
> +DEFINE_STATE_WRAPPER(get_selection, struct v4l2_subdev_selection);
> +DEFINE_STATE_WRAPPER(set_selection, struct v4l2_subdev_selection);
> +
> static const struct v4l2_subdev_pad_ops v4l2_subdev_call_pad_wrappers = {
> - .get_fmt = call_get_fmt,
> - .set_fmt = call_set_fmt,
> - .enum_mbus_code = call_enum_mbus_code,
> - .enum_frame_size = call_enum_frame_size,
> - .enum_frame_interval = call_enum_frame_interval,
> - .get_selection = call_get_selection,
> - .set_selection = call_set_selection,
> + .get_fmt = call_get_fmt_state,
> + .set_fmt = call_set_fmt_state,
> + .enum_mbus_code = call_enum_mbus_code_state,
> + .enum_frame_size = call_enum_frame_size_state,
> + .enum_frame_interval = call_enum_frame_interval_state,
> + .get_selection = call_get_selection_state,
> + .set_selection = call_set_selection_state,
> .get_edid = call_get_edid,
> .set_edid = call_set_edid,
> .dv_timings_cap = call_dv_timings_cap,
