From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: linux-media@vger.kernel.org, sakari.ailus@linux.intel.com,
Jacopo Mondi <jacopo+renesas@jmondi.org>,
niklas.soderlund+renesas@ragnatech.se,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Pratyush Yadav <p.yadav@ti.com>
Subject: Re: [PATCH v10 04/38] media: subdev: add subdev state locking
Date: Thu, 16 Dec 2021 15:39:57 +0200 [thread overview]
Message-ID: <YbtBrZ+OBxJVI1rc@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20211130141536.891878-5-tomi.valkeinen@ideasonboard.com>
Hi Tomi,
Thank you for the patch.
On Tue, Nov 30, 2021 at 04:15:02PM +0200, Tomi Valkeinen wrote:
> The V4L2 subdevs have managed without centralized locking for the state
> (previously pad_config), as the TRY state is supposedly safe (although I
> believe two TRY ioctls for the same fd would race), and the ACTIVE
> state, and its locking, is managed by the drivers internally.
>
> We now have ACTIVE state in a centralized position, and need locking.
> Strictly speaking the locking is only needed for new drivers that use
> the new state, as the current drivers continue behaving as they used to.
>
> Add a mutex to the struct v4l2_subdev_state, along with a few helper
> functions for locking/unlocking.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 3 +-
> drivers/media/platform/vsp1/vsp1_entity.c | 4 +-
> drivers/media/v4l2-core/v4l2-subdev.c | 38 +++++++++++++---
> drivers/staging/media/tegra-video/vi.c | 4 +-
> include/media/v4l2-subdev.h | 50 ++++++++++++++++++++-
> 5 files changed, 89 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index ba1d16ab1651..e6bd94d63e4f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -244,6 +244,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
> {
> struct v4l2_subdev *sd = vin_to_source(vin);
> struct v4l2_subdev_state *sd_state;
> + static struct lock_class_key key;
> struct v4l2_subdev_format format = {
> .which = which,
> .pad = vin->parallel.source_pad,
> @@ -252,7 +253,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
> u32 width, height;
> int ret;
>
> - sd_state = __v4l2_subdev_state_alloc(sd);
I would add here a
/*
* FIXME: Drop this call, drivers are not supposed to use
* __v4l2_subdev_state_alloc().
*/
Same below.
> + sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
> if (IS_ERR(sd_state))
> return PTR_ERR(sd_state);
>
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
> index 869cadc1468d..e607c3ae2520 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
> @@ -613,6 +613,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
> const char *name, unsigned int num_pads,
> const struct v4l2_subdev_ops *ops, u32 function)
> {
> + static struct lock_class_key key;
> struct v4l2_subdev *subdev;
> unsigned int i;
> int ret;
> @@ -675,7 +676,8 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
> * Allocate the pad configuration to store formats and selection
> * rectangles.
> */
> - entity->config = __v4l2_subdev_state_alloc(&entity->subdev);
> + entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
> + "vsp1:config->lock", &key);
> if (IS_ERR(entity->config)) {
> media_entity_cleanup(&entity->subdev.entity);
> return PTR_ERR(entity->config);
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 3289875d9ec1..2053fe1cd67d 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -27,8 +27,9 @@
> static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
> {
> struct v4l2_subdev_state *state;
> + static struct lock_class_key key;
>
> - state = __v4l2_subdev_state_alloc(sd);
> + state = __v4l2_subdev_state_alloc(sd, "fh->state->lock", &key);
> if (IS_ERR(state))
> return PTR_ERR(state);
>
> @@ -923,7 +924,9 @@ int v4l2_subdev_link_validate(struct media_link *link)
> }
> EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
>
> -struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
> +struct v4l2_subdev_state *
> +__v4l2_subdev_state_alloc(struct v4l2_subdev *sd, const char *lock_name,
> + struct lock_class_key *lock_key)
> {
> struct v4l2_subdev_state *state;
> int ret;
> @@ -932,6 +935,8 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
> if (!state)
> return ERR_PTR(-ENOMEM);
>
> + __mutex_init(&state->lock, lock_name, lock_key);
> +
> if (sd->entity.num_pads) {
> state->pads = kvmalloc_array(sd->entity.num_pads,
> sizeof(*state->pads),
> @@ -963,6 +968,8 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
> if (!state)
> return;
>
> + mutex_destroy(&state->lock);
> +
> kvfree(state->pads);
> kfree(state);
> }
> @@ -997,11 +1004,12 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
> }
> EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
>
> -int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
> +int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> + struct lock_class_key *key)
> {
> struct v4l2_subdev_state *state;
>
> - state = __v4l2_subdev_state_alloc(sd);
> + state = __v4l2_subdev_state_alloc(sd, name, key);
> if (IS_ERR(state))
> return PTR_ERR(state);
>
> @@ -1009,7 +1017,7 @@ int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
>
> return 0;
> }
> -EXPORT_SYMBOL_GPL(v4l2_subdev_init_finalize);
> +EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize);
>
> void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
> {
> @@ -1017,3 +1025,23 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
> sd->active_state = NULL;
> }
> EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
> +
> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd)
> +{
> + mutex_lock(&sd->active_state->lock);
> +
> + return sd->active_state;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_active_state);
> +
> +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
> +{
> + mutex_lock(&state->lock);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_state);
> +
> +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
> +{
> + mutex_unlock(&state->lock);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_unlock_state);
> diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
> index 66b9ce160472..3516735f0cad 100644
> --- a/drivers/staging/media/tegra-video/vi.c
> +++ b/drivers/staging/media/tegra-video/vi.c
> @@ -491,6 +491,7 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
> struct v4l2_pix_format *pix)
> {
> const struct tegra_video_format *fmtinfo;
> + static struct lock_class_key key;
> struct v4l2_subdev *subdev;
> struct v4l2_subdev_format fmt;
> struct v4l2_subdev_state *sd_state;
> @@ -507,7 +508,8 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
> if (!subdev)
> return -ENODEV;
>
> - sd_state = __v4l2_subdev_state_alloc(subdev);
> + sd_state = __v4l2_subdev_state_alloc(subdev, "tegra:state->lock",
> + &key);
> if (IS_ERR(sd_state))
> return PTR_ERR(sd_state);
> /*
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 87c3126b2754..1810dde9c7fc 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -658,6 +658,7 @@ struct v4l2_subdev_pad_config {
> /**
> * struct v4l2_subdev_state - Used for storing subdev state information.
> *
> + * @lock: mutex for the state
> * @pads: &struct v4l2_subdev_pad_config array
> *
> * This structure only needs to be passed to the pad op if the 'which' field
> @@ -665,6 +666,8 @@ struct v4l2_subdev_pad_config {
> * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL.
> */
> struct v4l2_subdev_state {
> + /* lock for the struct v4l2_subdev_state fields */
> + struct mutex lock;
I'm looking forward to the first AB-BA deadlock when subdevs will call
each other's operations in different orders :-) I'm pretty sure we'll
need a ww-mutex in v4l2_device to solve that. It's for later though.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> struct v4l2_subdev_pad_config *pads;
> };
>
> @@ -1156,10 +1159,14 @@ int v4l2_subdev_link_validate(struct media_link *link);
> * __v4l2_subdev_state_alloc - allocate v4l2_subdev_state
> *
> * @sd: pointer to &struct v4l2_subdev for which the state is being allocated.
> + * @lock_name: name of the state lock
> + * @key: lock_class_key for the lock
> *
> * Must call __v4l2_subdev_state_free() when state is no longer needed.
> */
> -struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd);
> +struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd,
> + const char *lock_name,
> + struct lock_class_key *key);
>
> /**
> * __v4l2_subdev_state_free - free a v4l2_subdev_state
> @@ -1248,7 +1255,16 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
> *
> * Must call v4l2_subdev_cleanup() when the subdev is being removed.
> */
> -int v4l2_subdev_init_finalize(struct v4l2_subdev *sd);
> +#define v4l2_subdev_init_finalize(sd) \
> + ({ \
> + static struct lock_class_key __key; \
> + const char *name = KBUILD_BASENAME \
> + ":" __stringify(__LINE__) ":subdev->state->lock"; \
> + __v4l2_subdev_init_finalize(sd, name, &__key); \
> + })
> +
> +int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> + struct lock_class_key *key);
>
> /**
> * v4l2_subdev_cleanup() - Release the resources needed by the subdevice
> @@ -1271,4 +1287,34 @@ v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
> return sd->active_state;
> }
>
> +/**
> + * v4l2_subdev_lock_active_state() - Lock and return the active subdev state for
> + * subdevice
> + * @sd: The subdevice
> + *
> + * Return the locked active state for the subdevice, or NULL if the subdev
> + * does not support active state.
> + *
> + * Must be unlocked with v4l2_subdev_unlock_state() after use.
> + */
> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd);
> +
> +/**
> + * v4l2_subdev_lock_state() - Lock the subdev state
> + * @state: The subdevice state
> + *
> + * Lock the given subdev state.
> + *
> + * Must be unlocked with v4l2_subdev_unlock_state() after use.
> + */
> +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
> +
> +/**
> + * v4l2_subdev_unlock_state() - Unlock the subdev state
> + * @state: The subdevice state
> + *
> + * Unlock the given subdev state.
> + */
> +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
> +
> #endif
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2021-12-16 13:40 UTC|newest]
Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-30 14:14 [PATCH v10 00/38] v4l: subdev internal routing and streams Tomi Valkeinen
2021-11-30 14:14 ` [PATCH v10 01/38] media: subdev: rename subdev-state alloc & free Tomi Valkeinen
2021-12-13 21:28 ` Jacopo Mondi
2021-11-30 14:15 ` [PATCH v10 02/38] media: subdev: add active state to struct v4l2_subdev Tomi Valkeinen
2021-12-13 21:30 ` Jacopo Mondi
2021-12-15 8:06 ` Tomi Valkeinen
2021-12-15 9:38 ` Jacopo Mondi
2021-12-15 15:35 ` Tomi Valkeinen
2021-12-15 16:25 ` Jacopo Mondi
2021-12-16 14:14 ` Laurent Pinchart
2021-12-16 14:41 ` Laurent Pinchart
2021-12-17 8:00 ` Tomi Valkeinen
2021-12-17 10:12 ` Hans Verkuil
2021-11-30 14:15 ` [PATCH v10 03/38] media: subdev: pass also the active state to subdevs from ioctls Tomi Valkeinen
2021-12-14 7:13 ` Jacopo Mondi
2021-12-15 8:13 ` Tomi Valkeinen
2021-12-16 13:47 ` Laurent Pinchart
2021-11-30 14:15 ` [PATCH v10 04/38] media: subdev: add subdev state locking Tomi Valkeinen
2021-12-14 7:42 ` Jacopo Mondi
2021-12-15 8:18 ` Tomi Valkeinen
2021-12-16 13:39 ` Laurent Pinchart [this message]
2021-12-17 11:23 ` Hans Verkuil
2021-11-30 14:15 ` [PATCH v10 05/38] media: subdev: Add v4l2_subdev_lock_and_return_state() Tomi Valkeinen
2021-12-16 14:34 ` Laurent Pinchart
2021-12-17 17:43 ` Sakari Ailus
2021-12-17 17:54 ` Laurent Pinchart
2021-12-20 11:48 ` Sakari Ailus
2021-12-17 11:48 ` Hans Verkuil
2021-12-17 17:55 ` Laurent Pinchart
2021-11-30 14:15 ` [PATCH v10 06/38] media: Documentation: add documentation about subdev state Tomi Valkeinen
2021-12-14 8:31 ` Jacopo Mondi
2021-12-16 14:50 ` Laurent Pinchart
2021-12-17 9:22 ` Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 07/38] media: entity: Use pad as a starting point for graph walk Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 08/38] media: entity: Use pads instead of entities in the media graph walk stack Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 09/38] media: entity: Walk the graph based on pads Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 10/38] media: mc: Start walk from a specific pad in use count calculation Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 11/38] media: entity: Add iterator helper for entity pads Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 12/38] media: entity: Move the pipeline from entity to pads Tomi Valkeinen
2022-01-07 19:52 ` Laurent Pinchart
2022-01-09 0:58 ` Laurent Pinchart
2021-11-30 14:15 ` [PATCH v10 13/38] media: entity: Use pad as the starting point for a pipeline Tomi Valkeinen
2022-01-11 23:39 ` Laurent Pinchart
2021-11-30 14:15 ` [PATCH v10 14/38] media: entity: Add has_route entity operation Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 15/38] media: entity: Add media_entity_has_route() function Tomi Valkeinen
2022-01-08 23:31 ` Laurent Pinchart
2022-01-10 7:18 ` Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 16/38] media: entity: Use routing information during graph traversal Tomi Valkeinen
2022-01-17 23:13 ` Laurent Pinchart
2021-11-30 14:15 ` [PATCH v10 17/38] media: entity: Skip link validation for pads to which there is no route Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 18/38] media: entity: Add an iterator helper for connected pads Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 19/38] media: entity: Add only connected pads to the pipeline Tomi Valkeinen
2022-01-07 19:57 ` Laurent Pinchart
2021-11-30 14:15 ` [PATCH v10 20/38] media: entity: Add debug information in graph walk route check Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 21/38] media: Add bus type to frame descriptors Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 22/38] media: Add CSI-2 bus configuration " Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 23/38] media: Add stream to frame descriptor Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 24/38] media: subdev: increase V4L2_FRAME_DESC_ENTRY_MAX to 8 Tomi Valkeinen
2021-12-14 8:35 ` Jacopo Mondi
2021-11-30 14:15 ` [PATCH v10 25/38] media: add V4L2_SUBDEV_FL_MULTIPLEXED Tomi Valkeinen
2021-12-14 8:41 ` Jacopo Mondi
2021-12-15 8:23 ` Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 26/38] media: add V4L2_SUBDEV_CAP_MPLEXED Tomi Valkeinen
2021-12-14 8:39 ` Jacopo Mondi
2021-12-21 18:07 ` Laurent Pinchart
2021-11-30 14:15 ` [PATCH v10 27/38] media: Documentation: Add GS_ROUTING documentation Tomi Valkeinen
2022-02-03 10:55 ` Kieran Bingham
2022-02-03 10:58 ` Kieran Bingham
2021-11-30 14:15 ` [PATCH v10 28/38] media: subdev: Add [GS]_ROUTING subdev ioctls and operations Tomi Valkeinen
2021-12-17 3:31 ` Laurent Pinchart
2021-12-21 17:37 ` Dave Stevenson
2021-11-30 14:15 ` [PATCH v10 29/38] media: subdev: add v4l2_subdev_has_route() Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 30/38] media: subdev: add v4l2_subdev_set_routing helper() Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 31/38] media: Documentation: add multiplexed streams documentation Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 32/38] media: subdev: add stream based configuration Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 33/38] media: subdev: use streams in v4l2_subdev_link_validate() Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 34/38] media: subdev: add "opposite" stream helper funcs Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 35/38] media: subdev: add v4l2_subdev_get_fmt() helper function Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 36/38] media: subdev: add v4l2_subdev_set_routing_with_fmt() helper Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 37/38] media: subdev: add v4l2_subdev_routing_validate_1_to_1 helper Tomi Valkeinen
2021-11-30 14:15 ` [PATCH v10 38/38] media: subdev: Add for_each_active_route() macro Tomi Valkeinen
2021-12-01 6:26 ` [PATCH v10 00/38] v4l: subdev internal routing and streams Tomi Valkeinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YbtBrZ+OBxJVI1rc@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=jacopo+renesas@jmondi.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=p.yadav@ti.com \
--cc=sakari.ailus@linux.intel.com \
--cc=tomi.valkeinen@ideasonboard.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).