Re: [PATCH v8 02/36] media: subdev: add active state to struct v4l2_subdev

[Jacopo Mondi <jacopo@jmondi.org>]

Hi Tomi,

On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote:
> Add a new 'state' field to struct v4l2_subdev to which we can store the
> active state of a subdev. This will place the subdev configuration into
> a known place, allowing us to use the state directly from the v4l2
> framework, thus simplifying the drivers.
>
> We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(),
> which need to be used by the drivers that support subdev state in struct
> v4l2_subdev.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++
>  include/media/v4l2-subdev.h           | 36 +++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 26a34a8e3d37..e1a794f69815 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>  	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
> +
> +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd)
> +{
> +	struct v4l2_subdev_state *state;
> +
> +	state = v4l2_alloc_subdev_state(sd);

So, I think this is one source of confusion about init_cfg.

v4l2_alloc_subdev_state() calls init_cfg() and 'state-aware' driver
are now supposed to allocate their state by calling
v4l2_subdev_alloc_state(), in the same way as the core does for the
file-handle ones.

This will lead to init_cfg to be called for the 'active' (ie owned by
the subdev) state, and then you need to add context to the state (by
adding a 'which' field) to know what state you're dealing with.

According to the init_cfg() documentation

 * @init_cfg: initialize the pad config to default values

the op has to be called in order to initialize the per-file-handle
context, not the active one.

I would rather just embed 'struct v4l2_subdev_state' in 'struct
v4l2_subdev', have the core going through the
'v4l2_subdev_alloc_state()' to initialize the per-fh state, but have
drivers initialize their own state at probe time. If they need for
some reason to access their 'active' state at init_cfg() time, they
caan fish it from their subdev.

If I'm not mistaken this will remove the need to have a which filed in
the state, as I think the 'context' should be inferred from the
'which' argument embedded in the ops-specific structures, and not held
in the state itself.

Thanks
   j


> +	if (IS_ERR(state))
> +		return PTR_ERR(state);
> +
> +	sd->state = state;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state);
> +
> +void v4l2_subdev_free_state(struct v4l2_subdev *sd)
> +{
> +	v4l2_free_subdev_state(sd->state);
> +	sd->state = NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_free_state);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 8701d2e7d893..ecaf040ead57 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -898,6 +898,8 @@ struct v4l2_subdev_platform_data {
>   * @subdev_notifier: A sub-device notifier implicitly registered for the sub-
>   *		     device using v4l2_async_register_subdev_sensor().
>   * @pdata: common part of subdevice platform data
> + * @state: active state for the subdev (NULL for subdevs tracking the state
> + * 	   internally)
>   *
>   * Each instance of a subdev driver should create this struct, either
>   * stand-alone or embedded in a larger struct.
> @@ -929,6 +931,7 @@ struct v4l2_subdev {
>  	struct v4l2_async_notifier *notifier;
>  	struct v4l2_async_notifier *subdev_notifier;
>  	struct v4l2_subdev_platform_data *pdata;
> +	struct v4l2_subdev_state *state;
>  };
>
>
> @@ -1217,4 +1220,37 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
>  void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>  			      const struct v4l2_event *ev);
>
> +/**
> + * v4l2_subdev_alloc_state() - Allocate active subdev state for subdevice
> + * @sd: The subdev for which the state is allocated
> + *
> + * This will allocate a subdev state and store it to
> + * &struct v4l2_subdev->state.
> + *
> + * Must call v4l2_subdev_free_state() when the state is no longer needed.
> + */
> +int v4l2_subdev_alloc_state(struct v4l2_subdev *sd);
> +
> +/**
> + * v4l2_subdev_free_state() - Free the active subdev state for subdevice
> + * @sd: The subdevice
> + *
> + * This will free the subdev's state and set
> + * &struct v4l2_subdev->state to NULL.
> + */
> +void v4l2_subdev_free_state(struct v4l2_subdev *sd);
> +
> +/**
> + * v4l2_subdev_get_active_state() - Return the active subdev state for subdevice
> + * @sd: The subdevice
> + *
> + * Return the active state for the subdevice, or NULL if the subdev does not
> + * support active state.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
> +{
> +	return sd->state;
> +}
> +
>  #endif
> --
> 2.25.1
>