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