| Message ID | 20240313072516.241106-12-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generic line based metadata support, internal pads | expand |
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
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 --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
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(-)