Re: [PATCH v2 16/27] media: v4l2-subdev: Add subdev state accessor helpers

[John Cox]

> The v4l2-subdev.c file offers three helpers to access the subdevice
> active state from a v4l2_subdev pointer:
>
> - v4l2_subdev_get_unlocked_active_state(sd)
> - v4l2_subdev_get_locked_active_state(sd)
> - v4l2_subdev_lock_and_get_active_state(sd)
>
> With the introduction of struct v4l2_subdev_context which contains a
> subdev_state as well, the actual "active" state is stored in three
> possible places:
> - A context associated with a v4l2_subdev_fh for context aware drivers
>   operated by context aware userspace
> - The default subdevice context for context aware drivers operated by
>   non-context aware userspace
> - The subdevice active state for non-context aware drivers
>
> Provide helpers similar in spirit the existing ones but accept as
> argument either a subdevice the context itself and retrieve the active
> state from the correct place.
>
> Helpers will be used in following patches for link validation.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 101 +++++++++++++++++++++++++++++++++
>  include/media/v4l2-subdev.h           | 104 +++++++++++++++++++---------------
>  2 files changed, 159 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 438f51980e5ac0f092ba6b0a979a376133968ddf..7372f61127c871cec44a3d1900e2b8bef34632b9 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -20,6 +20,7 @@
>  #include <linux/version.h>
>  #include <linux/videodev2.h>
>
> +#include <media/media-device.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> @@ -189,6 +190,106 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad)
>         return 0;
>  }
>
> +/* subdev state accessor helpers */
> +
> +/*
> + * Access the state from the subdevice.
> + *
> + * If the driver is context-aware use the state stored in the default context
> + * otherwise use the active state stored in the subdevice.
> + */
> +
> +struct v4l2_subdev_state *
> +v4l2_subdev_get_unlocked_active_state_from_sd(struct v4l2_subdev *sd)
> +{
> +       if (!sd)
> +               return NULL;
> +
> +       if (sd->default_context) {
> +               lockdep_assert_not_held(sd->default_context->state->lock);
> +
> +               return sd->default_context->state;
> +       }
> +
> +       if (sd->active_state)
> +               lockdep_assert_not_held(sd->active_state->lock);
> +       return sd->active_state;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_unlocked_active_state_from_sd);
> +
> +struct v4l2_subdev_state *
> +v4l2_subdev_get_locked_active_state_from_sd(struct v4l2_subdev *sd)
> +{
> +       if (!sd)
> +               return NULL;
> +
> +       if (sd->default_context) {
> +               lockdep_assert_held(sd->default_context->state->lock);
> +
> +               return sd->default_context->state;
> +       }
> +
> +       if (sd->active_state)
> +               lockdep_assert_held(sd->active_state->lock);
> +       return sd->active_state;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_locked_active_state_from_sd);
> +
> +struct v4l2_subdev_state *
> +v4l2_subdev_lock_and_get_active_state_from_sd(struct v4l2_subdev *sd)
> +{
> +       if (!sd)
> +               return NULL;
> +
> +       if (sd->default_context) {
> +               v4l2_subdev_lock_state(sd->default_context->state);
> +
> +               return sd->default_context->state;
> +       }
> +
> +       if (sd->active_state)
> +               v4l2_subdev_lock_state(sd->active_state);
> +       return sd->active_state;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_and_get_active_state_from_sd);
> +
> +/* Access the subdevice state from a subdvice context. */
> +struct v4l2_subdev_state *
> +v4l2_subdev_get_unlocked_active_state_from_ctx(struct v4l2_subdev_context *ctx)
> +{
> +       if (!ctx)
> +               return NULL;
> +
> +       if (ctx->state)
> +               lockdep_assert_not_held(ctx->state->lock);
> +       return ctx->state;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_unlocked_active_state_from_ctx);
> +
> +struct v4l2_subdev_state *
> +v4l2_subdev_get_locked_active_state_from_ctx(struct v4l2_subdev_context *ctx)
> +{
> +       if (!ctx)
> +               return NULL;
> +
> +       if (ctx->state)
> +               lockdep_assert_held(ctx->state->lock);
> +       return ctx->state;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_locked_active_state_from_ctx);
> +
> +struct v4l2_subdev_state *
> +v4l2_subdev_lock_and_get_active_state_from_ctx(struct v4l2_subdev_context *ctx)
> +{
> +       if (!ctx)
> +               return NULL;
> +
> +       if (ctx->state)
> +               v4l2_subdev_lock_state(ctx->state);
> +       return ctx->state;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_and_get_active_state_from_ctx);
> +
>  static int check_state(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
>                        u32 which, u32 pad, u32 stream)
>  {
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1fa42a9f322be0be44fc9308744f4f4ae0cf1606..8087c0ae3bc0a0a95512b4b0ff5257522a104ca0 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -29,16 +29,17 @@
>
>  #define        V4L2_DEVICE_NOTIFY_EVENT                _IOW('v', 2, struct v4l2_event)
>
> -struct v4l2_device;
> +struct led_classdev;
> +struct media_device_context;
> +struct tuner_setup;
>  struct v4l2_ctrl_handler;
> +struct v4l2_device;
>  struct v4l2_event;
>  struct v4l2_event_subscription;
>  struct v4l2_fh;
> +struct v4l2_mbus_frame_desc;
>  struct v4l2_subdev;
>  struct v4l2_subdev_fh;
> -struct tuner_setup;
> -struct v4l2_mbus_frame_desc;
> -struct led_classdev;
>
>  /**
>   * struct v4l2_decode_vbi_line - used to decode_vbi_line
> @@ -1968,64 +1969,75 @@ static inline void v4l2_subdev_unlock_states(struct v4l2_subdev_state *state1,
>                 mutex_unlock(state2->lock);
>  }
>
> +struct v4l2_subdev_state *
> +v4l2_subdev_get_unlocked_active_state_from_sd(struct v4l2_subdev *sd);
> +struct v4l2_subdev_state *
> +v4l2_subdev_get_locked_active_state_from_sd(struct v4l2_subdev *sd);
> +struct v4l2_subdev_state *
> +v4l2_subdev_lock_and_get_active_state_from_sd(struct v4l2_subdev *sd);
> +
> +struct v4l2_subdev_state *
> +v4l2_subdev_get_unlocked_active_state_from_ctx(struct v4l2_subdev_context *ctx);
> +struct v4l2_subdev_state *
> +v4l2_subdev_get_locked_active_state_from_ctx(struct v4l2_subdev_context *ctx);
> +struct v4l2_subdev_state *
> +v4l2_subdev_lock_and_get_active_state_from_ctx(struct v4l2_subdev_context *ctx);
> +
>  /**
> - * v4l2_subdev_get_unlocked_active_state() - Checks that the active subdev state
> - *                                          is unlocked and returns it
> - * @sd: The subdevice
> + * v4l2_subdev_get_unlocked_active_state() - Checks that the subdev state is
> + *                                          unlocked and returns it
> + * @sdctx: The subdevice, or the subdevice context
>   *
> - * Returns the active state for the subdevice, or NULL if the subdev does not
> - * support active state. If the state is not NULL, calls
> - * lockdep_assert_not_held() to issue a warning if the state is locked.
> + * Returns the subdevice state, or NULL if it is not valid. If the state is
> + * not NULL, calls lockdep_assert_not_held() to issue a warning if the state
> + * is locked.
>   *
> - * This function is to be used e.g. when getting the active state for the sole
> - * purpose of passing it forward, without accessing the state fields.
> + * This function is to be used e.g. when getting the state for the sole purpose
> + * of passing it forward, without accessing the state fields.
>   */
> -static inline struct v4l2_subdev_state *
> -v4l2_subdev_get_unlocked_active_state(struct v4l2_subdev *sd)
> -{
> -       if (sd->active_state)
> -               lockdep_assert_not_held(sd->active_state->lock);
> -       return sd->active_state;
> -}
> +#define v4l2_subdev_get_unlocked_active_state(sdctx)                           \
> +       _Generic((sdctx),                                                       \
> +               struct v4l2_subdev *:                                           \
> +                       v4l2_subdev_get_unlocked_active_state_from_sd,          \
> +               struct v4l2_subdev_context *:                                   \
> +                       v4l2_subdev_get_unlocked_active_state_from_ctx)         \
> +       (sdctx)
>
>  /**
> - * v4l2_subdev_get_locked_active_state() - Checks that the active subdev state
> - *                                        is locked and returns it
> - *
> - * @sd: The subdevice
> + * v4l2_subdev_get_locked_active_state() - Checks that the subdev state is
> + *                                        locked and returns it
> + * @sdctx: The subdevice, or the subdevice context
>   *
> - * Returns the active state for the subdevice, or NULL if the subdev does not
> - * support active state. If the state is not NULL, calls lockdep_assert_held()
> - * to issue a warning if the state is not locked.
> + * Returns the subdevice state, or NULL is not valid. If the state is not NULL,
> + * calls lockdep_assert_held() to issue a warning if the state is not locked.
>   *
> - * This function is to be used when the caller knows that the active state is
> + * This function is to be used when the caller knows that the context state is
>   * already locked.
>   */
> -static inline struct v4l2_subdev_state *
> -v4l2_subdev_get_locked_active_state(struct v4l2_subdev *sd)
> -{
> -       if (sd->active_state)
> -               lockdep_assert_held(sd->active_state->lock);
> -       return sd->active_state;
> -}
> +#define v4l2_subdev_get_locked_active_state(sdctx)                             \
> +       _Generic((sdctx),                                                       \
> +               struct v4l2_subdev *:                                           \
> +                       v4l2_subdev_get_locked_active_state_from_sd,            \
> +               struct v4l2_subdev_context *:                                   \
> +                       v4l2_subdev_get_locked_active_state_from_ctx)           \
> +       (sdctx)
>
>  /**
> - * v4l2_subdev_lock_and_get_active_state() - Locks and returns the active subdev
> - *                                          state for the subdevice
> - * @sd: The subdevice
> + * v4l2_subdev_lock_and_get_active_state_from_ctx() - Locks and returns the
> + *                                                   subdevice state
> + * @sdctx: The subdevice, or the subdevice context
>   *
> - * Returns the locked active state for the subdevice, or NULL if the subdev
> - * does not support active state.
> + * Returns the locked subdevice state, or NULL if it is not valid.
>   *
>   * The state must be unlocked with v4l2_subdev_unlock_state() after use.
>   */
> -static inline struct v4l2_subdev_state *
> -v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
> -{
> -       if (sd->active_state)
> -               v4l2_subdev_lock_state(sd->active_state);
> -       return sd->active_state;
> -}
> +#define v4l2_subdev_lock_and_get_active_state(sdctx)                           \
> +       _Generic((sdctx),                                                       \
> +               struct v4l2_subdev *:                                           \v4l2_subdev_lock_and_get_active_state_from_
> +                       v4l2_subdev_lock_and_get_active_state_from_sd,          \
> +               struct v4l2_subdev_context *:                                   \
> +                       v4l2_subdev_get_locked_active_state_from_ctx)           \
Cut and paste error?
This should be v4l2_subdev_lock_and_get_active_state_from_ctx?

Regards

John Cox

> +       (sdctx)
>
>  /**
>   * v4l2_subdev_init - initializes the sub-device struct
>
> --
> 2.49.0
>
>