Re: [PATCH v2 2/4] drm/atomic: Add accessor macros for the current state.

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel