[PATCH v2 0/4] drm/atomic: Add accessor macros for all atomic state.

[PATCH v2 0/4] drm/atomic: Add accessor macros for all atomic state.

[Maarten Lankhorst]

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(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/4] drm/atomic: Add new iterators over all state, v2.

[Maarten Lankhorst]

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);
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maarten Lankhorst]

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,
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Wed, Nov 16, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote:
> With checks! This will allow safe access to the current state,
> while ensuring that the correct locks are held.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  include/drm/drm_atomic.h       | 66 ++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_modeset_lock.h | 21 ++++++++++++++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index e527684dd394..462408a2d1b8 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state,
>  	return plane->state;
>  }
>  
> +
> +/**
> + * drm_atomic_get_current_plane_state - get current plane state
> + * @plane: plane to grab
> + *
> + * This function returns the current plane state for the given plane,
> + * with extra locking checks to make sure that the plane state can be
> + * retrieved safely.
> + *
> + * Returns:
> + *
> + * Pointer to the current plane state.
> + */
> +static inline struct drm_plane_state *
> +drm_atomic_get_current_plane_state(struct drm_plane *plane)
> +{
> +	struct drm_plane_state *plane_state = plane->state;
> +	struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL;
> +
> +	if (crtc)
> +		drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex);
> +	else
> +		drm_modeset_lock_assert_held(&plane->mutex);

Hmm. Daniel recently smashed me on the head with a big clue bat to point
out that accessing object->state isn't safe unless you hold the object lock.
So this thing seems suspicious. What's the use case for this?

I guess in this case it might be safe since a parallel update would lock
the crtc as well. But it does feel like promoting potentially dangerous
behaviour. Also there are no barriers so I'm not sure this would be
guaranteed to give us the correct answer anyway.

As far as all of these functions go, should they return const*? Presumably
you wouldn't want to allow changes to the current state.

