[v2.1,2/4] drm/atomic: Add accessor macros for the current state, v2.
diff mbox

Message ID c5e48c59-6033-8fc0-d7e7-614b12931e73@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst Nov. 17, 2016, 1:20 p.m. UTC
With checks! This will allow safe access to the current state,
while ensuring that the correct locks are held.

Changes since v1:
- Constify returned states.
- Require plane lock to return plane state, don't allow crtc_lock to
  be sufficient.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---

Comments

Daniel Vetter Dec. 9, 2016, 10:27 p.m. UTC | #1
On Thu, Nov 17, 2016 at 02:20:10PM +0100, Maarten Lankhorst wrote:
> With checks! This will allow safe access to the current state,
> while ensuring that the correct locks are held.
> 
> Changes since v1:
> - Constify returned states.
> - Require plane lock to return plane state, don't allow crtc_lock to
>   be sufficient.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I wanted to add some more docs to drm_atomic_state and related code and
realized this series here hasn't landed. What's the status? Anything
pending from this long discussion, Ville?

Or should I go through it and do some review&merging?
-Daniel

> ---
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index e527684dd394..9af5aba8c27c 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -334,6 +334,66 @@ __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 const struct drm_plane_state *
> +drm_atomic_get_current_plane_state(struct drm_plane *plane)
> +{
> +	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 const 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); \
> +	(const struct drm_connector_state *)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..3ca4ee838b07 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -109,6 +109,15 @@ 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
> +}
> +
>  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,
>
Daniel Vetter Dec. 14, 2016, 1:15 p.m. UTC | #2
On Thu, Nov 17, 2016 at 02:20:10PM +0100, Maarten Lankhorst wrote:
> With checks! This will allow safe access to the current state,
> while ensuring that the correct locks are held.
> 
> Changes since v1:
> - Constify returned states.
> - Require plane lock to return plane state, don't allow crtc_lock to
>   be sufficient.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index e527684dd394..9af5aba8c27c 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -334,6 +334,66 @@ __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 const struct drm_plane_state *
> +drm_atomic_get_current_plane_state(struct drm_plane *plane)
> +{
> +	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 const 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 */ \

Imo that's not a terrible good reason. Eventually will split drm_device.h
out from drmP.h, and then we can nuke the additional include. Can you pls
respin?
-Daniel

> +	struct drm_connector *con__ = (connector); \
> +	drm_modeset_lock_assert_held(&con__->dev->mode_config.connection_mutex); \
> +	(const struct drm_connector_state *)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..3ca4ee838b07 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -109,6 +109,15 @@ 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
> +}
> +
>  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,
>

Patch
diff mbox

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index e527684dd394..9af5aba8c27c 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -334,6 +334,66 @@  __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 const struct drm_plane_state *
+drm_atomic_get_current_plane_state(struct drm_plane *plane)
+{
+	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 const 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); \
+	(const struct drm_connector_state *)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..3ca4ee838b07 100644
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -109,6 +109,15 @@  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
+}
+
 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,