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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel