diff mbox series

[v8,11/38] media: v4l: subdev: Add a function to lock two sub-device states, use it

Message ID 20240313072516.241106-12-sakari.ailus@linux.intel.com (mailing list archive)
State Accepted
Headers
Series Generic line based metadata support, internal pads |

Commit Message

Sakari Ailus March 13, 2024, 7:24 a.m. UTC 
  Add two new functions, v4l2_subdev_lock_states() and
v4l2_subdev_unclock_states(), to acquire and release the state of two
sub-devices. They differ from calling v4l2_subdev_{un,}lock_state() so
that if the two states share the same lock, the lock is acquired only
once.

Also use the new functions in v4l2_subdev_link_validate().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 12 +++-----
 include/media/v4l2-subdev.h           | 40 +++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 8 deletions(-)

Comments

Julien Massot March 15, 2024, 3:42 p.m. UTC | #1 
On 3/13/24 08:24, Sakari Ailus wrote:
> Add two new functions, v4l2_subdev_lock_states() and
> v4l2_subdev_unclock_states(), to acquire and release the state of two
> sub-devices. They differ from calling v4l2_subdev_{un,}lock_state() so
> that if the two states share the same lock, the lock is acquired only
> once.
> 
> Also use the new functions in v4l2_subdev_link_validate().
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Julien Massot <julien.massot@collabora.com>
> ---
>   drivers/media/v4l2-core/v4l2-subdev.c | 12 +++-----
>   include/media/v4l2-subdev.h           | 40 +++++++++++++++++++++++++++
>   2 files changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4c6198c48dd6..6c34df19ea2f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1400,17 +1400,13 @@ int v4l2_subdev_link_validate(struct media_link *link)
>   
>   	states_locked = sink_state && source_state;
>   
> -	if (states_locked) {
> -		v4l2_subdev_lock_state(sink_state);
> -		v4l2_subdev_lock_state(source_state);
> -	}
> +	if (states_locked)
> +		v4l2_subdev_lock_states(sink_state, source_state);
>   
>   	ret = v4l2_subdev_link_validate_locked(link, states_locked);
>   
> -	if (states_locked) {
> -		v4l2_subdev_unlock_state(sink_state);
> -		v4l2_subdev_unlock_state(source_state);
> -	}
> +	if (states_locked)
> +		v4l2_subdev_unlock_states(sink_state, source_state);
>   
>   	return ret;
>   }
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a9e6b8146279..9cce48365975 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1724,6 +1724,46 @@ static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
>   	mutex_unlock(state->lock);
>   }
>   
> +/**
> + * v4l2_subdev_lock_states - Lock two sub-device states
> + * @state1: One subdevice state
> + * @state2: The other subdevice state
> + *
> + * Locks the state of two sub-devices.
> + *
> + * The states must be unlocked with v4l2_subdev_unlock_states() after use.
> + *
> + * This differs from calling v4l2_subdev_lock_state() on both states so that if
> + * the states share the same lock, the lock is acquired only once (so no
> + * deadlock occurs). The caller is responsible for ensuring the locks will
> + * always be acquired in the same order.
> + */
> +static inline void v4l2_subdev_lock_states(struct v4l2_subdev_state *state1,
> +					   struct v4l2_subdev_state *state2)
> +{
> +	mutex_lock(state1->lock);
> +	if (state1->lock != state2->lock)
> +		mutex_lock(state2->lock);
> +}
> +
> +/**
> + * v4l2_subdev_unlock_states() - Unlock two sub-device states
> + * @state1: One subdevice state
> + * @state2: The other subdevice state
> + *
> + * Unlocks the state of two sub-devices.
> + *
> + * This differs from calling v4l2_subdev_unlock_state() on both states so that
> + * if the states share the same lock, the lock is released only once.
> + */
> +static inline void v4l2_subdev_unlock_states(struct v4l2_subdev_state *state1,
> +					     struct v4l2_subdev_state *state2)
> +{
> +	mutex_unlock(state1->lock);
> +	if (state1->lock != state2->lock)
> +		mutex_unlock(state2->lock);
> +}
> +
>   /**
>    * v4l2_subdev_get_unlocked_active_state() - Checks that the active subdev state
>    *					     is unlocked and returns it
Laurent Pinchart March 20, 2024, 12:36 a.m. UTC | #2 
Hi Sakari,

Thank you for the patch.

On Wed, Mar 13, 2024 at 09:24:49AM +0200, Sakari Ailus wrote:
> Add two new functions, v4l2_subdev_lock_states() and
> v4l2_subdev_unclock_states(), to acquire and release the state of two
> sub-devices. They differ from calling v4l2_subdev_{un,}lock_state() so
> that if the two states share the same lock, the lock is acquired only
> once.

A bit of a hack, but I think we can live with it for now. A better long
term implementation will be to acquire all state locks from
media_pipeline_start() before doing any validation, and always using
different locks for different subdevs.

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> Also use the new functions in v4l2_subdev_link_validate().
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 12 +++-----
>  include/media/v4l2-subdev.h           | 40 +++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4c6198c48dd6..6c34df19ea2f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1400,17 +1400,13 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  
>  	states_locked = sink_state && source_state;
>  
> -	if (states_locked) {
> -		v4l2_subdev_lock_state(sink_state);
> -		v4l2_subdev_lock_state(source_state);
> -	}
> +	if (states_locked)
> +		v4l2_subdev_lock_states(sink_state, source_state);
>  
>  	ret = v4l2_subdev_link_validate_locked(link, states_locked);
>  
> -	if (states_locked) {
> -		v4l2_subdev_unlock_state(sink_state);
> -		v4l2_subdev_unlock_state(source_state);
> -	}
> +	if (states_locked)
> +		v4l2_subdev_unlock_states(sink_state, source_state);
>  
>  	return ret;
>  }
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a9e6b8146279..9cce48365975 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1724,6 +1724,46 @@ static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
>  	mutex_unlock(state->lock);
>  }
>  
> +/**
> + * v4l2_subdev_lock_states - Lock two sub-device states
> + * @state1: One subdevice state
> + * @state2: The other subdevice state
> + *
> + * Locks the state of two sub-devices.
> + *
> + * The states must be unlocked with v4l2_subdev_unlock_states() after use.
> + *
> + * This differs from calling v4l2_subdev_lock_state() on both states so that if
> + * the states share the same lock, the lock is acquired only once (so no
> + * deadlock occurs). The caller is responsible for ensuring the locks will
> + * always be acquired in the same order.
> + */
> +static inline void v4l2_subdev_lock_states(struct v4l2_subdev_state *state1,
> +					   struct v4l2_subdev_state *state2)
> +{
> +	mutex_lock(state1->lock);
> +	if (state1->lock != state2->lock)
> +		mutex_lock(state2->lock);
> +}
> +
> +/**
> + * v4l2_subdev_unlock_states() - Unlock two sub-device states
> + * @state1: One subdevice state
> + * @state2: The other subdevice state
> + *
> + * Unlocks the state of two sub-devices.
> + *
> + * This differs from calling v4l2_subdev_unlock_state() on both states so that
> + * if the states share the same lock, the lock is released only once.
> + */
> +static inline void v4l2_subdev_unlock_states(struct v4l2_subdev_state *state1,
> +					     struct v4l2_subdev_state *state2)
> +{
> +	mutex_unlock(state1->lock);
> +	if (state1->lock != state2->lock)
> +		mutex_unlock(state2->lock);
> +}
> +
>  /**
>   * v4l2_subdev_get_unlocked_active_state() - Checks that the active subdev state
>   *					     is unlocked and returns it
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 4c6198c48dd6..6c34df19ea2f 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1400,17 +1400,13 @@  int v4l2_subdev_link_validate(struct media_link *link)
 
 	states_locked = sink_state && source_state;
 
-	if (states_locked) {
-		v4l2_subdev_lock_state(sink_state);
-		v4l2_subdev_lock_state(source_state);
-	}
+	if (states_locked)
+		v4l2_subdev_lock_states(sink_state, source_state);
 
 	ret = v4l2_subdev_link_validate_locked(link, states_locked);
 
-	if (states_locked) {
-		v4l2_subdev_unlock_state(sink_state);
-		v4l2_subdev_unlock_state(source_state);
-	}
+	if (states_locked)
+		v4l2_subdev_unlock_states(sink_state, source_state);
 
 	return ret;
 }
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index a9e6b8146279..9cce48365975 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1724,6 +1724,46 @@  static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
 	mutex_unlock(state->lock);
 }
 
+/**
+ * v4l2_subdev_lock_states - Lock two sub-device states
+ * @state1: One subdevice state
+ * @state2: The other subdevice state
+ *
+ * Locks the state of two sub-devices.
+ *
+ * The states must be unlocked with v4l2_subdev_unlock_states() after use.
+ *
+ * This differs from calling v4l2_subdev_lock_state() on both states so that if
+ * the states share the same lock, the lock is acquired only once (so no
+ * deadlock occurs). The caller is responsible for ensuring the locks will
+ * always be acquired in the same order.
+ */
+static inline void v4l2_subdev_lock_states(struct v4l2_subdev_state *state1,
+					   struct v4l2_subdev_state *state2)
+{
+	mutex_lock(state1->lock);
+	if (state1->lock != state2->lock)
+		mutex_lock(state2->lock);
+}
+
+/**
+ * v4l2_subdev_unlock_states() - Unlock two sub-device states
+ * @state1: One subdevice state
+ * @state2: The other subdevice state
+ *
+ * Unlocks the state of two sub-devices.
+ *
+ * This differs from calling v4l2_subdev_unlock_state() on both states so that
+ * if the states share the same lock, the lock is released only once.
+ */
+static inline void v4l2_subdev_unlock_states(struct v4l2_subdev_state *state1,
+					     struct v4l2_subdev_state *state2)
+{
+	mutex_unlock(state1->lock);
+	if (state1->lock != state2->lock)
+		mutex_unlock(state2->lock);
+}
+
 /**
  * v4l2_subdev_get_unlocked_active_state() - Checks that the active subdev state
  *					     is unlocked and returns it