Second approach. Instead of trying to convert all drivers straight away, implement all macros that are required to get state working.
Current situation: Use obj->state, which can refer to old or new state. Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state. Use for_each_obj_in_state, which refers to new or old state.
New situation: When doing some dereferencing outside atomic_state, use drm_atomic_get_current_obj_state which has locking checks, instead of obj->state.
During atomic check: - Use drm_atomic_get_obj_state to add a object to the atomic state, or get the new state. - Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state, without adding the object if it's not part of the state. For planes and connectors the relevant crtc_state is added, so this will work to get the crtc_state from foo_state->crtc too, saves some error handling. :)
During atomic commit: - Do not use drm_atomic_get_obj_state, obj->state or drm_atomic_get_(existing_)obj_state any more, replace with drm_atomic_get_old/new_obj_state calls as required.
During both: - Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as needed. oldnew will be renamed to for_each_obj_in_state after all callers are converted to the new api.
This will give the correct state regardless of swapping.
Maarten Lankhorst (4): drm/atomic: Add new iterators over all state, v2. drm/atomic: Add accessor macros for the current state. drm/atomic: Add macros to access existing old/new state drm/atomic: Add checks to ensure get_state is not called after swapping.
drivers/gpu/drm/drm_atomic.c | 24 +++- drivers/gpu/drm/drm_atomic_helper.c | 47 ++++++- drivers/gpu/drm/i915/intel_display.c | 11 +- include/drm/drm_atomic.h | 247 ++++++++++++++++++++++++++++++++++- include/drm/drm_atomic_helper.h | 2 + include/drm/drm_modeset_lock.h | 21 +++ 6 files changed, 338 insertions(+), 14 deletions(-)
Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to replace the old for_each_xxx_in_state ones. This is useful for >1 flip depth and getting rid of all xxx->state dereferences.
This requires extra fixups done when committing a state after duplicating, which in general isn't valid but is used by suspend/resume. To handle these, introduce drm_atomic_helper_commit_duplicated_state which performs those fixups before checking & committing the state.
Changes since v1: - Remove nonblock parameter for commit_duplicated_state.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 6 +++ drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++++++-- drivers/gpu/drm/i915/intel_display.c | 11 ++--- include/drm/drm_atomic.h | 81 ++++++++++++++++++++++++++++++++++-- include/drm/drm_atomic_helper.h | 2 + 5 files changed, 136 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 57e0a6e96f6d..e80663154858 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -279,6 +279,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, return ERR_PTR(-ENOMEM);
state->crtcs[index].state = crtc_state; + state->crtcs[index].old_state = crtc->state; + state->crtcs[index].new_state = crtc_state; state->crtcs[index].ptr = crtc; crtc_state->state = state;
@@ -666,6 +668,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
state->planes[index].state = plane_state; state->planes[index].ptr = plane; + state->planes[index].old_state = plane->state; + state->planes[index].new_state = plane_state; plane_state->state = state;
DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", @@ -993,6 +997,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
drm_connector_reference(connector); state->connectors[index].state = connector_state; + state->connectors[index].old_state = connector->state; + state->connectors[index].new_state = connector_state; state->connectors[index].ptr = connector; connector_state->state = state;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 50077961228a..0e11bef26999 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2491,7 +2491,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all); * * See also: * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(), - * drm_atomic_helper_resume() + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state() */ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) { @@ -2532,6 +2532,47 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) EXPORT_SYMBOL(drm_atomic_helper_suspend);
/** + * drm_atomic_helper_commit_duplicated_state - commit duplicated state + * @state: duplicated atomic state to commit + * @ctx: pointer to acquire_ctx to use for commit. + * + * The state returned by drm_atomic_helper_duplicate_state() and + * drm_atomic_helper_suspend() is partially invalid, and needs to + * be fixed up before commit. + * + * Returns: + * 0 on success or a negative error code on failure. + * + * See also: + * drm_atomic_helper_suspend() + */ +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, + struct drm_modeset_acquire_ctx *ctx) +{ + int i; + struct drm_plane *plane; + struct drm_plane_state *plane_state; + struct drm_connector *connector; + struct drm_connector_state *conn_state; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + + state->acquire_ctx = ctx; + + for_each_new_plane_in_state(state, plane, plane_state, i) + state->planes[i].old_state = plane->state; + + for_each_new_crtc_in_state(state, crtc, crtc_state, i) + state->crtcs[i].old_state = crtc->state; + + for_each_new_connector_in_state(state, connector, conn_state, i) + state->connectors[i].old_state = connector->state; + + return drm_atomic_commit(state); +} +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state); + +/** * drm_atomic_helper_resume - subsystem-level resume helper * @dev: DRM device * @state: atomic state to resume to @@ -2554,9 +2595,9 @@ int drm_atomic_helper_resume(struct drm_device *dev, int err;
drm_mode_config_reset(dev); + drm_modeset_lock_all(dev); - state->acquire_ctx = config->acquire_ctx; - err = drm_atomic_commit(state); + err = drm_atomic_helper_commit_duplicated_state(state, config->acquire_ctx); drm_modeset_unlock_all(dev);
return err; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ebb8b833395..b2324f3b5ef1 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3496,7 +3496,8 @@ static void intel_update_primary_planes(struct drm_device *dev)
static int __intel_display_resume(struct drm_device *dev, - struct drm_atomic_state *state) + struct drm_atomic_state *state, + struct drm_modeset_acquire_ctx *ctx) { struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; @@ -3520,7 +3521,7 @@ __intel_display_resume(struct drm_device *dev, /* ignore any reset values/BIOS leftovers in the WM registers */ to_intel_atomic_state(state)->skip_intermediate_wm = true;
- ret = drm_atomic_commit(state); + ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
WARN_ON(ret == -EDEADLK); return ret; @@ -3614,7 +3615,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) */ intel_update_primary_planes(dev); } else { - ret = __intel_display_resume(dev, state); + ret = __intel_display_resume(dev, state, ctx); if (ret) DRM_ERROR("Restoring old state failed with %i\n", ret); } @@ -3634,7 +3635,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) dev_priv->display.hpd_irq_setup(dev_priv); spin_unlock_irq(&dev_priv->irq_lock);
- ret = __intel_display_resume(dev, state); + ret = __intel_display_resume(dev, state, ctx); if (ret) DRM_ERROR("Restoring old state failed with %i\n", ret);
@@ -17061,7 +17062,7 @@ void intel_display_resume(struct drm_device *dev) }
if (!ret) - ret = __intel_display_resume(dev, state); + ret = __intel_display_resume(dev, state, &ctx);
drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 331bb100b718..e527684dd394 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -137,18 +137,18 @@ struct drm_crtc_commit {
struct __drm_planes_state { struct drm_plane *ptr; - struct drm_plane_state *state; + struct drm_plane_state *state, *old_state, *new_state; };
struct __drm_crtcs_state { struct drm_crtc *ptr; - struct drm_crtc_state *state; + struct drm_crtc_state *state, *old_state, *new_state; struct drm_crtc_commit *commit; };
struct __drm_connnectors_state { struct drm_connector *ptr; - struct drm_connector_state *state; + struct drm_connector_state *state, *old_state, *new_state; };
/** @@ -381,6 +381,31 @@ int drm_atomic_debugfs_init(struct drm_minor *minor); (__i)++) \ for_each_if (connector)
+#define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->num_connector && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (old_connector_state) = (__state)->connectors[__i].old_state, \ + (new_connector_state) = (__state)->connectors[__i].new_state, 1); \ + (__i)++) \ + for_each_if (connector) + +#define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->num_connector && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (old_connector_state) = (__state)->connectors[__i].old_state, 1); \ + (__i)++) \ + for_each_if (connector) + +#define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->num_connector && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (new_connector_state) = (__state)->connectors[__i].new_state, 1); \ + (__i)++) \ + for_each_if (connector) + #define for_each_crtc_in_state(__state, crtc, crtc_state, __i) \ for ((__i) = 0; \ (__i) < (__state)->dev->mode_config.num_crtc && \ @@ -389,6 +414,31 @@ int drm_atomic_debugfs_init(struct drm_minor *minor); (__i)++) \ for_each_if (crtc_state)
+#define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_crtc && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (old_crtc_state) = (__state)->crtcs[__i].old_state, \ + (new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \ + (__i)++) \ + for_each_if (crtc) + +#define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_crtc && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (old_crtc_state) = (__state)->crtcs[__i].old_state, 1); \ + (__i)++) \ + for_each_if (crtc) + +#define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_crtc && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \ + (__i)++) \ + for_each_if (crtc) + #define for_each_plane_in_state(__state, plane, plane_state, __i) \ for ((__i) = 0; \ (__i) < (__state)->dev->mode_config.num_total_plane && \ @@ -397,6 +447,31 @@ int drm_atomic_debugfs_init(struct drm_minor *minor); (__i)++) \ for_each_if (plane_state)
+#define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_total_plane && \ + ((plane) = (__state)->planes[__i].ptr, \ + (old_plane_state) = (__state)->planes[__i].old_state, \ + (new_plane_state) = (__state)->planes[__i].new_state, 1); \ + (__i)++) \ + for_each_if (plane) + +#define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_total_plane && \ + ((plane) = (__state)->planes[__i].ptr, \ + (old_plane_state) = (__state)->planes[__i].old_state, 1); \ + (__i)++) \ + for_each_if (plane) + +#define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_total_plane && \ + ((plane) = (__state)->planes[__i].ptr, \ + (new_plane_state) = (__state)->planes[__i].new_state, 1); \ + (__i)++) \ + for_each_if (plane) + /** * drm_atomic_crtc_needs_modeset - compute combined modeset need * @state: &drm_crtc_state for the CRTC diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 7ff92b09fd9c..f5723831a191 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -108,6 +108,8 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set, int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx); struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev); +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, + struct drm_modeset_acquire_ctx *ctx); int drm_atomic_helper_resume(struct drm_device *dev, struct drm_atomic_state *state);
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); + + 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,
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
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 */ \ + 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,
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,
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,
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. Changes since v2: - Include drmP.h for drm_device, change the macro drm_atomic_get_current_connector_state back to a function.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- include/drm/drm_atomic.h | 62 ++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_modeset_lock.h | 9 ++++++ 2 files changed, 71 insertions(+)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index ca9b32d157bd..63b5fc8e1fdc 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -29,6 +29,7 @@ #define DRM_ATOMIC_H_
#include <drm/drm_crtc.h> +#include <drm/drmP.h>
/** * struct drm_crtc_commit - track modeset commits on a CRTC @@ -335,6 +336,67 @@ __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. + */ +static inline const struct drm_connector_state * +drm_atomic_get_current_connector_state(struct drm_connector *connector) +{ + drm_modeset_lock_assert_held(&connector->dev->mode_config.connection_mutex); + + return connector->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,
During atomic check/commit, these macros should be used in place of get_existing_state.
We also ban the use of get_xx_state after atomic check, because at that point no new locks should be taken and get_new/old_state should be used instead.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- include/drm/drm_atomic.h | 100 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 462408a2d1b8..7be178264732 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -264,6 +264,36 @@ drm_atomic_get_existing_crtc_state(struct drm_atomic_state *state, }
/** + * drm_atomic_get_old_crtc_state - get old crtc state, if it exists + * @state: global atomic state object + * @crtc: crtc to grab + * + * This function returns the old crtc state for the given crtc, or + * NULL if the crtc is not part of the global atomic state. + */ +static inline struct drm_crtc_state * +drm_atomic_get_old_crtc_state(struct drm_atomic_state *state, + struct drm_crtc *crtc) +{ + return state->crtcs[drm_crtc_index(crtc)].old_state; +} + +/** + * drm_atomic_get_new_crtc_state - get new crtc state, if it exists + * @state: global atomic state object + * @crtc: crtc to grab + * + * This function returns the new crtc state for the given crtc, or + * NULL if the crtc is not part of the global atomic state. + */ +static inline struct drm_crtc_state * +drm_atomic_get_new_crtc_state(struct drm_atomic_state *state, + struct drm_crtc *crtc) +{ + return state->crtcs[drm_crtc_index(crtc)].new_state; +} + +/** * drm_atomic_get_existing_plane_state - get plane state, if it exists * @state: global atomic state object * @plane: plane to grab @@ -279,6 +309,36 @@ drm_atomic_get_existing_plane_state(struct drm_atomic_state *state, }
/** + * drm_atomic_get_old_plane_state - get plane state, if it exists + * @state: global atomic state object + * @plane: plane to grab + * + * This function returns the old plane state for the given plane, or + * NULL if the plane is not part of the global atomic state. + */ +static inline struct drm_plane_state * +drm_atomic_get_old_plane_state(struct drm_atomic_state *state, + struct drm_plane *plane) +{ + return state->planes[drm_plane_index(plane)].old_state; +} + +/** + * drm_atomic_get_new_plane_state - get plane state, if it exists + * @state: global atomic state object + * @plane: plane to grab + * + * This function returns the new plane state for the given plane, or + * NULL if the plane is not part of the global atomic state. + */ +static inline struct drm_plane_state * +drm_atomic_get_new_plane_state(struct drm_atomic_state *state, + struct drm_plane *plane) +{ + return state->planes[drm_plane_index(plane)].new_state; +} + +/** * drm_atomic_get_existing_connector_state - get connector state, if it exists * @state: global atomic state object * @connector: connector to grab @@ -299,6 +359,46 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state, }
/** + * drm_atomic_get_old_connector_state - get connector state, if it exists + * @state: global atomic state object + * @connector: connector to grab + * + * This function returns the old connector state for the given connector, + * or NULL if the connector is not part of the global atomic state. + */ +static inline struct drm_connector_state * +drm_atomic_get_old_connector_state(struct drm_atomic_state *state, + struct drm_connector *connector) +{ + int index = drm_connector_index(connector); + + if (index >= state->num_connector) + return NULL; + + return state->connectors[index].old_state; +} + +/** + * drm_atomic_get_new_connector_state - get connector state, if it exists + * @state: global atomic state object + * @connector: connector to grab + * + * This function returns the new connector state for the given connector, + * or NULL if the connector is not part of the global atomic state. + */ +static inline struct drm_connector_state * +drm_atomic_get_new_connector_state(struct drm_atomic_state *state, + struct drm_connector *connector) +{ + int index = drm_connector_index(connector); + + if (index >= state->num_connector) + return NULL; + + return state->connectors[index].new_state; +} + +/** * __drm_atomic_get_current_plane_state - get current plane state * @state: global atomic state object * @plane: plane to grab
On Wed, Nov 16, 2016 at 02:58:07PM +0100, Maarten Lankhorst wrote:
During atomic check/commit, these macros should be used in place of get_existing_state.
We also ban the use of get_xx_state after atomic check, because at that point no new locks should be taken and get_new/old_state should be used instead.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
So one slightly annoying thing with the new/old split is that const-ness of these changes. In atomic_check the old state should be const, in atomic_commit the new state. By going to the new/old split we're losing that safeguard. Otoh we have lots and lots of exceptions to that rule ... not sure how much aiming for const correctness is worth it. -Daniel
include/drm/drm_atomic.h | 100 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 462408a2d1b8..7be178264732 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -264,6 +264,36 @@ drm_atomic_get_existing_crtc_state(struct drm_atomic_state *state, }
/**
- drm_atomic_get_old_crtc_state - get old crtc state, if it exists
- @state: global atomic state object
- @crtc: crtc to grab
- This function returns the old crtc state for the given crtc, or
- NULL if the crtc is not part of the global atomic state.
- */
+static inline struct drm_crtc_state * +drm_atomic_get_old_crtc_state(struct drm_atomic_state *state,
struct drm_crtc *crtc)+{
- return state->crtcs[drm_crtc_index(crtc)].old_state;
+}
+/**
- drm_atomic_get_new_crtc_state - get new crtc state, if it exists
- @state: global atomic state object
- @crtc: crtc to grab
- This function returns the new crtc state for the given crtc, or
- NULL if the crtc is not part of the global atomic state.
- */
+static inline struct drm_crtc_state * +drm_atomic_get_new_crtc_state(struct drm_atomic_state *state,
struct drm_crtc *crtc)+{
- return state->crtcs[drm_crtc_index(crtc)].new_state;
+}
+/**
- drm_atomic_get_existing_plane_state - get plane state, if it exists
- @state: global atomic state object
- @plane: plane to grab
@@ -279,6 +309,36 @@ drm_atomic_get_existing_plane_state(struct drm_atomic_state *state, }
/**
- drm_atomic_get_old_plane_state - get plane state, if it exists
- @state: global atomic state object
- @plane: plane to grab
- This function returns the old plane state for the given plane, or
- NULL if the plane is not part of the global atomic state.
- */
+static inline struct drm_plane_state * +drm_atomic_get_old_plane_state(struct drm_atomic_state *state,
struct drm_plane *plane)+{
- return state->planes[drm_plane_index(plane)].old_state;
+}
+/**
- drm_atomic_get_new_plane_state - get plane state, if it exists
- @state: global atomic state object
- @plane: plane to grab
- This function returns the new plane state for the given plane, or
- NULL if the plane is not part of the global atomic state.
- */
+static inline struct drm_plane_state * +drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
struct drm_plane *plane)+{
- return state->planes[drm_plane_index(plane)].new_state;
+}
+/**
- drm_atomic_get_existing_connector_state - get connector state, if it exists
- @state: global atomic state object
- @connector: connector to grab
@@ -299,6 +359,46 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state, }
/**
- drm_atomic_get_old_connector_state - get connector state, if it exists
- @state: global atomic state object
- @connector: connector to grab
- This function returns the old connector state for the given connector,
- or NULL if the connector is not part of the global atomic state.
- */
+static inline struct drm_connector_state * +drm_atomic_get_old_connector_state(struct drm_atomic_state *state,
struct drm_connector *connector)+{
- int index = drm_connector_index(connector);
- if (index >= state->num_connector)
return NULL;- return state->connectors[index].old_state;
+}
+/**
- drm_atomic_get_new_connector_state - get connector state, if it exists
- @state: global atomic state object
- @connector: connector to grab
- This function returns the new connector state for the given connector,
- or NULL if the connector is not part of the global atomic state.
- */
+static inline struct drm_connector_state * +drm_atomic_get_new_connector_state(struct drm_atomic_state *state,
struct drm_connector *connector)+{
- int index = drm_connector_index(connector);
- if (index >= state->num_connector)
return NULL;- return state->connectors[index].new_state;
+}
+/**
- __drm_atomic_get_current_plane_state - get current plane state
- @state: global atomic state object
- @plane: plane to grab
-- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
This isn't allowed, callers should use get_new_state or get_old_state, but lets be paranoid..
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index e80663154858..81080e37091d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -267,8 +267,12 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, WARN_ON(!state->acquire_ctx);
crtc_state = drm_atomic_get_existing_crtc_state(state, crtc); - if (crtc_state) + if (crtc_state) { + WARN(state->crtcs[index].state == state->crtcs[index].old_state, + "drm_atomic_get_crtc_state() used after swapping state!\n"); + return crtc_state; + }
ret = drm_modeset_lock(&crtc->mutex, state->acquire_ctx); if (ret) @@ -655,8 +659,12 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, WARN_ON(!state->acquire_ctx);
plane_state = drm_atomic_get_existing_plane_state(state, plane); - if (plane_state) + if (plane_state) { + WARN(state->planes[index].state == state->planes[index].old_state, + "drm_atomic_get_plane_state() used after swapping state!\n"); + return plane_state; + }
ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx); if (ret) @@ -988,8 +996,12 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state, state->num_connector = alloc; }
- if (state->connectors[index].state) + if (state->connectors[index].state) { + WARN(state->connectors[index].state == state->connectors[index].old_state, + "drm_atomic_get_connector_state() used after swapping state!\n"); + return state->connectors[index].state; + }
connector_state = connector->funcs->atomic_duplicate_state(connector); if (!connector_state)
On Wed, Nov 16, 2016 at 02:58:04PM +0100, Maarten Lankhorst wrote:
Second approach. Instead of trying to convert all drivers straight away, implement all macros that are required to get state working.
Current situation: Use obj->state, which can refer to old or new state. Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state. Use for_each_obj_in_state, which refers to new or old state.
New situation: When doing some dereferencing outside atomic_state, use drm_atomic_get_current_obj_state which has locking checks, instead of obj->state.
During atomic check:
- Use drm_atomic_get_obj_state to add a object to the atomic state, or get the new state.
- Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state, without adding the object if it's not part of the state. For planes and connectors the relevant crtc_state is added, so this will work to get the crtc_state from foo_state->crtc too, saves some error handling. :)
Hm, this needs to check looking, somehow. Otherwise everyone just randomly peeks at state and all hell breaks loose once more. Or why do you want to avoid adding state for CRTCs?
During atomic commit:
- Do not use drm_atomic_get_obj_state, obj->state or drm_atomic_get_(existing_)obj_state any more, replace with drm_atomic_get_old/new_obj_state calls as required.
Wild idea, can we enforce this? E.g. with a drm_mode_config->in_atomic_check atomic counter that we inc/dec around the atomic_check call, and then a WARN_ON(!dev->mode_config.in_atomic_check); It will have some false positives when concurrent atomic commits happen, but for testing it should catch all offenders.
Of course this won't catch obj->state access, but we can fix that by renaming to obj->_state ...
During both:
- Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as needed. oldnew will be renamed to for_each_obj_in_state after all callers are converted to the new api.
This will give the correct state regardless of swapping.
Otherwise sounds like a reasonable plan I think. -Daniel
Maarten Lankhorst (4): drm/atomic: Add new iterators over all state, v2. drm/atomic: Add accessor macros for the current state. drm/atomic: Add macros to access existing old/new state drm/atomic: Add checks to ensure get_state is not called after swapping.
drivers/gpu/drm/drm_atomic.c | 24 +++- drivers/gpu/drm/drm_atomic_helper.c | 47 ++++++- drivers/gpu/drm/i915/intel_display.c | 11 +- include/drm/drm_atomic.h | 247 ++++++++++++++++++++++++++++++++++- include/drm/drm_atomic_helper.h | 2 + include/drm/drm_modeset_lock.h | 21 +++ 6 files changed, 338 insertions(+), 14 deletions(-)
-- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 16-11-16 om 15:18 schreef Daniel Vetter:
On Wed, Nov 16, 2016 at 02:58:04PM +0100, Maarten Lankhorst wrote:
Second approach. Instead of trying to convert all drivers straight away, implement all macros that are required to get state working.
Current situation: Use obj->state, which can refer to old or new state. Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state. Use for_each_obj_in_state, which refers to new or old state.
New situation: When doing some dereferencing outside atomic_state, use drm_atomic_get_current_obj_state which has locking checks, instead of obj->state.
During atomic check:
- Use drm_atomic_get_obj_state to add a object to the atomic state, or get the new state.
- Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state, without adding the object if it's not part of the state. For planes and connectors the relevant crtc_state is added, so this will work to get the crtc_state from foo_state->crtc too, saves some error handling. :)
Hm, this needs to check looking, somehow. Otherwise everyone just randomly peeks at state and all hell breaks loose once more. Or why do you want to avoid adding state for CRTCs?
We don't avoid adding state for crtc's, we always add them as required.
Some ->check_plane callbacks do things like:
if (plane_state->crtc) { crtc_state = get_crtc_state(plane_state->crtc); ret = PTR_ERR_OR_ZERO(crtc_state); if (ret) return ret; }
which can be simplified to
if (plane_state->crtc) crtc_state = get_crtc_state(plane_state->crtc); /* No need to null check */
Same for grabbing crtc_state from connector_state.
No changes in locking required. In fact when called from atomic_commit_tail all locks may have been dropped already.
During atomic commit:
- Do not use drm_atomic_get_obj_state, obj->state or drm_atomic_get_(existing_)obj_state any more, replace with drm_atomic_get_old/new_obj_state calls as required.
Wild idea, can we enforce this? E.g. with a drm_mode_config->in_atomic_check atomic counter that we inc/dec around the atomic_check call, and then a WARN_ON(!dev->mode_config.in_atomic_check); It will have some false positives when concurrent atomic commits happen, but for testing it should catch all offenders.
Of course this won't catch obj->state access, but we can fix that by renaming to obj->_state ...
drm_atomic_get_existing is allowed for now, but should be converted. Patch 4 adds WARNS if used after swap_state, which is probably where most offenders are.
Relatedly.. can we revive the ww_acquire_done patch again? Needs a fix for i915 load detect though..
During both:
- Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as needed. oldnew will be renamed to for_each_obj_in_state after all callers are converted to the new api.
This will give the correct state regardless of swapping.
Otherwise sounds like a reasonable plan I think.
Ok good. :)
~Maarten
On Wed, Nov 16, 2016 at 05:11:31PM +0100, Maarten Lankhorst wrote:
Op 16-11-16 om 15:18 schreef Daniel Vetter:
On Wed, Nov 16, 2016 at 02:58:04PM +0100, Maarten Lankhorst wrote:
Second approach. Instead of trying to convert all drivers straight away, implement all macros that are required to get state working.
Current situation: Use obj->state, which can refer to old or new state. Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state. Use for_each_obj_in_state, which refers to new or old state.
New situation: When doing some dereferencing outside atomic_state, use drm_atomic_get_current_obj_state which has locking checks, instead of obj->state.
During atomic check:
- Use drm_atomic_get_obj_state to add a object to the atomic state, or get the new state.
- Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state, without adding the object if it's not part of the state. For planes and connectors the relevant crtc_state is added, so this will work to get the crtc_state from foo_state->crtc too, saves some error handling. :)
Hm, this needs to check looking, somehow. Otherwise everyone just randomly peeks at state and all hell breaks loose once more. Or why do you want to avoid adding state for CRTCs?
We don't avoid adding state for crtc's, we always add them as required.
Some ->check_plane callbacks do things like:
if (plane_state->crtc) { crtc_state = get_crtc_state(plane_state->crtc); ret = PTR_ERR_OR_ZERO(crtc_state); if (ret) return ret; }
which can be simplified to
if (plane_state->crtc) crtc_state = get_crtc_state(plane_state->crtc); /* No need to null check */
Same for grabbing crtc_state from connector_state.
No changes in locking required. In fact when called from atomic_commit_tail all locks may have been dropped already.
During atomic commit:
- Do not use drm_atomic_get_obj_state, obj->state or drm_atomic_get_(existing_)obj_state any more, replace with drm_atomic_get_old/new_obj_state calls as required.
Wild idea, can we enforce this? E.g. with a drm_mode_config->in_atomic_check atomic counter that we inc/dec around the atomic_check call, and then a WARN_ON(!dev->mode_config.in_atomic_check); It will have some false positives when concurrent atomic commits happen, but for testing it should catch all offenders.
Of course this won't catch obj->state access, but we can fix that by renaming to obj->_state ...
drm_atomic_get_existing is allowed for now, but should be converted. Patch 4 adds WARNS if used after swap_state, which is probably where most offenders are.
Relatedly.. can we revive the ww_acquire_done patch again? Needs a fix for i915 load detect though..
Yeah, just bring it on. -Daniel
During both:
- Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as needed. oldnew will be renamed to for_each_obj_in_state after all callers are converted to the new api.
This will give the correct state regardless of swapping.
Otherwise sounds like a reasonable plan I think.
Ok good. :)
~Maarten
On Wed, Nov 16, 2016 at 02:58:04PM +0100, Maarten Lankhorst wrote:
Second approach. Instead of trying to convert all drivers straight away, implement all macros that are required to get state working.
Current situation: Use obj->state, which can refer to old or new state. Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state. Use for_each_obj_in_state, which refers to new or old state.
New situation: When doing some dereferencing outside atomic_state, use drm_atomic_get_current_obj_state which has locking checks, instead of obj->state.
During atomic check:
- Use drm_atomic_get_obj_state to add a object to the atomic state, or get the new state.
- Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state, without adding the object if it's not part of the state. For planes and connectors the relevant crtc_state is added, so this will work to get the crtc_state from foo_state->crtc too, saves some error handling. :)
During atomic commit:
- Do not use drm_atomic_get_obj_state, obj->state or drm_atomic_get_(existing_)obj_state any more, replace with drm_atomic_get_old/new_obj_state calls as required.
During both:
- Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as needed. oldnew will be renamed to for_each_obj_in_state after all callers are converted to the new api.
This will give the correct state regardless of swapping.
So this is all nice, but fundamentally it's pile of new code with 0 users. Where are those patches? I think at least converting all the core+helpers to use it consistently would be needed to justify this. -Daniel
Maarten Lankhorst (4): drm/atomic: Add new iterators over all state, v2. drm/atomic: Add accessor macros for the current state. drm/atomic: Add macros to access existing old/new state drm/atomic: Add checks to ensure get_state is not called after swapping.
drivers/gpu/drm/drm_atomic.c | 24 +++- drivers/gpu/drm/drm_atomic_helper.c | 47 ++++++- drivers/gpu/drm/i915/intel_display.c | 11 +- include/drm/drm_atomic.h | 247 ++++++++++++++++++++++++++++++++++- include/drm/drm_atomic_helper.h | 2 + include/drm/drm_modeset_lock.h | 21 +++ 6 files changed, 338 insertions(+), 14 deletions(-)
-- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 14-12-16 om 14:17 schreef Daniel Vetter:
On Wed, Nov 16, 2016 at 02:58:04PM +0100, Maarten Lankhorst wrote:
Second approach. Instead of trying to convert all drivers straight away, implement all macros that are required to get state working.
Current situation: Use obj->state, which can refer to old or new state. Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state. Use for_each_obj_in_state, which refers to new or old state.
New situation: When doing some dereferencing outside atomic_state, use drm_atomic_get_current_obj_state which has locking checks, instead of obj->state.
During atomic check:
- Use drm_atomic_get_obj_state to add a object to the atomic state, or get the new state.
- Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state, without adding the object if it's not part of the state. For planes and connectors the relevant crtc_state is added, so this will work to get the crtc_state from foo_state->crtc too, saves some error handling. :)
During atomic commit:
- Do not use drm_atomic_get_obj_state, obj->state or drm_atomic_get_(existing_)obj_state any more, replace with drm_atomic_get_old/new_obj_state calls as required.
During both:
- Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as needed. oldnew will be renamed to for_each_obj_in_state after all callers are converted to the new api.
This will give the correct state regardless of swapping.
So this is all nice, but fundamentally it's pile of new code with 0 users. Where are those patches? I think at least converting all the core+helpers to use it consistently would be needed to justify this.
I deliberately didn't send those patches this time because the drivers and atomic core change all the time. I would have had to respin this whole series at least 4 times. :) I have the patches for conversion in my tree based on -tip.
https://cgit.freedesktop.org/~mlankhorst/linux/log/?h=nightly
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maarten Lankhorst -
Ville Syrjälä