> +
> +	return plane_state;
> +}
> +
> +/**
> + * drm_atomic_get_current_crtc_state - get current crtc state
> + * @crtc: crtc to grab
> + *
> + * This function returns the current crtc state for the given crtc,
> + * with extra locking checks to make sure that the crtc state can be
> + * retrieved safely.
> + *
> + * Returns:
> + *
> + * Pointer to the current crtc state.
> + */
> +static inline struct drm_crtc_state *
> +drm_atomic_get_current_crtc_state(struct drm_crtc *crtc)
> +{
> +	drm_modeset_lock_assert_held(&crtc->mutex);
> +
> +	return crtc->state;
> +}
> +
> +/**
> + * drm_atomic_get_current_connector_state - get current connector state
> + * @connector: connector to grab
> + *
> + * This function returns the current connector state for the given connector,
> + * with extra locking checks to make sure that the connector state can be
> + * retrieved safely.
> + *
> + * Returns:
> + *
> + * Pointer to the current connector state.
> + */
> +#define drm_atomic_get_current_connector_state(connector) \
> +({ /* Implemented as macro to avoid including drmP for struct drm_device */ \
> +	struct drm_connector *con__ = (connector); \
> +	drm_modeset_lock_assert_held(&con__->dev->mode_config.connection_mutex); \
> +	con__->state; \
> +})
> +
>  int __must_check
>  drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  			     struct drm_display_mode *mode);
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index d918ce45ec2c..7635ff18ab99 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -109,6 +109,27 @@ static inline bool drm_modeset_is_locked(struct drm_modeset_lock *lock)
>  	return ww_mutex_is_locked(&lock->mutex);
>  }
>  
> +static inline void drm_modeset_lock_assert_held(struct drm_modeset_lock *lock)
> +{
> +#ifdef CONFIG_LOCKDEP
> +	lockdep_assert_held(&lock->mutex.base);
> +#else
> +	WARN_ON(!drm_modeset_is_locked(lock));
> +#endif
> +}
> +
> +static inline void drm_modeset_lock_assert_one_held(struct drm_modeset_lock *lock1,
> +						    struct drm_modeset_lock *lock2)
> +{
> +#ifdef CONFIG_LOCKDEP
> +	WARN_ON(debug_locks &&
> +		!lockdep_is_held(&lock1->mutex.base) &&
> +		!lockdep_is_held(&lock2->mutex.base));
> +#else
> +	WARN_ON(!drm_modeset_is_locked(lock1) && !drm_modeset_is_locked(lock2));
> +#endif
> +}
> +
>  int drm_modeset_lock(struct drm_modeset_lock *lock,
>  		struct drm_modeset_acquire_ctx *ctx);
>  int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock,
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[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.
-Daniel

> 
> > +
> > +	return plane_state;
> > +}
> > +
> > +/**
> > + * drm_atomic_get_current_crtc_state - get current crtc state
> > + * @crtc: crtc to grab
> > + *
> > + * This function returns the current crtc state for the given crtc,
> > + * with extra locking checks to make sure that the crtc state can be
> > + * retrieved safely.
> > + *
> > + * Returns:
> > + *
> > + * Pointer to the current crtc state.
> > + */
> > +static inline struct drm_crtc_state *
> > +drm_atomic_get_current_crtc_state(struct drm_crtc *crtc)
> > +{
> > +	drm_modeset_lock_assert_held(&crtc->mutex);
> > +
> > +	return crtc->state;
> > +}
> > +
> > +/**
> > + * drm_atomic_get_current_connector_state - get current connector state
> > + * @connector: connector to grab
> > + *
> > + * This function returns the current connector state for the given connector,
> > + * with extra locking checks to make sure that the connector state can be
> > + * retrieved safely.
> > + *
> > + * Returns:
> > + *
> > + * Pointer to the current connector state.
> > + */
> > +#define drm_atomic_get_current_connector_state(connector) \
> > +({ /* Implemented as macro to avoid including drmP for struct drm_device */ \
> > +	struct drm_connector *con__ = (connector); \
> > +	drm_modeset_lock_assert_held(&con__->dev->mode_config.connection_mutex); \
> > +	con__->state; \
> > +})
> > +
> >  int __must_check
> >  drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
> >  			     struct drm_display_mode *mode);
> > diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> > index d918ce45ec2c..7635ff18ab99 100644
> > --- a/include/drm/drm_modeset_lock.h
> > +++ b/include/drm/drm_modeset_lock.h
> > @@ -109,6 +109,27 @@ static inline bool drm_modeset_is_locked(struct drm_modeset_lock *lock)
> >  	return ww_mutex_is_locked(&lock->mutex);
> >  }
> >  
> > +static inline void drm_modeset_lock_assert_held(struct drm_modeset_lock *lock)
> > +{
> > +#ifdef CONFIG_LOCKDEP
> > +	lockdep_assert_held(&lock->mutex.base);
> > +#else
> > +	WARN_ON(!drm_modeset_is_locked(lock));
> > +#endif
> > +}
> > +
> > +static inline void drm_modeset_lock_assert_one_held(struct drm_modeset_lock *lock1,
> > +						    struct drm_modeset_lock *lock2)
> > +{
> > +#ifdef CONFIG_LOCKDEP
> > +	WARN_ON(debug_locks &&
> > +		!lockdep_is_held(&lock1->mutex.base) &&
> > +		!lockdep_is_held(&lock2->mutex.base));
> > +#else
> > +	WARN_ON(!drm_modeset_is_locked(lock1) && !drm_modeset_is_locked(lock2));
> > +#endif
> > +}
> > +
> >  int drm_modeset_lock(struct drm_modeset_lock *lock,
> >  		struct drm_modeset_acquire_ctx *ctx);
> >  int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock,
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[Maarten Lankhorst]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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?

> 
> 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

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

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

[Maarten Lankhorst]

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


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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.

> >> 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
> 

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

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

[Maarten Lankhorst]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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

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

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

[Maarten Lankhorst]

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,

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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,
> 

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

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

[Maarten Lankhorst]

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,
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/4] drm/atomic: Add macros to access existing old/new state

[Maarten Lankhorst]

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
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/4] drm/atomic: Add macros to access existing old/new state

[Daniel Vetter]

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

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

[PATCH v2 4/4] drm/atomic: Add checks to ensure get_state is not called after swapping.

[Maarten Lankhorst]

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)
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/4] drm/atomic: Add accessor macros for all atomic state.

[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?

> 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

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

Re: [PATCH v2 0/4] drm/atomic: Add accessor macros for all atomic state.

[Maarten Lankhorst]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/4] drm/atomic: Add accessor macros for all atomic state.

[Daniel Vetter]

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

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

Re: [PATCH v2 0/4] drm/atomic: Add accessor macros for all atomic state.

[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.
-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

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

Re: [PATCH v2 0/4] drm/atomic: Add accessor macros for all atomic state.

[Maarten Lankhorst]

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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel