| Message ID | 1479304688-24010-3-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 16, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote: > With checks! This will allow safe access to the current state, > while ensuring that the correct locks are held. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > include/drm/drm_atomic.h | 66 ++++++++++++++++++++++++++++++++++++++++++ > include/drm/drm_modeset_lock.h | 21 ++++++++++++++ > 2 files changed, 87 insertions(+) > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index e527684dd394..462408a2d1b8 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, > return plane->state; > } > > + > +/** > + * drm_atomic_get_current_plane_state - get current plane state > + * @plane: plane to grab > + * > + * This function returns the current plane state for the given plane, > + * with extra locking checks to make sure that the plane state can be > + * retrieved safely. > + * > + * Returns: > + * > + * Pointer to the current plane state. > + */ > +static inline struct drm_plane_state * > +drm_atomic_get_current_plane_state(struct drm_plane *plane) > +{ > + struct drm_plane_state *plane_state = plane->state; > + struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL; > + > + if (crtc) > + drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex); > + else > + drm_modeset_lock_assert_held(&plane->mutex); Hmm. Daniel recently smashed me on the head with a big clue bat to point out that accessing object->state isn't safe unless you hold the object lock. So this thing seems suspicious. What's the use case for this? I guess in this case it might be safe since a parallel update would lock the crtc as well. But it does feel like promoting potentially dangerous behaviour. Also there are no barriers so I'm not sure this would be guaranteed to give us the correct answer anyway. As far as all of these functions go, should they return const*? Presumably you wouldn't want to allow changes to the current state. > + > + return plane_state; > +} > + > +/** > + * drm_atomic_get_current_crtc_state - get current crtc state > + * @crtc: crtc to grab > + * > + * This function returns the current crtc state for the given crtc, > + * with extra locking checks to make sure that the crtc state can be > + * retrieved safely. > + * > + * Returns: > + * > + * Pointer to the current crtc state. > + */ > +static inline struct drm_crtc_state * > +drm_atomic_get_current_crtc_state(struct drm_crtc *crtc) > +{ > + drm_modeset_lock_assert_held(&crtc->mutex); > + > + return crtc->state; > +} > + > +/** > + * drm_atomic_get_current_connector_state - get current connector state > + * @connector: connector to grab > + * > + * This function returns the current connector state for the given connector, > + * with extra locking checks to make sure that the connector state can be > + * retrieved safely. > + * > + * Returns: > + * > + * Pointer to the current connector state. > + */ > +#define drm_atomic_get_current_connector_state(connector) \ > +({ /* Implemented as macro to avoid including drmP for struct drm_device */ \ > + struct drm_connector *con__ = (connector); \ > + drm_modeset_lock_assert_held(&con__->dev->mode_config.connection_mutex); \ > + con__->state; \ > +}) > + > int __must_check > drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, > struct drm_display_mode *mode); > diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h > index d918ce45ec2c..7635ff18ab99 100644 > --- a/include/drm/drm_modeset_lock.h > +++ b/include/drm/drm_modeset_lock.h > @@ -109,6 +109,27 @@ static inline bool drm_modeset_is_locked(struct drm_modeset_lock *lock) > return ww_mutex_is_locked(&lock->mutex); > } > > +static inline void drm_modeset_lock_assert_held(struct drm_modeset_lock *lock) > +{ > +#ifdef CONFIG_LOCKDEP > + lockdep_assert_held(&lock->mutex.base); > +#else > + WARN_ON(!drm_modeset_is_locked(lock)); > +#endif > +} > + > +static inline void drm_modeset_lock_assert_one_held(struct drm_modeset_lock *lock1, > + struct drm_modeset_lock *lock2) > +{ > +#ifdef CONFIG_LOCKDEP > + WARN_ON(debug_locks && > + !lockdep_is_held(&lock1->mutex.base) && > + !lockdep_is_held(&lock2->mutex.base)); > +#else > + WARN_ON(!drm_modeset_is_locked(lock1) && !drm_modeset_is_locked(lock2)); > +#endif > +} > + > int drm_modeset_lock(struct drm_modeset_lock *lock, > struct drm_modeset_acquire_ctx *ctx); > int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock, > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Nov 16, 2016 at 04:35:45PM +0200, Ville Syrjälä wrote: > On Wed, Nov 16, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote: > > With checks! This will allow safe access to the current state, > > while ensuring that the correct locks are held. > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > include/drm/drm_atomic.h | 66 ++++++++++++++++++++++++++++++++++++++++++ > > include/drm/drm_modeset_lock.h | 21 ++++++++++++++ > > 2 files changed, 87 insertions(+) > > > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > > index e527684dd394..462408a2d1b8 100644 > > --- a/include/drm/drm_atomic.h > > +++ b/include/drm/drm_atomic.h > > @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, > > return plane->state; > > } > > > > + > > +/** > > + * drm_atomic_get_current_plane_state - get current plane state > > + * @plane: plane to grab > > + * > > + * This function returns the current plane state for the given plane, > > + * with extra locking checks to make sure that the plane state can be > > + * retrieved safely. > > + * > > + * Returns: > > + * > > + * Pointer to the current plane state. > > + */ > > +static inline struct drm_plane_state * > > +drm_atomic_get_current_plane_state(struct drm_plane *plane) > > +{ > > + struct drm_plane_state *plane_state = plane->state; > > + struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL; > > + > > + if (crtc) > > + drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex); > > + else > > + drm_modeset_lock_assert_held(&plane->mutex); > > Hmm. Daniel recently smashed me on the head with a big clue bat to point > out that accessing object->state isn't safe unless you hold the object lock. > So this thing seems suspicious. What's the use case for this? > > I guess in this case it might be safe since a parallel update would lock > the crtc as well. But it does feel like promoting potentially dangerous > behaviour. Also there are no barriers so I'm not sure this would be > guaranteed to give us the correct answer anyway. > > As far as all of these functions go, should they return const*? Presumably > you wouldn't want to allow changes to the current state. Yep, need at least a lockdep check for the plane->mutex. And imo also a check that we're indeed in atomic_check per the idea I dropped on the cover letter. And +1 on const * for these, that seems like a very important check. -Daniel > > > + > > + return plane_state; > > +} > > + > > +/** > > + * drm_atomic_get_current_crtc_state - get current crtc state > > + * @crtc: crtc to grab > > + * > > + * This function returns the current crtc state for the given crtc, > > + * with extra locking checks to make sure that the crtc state can be > > + * retrieved safely. > > + * > > + * Returns: > > + * > > + * Pointer to the current crtc state. > > + */ > > +static inline struct drm_crtc_state * > > +drm_atomic_get_current_crtc_state(struct drm_crtc *crtc) > > +{ > > + drm_modeset_lock_assert_held(&crtc->mutex); > > + > > + return crtc->state; > > +} > > + > > +/** > > + * drm_atomic_get_current_connector_state - get current connector state > > + * @connector: connector to grab > > + * > > + * This function returns the current connector state for the given connector, > > + * with extra locking checks to make sure that the connector state can be > > + * retrieved safely. > > + * > > + * Returns: > > + * > > + * Pointer to the current connector state. > > + */ > > +#define drm_atomic_get_current_connector_state(connector) \ > > +({ /* Implemented as macro to avoid including drmP for struct drm_device */ \ > > + struct drm_connector *con__ = (connector); \ > > + drm_modeset_lock_assert_held(&con__->dev->mode_config.connection_mutex); \ > > + con__->state; \ > > +}) > > + > > int __must_check > > drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, > > struct drm_display_mode *mode); > > diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h > > index d918ce45ec2c..7635ff18ab99 100644 > > --- a/include/drm/drm_modeset_lock.h > > +++ b/include/drm/drm_modeset_lock.h > > @@ -109,6 +109,27 @@ static inline bool drm_modeset_is_locked(struct drm_modeset_lock *lock) > > return ww_mutex_is_locked(&lock->mutex); > > } > > > > +static inline void drm_modeset_lock_assert_held(struct drm_modeset_lock *lock) > > +{ > > +#ifdef CONFIG_LOCKDEP > > + lockdep_assert_held(&lock->mutex.base); > > +#else > > + WARN_ON(!drm_modeset_is_locked(lock)); > > +#endif > > +} > > + > > +static inline void drm_modeset_lock_assert_one_held(struct drm_modeset_lock *lock1, > > + struct drm_modeset_lock *lock2) > > +{ > > +#ifdef CONFIG_LOCKDEP > > + WARN_ON(debug_locks && > > + !lockdep_is_held(&lock1->mutex.base) && > > + !lockdep_is_held(&lock2->mutex.base)); > > +#else > > + WARN_ON(!drm_modeset_is_locked(lock1) && !drm_modeset_is_locked(lock2)); > > +#endif > > +} > > + > > int drm_modeset_lock(struct drm_modeset_lock *lock, > > struct drm_modeset_acquire_ctx *ctx); > > int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock, > > -- > > 2.7.4 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 16-11-16 om 16:04 schreef Daniel Vetter: > On Wed, Nov 16, 2016 at 04:35:45PM +0200, Ville Syrjälä wrote: >> On Wed, Nov 16, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote: >>> With checks! This will allow safe access to the current state, >>> while ensuring that the correct locks are held. >>> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> --- >>> include/drm/drm_atomic.h | 66 ++++++++++++++++++++++++++++++++++++++++++ >>> include/drm/drm_modeset_lock.h | 21 ++++++++++++++ >>> 2 files changed, 87 insertions(+) >>> >>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >>> index e527684dd394..462408a2d1b8 100644 >>> --- a/include/drm/drm_atomic.h >>> +++ b/include/drm/drm_atomic.h >>> @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, >>> return plane->state; >>> } >>> >>> + >>> +/** >>> + * drm_atomic_get_current_plane_state - get current plane state >>> + * @plane: plane to grab >>> + * >>> + * This function returns the current plane state for the given plane, >>> + * with extra locking checks to make sure that the plane state can be >>> + * retrieved safely. >>> + * >>> + * Returns: >>> + * >>> + * Pointer to the current plane state. >>> + */ >>> +static inline struct drm_plane_state * >>> +drm_atomic_get_current_plane_state(struct drm_plane *plane) >>> +{ >>> + struct drm_plane_state *plane_state = plane->state; >>> + struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL; >>> + >>> + if (crtc) >>> + drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex); >>> + else >>> + drm_modeset_lock_assert_held(&plane->mutex); >> Hmm. Daniel recently smashed me on the head with a big clue bat to point >> out that accessing object->state isn't safe unless you hold the object lock. >> So this thing seems suspicious. What's the use case for this? >> >> I guess in this case it might be safe since a parallel update would lock >> the crtc as well. But it does feel like promoting potentially dangerous >> behaviour. Also there are no barriers so I'm not sure this would be >> guaranteed to give us the correct answer anyway. >> >> As far as all of these functions go, should they return const*? Presumably >> you wouldn't want to allow changes to the current state. > Yep, need at least a lockdep check for the plane->mutex. And imo also a > check that we're indeed in atomic_check per the idea I dropped on the > cover letter. > > And +1 on const * for these, that seems like a very important check. Well I allowed for crtc lock held because the __ function uses crtc->mutex as safety lock. I thought of const, but some code like i915_page_flip manipulates the current state with the right locks held. Perhaps we should disallow that. :) ~Maarten
On Wed, Nov 16, 2016 at 05:11:56PM +0100, Maarten Lankhorst wrote: > Op 16-11-16 om 16:04 schreef Daniel Vetter: > > On Wed, Nov 16, 2016 at 04:35:45PM +0200, Ville Syrjälä wrote: > >> On Wed, Nov 16, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote: > >>> With checks! This will allow safe access to the current state, > >>> while ensuring that the correct locks are held. > >>> > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> --- > >>> include/drm/drm_atomic.h | 66 ++++++++++++++++++++++++++++++++++++++++++ > >>> include/drm/drm_modeset_lock.h | 21 ++++++++++++++ > >>> 2 files changed, 87 insertions(+) > >>> > >>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > >>> index e527684dd394..462408a2d1b8 100644 > >>> --- a/include/drm/drm_atomic.h > >>> +++ b/include/drm/drm_atomic.h > >>> @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, > >>> return plane->state; > >>> } > >>> > >>> + > >>> +/** > >>> + * drm_atomic_get_current_plane_state - get current plane state > >>> + * @plane: plane to grab > >>> + * > >>> + * This function returns the current plane state for the given plane, > >>> + * with extra locking checks to make sure that the plane state can be > >>> + * retrieved safely. > >>> + * > >>> + * Returns: > >>> + * > >>> + * Pointer to the current plane state. > >>> + */ > >>> +static inline struct drm_plane_state * > >>> +drm_atomic_get_current_plane_state(struct drm_plane *plane) > >>> +{ > >>> + struct drm_plane_state *plane_state = plane->state; > >>> + struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL; > >>> + > >>> + if (crtc) > >>> + drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex); > >>> + else > >>> + drm_modeset_lock_assert_held(&plane->mutex); > >> Hmm. Daniel recently smashed me on the head with a big clue bat to point > >> out that accessing object->state isn't safe unless you hold the object lock. > >> So this thing seems suspicious. What's the use case for this? > >> > >> I guess in this case it might be safe since a parallel update would lock > >> the crtc as well. But it does feel like promoting potentially dangerous > >> behaviour. Also there are no barriers so I'm not sure this would be > >> guaranteed to give us the correct answer anyway. > >> > >> As far as all of these functions go, should they return const*? Presumably > >> you wouldn't want to allow changes to the current state. > > Yep, need at least a lockdep check for the plane->mutex. And imo also a > > check that we're indeed in atomic_check per the idea I dropped on the > > cover letter. > > > > And +1 on const * for these, that seems like a very important check. > Well I allowed for crtc lock held because the __ function uses crtc->mutex as safety lock. > > I thought of const, but some code like i915_page_flip manipulates the current state with the right locks held. > Perhaps we should disallow that. :) Yup, since that code is going to die anyway. And if any driver has a need for this, then they better open code (and have lots of answers for lots of questions really). -Daniel
On Wed, Nov 16, 2016 at 05:11:56PM +0100, Maarten Lankhorst wrote: > Op 16-11-16 om 16:04 schreef Daniel Vetter: > > On Wed, Nov 16, 2016 at 04:35:45PM +0200, Ville Syrjälä wrote: > >> On Wed, Nov 16, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote: > >>> With checks! This will allow safe access to the current state, > >>> while ensuring that the correct locks are held. > >>> > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> --- > >>> include/drm/drm_atomic.h | 66 ++++++++++++++++++++++++++++++++++++++++++ > >>> include/drm/drm_modeset_lock.h | 21 ++++++++++++++ > >>> 2 files changed, 87 insertions(+) > >>> > >>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > >>> index e527684dd394..462408a2d1b8 100644 > >>> --- a/include/drm/drm_atomic.h > >>> +++ b/include/drm/drm_atomic.h > >>> @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, > >>> return plane->state; > >>> } > >>> > >>> + > >>> +/** > >>> + * drm_atomic_get_current_plane_state - get current plane state > >>> + * @plane: plane to grab > >>> + * > >>> + * This function returns the current plane state for the given plane, > >>> + * with extra locking checks to make sure that the plane state can be > >>> + * retrieved safely. > >>> + * > >>> + * Returns: > >>> + * > >>> + * Pointer to the current plane state. > >>> + */ > >>> +static inline struct drm_plane_state * > >>> +drm_atomic_get_current_plane_state(struct drm_plane *plane) > >>> +{ > >>> + struct drm_plane_state *plane_state = plane->state; > >>> + struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL; > >>> + > >>> + if (crtc) > >>> + drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex); > >>> + else > >>> + drm_modeset_lock_assert_held(&plane->mutex); > >> Hmm. Daniel recently smashed me on the head with a big clue bat to point > >> out that accessing object->state isn't safe unless you hold the object lock. > >> So this thing seems suspicious. What's the use case for this? > >> > >> I guess in this case it might be safe since a parallel update would lock > >> the crtc as well. But it does feel like promoting potentially dangerous > >> behaviour. Also there are no barriers so I'm not sure this would be > >> guaranteed to give us the correct answer anyway. > >> > >> As far as all of these functions go, should they return const*? Presumably > >> you wouldn't want to allow changes to the current state. > > Yep, need at least a lockdep check for the plane->mutex. And imo also a > > check that we're indeed in atomic_check per the idea I dropped on the > > cover letter. > > > > And +1 on const * for these, that seems like a very important check. > Well I allowed for crtc lock held because the __ function uses crtc->mutex as safety lock. What is this so called __ function exactly? > > I thought of const, but some code like i915_page_flip manipulates the current state with the right locks held. > Perhaps we should disallow that. :) > > ~Maarten
Op 16-11-16 om 17:32 schreef Ville Syrjälä: > On Wed, Nov 16, 2016 at 05:11:56PM +0100, Maarten Lankhorst wrote: >> Op 16-11-16 om 16:04 schreef Daniel Vetter: >>> On Wed, Nov 16, 2016 at 04:35:45PM +0200, Ville Syrjälä wrote: >>>> On Wed, Nov 16, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote: >>>>> With checks! This will allow safe access to the current state, >>>>> while ensuring that the correct locks are held. >>>>> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>> --- >>>>> include/drm/drm_atomic.h | 66 ++++++++++++++++++++++++++++++++++++++++++ >>>>> include/drm/drm_modeset_lock.h | 21 ++++++++++++++ >>>>> 2 files changed, 87 insertions(+) >>>>> >>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >>>>> index e527684dd394..462408a2d1b8 100644 >>>>> --- a/include/drm/drm_atomic.h >>>>> +++ b/include/drm/drm_atomic.h >>>>> @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, >>>>> return plane->state; >>>>> } >>>>> >>>>> + >>>>> +/** >>>>> + * drm_atomic_get_current_plane_state - get current plane state >>>>> + * @plane: plane to grab >>>>> + * >>>>> + * This function returns the current plane state for the given plane, >>>>> + * with extra locking checks to make sure that the plane state can be >>>>> + * retrieved safely. >>>>> + * >>>>> + * Returns: >>>>> + * >>>>> + * Pointer to the current plane state. >>>>> + */ >>>>> +static inline struct drm_plane_state * >>>>> +drm_atomic_get_current_plane_state(struct drm_plane *plane) >>>>> +{ >>>>> + struct drm_plane_state *plane_state = plane->state; >>>>> + struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL; >>>>> + >>>>> + if (crtc) >>>>> + drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex); >>>>> + else >>>>> + drm_modeset_lock_assert_held(&plane->mutex); >>>> Hmm. Daniel recently smashed me on the head with a big clue bat to point >>>> out that accessing object->state isn't safe unless you hold the object lock. >>>> So this thing seems suspicious. What's the use case for this? >>>> >>>> I guess in this case it might be safe since a parallel update would lock >>>> the crtc as well. But it does feel like promoting potentially dangerous >>>> behaviour. Also there are no barriers so I'm not sure this would be >>>> guaranteed to give us the correct answer anyway. >>>> >>>> As far as all of these functions go, should they return const*? Presumably >>>> you wouldn't want to allow changes to the current state. >>> Yep, need at least a lockdep check for the plane->mutex. And imo also a >>> check that we're indeed in atomic_check per the idea I dropped on the >>> cover letter. >>> >>> And +1 on const * for these, that seems like a very important check. >> Well I allowed for crtc lock held because the __ function uses crtc->mutex as safety lock. > What is this so called __ function exactly? __drm_atomic_get_current_plane_state, which is only used by drm_atomic_crtc_state_for_each_plane_state. It iterates over crtc_state->plane_mask and then gets new_plane_state if available, or plane->state if the plane is not part of the state. This is mostly used for watermark calculations. >> I thought of const, but some code like i915_page_flip manipulates the current state with the right locks held. >> Perhaps we should disallow that. :) >> >> ~Maarten
On Thu, Nov 17, 2016 at 12:58:00PM +0100, Maarten Lankhorst wrote: > Op 16-11-16 om 17:32 schreef Ville Syrjälä: > > On Wed, Nov 16, 2016 at 05:11:56PM +0100, Maarten Lankhorst wrote: > >> Op 16-11-16 om 16:04 schreef Daniel Vetter: > >>> On Wed, Nov 16, 2016 at 04:35:45PM +0200, Ville Syrjälä wrote: > >>>> On Wed, Nov 16, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote: > >>>>> With checks! This will allow safe access to the current state, > >>>>> while ensuring that the correct locks are held. > >>>>> > >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>> --- > >>>>> include/drm/drm_atomic.h | 66 ++++++++++++++++++++++++++++++++++++++++++ > >>>>> include/drm/drm_modeset_lock.h | 21 ++++++++++++++ > >>>>> 2 files changed, 87 insertions(+) > >>>>> > >>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > >>>>> index e527684dd394..462408a2d1b8 100644 > >>>>> --- a/include/drm/drm_atomic.h > >>>>> +++ b/include/drm/drm_atomic.h > >>>>> @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, > >>>>> return plane->state; > >>>>> } > >>>>> > >>>>> + > >>>>> +/** > >>>>> + * drm_atomic_get_current_plane_state - get current plane state > >>>>> + * @plane: plane to grab > >>>>> + * > >>>>> + * This function returns the current plane state for the given plane, > >>>>> + * with extra locking checks to make sure that the plane state can be > >>>>> + * retrieved safely. > >>>>> + * > >>>>> + * Returns: > >>>>> + * > >>>>> + * Pointer to the current plane state. > >>>>> + */ > >>>>> +static inline struct drm_plane_state * > >>>>> +drm_atomic_get_current_plane_state(struct drm_plane *plane) > >>>>> +{ > >>>>> + struct drm_plane_state *plane_state = plane->state; > >>>>> + struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL; > >>>>> + > >>>>> + if (crtc) > >>>>> + drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex); > >>>>> + else > >>>>> + drm_modeset_lock_assert_held(&plane->mutex); > >>>> Hmm. Daniel recently smashed me on the head with a big clue bat to point > >>>> out that accessing object->state isn't safe unless you hold the object lock. > >>>> So this thing seems suspicious. What's the use case for this? > >>>> > >>>> I guess in this case it might be safe since a parallel update would lock > >>>> the crtc as well. But it does feel like promoting potentially dangerous > >>>> behaviour. Also there are no barriers so I'm not sure this would be > >>>> guaranteed to give us the correct answer anyway. > >>>> > >>>> As far as all of these functions go, should they return const*? Presumably > >>>> you wouldn't want to allow changes to the current state. > >>> Yep, need at least a lockdep check for the plane->mutex. And imo also a > >>> check that we're indeed in atomic_check per the idea I dropped on the > >>> cover letter. > >>> > >>> And +1 on const * for these, that seems like a very important check. > >> Well I allowed for crtc lock held because the __ function uses crtc->mutex as safety lock. > > What is this so called __ function exactly? > __drm_atomic_get_current_plane_state, which is only used by drm_atomic_crtc_state_for_each_plane_state. > > It iterates over crtc_state->plane_mask and then gets new_plane_state if available, or plane->state if the plane is not part of the state. > This is mostly used for watermark calculations. Sounds like we should kill that sucker and make things clearer by enforcing the "want to access foo->state? then grab foo->lock first" rule. > >> I thought of const, but some code like i915_page_flip manipulates the current state with the right locks held. > >> Perhaps we should disallow that. :) > >> > >> ~Maarten >
Op 17-11-16 om 13:26 schreef Ville Syrjälä: > On Thu, Nov 17, 2016 at 12:58:00PM +0100, Maarten Lankhorst wrote: >> Op 16-11-16 om 17:32 schreef Ville Syrjälä: >>> On Wed, Nov 16, 2016 at 05:11:56PM +0100, Maarten Lankhorst wrote: >>>> Op 16-11-16 om 16:04 schreef Daniel Vetter: >>>>> On Wed, Nov 16, 2016 at 04:35:45PM +0200, Ville Syrjälä wrote: >>>>>> On Wed, Nov 16, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote: >>>>>>> With checks! This will allow safe access to the current state, >>>>>>> while ensuring that the correct locks are held. >>>>>>> >>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>>> --- >>>>>>> include/drm/drm_atomic.h | 66 ++++++++++++++++++++++++++++++++++++++++++ >>>>>>> include/drm/drm_modeset_lock.h | 21 ++++++++++++++ >>>>>>> 2 files changed, 87 insertions(+) >>>>>>> >>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >>>>>>> index e527684dd394..462408a2d1b8 100644 >>>>>>> --- a/include/drm/drm_atomic.h >>>>>>> +++ b/include/drm/drm_atomic.h >>>>>>> @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, >>>>>>> return plane->state; >>>>>>> } >>>>>>> >>>>>>> + >>>>>>> +/** >>>>>>> + * drm_atomic_get_current_plane_state - get current plane state >>>>>>> + * @plane: plane to grab >>>>>>> + * >>>>>>> + * This function returns the current plane state for the given plane, >>>>>>> + * with extra locking checks to make sure that the plane state can be >>>>>>> + * retrieved safely. >>>>>>> + * >>>>>>> + * Returns: >>>>>>> + * >>>>>>> + * Pointer to the current plane state. >>>>>>> + */ >>>>>>> +static inline struct drm_plane_state * >>>>>>> +drm_atomic_get_current_plane_state(struct drm_plane *plane) >>>>>>> +{ >>>>>>> + struct drm_plane_state *plane_state = plane->state; >>>>>>> + struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL; >>>>>>> + >>>>>>> + if (crtc) >>>>>>> + drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex); >>>>>>> + else >>>>>>> + drm_modeset_lock_assert_held(&plane->mutex); >>>>>> Hmm. Daniel recently smashed me on the head with a big clue bat to point >>>>>> out that accessing object->state isn't safe unless you hold the object lock. >>>>>> So this thing seems suspicious. What's the use case for this? >>>>>> >>>>>> I guess in this case it might be safe since a parallel update would lock >>>>>> the crtc as well. But it does feel like promoting potentially dangerous >>>>>> behaviour. Also there are no barriers so I'm not sure this would be >>>>>> guaranteed to give us the correct answer anyway. >>>>>> >>>>>> As far as all of these functions go, should they return const*? Presumably >>>>>> you wouldn't want to allow changes to the current state. >>>>> Yep, need at least a lockdep check for the plane->mutex. And imo also a >>>>> check that we're indeed in atomic_check per the idea I dropped on the >>>>> cover letter. >>>>> >>>>> And +1 on const * for these, that seems like a very important check. >>>> Well I allowed for crtc lock held because the __ function uses crtc->mutex as safety lock. >>> What is this so called __ function exactly? >> __drm_atomic_get_current_plane_state, which is only used by drm_atomic_crtc_state_for_each_plane_state. >> >> It iterates over crtc_state->plane_mask and then gets new_plane_state if available, or plane->state if the plane is not part of the state. >> This is mostly used for watermark calculations. > Sounds like we should kill that sucker and make things clearer by > enforcing the "want to access foo->state? then grab foo->lock first" > rule. Except adding all planes to cursor updates would result in unintended behavior. And there are already patches to use the sprite plane instead of the cursor plane for skylake, so it's not just theoretical. :) Testcases: flip-vs-cursor-busy-crc-legacy flip-vs-cursor-busy-crc-atomic ~Maarten
On Thu, Nov 17, 2016 at 01:42:13PM +0100, Maarten Lankhorst wrote: > Op 17-11-16 om 13:26 schreef Ville Syrjälä: > > On Thu, Nov 17, 2016 at 12:58:00PM +0100, Maarten Lankhorst wrote: > >> Op 16-11-16 om 17:32 schreef Ville Syrjälä: > >>> On Wed, Nov 16, 2016 at 05:11:56PM +0100, Maarten Lankhorst wrote: > >>>> Op 16-11-16 om 16:04 schreef Daniel Vetter: > >>>>> On Wed, Nov 16, 2016 at 04:35:45PM +0200, Ville Syrjälä wrote: > >>>>>> On Wed, Nov 16, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote: > >>>>>>> With checks! This will allow safe access to the current state, > >>>>>>> while ensuring that the correct locks are held. > >>>>>>> > >>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>>>> --- > >>>>>>> include/drm/drm_atomic.h | 66 ++++++++++++++++++++++++++++++++++++++++++ > >>>>>>> include/drm/drm_modeset_lock.h | 21 ++++++++++++++ > >>>>>>> 2 files changed, 87 insertions(+) > >>>>>>> > >>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > >>>>>>> index e527684dd394..462408a2d1b8 100644 > >>>>>>> --- a/include/drm/drm_atomic.h > >>>>>>> +++ b/include/drm/drm_atomic.h > >>>>>>> @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, > >>>>>>> return plane->state; > >>>>>>> } > >>>>>>> > >>>>>>> + > >>>>>>> +/** > >>>>>>> + * drm_atomic_get_current_plane_state - get current plane state > >>>>>>> + * @plane: plane to grab > >>>>>>> + * > >>>>>>> + * This function returns the current plane state for the given plane, > >>>>>>> + * with extra locking checks to make sure that the plane state can be > >>>>>>> + * retrieved safely. > >>>>>>> + * > >>>>>>> + * Returns: > >>>>>>> + * > >>>>>>> + * Pointer to the current plane state. > >>>>>>> + */ > >>>>>>> +static inline struct drm_plane_state * > >>>>>>> +drm_atomic_get_current_plane_state(struct drm_plane *plane) > >>>>>>> +{ > >>>>>>> + struct drm_plane_state *plane_state = plane->state; > >>>>>>> + struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL; > >>>>>>> + > >>>>>>> + if (crtc) > >>>>>>> + drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex); > >>>>>>> + else > >>>>>>> + drm_modeset_lock_assert_held(&plane->mutex); > >>>>>> Hmm. Daniel recently smashed me on the head with a big clue bat to point > >>>>>> out that accessing object->state isn't safe unless you hold the object lock. > >>>>>> So this thing seems suspicious. What's the use case for this? > >>>>>> > >>>>>> I guess in this case it might be safe since a parallel update would lock > >>>>>> the crtc as well. But it does feel like promoting potentially dangerous > >>>>>> behaviour. Also there are no barriers so I'm not sure this would be > >>>>>> guaranteed to give us the correct answer anyway. > >>>>>> > >>>>>> As far as all of these functions go, should they return const*? Presumably > >>>>>> you wouldn't want to allow changes to the current state. > >>>>> Yep, need at least a lockdep check for the plane->mutex. And imo also a > >>>>> check that we're indeed in atomic_check per the idea I dropped on the > >>>>> cover letter. > >>>>> > >>>>> And +1 on const * for these, that seems like a very important check. > >>>> Well I allowed for crtc lock held because the __ function uses crtc->mutex as safety lock. > >>> What is this so called __ function exactly? > >> __drm_atomic_get_current_plane_state, which is only used by drm_atomic_crtc_state_for_each_plane_state. > >> > >> It iterates over crtc_state->plane_mask and then gets new_plane_state if available, or plane->state if the plane is not part of the state. > >> This is mostly used for watermark calculations. > > Sounds like we should kill that sucker and make things clearer by > > enforcing the "want to access foo->state? then grab foo->lock first" > > rule. > Except adding all planes to cursor updates would result in unintended behavior. That wasn't my suggestion. > And there are already patches to use the sprite plane instead of the cursor plane for skylake, so it's not just theoretical. :) > > Testcases: > flip-vs-cursor-busy-crc-legacy > flip-vs-cursor-busy-crc-atomic > > ~Maarten
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index e527684dd394..462408a2d1b8 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, return plane->state; } + +/** + * drm_atomic_get_current_plane_state - get current plane state + * @plane: plane to grab + * + * This function returns the current plane state for the given plane, + * with extra locking checks to make sure that the plane state can be + * retrieved safely. + * + * Returns: + * + * Pointer to the current plane state. + */ +static inline struct drm_plane_state * +drm_atomic_get_current_plane_state(struct drm_plane *plane) +{ + struct drm_plane_state *plane_state = plane->state; + struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL; + + if (crtc) + drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex); + else + drm_modeset_lock_assert_held(&plane->mutex); + + return plane_state; +} + +/** + * drm_atomic_get_current_crtc_state - get current crtc state + * @crtc: crtc to grab + * + * This function returns the current crtc state for the given crtc, + * with extra locking checks to make sure that the crtc state can be + * retrieved safely. + * + * Returns: + * + * Pointer to the current crtc state. + */ +static inline struct drm_crtc_state * +drm_atomic_get_current_crtc_state(struct drm_crtc *crtc) +{ + drm_modeset_lock_assert_held(&crtc->mutex); + + return crtc->state; +} + +/** + * drm_atomic_get_current_connector_state - get current connector state + * @connector: connector to grab + * + * This function returns the current connector state for the given connector, + * with extra locking checks to make sure that the connector state can be + * retrieved safely. + * + * Returns: + * + * Pointer to the current connector state. + */ +#define drm_atomic_get_current_connector_state(connector) \ +({ /* Implemented as macro to avoid including drmP for struct drm_device */ \ + struct drm_connector *con__ = (connector); \ + drm_modeset_lock_assert_held(&con__->dev->mode_config.connection_mutex); \ + con__->state; \ +}) + int __must_check drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, struct drm_display_mode *mode); diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h index d918ce45ec2c..7635ff18ab99 100644 --- a/include/drm/drm_modeset_lock.h +++ b/include/drm/drm_modeset_lock.h @@ -109,6 +109,27 @@ static inline bool drm_modeset_is_locked(struct drm_modeset_lock *lock) return ww_mutex_is_locked(&lock->mutex); } +static inline void drm_modeset_lock_assert_held(struct drm_modeset_lock *lock) +{ +#ifdef CONFIG_LOCKDEP + lockdep_assert_held(&lock->mutex.base); +#else + WARN_ON(!drm_modeset_is_locked(lock)); +#endif +} + +static inline void drm_modeset_lock_assert_one_held(struct drm_modeset_lock *lock1, + struct drm_modeset_lock *lock2) +{ +#ifdef CONFIG_LOCKDEP + WARN_ON(debug_locks && + !lockdep_is_held(&lock1->mutex.base) && + !lockdep_is_held(&lock2->mutex.base)); +#else + WARN_ON(!drm_modeset_is_locked(lock1) && !drm_modeset_is_locked(lock2)); +#endif +} + int drm_modeset_lock(struct drm_modeset_lock *lock, struct drm_modeset_acquire_ctx *ctx); int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock,
With checks! This will allow safe access to the current state, while ensuring that the correct locks are held. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- include/drm/drm_atomic.h | 66 ++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_modeset_lock.h | 21 ++++++++++++++ 2 files changed, 87 insertions(+)