[PATCH 0/8] atomic prep work

[PATCH 0/8] atomic prep work

[Daniel Vetter]

Hi all,

So I've split out every single hunk that touches existing code from my atomic
series and this is it. It mostly touches error handling code and other more
exceptional stuff. My idea is that if we get this in now we have a bit more
leeway with the actual atomic infrastructure work since that won't touch any
code any more which is used by current drivers.

Comments, flames and reviews highly welcome.

Cheers, Daniel

Daniel Vetter (8):
  drm: Add drm_plane/connector_index
  drm: Move modeset_lock_all helpers to drm_modeset_lock.[hc]
  drm: Handle legacy per-crtc locking with full acquire ctx
  drm: Move ->old_fb from crtc to plane
  drm: trylock modest locking for fbdev panics
  drm: Add a plane->reset hook
  drm/irq: Implement a generic vblank_wait function
  drm/i915: Use generic vblank wait

 drivers/gpu/drm/drm_crtc.c           | 194 +++++++++++++------------------
 drivers/gpu/drm/drm_fb_helper.c      |  10 +-
 drivers/gpu/drm/drm_irq.c            |  45 ++++++++
 drivers/gpu/drm/drm_modeset_lock.c   | 213 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_display.c |  41 +------
 include/drm/drmP.h                   |   2 +
 include/drm/drm_crtc.h               |  21 ++--
 include/drm/drm_modeset_lock.h       |  16 +++
 8 files changed, 373 insertions(+), 169 deletions(-)

-- 
2.0.1

[PATCH 1/8] drm: Add drm_plane/connector_index

[Daniel Vetter]

In the atomic state we'll have an array of states for crtcs, planes
and connectors and need to be able to at them by their index. We
already have a drm_crtc_index function so add the missing ones for
planes and connectors.

If it later on turns out that the list walking is too expensive we can
add the index to the relevant modeset objects.

Rob Clark doesn't like the loops too much, but we can always add an
obj->idx parameter later on. And for now reiterating is actually safer
since nowadays we have hotpluggable connectors (thanks to DP MST).

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     |  2 ++
 2 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 805240b11229..5a494caa8c9a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -937,6 +937,29 @@ void drm_connector_cleanup(struct drm_connector *connector)
 EXPORT_SYMBOL(drm_connector_cleanup);
 
 /**
+ * drm_plane_index - find the index of a registered CRTC
+ * @plane: CRTC to find index for
+ *
+ * Given a registered CRTC, return the index of that CRTC within a DRM
+ * device's list of CRTCs.
+ */
+unsigned int drm_connector_index(struct drm_connector *connector)
+{
+	unsigned int index = 0;
+	struct drm_connector *tmp;
+
+	list_for_each_entry(tmp, &connector->dev->mode_config.connector_list, head) {
+		if (tmp == connector)
+			return index;
+
+		index++;
+	}
+
+	BUG();
+}
+EXPORT_SYMBOL(drm_connector_index);
+
+/**
  * drm_connector_register - register a connector
  * @connector: the connector to register
  *
@@ -1239,6 +1262,29 @@ void drm_plane_cleanup(struct drm_plane *plane)
 EXPORT_SYMBOL(drm_plane_cleanup);
 
 /**
+ * drm_plane_index - find the index of a registered CRTC
+ * @plane: CRTC to find index for
+ *
+ * Given a registered CRTC, return the index of that CRTC within a DRM
+ * device's list of CRTCs.
+ */
+unsigned int drm_plane_index(struct drm_plane *plane)
+{
+	unsigned int index = 0;
+	struct drm_plane *tmp;
+
+	list_for_each_entry(tmp, &plane->dev->mode_config.plane_list, head) {
+		if (tmp == plane)
+			return index;
+
+		index++;
+	}
+
+	BUG();
+}
+EXPORT_SYMBOL(drm_plane_index);
+
+/**
  * drm_plane_force_disable - Forcibly disable a plane
  * @plane: plane to disable
  *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f1105d0da059..4cae44611ab0 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -903,6 +903,7 @@ int drm_connector_register(struct drm_connector *connector);
 void drm_connector_unregister(struct drm_connector *connector);
 
 extern void drm_connector_cleanup(struct drm_connector *connector);
+extern unsigned int drm_connector_index(struct drm_connector *crtc);
 /* helper to unplug all connectors from sysfs for device */
 extern void drm_connector_unplug_all(struct drm_device *dev);
 
@@ -942,6 +943,7 @@ extern int drm_plane_init(struct drm_device *dev,
 			  const uint32_t *formats, uint32_t format_count,
 			  bool is_primary);
 extern void drm_plane_cleanup(struct drm_plane *plane);
+extern unsigned int drm_plane_index(struct drm_plane *crtc);
 extern void drm_plane_force_disable(struct drm_plane *plane);
 extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 				   int x, int y,
-- 
2.0.1

Re: [Intel-gfx] [PATCH 1/8] drm: Add drm_plane/connector_index

[Matt Roper]

On Tue, Jul 29, 2014 at 11:32:16PM +0200, Daniel Vetter wrote:
> In the atomic state we'll have an array of states for crtcs, planes
> and connectors and need to be able to at them by their index. We
> already have a drm_crtc_index function so add the missing ones for
> planes and connectors.
> 
> If it later on turns out that the list walking is too expensive we can
> add the index to the relevant modeset objects.
> 
> Rob Clark doesn't like the loops too much, but we can always add an
> obj->idx parameter later on. And for now reiterating is actually safer
> since nowadays we have hotpluggable connectors (thanks to DP MST).
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_crtc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     |  2 ++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 805240b11229..5a494caa8c9a 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -937,6 +937,29 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  EXPORT_SYMBOL(drm_connector_cleanup);
>  
>  /**
> + * drm_plane_index - find the index of a registered CRTC

Looks like some copy/paste that needs an update...your kerneldoc calls
the function drm_*PLANE*_index and then talks about CRTC's, but the
actual function is for connectors...

> + * @plane: CRTC to find index for
> + *
> + * Given a registered CRTC, return the index of that CRTC within a DRM
> + * device's list of CRTCs.
> + */
> +unsigned int drm_connector_index(struct drm_connector *connector)
> +{
> +	unsigned int index = 0;
> +	struct drm_connector *tmp;
> +
> +	list_for_each_entry(tmp, &connector->dev->mode_config.connector_list, head) {
> +		if (tmp == connector)
> +			return index;
> +
> +		index++;
> +	}
> +
> +	BUG();
> +}
> +EXPORT_SYMBOL(drm_connector_index);
> +
> +/**
>   * drm_connector_register - register a connector
>   * @connector: the connector to register
>   *
> @@ -1239,6 +1262,29 @@ void drm_plane_cleanup(struct drm_plane *plane)
>  EXPORT_SYMBOL(drm_plane_cleanup);
>  
>  /**
> + * drm_plane_index - find the index of a registered CRTC
> + * @plane: CRTC to find index for
> + *
> + * Given a registered CRTC, return the index of that CRTC within a DRM
> + * device's list of CRTCs.
> + */

More copy/paste referenecs to CRTC's.

> +unsigned int drm_plane_index(struct drm_plane *plane)
> +{
> +	unsigned int index = 0;
> +	struct drm_plane *tmp;
> +
> +	list_for_each_entry(tmp, &plane->dev->mode_config.plane_list, head) {
> +		if (tmp == plane)
> +			return index;
> +
> +		index++;
> +	}
> +
> +	BUG();
> +}
> +EXPORT_SYMBOL(drm_plane_index);
> +
> +/**
>   * drm_plane_force_disable - Forcibly disable a plane
>   * @plane: plane to disable
>   *
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f1105d0da059..4cae44611ab0 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -903,6 +903,7 @@ int drm_connector_register(struct drm_connector *connector);
>  void drm_connector_unregister(struct drm_connector *connector);
>  
>  extern void drm_connector_cleanup(struct drm_connector *connector);
> +extern unsigned int drm_connector_index(struct drm_connector *crtc);
                                                                 ^^^^
                                                                 connector?
>  /* helper to unplug all connectors from sysfs for device */
>  extern void drm_connector_unplug_all(struct drm_device *dev);
>  
> @@ -942,6 +943,7 @@ extern int drm_plane_init(struct drm_device *dev,
>  			  const uint32_t *formats, uint32_t format_count,
>  			  bool is_primary);
>  extern void drm_plane_cleanup(struct drm_plane *plane);
> +extern unsigned int drm_plane_index(struct drm_plane *crtc);
                                                         ^^^^
                                                         plane?

>  extern void drm_plane_force_disable(struct drm_plane *plane);
>  extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  				   int x, int y,
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

[PATCH] drm: Add drm_plane/connector_index

[Daniel Vetter]

In the atomic state we'll have an array of states for crtcs, planes
and connectors and need to be able to at them by their index. We
already have a drm_crtc_index function so add the missing ones for
planes and connectors.

If it later on turns out that the list walking is too expensive we can
add the index to the relevant modeset objects.

Rob Clark doesn't like the loops too much, but we can always add an
obj->idx parameter later on. And for now reiterating is actually safer
since nowadays we have hotpluggable connectors (thanks to DP MST).

v2: Fix embarrassing copypasta fail in kerneldoc and header
declarations, spotted by Matt Roper.

Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     |  2 ++
 2 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 805240b11229..ff1f2f46ef1d 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -937,6 +937,29 @@ void drm_connector_cleanup(struct drm_connector *connector)
 EXPORT_SYMBOL(drm_connector_cleanup);
 
 /**
+ * drm_connector_index - find the index of a registered connector
+ * @connector: connector to find index for
+ *
+ * Given a registered connector, return the index of that connector within a DRM
+ * device's list of connectors.
+ */
+unsigned int drm_connector_index(struct drm_connector *connector)
+{
+	unsigned int index = 0;
+	struct drm_connector *tmp;
+
+	list_for_each_entry(tmp, &connector->dev->mode_config.connector_list, head) {
+		if (tmp == connector)
+			return index;
+
+		index++;
+	}
+
+	BUG();
+}
+EXPORT_SYMBOL(drm_connector_index);
+
+/**
  * drm_connector_register - register a connector
  * @connector: the connector to register
  *
@@ -1239,6 +1262,29 @@ void drm_plane_cleanup(struct drm_plane *plane)
 EXPORT_SYMBOL(drm_plane_cleanup);
 
 /**
+ * drm_plane_index - find the index of a registered plane
+ * @plane: plane to find index for
+ *
+ * Given a registered plane, return the index of that CRTC within a DRM
+ * device's list of planes.
+ */
+unsigned int drm_plane_index(struct drm_plane *plane)
+{
+	unsigned int index = 0;
+	struct drm_plane *tmp;
+
+	list_for_each_entry(tmp, &plane->dev->mode_config.plane_list, head) {
+		if (tmp == plane)
+			return index;
+
+		index++;
+	}
+
+	BUG();
+}
+EXPORT_SYMBOL(drm_plane_index);
+
+/**
  * drm_plane_force_disable - Forcibly disable a plane
  * @plane: plane to disable
  *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f1105d0da059..3bd60313250c 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -903,6 +903,7 @@ int drm_connector_register(struct drm_connector *connector);
 void drm_connector_unregister(struct drm_connector *connector);
 
 extern void drm_connector_cleanup(struct drm_connector *connector);
+extern unsigned int drm_connector_index(struct drm_connector *connector);
 /* helper to unplug all connectors from sysfs for device */
 extern void drm_connector_unplug_all(struct drm_device *dev);
 
@@ -942,6 +943,7 @@ extern int drm_plane_init(struct drm_device *dev,
 			  const uint32_t *formats, uint32_t format_count,
 			  bool is_primary);
 extern void drm_plane_cleanup(struct drm_plane *plane);
+extern unsigned int drm_plane_index(struct drm_plane *plane);
 extern void drm_plane_force_disable(struct drm_plane *plane);
 extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 				   int x, int y,
-- 
2.0.1

[PATCH 2/8] drm: Move modeset_lock_all helpers to drm_modeset_lock.[hc]

[Daniel Vetter]

Somehow we've forgotten about this little bit of OCD.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c         | 95 --------------------------------------
 drivers/gpu/drm/drm_modeset_lock.c | 95 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h             |  4 --
 include/drm/drm_modeset_lock.h     |  5 ++
 4 files changed, 100 insertions(+), 99 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5a494caa8c9a..ff583bec31f9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -45,101 +45,6 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
 							struct drm_mode_fb_cmd2 *r,
 							struct drm_file *file_priv);
 
-/**
- * drm_modeset_lock_all - take all modeset locks
- * @dev: drm device
- *
- * This function takes all modeset locks, suitable where a more fine-grained
- * scheme isn't (yet) implemented. Locks must be dropped with
- * drm_modeset_unlock_all.
- */
-void drm_modeset_lock_all(struct drm_device *dev)
-{
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx;
-	int ret;
-
-	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
-	if (WARN_ON(!ctx))
-		return;
-
-	mutex_lock(&config->mutex);
-
-	drm_modeset_acquire_init(ctx, 0);
-
-retry:
-	ret = drm_modeset_lock(&config->connection_mutex, ctx);
-	if (ret)
-		goto fail;
-	ret = drm_modeset_lock_all_crtcs(dev, ctx);
-	if (ret)
-		goto fail;
-
-	WARN_ON(config->acquire_ctx);
-
-	/* now we hold the locks, so now that it is safe, stash the
-	 * ctx for drm_modeset_unlock_all():
-	 */
-	config->acquire_ctx = ctx;
-
-	drm_warn_on_modeset_not_all_locked(dev);
-
-	return;
-
-fail:
-	if (ret == -EDEADLK) {
-		drm_modeset_backoff(ctx);
-		goto retry;
-	}
-}
-EXPORT_SYMBOL(drm_modeset_lock_all);
-
-/**
- * drm_modeset_unlock_all - drop all modeset locks
- * @dev: device
- *
- * This function drop all modeset locks taken by drm_modeset_lock_all.
- */
-void drm_modeset_unlock_all(struct drm_device *dev)
-{
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
-
-	if (WARN_ON(!ctx))
-		return;
-
-	config->acquire_ctx = NULL;
-	drm_modeset_drop_locks(ctx);
-	drm_modeset_acquire_fini(ctx);
-
-	kfree(ctx);
-
-	mutex_unlock(&dev->mode_config.mutex);
-}
-EXPORT_SYMBOL(drm_modeset_unlock_all);
-
-/**
- * drm_warn_on_modeset_not_all_locked - check that all modeset locks are locked
- * @dev: device
- *
- * Useful as a debug assert.
- */
-void drm_warn_on_modeset_not_all_locked(struct drm_device *dev)
-{
-	struct drm_crtc *crtc;
-
-	/* Locking is currently fubar in the panic handler. */
-	if (oops_in_progress)
-		return;
-
-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
-		WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
-
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
-	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
-}
-EXPORT_SYMBOL(drm_warn_on_modeset_not_all_locked);
-
 /* Avoid boilerplate.  I'm tired of typing. */
 #define DRM_ENUM_NAME_FN(fnname, list)				\
 	const char *fnname(int val)				\
diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
index 0dc57d5ecd10..73e6534fd0aa 100644
--- a/drivers/gpu/drm/drm_modeset_lock.c
+++ b/drivers/gpu/drm/drm_modeset_lock.c
@@ -57,6 +57,101 @@
 
 
 /**
+ * drm_modeset_lock_all - take all modeset locks
+ * @dev: drm device
+ *
+ * This function takes all modeset locks, suitable where a more fine-grained
+ * scheme isn't (yet) implemented. Locks must be dropped with
+ * drm_modeset_unlock_all.
+ */
+void drm_modeset_lock_all(struct drm_device *dev)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx;
+	int ret;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (WARN_ON(!ctx))
+		return;
+
+	mutex_lock(&config->mutex);
+
+	drm_modeset_acquire_init(ctx, 0);
+
+retry:
+	ret = drm_modeset_lock(&config->connection_mutex, ctx);
+	if (ret)
+		goto fail;
+	ret = drm_modeset_lock_all_crtcs(dev, ctx);
+	if (ret)
+		goto fail;
+
+	WARN_ON(config->acquire_ctx);
+
+	/* now we hold the locks, so now that it is safe, stash the
+	 * ctx for drm_modeset_unlock_all():
+	 */
+	config->acquire_ctx = ctx;
+
+	drm_warn_on_modeset_not_all_locked(dev);
+
+	return;
+
+fail:
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(ctx);
+		goto retry;
+	}
+}
+EXPORT_SYMBOL(drm_modeset_lock_all);
+
+/**
+ * drm_modeset_unlock_all - drop all modeset locks
+ * @dev: device
+ *
+ * This function drop all modeset locks taken by drm_modeset_lock_all.
+ */
+void drm_modeset_unlock_all(struct drm_device *dev)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
+
+	if (WARN_ON(!ctx))
+		return;
+
+	config->acquire_ctx = NULL;
+	drm_modeset_drop_locks(ctx);
+	drm_modeset_acquire_fini(ctx);
+
+	kfree(ctx);
+
+	mutex_unlock(&dev->mode_config.mutex);
+}
+EXPORT_SYMBOL(drm_modeset_unlock_all);
+
+/**
+ * drm_warn_on_modeset_not_all_locked - check that all modeset locks are locked
+ * @dev: device
+ *
+ * Useful as a debug assert.
+ */
+void drm_warn_on_modeset_not_all_locked(struct drm_device *dev)
+{
+	struct drm_crtc *crtc;
+
+	/* Locking is currently fubar in the panic handler. */
+	if (oops_in_progress)
+		return;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+}
+EXPORT_SYMBOL(drm_warn_on_modeset_not_all_locked);
+
+/**
  * drm_modeset_acquire_init - initialize acquire context
  * @ctx: the acquire context
  * @flags: for future
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 4cae44611ab0..0740cf03c1b1 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -218,10 +218,6 @@ struct drm_property {
 	struct list_head enum_blob_list;
 };
 
-void drm_modeset_lock_all(struct drm_device *dev);
-void drm_modeset_unlock_all(struct drm_device *dev);
-void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
-
 struct drm_crtc;
 struct drm_connector;
 struct drm_encoder;
diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
index 402aa7a6a058..cf61e857bc06 100644
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -120,6 +120,11 @@ int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock,
 void drm_modeset_unlock(struct drm_modeset_lock *lock);
 
 struct drm_device;
+
+void drm_modeset_lock_all(struct drm_device *dev);
+void drm_modeset_unlock_all(struct drm_device *dev);
+void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
+
 int drm_modeset_lock_all_crtcs(struct drm_device *dev,
 		struct drm_modeset_acquire_ctx *ctx);
 
-- 
2.0.1

Re: [PATCH 2/8] drm: Move modeset_lock_all helpers to drm_modeset_lock.[hc]

[Dave Airlie]

On 30 July 2014 07:32, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Somehow we've forgotten about this little bit of OCD.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Dave Airlie <airlied@redhat.com>

[PATCH 3/8] drm: Handle legacy per-crtc locking with full acquire ctx

[Daniel Vetter]

So drivers using the atomic interfaces expect that they can acquire
additional locks internal to the driver as-needed. Examples would be
locks to protect shared state like shared display PLLs.

Unfortunately the legacy ioctls assume that all locking is fully done
by the drm core. Now for those paths which grab all locks we already
have to keep around an acquire context in dev->mode_config. Helper
functions that implement legacy interfaces in terms of atomic support
can therefore grab this acquire contexts and reuse it.

The only interfaces left are the cursor and pageflip ioctls. So add
functions to grab the crtc lock these need using an acquire context
and preserve it for atomic drivers to reuse.

v2:
- Fixup comments&kerneldoc.
- Drop the WARNING from modeset_lock_all_crtcs since that can be used
  in legacy paths with crtc locking.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c         |  8 ++--
 drivers/gpu/drm/drm_modeset_lock.c | 84 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h             |  6 +++
 include/drm/drm_modeset_lock.h     |  5 +++
 4 files changed, 99 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index ff583bec31f9..c09374038f9a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2714,7 +2714,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 	if (crtc->cursor)
 		return drm_mode_cursor_universal(crtc, req, file_priv);
 
-	drm_modeset_lock(&crtc->mutex, NULL);
+	drm_modeset_lock_crtc(crtc);
 	if (req->flags & DRM_MODE_CURSOR_BO) {
 		if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
 			ret = -ENXIO;
@@ -2738,7 +2738,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 		}
 	}
 out:
-	drm_modeset_unlock(&crtc->mutex);
+	drm_modeset_unlock_crtc(crtc);
 
 	return ret;
 
@@ -4474,7 +4474,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (!crtc)
 		return -ENOENT;
 
-	drm_modeset_lock(&crtc->mutex, NULL);
+	drm_modeset_lock_crtc(crtc);
 	if (crtc->primary->fb == NULL) {
 		/* The framebuffer is currently unbound, presumably
 		 * due to a hotplug event, that userspace has not
@@ -4558,7 +4558,7 @@ out:
 		drm_framebuffer_unreference(fb);
 	if (old_fb)
 		drm_framebuffer_unreference(old_fb);
-	drm_modeset_unlock(&crtc->mutex);
+	drm_modeset_unlock_crtc(crtc);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
index 73e6534fd0aa..4d2aa549c614 100644
--- a/drivers/gpu/drm/drm_modeset_lock.c
+++ b/drivers/gpu/drm/drm_modeset_lock.c
@@ -130,6 +130,90 @@ void drm_modeset_unlock_all(struct drm_device *dev)
 EXPORT_SYMBOL(drm_modeset_unlock_all);
 
 /**
+ * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx
+ * @crtc: drm crtc
+ *
+ * This function locks the given crtc using a hidden acquire context. This is
+ * necessary so that drivers internally using the atomic interfaces can grab
+ * furether locks with the lock acquire context.
+ */
+void drm_modeset_lock_crtc(struct drm_crtc *crtc)
+{
+	struct drm_modeset_acquire_ctx *ctx;
+	int ret;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (WARN_ON(!ctx))
+		return;
+
+	drm_modeset_acquire_init(ctx, 0);
+
+retry:
+	ret = drm_modeset_lock(&crtc->mutex, ctx);
+	if (ret)
+		goto fail;
+
+	WARN_ON(crtc->acquire_ctx);
+
+	/* now we hold the locks, so now that it is safe, stash the
+	 * ctx for drm_modeset_unlock_crtc():
+	 */
+	crtc->acquire_ctx = ctx;
+
+	return;
+
+fail:
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(ctx);
+		goto retry;
+	}
+}
+EXPORT_SYMBOL(drm_modeset_lock_crtc);
+
+/**
+ * drm_modeset_legacy_acquire_ctx - find acquire ctx for legacy ioctls
+ * crtc: drm crtc
+ *
+ * Legacy ioctl operations like cursor updates or page flips only have per-crtc
+ * locking, and store the acquire ctx in the corresponding crtc. All other
+ * legacy operations take all locks and use a global acquire context. This
+ * function grabs the right one.
+ */
+struct drm_modeset_acquire_ctx *
+drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc)
+{
+	if (crtc->acquire_ctx)
+		return crtc->acquire_ctx;
+
+	WARN_ON(!crtc->dev->mode_config.acquire_ctx);
+
+	return crtc->dev->mode_config.acquire_ctx;
+}
+EXPORT_SYMBOL(drm_modeset_legacy_acquire_ctx);
+
+/**
+ * drm_modeset_unlock_crtc - drop crtc lock
+ * @crtc: drm crtc
+ *
+ * This drops the crtc lock acquire with drm_modeset_lock_crtc() and all other
+ * locks acquired through the hidden context.
+ */
+void drm_modeset_unlock_crtc(struct drm_crtc *crtc)
+{
+	struct drm_modeset_acquire_ctx *ctx = crtc->acquire_ctx;
+
+	if (WARN_ON(!ctx))
+		return;
+
+	crtc->acquire_ctx = NULL;
+	drm_modeset_drop_locks(ctx);
+	drm_modeset_acquire_fini(ctx);
+
+	kfree(ctx);
+}
+EXPORT_SYMBOL(drm_modeset_unlock_crtc);
+
+/**
  * drm_warn_on_modeset_not_all_locked - check that all modeset locks are locked
  * @dev: device
  *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 0740cf03c1b1..b0e30c5526ce 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -371,6 +371,12 @@ struct drm_crtc {
 	void *helper_private;
 
 	struct drm_object_properties properties;
+
+	/*
+	 * For legacy crtc ioctls so that atomic drivers can get at the locking
+	 * acquire context.
+	 */
+	struct drm_modeset_acquire_ctx *acquire_ctx;
 };
 
 
diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
index cf61e857bc06..d38e1508f11a 100644
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -120,10 +120,15 @@ int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock,
 void drm_modeset_unlock(struct drm_modeset_lock *lock);
 
 struct drm_device;
+struct drm_crtc;
 
 void drm_modeset_lock_all(struct drm_device *dev);
 void drm_modeset_unlock_all(struct drm_device *dev);
+void drm_modeset_lock_crtc(struct drm_crtc *crtc);
+void drm_modeset_unlock_crtc(struct drm_crtc *crtc);
 void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
+struct drm_modeset_acquire_ctx *
+drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc);
 
 int drm_modeset_lock_all_crtcs(struct drm_device *dev,
 		struct drm_modeset_acquire_ctx *ctx);
-- 
2.0.1

Re: [PATCH 3/8] drm: Handle legacy per-crtc locking with full acquire ctx

[Dave Airlie]

> ---
>  drivers/gpu/drm/drm_crtc.c         |  8 ++--
>  drivers/gpu/drm/drm_modeset_lock.c | 84 ++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h             |  6 +++
>  include/drm/drm_modeset_lock.h     |  5 +++
>  4 files changed, 99 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index ff583bec31f9..c09374038f9a 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2714,7 +2714,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
>         if (crtc->cursor)
>                 return drm_mode_cursor_universal(crtc, req, file_priv);
>
> -       drm_modeset_lock(&crtc->mutex, NULL);
> +       drm_modeset_lock_crtc(crtc);
>         if (req->flags & DRM_MODE_CURSOR_BO) {
>                 if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
>                         ret = -ENXIO;
> @@ -2738,7 +2738,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
>                 }
>         }
>  out:
> -       drm_modeset_unlock(&crtc->mutex);
> +       drm_modeset_unlock_crtc(crtc);
>
>         return ret;
>
> @@ -4474,7 +4474,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>         if (!crtc)
>                 return -ENOENT;
>
> -       drm_modeset_lock(&crtc->mutex, NULL);
> +       drm_modeset_lock_crtc(crtc);
>         if (crtc->primary->fb == NULL) {
>                 /* The framebuffer is currently unbound, presumably
>                  * due to a hotplug event, that userspace has not
> @@ -4558,7 +4558,7 @@ out:
>                 drm_framebuffer_unreference(fb);
>         if (old_fb)
>                 drm_framebuffer_unreference(old_fb);
> -       drm_modeset_unlock(&crtc->mutex);
> +       drm_modeset_unlock_crtc(crtc);
>
>         return ret;
>  }
> diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
> index 73e6534fd0aa..4d2aa549c614 100644
> --- a/drivers/gpu/drm/drm_modeset_lock.c
> +++ b/drivers/gpu/drm/drm_modeset_lock.c
> @@ -130,6 +130,90 @@ void drm_modeset_unlock_all(struct drm_device *dev)
>  EXPORT_SYMBOL(drm_modeset_unlock_all);
>
>  /**
> + * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx
> + * @crtc: drm crtc
> + *
> + * This function locks the given crtc using a hidden acquire context. This is
> + * necessary so that drivers internally using the atomic interfaces can grab
> + * furether locks with the lock acquire context.

^ typo - further

Otherwise

Reviewed-by: Dave Airlie <airlied@redhat.com>

[PATCH] drm: Handle legacy per-crtc locking with full acquire ctx

[Daniel Vetter]

So drivers using the atomic interfaces expect that they can acquire
additional locks internal to the driver as-needed. Examples would be
locks to protect shared state like shared display PLLs.

Unfortunately the legacy ioctls assume that all locking is fully done
by the drm core. Now for those paths which grab all locks we already
have to keep around an acquire context in dev->mode_config. Helper
functions that implement legacy interfaces in terms of atomic support
can therefore grab this acquire contexts and reuse it.

The only interfaces left are the cursor and pageflip ioctls. So add
functions to grab the crtc lock these need using an acquire context
and preserve it for atomic drivers to reuse.

v2:
- Fixup comments&kerneldoc.
- Drop the WARNING from modeset_lock_all_crtcs since that can be used
  in legacy paths with crtc locking.

v3: Fix a type on the kerneldoc Dave spotted.

Cc: Dave Airlie <airlied@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c         |  8 ++--
 drivers/gpu/drm/drm_modeset_lock.c | 84 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h             |  6 +++
 include/drm/drm_modeset_lock.h     |  5 +++
 4 files changed, 99 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d90374e6a8cb..cb741cd8bfe9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2714,7 +2714,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 	if (crtc->cursor)
 		return drm_mode_cursor_universal(crtc, req, file_priv);
 
-	drm_modeset_lock(&crtc->mutex, NULL);
+	drm_modeset_lock_crtc(crtc);
 	if (req->flags & DRM_MODE_CURSOR_BO) {
 		if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
 			ret = -ENXIO;
@@ -2738,7 +2738,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 		}
 	}
 out:
-	drm_modeset_unlock(&crtc->mutex);
+	drm_modeset_unlock_crtc(crtc);
 
 	return ret;
 
@@ -4474,7 +4474,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (!crtc)
 		return -ENOENT;
 
-	drm_modeset_lock(&crtc->mutex, NULL);
+	drm_modeset_lock_crtc(crtc);
 	if (crtc->primary->fb == NULL) {
 		/* The framebuffer is currently unbound, presumably
 		 * due to a hotplug event, that userspace has not
@@ -4558,7 +4558,7 @@ out:
 		drm_framebuffer_unreference(fb);
 	if (old_fb)
 		drm_framebuffer_unreference(old_fb);
-	drm_modeset_unlock(&crtc->mutex);
+	drm_modeset_unlock_crtc(crtc);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
index 73e6534fd0aa..4753c8bd5ab5 100644
--- a/drivers/gpu/drm/drm_modeset_lock.c
+++ b/drivers/gpu/drm/drm_modeset_lock.c
@@ -130,6 +130,90 @@ void drm_modeset_unlock_all(struct drm_device *dev)
 EXPORT_SYMBOL(drm_modeset_unlock_all);
 
 /**
+ * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx
+ * @crtc: drm crtc
+ *
+ * This function locks the given crtc using a hidden acquire context. This is
+ * necessary so that drivers internally using the atomic interfaces can grab
+ * further locks with the lock acquire context.
+ */
+void drm_modeset_lock_crtc(struct drm_crtc *crtc)
+{
+	struct drm_modeset_acquire_ctx *ctx;
+	int ret;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (WARN_ON(!ctx))
+		return;
+
+	drm_modeset_acquire_init(ctx, 0);
+
+retry:
+	ret = drm_modeset_lock(&crtc->mutex, ctx);
+	if (ret)
+		goto fail;
+
+	WARN_ON(crtc->acquire_ctx);
+
+	/* now we hold the locks, so now that it is safe, stash the
+	 * ctx for drm_modeset_unlock_crtc():
+	 */
+	crtc->acquire_ctx = ctx;
+
+	return;
+
+fail:
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(ctx);
+		goto retry;
+	}
+}
+EXPORT_SYMBOL(drm_modeset_lock_crtc);
+
+/**
+ * drm_modeset_legacy_acquire_ctx - find acquire ctx for legacy ioctls
+ * crtc: drm crtc
+ *
+ * Legacy ioctl operations like cursor updates or page flips only have per-crtc
+ * locking, and store the acquire ctx in the corresponding crtc. All other
+ * legacy operations take all locks and use a global acquire context. This
+ * function grabs the right one.
+ */
+struct drm_modeset_acquire_ctx *
+drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc)
+{
+	if (crtc->acquire_ctx)
+		return crtc->acquire_ctx;
+
+	WARN_ON(!crtc->dev->mode_config.acquire_ctx);
+
+	return crtc->dev->mode_config.acquire_ctx;
+}
+EXPORT_SYMBOL(drm_modeset_legacy_acquire_ctx);
+
+/**
+ * drm_modeset_unlock_crtc - drop crtc lock
+ * @crtc: drm crtc
+ *
+ * This drops the crtc lock acquire with drm_modeset_lock_crtc() and all other
+ * locks acquired through the hidden context.
+ */
+void drm_modeset_unlock_crtc(struct drm_crtc *crtc)
+{
+	struct drm_modeset_acquire_ctx *ctx = crtc->acquire_ctx;
+
+	if (WARN_ON(!ctx))
+		return;
+
+	crtc->acquire_ctx = NULL;
+	drm_modeset_drop_locks(ctx);
+	drm_modeset_acquire_fini(ctx);
+
+	kfree(ctx);
+}
+EXPORT_SYMBOL(drm_modeset_unlock_crtc);
+
+/**
  * drm_warn_on_modeset_not_all_locked - check that all modeset locks are locked
  * @dev: device
  *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 4521c59d15fc..8350afe96fa9 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -371,6 +371,12 @@ struct drm_crtc {
 	void *helper_private;
 
 	struct drm_object_properties properties;
+
+	/*
+	 * For legacy crtc ioctls so that atomic drivers can get at the locking
+	 * acquire context.
+	 */
+	struct drm_modeset_acquire_ctx *acquire_ctx;
 };
 
 
diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
index cf61e857bc06..d38e1508f11a 100644
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -120,10 +120,15 @@ int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock,
 void drm_modeset_unlock(struct drm_modeset_lock *lock);
 
 struct drm_device;
+struct drm_crtc;
 
 void drm_modeset_lock_all(struct drm_device *dev);
 void drm_modeset_unlock_all(struct drm_device *dev);
+void drm_modeset_lock_crtc(struct drm_crtc *crtc);
+void drm_modeset_unlock_crtc(struct drm_crtc *crtc);
 void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
+struct drm_modeset_acquire_ctx *
+drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc);
 
 int drm_modeset_lock_all_crtcs(struct drm_device *dev,
 		struct drm_modeset_acquire_ctx *ctx);
-- 
2.0.1

[PATCH 4/8] drm: Move ->old_fb from crtc to plane

[Daniel Vetter]

Atomic implemenations for legacy ioctls must be able to drop locks.
Which doesn't cause havoc since we only do that while constructing
the new state, so no driver or hardware state change has happened.

The only troubling bit is the fb refcounting the core does - if
someone else has snuck in then it might potentially unref an
outdated framebuffer. To fix that move the old_fb temporary storage
into struct drm_plane for all ioctls, so that the atomic helpers can
update it.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 40 ++++++++++++++++++++++++----------------
 include/drm/drm_crtc.h     |  8 ++++----
 2 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index c09374038f9a..bacf565449d5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1200,19 +1200,21 @@ EXPORT_SYMBOL(drm_plane_index);
  */
 void drm_plane_force_disable(struct drm_plane *plane)
 {
-	struct drm_framebuffer *old_fb = plane->fb;
 	int ret;
 
-	if (!old_fb)
+	if (!plane->fb)
 		return;
 
+	plane->old_fb = plane->fb;
 	ret = plane->funcs->disable_plane(plane);
 	if (ret) {
 		DRM_ERROR("failed to disable plane with busy fb\n");
+		plane->old_fb = NULL;
 		return;
 	}
 	/* disconnect the plane from the fb and crtc: */
-	__drm_framebuffer_unreference(old_fb);
+	__drm_framebuffer_unreference(plane->old_fb);
+	plane->old_fb = NULL;
 	plane->fb = NULL;
 	plane->crtc = NULL;
 }
@@ -2188,7 +2190,7 @@ static int setplane_internal(struct drm_plane *plane,
 			     uint32_t src_w, uint32_t src_h)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_framebuffer *old_fb = NULL;
+	struct drm_framebuffer *old_fb;
 	int ret = 0;
 	unsigned int fb_width, fb_height;
 	int i;
@@ -2196,14 +2198,16 @@ static int setplane_internal(struct drm_plane *plane,
 	/* No fb means shut it down */
 	if (!fb) {
 		drm_modeset_lock_all(dev);
-		old_fb = plane->fb;
+		plane->old_fb = plane->fb;
 		ret = plane->funcs->disable_plane(plane);
 		if (!ret) {
 			plane->crtc = NULL;
 			plane->fb = NULL;
 		} else {
-			old_fb = NULL;
+			plane->old_fb = NULL;
 		}
+		old_fb = plane->old_fb;
+		plane->old_fb = NULL;
 		drm_modeset_unlock_all(dev);
 		goto out;
 	}
@@ -2245,7 +2249,7 @@ static int setplane_internal(struct drm_plane *plane,
 	}
 
 	drm_modeset_lock_all(dev);
-	old_fb = plane->fb;
+	plane->old_fb = plane->fb;
 	ret = plane->funcs->update_plane(plane, crtc, fb,
 					 crtc_x, crtc_y, crtc_w, crtc_h,
 					 src_x, src_y, src_w, src_h);
@@ -2254,8 +2258,10 @@ static int setplane_internal(struct drm_plane *plane,
 		plane->fb = fb;
 		fb = NULL;
 	} else {
-		old_fb = NULL;
+		plane->old_fb = NULL;
 	}
+	old_fb = plane->old_fb;
+	plane->old_fb = NULL;
 	drm_modeset_unlock_all(dev);
 
 out:
@@ -2369,7 +2375,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
 	 * crtcs. Atomic modeset will have saner semantics ...
 	 */
 	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head)
-		tmp->old_fb = tmp->primary->fb;
+		tmp->primary->old_fb = tmp->primary->fb;
 
 	fb = set->fb;
 
@@ -2382,8 +2388,9 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
 	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) {
 		if (tmp->primary->fb)
 			drm_framebuffer_reference(tmp->primary->fb);
-		if (tmp->old_fb)
-			drm_framebuffer_unreference(tmp->old_fb);
+		if (tmp->primary->old_fb)
+			drm_framebuffer_unreference(tmp->primary->old_fb);
+		tmp->primary->old_fb = NULL;
 	}
 
 	return ret;
@@ -4458,7 +4465,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 {
 	struct drm_mode_crtc_page_flip *page_flip = data;
 	struct drm_crtc *crtc;
-	struct drm_framebuffer *fb = NULL, *old_fb = NULL;
+	struct drm_framebuffer *fb = NULL;
 	struct drm_pending_vblank_event *e = NULL;
 	unsigned long flags;
 	int ret = -EINVAL;
@@ -4530,7 +4537,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			(void (*) (struct drm_pending_event *)) kfree;
 	}
 
-	old_fb = crtc->primary->fb;
+	crtc->primary->old_fb = crtc->primary->fb;
 	ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
 	if (ret) {
 		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
@@ -4540,7 +4547,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			kfree(e);
 		}
 		/* Keep the old fb, don't unref it. */
-		old_fb = NULL;
+		crtc->primary->old_fb = NULL;
 	} else {
 		/*
 		 * Warn if the driver hasn't properly updated the crtc->fb
@@ -4556,8 +4563,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 out:
 	if (fb)
 		drm_framebuffer_unreference(fb);
-	if (old_fb)
-		drm_framebuffer_unreference(old_fb);
+	if (crtc->primary->old_fb)
+		drm_framebuffer_unreference(crtc->primary->old_fb);
+	crtc->primary->old_fb = NULL;
 	drm_modeset_unlock_crtc(crtc);
 
 	return ret;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index b0e30c5526ce..5fffb5c53ba6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -341,10 +341,6 @@ struct drm_crtc {
 	int cursor_x;
 	int cursor_y;
 
-	/* Temporary tracking of the old fb while a modeset is ongoing. Used
-	 * by drm_mode_set_config_internal to implement correct refcounting. */
-	struct drm_framebuffer *old_fb;
-
 	bool enabled;
 
 	/* Requested mode from modesetting. */
@@ -622,6 +618,10 @@ struct drm_plane {
 	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb;
 
+	/* Temporary tracking of the old fb while a modeset is ongoing. Used
+	 * by drm_mode_set_config_internal to implement correct refcounting. */
+	struct drm_framebuffer *old_fb;
+
 	const struct drm_plane_funcs *funcs;
 
 	struct drm_object_properties properties;
-- 
2.0.1

Re: [PATCH 4/8] drm: Move ->old_fb from crtc to plane

[Dave Airlie]

On 30 July 2014 07:32, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Atomic implemenations for legacy ioctls must be able to drop locks.
> Which doesn't cause havoc since we only do that while constructing
> the new state, so no driver or hardware state change has happened.
>
> The only troubling bit is the fb refcounting the core does - if
> someone else has snuck in then it might potentially unref an
> outdated framebuffer. To fix that move the old_fb temporary storage
> into struct drm_plane for all ioctls, so that the atomic helpers can
> update it.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Seems to make sense to me.

Reviewed-by: Dave Airlie <airlied@redhat.com>

Re: [Intel-gfx] [PATCH 4/8] drm: Move ->old_fb from crtc to plane

[Matt Roper]

On Tue, Jul 29, 2014 at 11:32:19PM +0200, Daniel Vetter wrote:
> Atomic implemenations for legacy ioctls must be able to drop locks.
> Which doesn't cause havoc since we only do that while constructing
> the new state, so no driver or hardware state change has happened.
> 
> The only troubling bit is the fb refcounting the core does - if
> someone else has snuck in then it might potentially unref an
> outdated framebuffer. To fix that move the old_fb temporary storage
> into struct drm_plane for all ioctls, so that the atomic helpers can
> update it.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_crtc.c | 40 ++++++++++++++++++++++++----------------
>  include/drm/drm_crtc.h     |  8 ++++----
>  2 files changed, 28 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index c09374038f9a..bacf565449d5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1200,19 +1200,21 @@ EXPORT_SYMBOL(drm_plane_index);
>   */
>  void drm_plane_force_disable(struct drm_plane *plane)
>  {
> -	struct drm_framebuffer *old_fb = plane->fb;
>  	int ret;
>  
> -	if (!old_fb)
> +	if (!plane->fb)
>  		return;
>  
> +	plane->old_fb = plane->fb;
>  	ret = plane->funcs->disable_plane(plane);
>  	if (ret) {
>  		DRM_ERROR("failed to disable plane with busy fb\n");
> +		plane->old_fb = NULL;
>  		return;
>  	}
>  	/* disconnect the plane from the fb and crtc: */
> -	__drm_framebuffer_unreference(old_fb);
> +	__drm_framebuffer_unreference(plane->old_fb);
> +	plane->old_fb = NULL;
>  	plane->fb = NULL;
>  	plane->crtc = NULL;
>  }
> @@ -2188,7 +2190,7 @@ static int setplane_internal(struct drm_plane *plane,
>  			     uint32_t src_w, uint32_t src_h)
>  {
>  	struct drm_device *dev = plane->dev;
> -	struct drm_framebuffer *old_fb = NULL;
> +	struct drm_framebuffer *old_fb;

I think there may be cases where old_fb gets unref'd without ever being
set if we drop the NULL assignment.  E.g., if the possible_crtcs test or
the format test fail, we jump down to out and then test the value +
unref which could be garbage.

Would it be simpler to just drm_modeset_lock_all() unconditionally at
the start of the function and then just unlock after the unrefs at the
end of the function so that we don't need a local old_fb?

>  	int ret = 0;
>  	unsigned int fb_width, fb_height;
>  	int i;
> @@ -2196,14 +2198,16 @@ static int setplane_internal(struct drm_plane *plane,
>  	/* No fb means shut it down */
>  	if (!fb) {
>  		drm_modeset_lock_all(dev);
> -		old_fb = plane->fb;
> +		plane->old_fb = plane->fb;
>  		ret = plane->funcs->disable_plane(plane);
>  		if (!ret) {
>  			plane->crtc = NULL;
>  			plane->fb = NULL;
>  		} else {
> -			old_fb = NULL;
> +			plane->old_fb = NULL;
>  		}
> +		old_fb = plane->old_fb;
> +		plane->old_fb = NULL;
>  		drm_modeset_unlock_all(dev);
>  		goto out;
>  	}
> @@ -2245,7 +2249,7 @@ static int setplane_internal(struct drm_plane *plane,
>  	}
>  
>  	drm_modeset_lock_all(dev);
> -	old_fb = plane->fb;
> +	plane->old_fb = plane->fb;
>  	ret = plane->funcs->update_plane(plane, crtc, fb,
>  					 crtc_x, crtc_y, crtc_w, crtc_h,
>  					 src_x, src_y, src_w, src_h);
> @@ -2254,8 +2258,10 @@ static int setplane_internal(struct drm_plane *plane,
>  		plane->fb = fb;
>  		fb = NULL;
>  	} else {
> -		old_fb = NULL;
> +		plane->old_fb = NULL;
>  	}
> +	old_fb = plane->old_fb;
> +	plane->old_fb = NULL;
>  	drm_modeset_unlock_all(dev);
>  
>  out:
> @@ -2369,7 +2375,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
>  	 * crtcs. Atomic modeset will have saner semantics ...
>  	 */
>  	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head)
> -		tmp->old_fb = tmp->primary->fb;
> +		tmp->primary->old_fb = tmp->primary->fb;
>  
>  	fb = set->fb;
>  
> @@ -2382,8 +2388,9 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
>  	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) {
>  		if (tmp->primary->fb)
>  			drm_framebuffer_reference(tmp->primary->fb);
> -		if (tmp->old_fb)
> -			drm_framebuffer_unreference(tmp->old_fb);
> +		if (tmp->primary->old_fb)
> +			drm_framebuffer_unreference(tmp->primary->old_fb);
> +		tmp->primary->old_fb = NULL;
>  	}
>  
>  	return ret;
> @@ -4458,7 +4465,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  {
>  	struct drm_mode_crtc_page_flip *page_flip = data;
>  	struct drm_crtc *crtc;
> -	struct drm_framebuffer *fb = NULL, *old_fb = NULL;
> +	struct drm_framebuffer *fb = NULL;
>  	struct drm_pending_vblank_event *e = NULL;
>  	unsigned long flags;
>  	int ret = -EINVAL;
> @@ -4530,7 +4537,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  			(void (*) (struct drm_pending_event *)) kfree;
>  	}
>  
> -	old_fb = crtc->primary->fb;
> +	crtc->primary->old_fb = crtc->primary->fb;
>  	ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
>  	if (ret) {
>  		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> @@ -4540,7 +4547,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  			kfree(e);
>  		}
>  		/* Keep the old fb, don't unref it. */
> -		old_fb = NULL;
> +		crtc->primary->old_fb = NULL;
>  	} else {
>  		/*
>  		 * Warn if the driver hasn't properly updated the crtc->fb
> @@ -4556,8 +4563,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  out:
>  	if (fb)
>  		drm_framebuffer_unreference(fb);
> -	if (old_fb)
> -		drm_framebuffer_unreference(old_fb);
> +	if (crtc->primary->old_fb)
> +		drm_framebuffer_unreference(crtc->primary->old_fb);
> +	crtc->primary->old_fb = NULL;
>  	drm_modeset_unlock_crtc(crtc);
>  
>  	return ret;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b0e30c5526ce..5fffb5c53ba6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -341,10 +341,6 @@ struct drm_crtc {
>  	int cursor_x;
>  	int cursor_y;
>  
> -	/* Temporary tracking of the old fb while a modeset is ongoing. Used
> -	 * by drm_mode_set_config_internal to implement correct refcounting. */
> -	struct drm_framebuffer *old_fb;
> -
>  	bool enabled;
>  
>  	/* Requested mode from modesetting. */
> @@ -622,6 +618,10 @@ struct drm_plane {
>  	struct drm_crtc *crtc;
>  	struct drm_framebuffer *fb;
>  
> +	/* Temporary tracking of the old fb while a modeset is ongoing. Used
> +	 * by drm_mode_set_config_internal to implement correct refcounting. */

Might want to update the wording of this comment slightly since it isn't
just for drm_mode_set_config_internal (or modesets) anymore.



Matt

> +	struct drm_framebuffer *old_fb;
> +
>  	const struct drm_plane_funcs *funcs;
>  
>  	struct drm_object_properties properties;
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 4/8] drm: Move ->old_fb from crtc to plane

[Daniel Vetter]

On Tue, Jul 29, 2014 at 04:46:11PM -0700, Matt Roper wrote:
> On Tue, Jul 29, 2014 at 11:32:19PM +0200, Daniel Vetter wrote:
> > Atomic implemenations for legacy ioctls must be able to drop locks.
> > Which doesn't cause havoc since we only do that while constructing
> > the new state, so no driver or hardware state change has happened.
> > 
> > The only troubling bit is the fb refcounting the core does - if
> > someone else has snuck in then it might potentially unref an
> > outdated framebuffer. To fix that move the old_fb temporary storage
> > into struct drm_plane for all ioctls, so that the atomic helpers can
> > update it.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 40 ++++++++++++++++++++++++----------------
> >  include/drm/drm_crtc.h     |  8 ++++----
> >  2 files changed, 28 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index c09374038f9a..bacf565449d5 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -1200,19 +1200,21 @@ EXPORT_SYMBOL(drm_plane_index);
> >   */
> >  void drm_plane_force_disable(struct drm_plane *plane)
> >  {
> > -	struct drm_framebuffer *old_fb = plane->fb;
> >  	int ret;
> >  
> > -	if (!old_fb)
> > +	if (!plane->fb)
> >  		return;
> >  
> > +	plane->old_fb = plane->fb;
> >  	ret = plane->funcs->disable_plane(plane);
> >  	if (ret) {
> >  		DRM_ERROR("failed to disable plane with busy fb\n");
> > +		plane->old_fb = NULL;
> >  		return;
> >  	}
> >  	/* disconnect the plane from the fb and crtc: */
> > -	__drm_framebuffer_unreference(old_fb);
> > +	__drm_framebuffer_unreference(plane->old_fb);
> > +	plane->old_fb = NULL;
> >  	plane->fb = NULL;
> >  	plane->crtc = NULL;
> >  }
> > @@ -2188,7 +2190,7 @@ static int setplane_internal(struct drm_plane *plane,
> >  			     uint32_t src_w, uint32_t src_h)
> >  {
> >  	struct drm_device *dev = plane->dev;
> > -	struct drm_framebuffer *old_fb = NULL;
> > +	struct drm_framebuffer *old_fb;
> 
> I think there may be cases where old_fb gets unref'd without ever being
> set if we drop the NULL assignment.  E.g., if the possible_crtcs test or
> the format test fail, we jump down to out and then test the value +
> unref which could be garbage.

Oops, totally missed that. And somehow also missed the gcc warning about
unitialized usage of old_fb - that one was the reason why I've dropped the
initializer. Looks like I've failed.

> Would it be simpler to just drm_modeset_lock_all() unconditionally at
> the start of the function and then just unlock after the unrefs at the
> end of the function so that we don't need a local old_fb?

Yeah considered that and since you're suggesting this too I'll do it.
Trying hard to not grab locks for the error case is fairly pointless
optimization.

> 
> >  	int ret = 0;
> >  	unsigned int fb_width, fb_height;
> >  	int i;
> > @@ -2196,14 +2198,16 @@ static int setplane_internal(struct drm_plane *plane,
> >  	/* No fb means shut it down */
> >  	if (!fb) {
> >  		drm_modeset_lock_all(dev);
> > -		old_fb = plane->fb;
> > +		plane->old_fb = plane->fb;
> >  		ret = plane->funcs->disable_plane(plane);
> >  		if (!ret) {
> >  			plane->crtc = NULL;
> >  			plane->fb = NULL;
> >  		} else {
> > -			old_fb = NULL;
> > +			plane->old_fb = NULL;
> >  		}
> > +		old_fb = plane->old_fb;
> > +		plane->old_fb = NULL;
> >  		drm_modeset_unlock_all(dev);
> >  		goto out;
> >  	}
> > @@ -2245,7 +2249,7 @@ static int setplane_internal(struct drm_plane *plane,
> >  	}
> >  
> >  	drm_modeset_lock_all(dev);
> > -	old_fb = plane->fb;
> > +	plane->old_fb = plane->fb;
> >  	ret = plane->funcs->update_plane(plane, crtc, fb,
> >  					 crtc_x, crtc_y, crtc_w, crtc_h,
> >  					 src_x, src_y, src_w, src_h);
> > @@ -2254,8 +2258,10 @@ static int setplane_internal(struct drm_plane *plane,
> >  		plane->fb = fb;
> >  		fb = NULL;
> >  	} else {
> > -		old_fb = NULL;
> > +		plane->old_fb = NULL;
> >  	}
> > +	old_fb = plane->old_fb;
> > +	plane->old_fb = NULL;
> >  	drm_modeset_unlock_all(dev);
> >  
> >  out:
> > @@ -2369,7 +2375,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
> >  	 * crtcs. Atomic modeset will have saner semantics ...
> >  	 */
> >  	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head)
> > -		tmp->old_fb = tmp->primary->fb;
> > +		tmp->primary->old_fb = tmp->primary->fb;
> >  
> >  	fb = set->fb;
> >  
> > @@ -2382,8 +2388,9 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
> >  	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) {
> >  		if (tmp->primary->fb)
> >  			drm_framebuffer_reference(tmp->primary->fb);
> > -		if (tmp->old_fb)
> > -			drm_framebuffer_unreference(tmp->old_fb);
> > +		if (tmp->primary->old_fb)
> > +			drm_framebuffer_unreference(tmp->primary->old_fb);
> > +		tmp->primary->old_fb = NULL;
> >  	}
> >  
> >  	return ret;
> > @@ -4458,7 +4465,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  {
> >  	struct drm_mode_crtc_page_flip *page_flip = data;
> >  	struct drm_crtc *crtc;
> > -	struct drm_framebuffer *fb = NULL, *old_fb = NULL;
> > +	struct drm_framebuffer *fb = NULL;
> >  	struct drm_pending_vblank_event *e = NULL;
> >  	unsigned long flags;
> >  	int ret = -EINVAL;
> > @@ -4530,7 +4537,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  			(void (*) (struct drm_pending_event *)) kfree;
> >  	}
> >  
> > -	old_fb = crtc->primary->fb;
> > +	crtc->primary->old_fb = crtc->primary->fb;
> >  	ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
> >  	if (ret) {
> >  		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> > @@ -4540,7 +4547,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  			kfree(e);
> >  		}
> >  		/* Keep the old fb, don't unref it. */
> > -		old_fb = NULL;
> > +		crtc->primary->old_fb = NULL;
> >  	} else {
> >  		/*
> >  		 * Warn if the driver hasn't properly updated the crtc->fb
> > @@ -4556,8 +4563,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  out:
> >  	if (fb)
> >  		drm_framebuffer_unreference(fb);
> > -	if (old_fb)
> > -		drm_framebuffer_unreference(old_fb);
> > +	if (crtc->primary->old_fb)
> > +		drm_framebuffer_unreference(crtc->primary->old_fb);
> > +	crtc->primary->old_fb = NULL;
> >  	drm_modeset_unlock_crtc(crtc);
> >  
> >  	return ret;
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index b0e30c5526ce..5fffb5c53ba6 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -341,10 +341,6 @@ struct drm_crtc {
> >  	int cursor_x;
> >  	int cursor_y;
> >  
> > -	/* Temporary tracking of the old fb while a modeset is ongoing. Used
> > -	 * by drm_mode_set_config_internal to implement correct refcounting. */
> > -	struct drm_framebuffer *old_fb;
> > -
> >  	bool enabled;
> >  
> >  	/* Requested mode from modesetting. */
> > @@ -622,6 +618,10 @@ struct drm_plane {
> >  	struct drm_crtc *crtc;
> >  	struct drm_framebuffer *fb;
> >  
> > +	/* Temporary tracking of the old fb while a modeset is ongoing. Used
> > +	 * by drm_mode_set_config_internal to implement correct refcounting. */
> 
> Might want to update the wording of this comment slightly since it isn't
> just for drm_mode_set_config_internal (or modesets) anymore.

Good idea, will augment.
-Daniel

> 
> 
> 
> Matt
> 
> > +	struct drm_framebuffer *old_fb;
> > +
> >  	const struct drm_plane_funcs *funcs;
> >  
> >  	struct drm_object_properties properties;
> > -- 
> > 2.0.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm: Move ->old_fb from crtc to plane

[Daniel Vetter]

Atomic implemenations for legacy ioctls must be able to drop locks.
Which doesn't cause havoc since we only do that while constructing
the new state, so no driver or hardware state change has happened.

The only troubling bit is the fb refcounting the core does - if
someone else has snuck in then it might potentially unref an
outdated framebuffer. To fix that move the old_fb temporary storage
into struct drm_plane for all ioctls, so that the atomic helpers can
update it.

v2: Fix up the error case handling as suggested by Matt Roper and just
grab locks uncoditionally - there's no point in optimizing the locking
for when userspace gets it wrong.

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 46 ++++++++++++++++++++++++----------------------
 include/drm/drm_crtc.h     |  8 ++++----
 2 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index cb741cd8bfe9..c8f9911bd238 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1200,19 +1200,21 @@ EXPORT_SYMBOL(drm_plane_index);
  */
 void drm_plane_force_disable(struct drm_plane *plane)
 {
-	struct drm_framebuffer *old_fb = plane->fb;
 	int ret;
 
-	if (!old_fb)
+	if (!plane->fb)
 		return;
 
+	plane->old_fb = plane->fb;
 	ret = plane->funcs->disable_plane(plane);
 	if (ret) {
 		DRM_ERROR("failed to disable plane with busy fb\n");
+		plane->old_fb = NULL;
 		return;
 	}
 	/* disconnect the plane from the fb and crtc: */
-	__drm_framebuffer_unreference(old_fb);
+	__drm_framebuffer_unreference(plane->old_fb);
+	plane->old_fb = NULL;
 	plane->fb = NULL;
 	plane->crtc = NULL;
 }
@@ -2188,23 +2190,21 @@ static int setplane_internal(struct drm_plane *plane,
 			     uint32_t src_w, uint32_t src_h)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_framebuffer *old_fb = NULL;
 	int ret = 0;
 	unsigned int fb_width, fb_height;
 	int i;
 
+	drm_modeset_lock_all(dev);
 	/* No fb means shut it down */
 	if (!fb) {
-		drm_modeset_lock_all(dev);
-		old_fb = plane->fb;
+		plane->old_fb = plane->fb;
 		ret = plane->funcs->disable_plane(plane);
 		if (!ret) {
 			plane->crtc = NULL;
 			plane->fb = NULL;
 		} else {
-			old_fb = NULL;
+			plane->old_fb = NULL;
 		}
-		drm_modeset_unlock_all(dev);
 		goto out;
 	}
 
@@ -2244,8 +2244,7 @@ static int setplane_internal(struct drm_plane *plane,
 		goto out;
 	}
 
-	drm_modeset_lock_all(dev);
-	old_fb = plane->fb;
+	plane->old_fb = plane->fb;
 	ret = plane->funcs->update_plane(plane, crtc, fb,
 					 crtc_x, crtc_y, crtc_w, crtc_h,
 					 src_x, src_y, src_w, src_h);
@@ -2254,15 +2253,16 @@ static int setplane_internal(struct drm_plane *plane,
 		plane->fb = fb;
 		fb = NULL;
 	} else {
-		old_fb = NULL;
+		plane->old_fb = NULL;
 	}
-	drm_modeset_unlock_all(dev);
 
 out:
 	if (fb)
 		drm_framebuffer_unreference(fb);
-	if (old_fb)
-		drm_framebuffer_unreference(old_fb);
+	if (plane->old_fb)
+		drm_framebuffer_unreference(plane->old_fb);
+	plane->old_fb = NULL;
+	drm_modeset_unlock_all(dev);
 
 	return ret;
 
@@ -2369,7 +2369,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
 	 * crtcs. Atomic modeset will have saner semantics ...
 	 */
 	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head)
-		tmp->old_fb = tmp->primary->fb;
+		tmp->primary->old_fb = tmp->primary->fb;
 
 	fb = set->fb;
 
@@ -2382,8 +2382,9 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
 	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) {
 		if (tmp->primary->fb)
 			drm_framebuffer_reference(tmp->primary->fb);
-		if (tmp->old_fb)
-			drm_framebuffer_unreference(tmp->old_fb);
+		if (tmp->primary->old_fb)
+			drm_framebuffer_unreference(tmp->primary->old_fb);
+		tmp->primary->old_fb = NULL;
 	}
 
 	return ret;
@@ -4458,7 +4459,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 {
 	struct drm_mode_crtc_page_flip *page_flip = data;
 	struct drm_crtc *crtc;
-	struct drm_framebuffer *fb = NULL, *old_fb = NULL;
+	struct drm_framebuffer *fb = NULL;
 	struct drm_pending_vblank_event *e = NULL;
 	unsigned long flags;
 	int ret = -EINVAL;
@@ -4530,7 +4531,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			(void (*) (struct drm_pending_event *)) kfree;
 	}
 
-	old_fb = crtc->primary->fb;
+	crtc->primary->old_fb = crtc->primary->fb;
 	ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
 	if (ret) {
 		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
@@ -4540,7 +4541,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			kfree(e);
 		}
 		/* Keep the old fb, don't unref it. */
-		old_fb = NULL;
+		crtc->primary->old_fb = NULL;
 	} else {
 		/*
 		 * Warn if the driver hasn't properly updated the crtc->fb
@@ -4556,8 +4557,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 out:
 	if (fb)
 		drm_framebuffer_unreference(fb);
-	if (old_fb)
-		drm_framebuffer_unreference(old_fb);
+	if (crtc->primary->old_fb)
+		drm_framebuffer_unreference(crtc->primary->old_fb);
+	crtc->primary->old_fb = NULL;
 	drm_modeset_unlock_crtc(crtc);
 
 	return ret;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8350afe96fa9..8b826ddb7ecb 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -341,10 +341,6 @@ struct drm_crtc {
 	int cursor_x;
 	int cursor_y;
 
-	/* Temporary tracking of the old fb while a modeset is ongoing. Used
-	 * by drm_mode_set_config_internal to implement correct refcounting. */
-	struct drm_framebuffer *old_fb;
-
 	bool enabled;
 
 	/* Requested mode from modesetting. */
@@ -622,6 +618,10 @@ struct drm_plane {
 	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb;
 
+	/* Temporary tracking of the old fb while a modeset is ongoing. Used
+	 * by drm_mode_set_config_internal to implement correct refcounting. */
+	struct drm_framebuffer *old_fb;
+
 	const struct drm_plane_funcs *funcs;
 
 	struct drm_object_properties properties;
-- 
2.0.1

[PATCH 5/8] drm: trylock modest locking for fbdev panics

[Daniel Vetter]

In the fbdev code we want to do trylocks only to avoid deadlocks and
other ugly issues. Thus far we've only grabbed the overall modeset
lock, but that already failed to exclude a pile of potential
concurrent operations. With proper atomic support this will be worse.

So add a trylock mode to the modeset locking code which attempts all
locks only with trylocks, if possible. We need to track this in the
locking functions themselves and can't restrict this to drivers since
driver-private w/w mutexes must be treated the same way.

There's still the issue that other driver private locks aren't handled
here at all, but well can't have everything. With this we will at
least not regress, even once atomic allows lots of concurrent kms
activity.

Aside: We should move the acquire context to stack-based allocation in
the callers to get rid of that awful WARN_ON(kmalloc_failed) control
flow which just blows up when memory is short. But that's material for
separate patches.

v2:
- Fix logic inversion fumble in the fb helper.
- Add proper kerneldoc.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c    | 10 +++----
 drivers/gpu/drm/drm_modeset_lock.c | 56 ++++++++++++++++++++++++++++++--------
 include/drm/drm_modeset_lock.h     |  6 ++++
 3 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 3144db9dc0f1..841e039ba028 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -419,11 +419,11 @@ static bool drm_fb_helper_force_kernel_mode(void)
 		if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 			continue;
 
-		/* NOTE: we use lockless flag below to avoid grabbing other
-		 * modeset locks.  So just trylock the underlying mutex
-		 * directly:
+		/*
+		 * NOTE: Use trylock mode to avoid deadlocks and sleeping in
+		 * panic context.
 		 */
-		if (!mutex_trylock(&dev->mode_config.mutex)) {
+		if (__drm_modeset_lock_all(dev, true) != 0) {
 			error = true;
 			continue;
 		}
@@ -432,7 +432,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
 		if (ret)
 			error = true;
 
-		mutex_unlock(&dev->mode_config.mutex);
+		drm_modeset_unlock_all(dev);
 	}
 	return error;
 }
diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
index 4d2aa549c614..acfe187609b0 100644
--- a/drivers/gpu/drm/drm_modeset_lock.c
+++ b/drivers/gpu/drm/drm_modeset_lock.c
@@ -57,26 +57,37 @@
 
 
 /**
- * drm_modeset_lock_all - take all modeset locks
- * @dev: drm device
+ * __drm_modeset_lock_all - internal helper to grab all modeset locks
+ * @dev: DRM device
+ * @trylock: trylock mode for atomic contexts
  *
- * This function takes all modeset locks, suitable where a more fine-grained
- * scheme isn't (yet) implemented. Locks must be dropped with
- * drm_modeset_unlock_all.
+ * This is a special version of drm_modeset_lock_all() which can also be used in
+ * atomic contexts. Then @trylock must be set to true.
+ *
+ * Returns:
+ * 0 on success or negative error code on failure.
  */
-void drm_modeset_lock_all(struct drm_device *dev)
+int __drm_modeset_lock_all(struct drm_device *dev,
+			   bool trylock)
 {
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_modeset_acquire_ctx *ctx;
 	int ret;
 
-	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
-	if (WARN_ON(!ctx))
-		return;
+	ctx = kzalloc(sizeof(*ctx),
+		      trylock ? GFP_ATOMIC : GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
 
-	mutex_lock(&config->mutex);
+	if (trylock) {
+		if (!mutex_trylock(&config->mutex))
+			return -EBUSY;
+	} else {
+		mutex_lock(&config->mutex);
+	}
 
 	drm_modeset_acquire_init(ctx, 0);
+	ctx->trylock_only = trylock;
 
 retry:
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
@@ -95,13 +106,29 @@ retry:
 
 	drm_warn_on_modeset_not_all_locked(dev);
 
-	return;
+	return 0;
 
 fail:
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(ctx);
 		goto retry;
 	}
+
+	return ret;
+}
+EXPORT_SYMBOL(__drm_modeset_lock_all);
+
+/**
+ * drm_modeset_lock_all - take all modeset locks
+ * @dev: drm device
+ *
+ * This function takes all modeset locks, suitable where a more fine-grained
+ * scheme isn't (yet) implemented. Locks must be dropped with
+ * drm_modeset_unlock_all.
+ */
+void drm_modeset_lock_all(struct drm_device *dev)
+{
+	WARN_ON(__drm_modeset_lock_all(dev, false) != 0);
 }
 EXPORT_SYMBOL(drm_modeset_lock_all);
 
@@ -287,7 +314,12 @@ static inline int modeset_lock(struct drm_modeset_lock *lock,
 
 	WARN_ON(ctx->contended);
 
-	if (interruptible && slow) {
+	if (ctx->trylock_only) {
+		if (!ww_mutex_trylock(&lock->mutex))
+			return -EBUSY;
+		else
+			return 0;
+	} else if (interruptible && slow) {
 		ret = ww_mutex_lock_slow_interruptible(&lock->mutex, &ctx->ww_ctx);
 	} else if (interruptible) {
 		ret = ww_mutex_lock_interruptible(&lock->mutex, &ctx->ww_ctx);
diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
index d38e1508f11a..a3f736d24382 100644
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -53,6 +53,11 @@ struct drm_modeset_acquire_ctx {
 	 * list of held locks (drm_modeset_lock)
 	 */
 	struct list_head locked;
+
+	/**
+	 * Trylock mode, use only for panic handlers!
+	 */
+	bool trylock_only;
 };
 
 /**
@@ -123,6 +128,7 @@ struct drm_device;
 struct drm_crtc;
 
 void drm_modeset_lock_all(struct drm_device *dev);
+int __drm_modeset_lock_all(struct drm_device *dev, bool trylock);
 void drm_modeset_unlock_all(struct drm_device *dev);
 void drm_modeset_lock_crtc(struct drm_crtc *crtc);
 void drm_modeset_unlock_crtc(struct drm_crtc *crtc);
-- 
2.0.1

Re: [PATCH 5/8] drm: trylock modest locking for fbdev panics

[Matt Roper]

On Tue, Jul 29, 2014 at 11:32:20PM +0200, Daniel Vetter wrote:
> In the fbdev code we want to do trylocks only to avoid deadlocks and
> other ugly issues. Thus far we've only grabbed the overall modeset
> lock, but that already failed to exclude a pile of potential
> concurrent operations. With proper atomic support this will be worse.
> 
> So add a trylock mode to the modeset locking code which attempts all
> locks only with trylocks, if possible. We need to track this in the
> locking functions themselves and can't restrict this to drivers since
> driver-private w/w mutexes must be treated the same way.
> 
> There's still the issue that other driver private locks aren't handled
> here at all, but well can't have everything. With this we will at
> least not regress, even once atomic allows lots of concurrent kms
> activity.
> 
> Aside: We should move the acquire context to stack-based allocation in
> the callers to get rid of that awful WARN_ON(kmalloc_failed) control
> flow which just blows up when memory is short. But that's material for
> separate patches.
> 
> v2:
> - Fix logic inversion fumble in the fb helper.
> - Add proper kerneldoc.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c    | 10 +++----
>  drivers/gpu/drm/drm_modeset_lock.c | 56 ++++++++++++++++++++++++++++++--------
>  include/drm/drm_modeset_lock.h     |  6 ++++
>  3 files changed, 55 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 3144db9dc0f1..841e039ba028 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -419,11 +419,11 @@ static bool drm_fb_helper_force_kernel_mode(void)
>  		if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  			continue;
>  
> -		/* NOTE: we use lockless flag below to avoid grabbing other
> -		 * modeset locks.  So just trylock the underlying mutex
> -		 * directly:
> +		/*
> +		 * NOTE: Use trylock mode to avoid deadlocks and sleeping in
> +		 * panic context.
>  		 */
> -		if (!mutex_trylock(&dev->mode_config.mutex)) {
> +		if (__drm_modeset_lock_all(dev, true) != 0) {
>  			error = true;
>  			continue;
>  		}

Can we succeed locking config->mutex and connection_mutex inside
__drm_modeset_lock_all(), but then fail to lock one of the CRTC mutexes
in drm_modeset_lock_all_crtcs()?  If so, __drm_modeset_lock_all() will
return -EBUSY, but not drop the locks it acquired, and then it seems
like we can return from the function here without ever dropping locks.


Matt


> @@ -432,7 +432,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
>  		if (ret)
>  			error = true;
>  
> -		mutex_unlock(&dev->mode_config.mutex);
> +		drm_modeset_unlock_all(dev);
>  	}
>  	return error;
>  }
> diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
> index 4d2aa549c614..acfe187609b0 100644
> --- a/drivers/gpu/drm/drm_modeset_lock.c
> +++ b/drivers/gpu/drm/drm_modeset_lock.c
> @@ -57,26 +57,37 @@
>  
>  
>  /**
> - * drm_modeset_lock_all - take all modeset locks
> - * @dev: drm device
> + * __drm_modeset_lock_all - internal helper to grab all modeset locks
> + * @dev: DRM device
> + * @trylock: trylock mode for atomic contexts
>   *
> - * This function takes all modeset locks, suitable where a more fine-grained
> - * scheme isn't (yet) implemented. Locks must be dropped with
> - * drm_modeset_unlock_all.
> + * This is a special version of drm_modeset_lock_all() which can also be used in
> + * atomic contexts. Then @trylock must be set to true.
> + *
> + * Returns:
> + * 0 on success or negative error code on failure.
>   */
> -void drm_modeset_lock_all(struct drm_device *dev)
> +int __drm_modeset_lock_all(struct drm_device *dev,
> +			   bool trylock)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_modeset_acquire_ctx *ctx;
>  	int ret;
>  
> -	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> -	if (WARN_ON(!ctx))
> -		return;
> +	ctx = kzalloc(sizeof(*ctx),
> +		      trylock ? GFP_ATOMIC : GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
>  
> -	mutex_lock(&config->mutex);
> +	if (trylock) {
> +		if (!mutex_trylock(&config->mutex))
> +			return -EBUSY;
> +	} else {
> +		mutex_lock(&config->mutex);
> +	}
>  
>  	drm_modeset_acquire_init(ctx, 0);
> +	ctx->trylock_only = trylock;
>  
>  retry:
>  	ret = drm_modeset_lock(&config->connection_mutex, ctx);
> @@ -95,13 +106,29 @@ retry:
>  
>  	drm_warn_on_modeset_not_all_locked(dev);
>  
> -	return;
> +	return 0;
>  
>  fail:
>  	if (ret == -EDEADLK) {
>  		drm_modeset_backoff(ctx);
>  		goto retry;
>  	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(__drm_modeset_lock_all);
> +
> +/**
> + * drm_modeset_lock_all - take all modeset locks
> + * @dev: drm device
> + *
> + * This function takes all modeset locks, suitable where a more fine-grained
> + * scheme isn't (yet) implemented. Locks must be dropped with
> + * drm_modeset_unlock_all.
> + */
> +void drm_modeset_lock_all(struct drm_device *dev)
> +{
> +	WARN_ON(__drm_modeset_lock_all(dev, false) != 0);
>  }
>  EXPORT_SYMBOL(drm_modeset_lock_all);
>  
> @@ -287,7 +314,12 @@ static inline int modeset_lock(struct drm_modeset_lock *lock,
>  
>  	WARN_ON(ctx->contended);
>  
> -	if (interruptible && slow) {
> +	if (ctx->trylock_only) {
> +		if (!ww_mutex_trylock(&lock->mutex))
> +			return -EBUSY;
> +		else
> +			return 0;
> +	} else if (interruptible && slow) {
>  		ret = ww_mutex_lock_slow_interruptible(&lock->mutex, &ctx->ww_ctx);
>  	} else if (interruptible) {
>  		ret = ww_mutex_lock_interruptible(&lock->mutex, &ctx->ww_ctx);
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index d38e1508f11a..a3f736d24382 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -53,6 +53,11 @@ struct drm_modeset_acquire_ctx {
>  	 * list of held locks (drm_modeset_lock)
>  	 */
>  	struct list_head locked;
> +
> +	/**
> +	 * Trylock mode, use only for panic handlers!
> +	 */
> +	bool trylock_only;
>  };
>  
>  /**
> @@ -123,6 +128,7 @@ struct drm_device;
>  struct drm_crtc;
>  
>  void drm_modeset_lock_all(struct drm_device *dev);
> +int __drm_modeset_lock_all(struct drm_device *dev, bool trylock);
>  void drm_modeset_unlock_all(struct drm_device *dev);
>  void drm_modeset_lock_crtc(struct drm_crtc *crtc);
>  void drm_modeset_unlock_crtc(struct drm_crtc *crtc);
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 5/8] drm: trylock modest locking for fbdev panics

[Daniel Vetter]

On Wed, Jul 30, 2014 at 08:56:07AM -0700, Matt Roper wrote:
> On Tue, Jul 29, 2014 at 11:32:20PM +0200, Daniel Vetter wrote:
> > In the fbdev code we want to do trylocks only to avoid deadlocks and
> > other ugly issues. Thus far we've only grabbed the overall modeset
> > lock, but that already failed to exclude a pile of potential
> > concurrent operations. With proper atomic support this will be worse.
> > 
> > So add a trylock mode to the modeset locking code which attempts all
> > locks only with trylocks, if possible. We need to track this in the
> > locking functions themselves and can't restrict this to drivers since
> > driver-private w/w mutexes must be treated the same way.
> > 
> > There's still the issue that other driver private locks aren't handled
> > here at all, but well can't have everything. With this we will at
> > least not regress, even once atomic allows lots of concurrent kms
> > activity.
> > 
> > Aside: We should move the acquire context to stack-based allocation in
> > the callers to get rid of that awful WARN_ON(kmalloc_failed) control
> > flow which just blows up when memory is short. But that's material for
> > separate patches.
> > 
> > v2:
> > - Fix logic inversion fumble in the fb helper.
> > - Add proper kerneldoc.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c    | 10 +++----
> >  drivers/gpu/drm/drm_modeset_lock.c | 56 ++++++++++++++++++++++++++++++--------
> >  include/drm/drm_modeset_lock.h     |  6 ++++
> >  3 files changed, 55 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 3144db9dc0f1..841e039ba028 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -419,11 +419,11 @@ static bool drm_fb_helper_force_kernel_mode(void)
> >  		if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> >  			continue;
> >  
> > -		/* NOTE: we use lockless flag below to avoid grabbing other
> > -		 * modeset locks.  So just trylock the underlying mutex
> > -		 * directly:
> > +		/*
> > +		 * NOTE: Use trylock mode to avoid deadlocks and sleeping in
> > +		 * panic context.
> >  		 */
> > -		if (!mutex_trylock(&dev->mode_config.mutex)) {
> > +		if (__drm_modeset_lock_all(dev, true) != 0) {
> >  			error = true;
> >  			continue;
> >  		}
> 
> Can we succeed locking config->mutex and connection_mutex inside
> __drm_modeset_lock_all(), but then fail to lock one of the CRTC mutexes
> in drm_modeset_lock_all_crtcs()?  If so, __drm_modeset_lock_all() will
> return -EBUSY, but not drop the locks it acquired, and then it seems
> like we can return from the function here without ever dropping locks.

Well it's the panic code, so after this the machine is officially dead
anyway. The only thing left to do is get the dmesg out with as little risk
as possible.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 6/8] drm: Add a plane->reset hook

[Daniel Vetter]

In general having this can't hurt, and the atomic helpers will need
it to be able to reset the state objects properly. The overall idea
is to reset in the order pixels flow, so planes -> crtcs ->
encoders -> connectors.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 5 +++++
 include/drm/drm_crtc.h     | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index bacf565449d5..ff705ea3f50e 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4582,9 +4582,14 @@ out:
 void drm_mode_config_reset(struct drm_device *dev)
 {
 	struct drm_crtc *crtc;
+	struct drm_crtc *plane;
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head)
+		if (plane->funcs->reset)
+			plane->funcs->reset(plane);
+
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
 		if (crtc->funcs->reset)
 			crtc->funcs->reset(crtc);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 5fffb5c53ba6..0115b80a0632 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -580,6 +580,7 @@ struct drm_plane_funcs {
 			    uint32_t src_w, uint32_t src_h);
 	int (*disable_plane)(struct drm_plane *plane);
 	void (*destroy)(struct drm_plane *plane);
+	void (*reset)(struct drm_plane *plane);
 
 	int (*set_property)(struct drm_plane *plane,
 			    struct drm_property *property, uint64_t val);
-- 
2.0.1

[PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Daniel Vetter]

As usual in both a crtc index and a struct drm_crtc * version.

The function assumes that no one drivers their display below 10Hz, and
it will complain if the vblank wait takes longer than that.

v2: Also check dev->max_vblank_counter since some drivers register a
fake get_vblank_counter function.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h        |  2 ++
 2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 0de123afdb34..76024fdde452 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -999,6 +999,51 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc)
 EXPORT_SYMBOL(drm_crtc_vblank_put);
 
 /**
+ * drm_vblank_wait - wait for one vblank
+ * @dev: DRM device
+ * @crtc: crtc index
+ *
+ * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
+ * It is a failure to call this when the vblank irq for @crtc is disable, e.g.
+ * due to lack of driver support or because the crtc is off.
+ */
+void drm_vblank_wait(struct drm_device *dev, int crtc)
+{
+	int ret;
+	u32 last;
+
+	ret = drm_vblank_get(dev, crtc);
+	if (WARN_ON(ret))
+		return;
+
+	last = drm_vblank_count(dev, crtc);
+
+#define C (last != drm_vblank_count(dev, crtc))
+	ret = wait_event_timeout(dev->vblank[crtc].queue,
+				 C, msecs_to_jiffies(100));
+
+	WARN_ON(ret == 0);
+#undef C
+
+	drm_vblank_put(dev, crtc);
+}
+EXPORT_SYMBOL(drm_vblank_wait);
+
+/**
+ * drm_crtc_vblank_wait - wait for one vblank
+ * @crtc: DRM crtc
+ *
+ * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
+ * It is a failure to call this when the vblank irq for @crtc is disable, e.g.
+ * due to lack of driver support or because the crtc is off.
+ */
+void drm_crtc_vblank_wait(struct drm_crtc *crtc)
+{
+	drm_vblank_wait(crtc->dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_wait);
+
+/**
  * drm_vblank_off - disable vblank events on a CRTC
  * @dev: DRM device
  * @crtc: CRTC in question
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 06a673894c47..f72e5ef5f7b0 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1355,6 +1355,8 @@ extern int drm_vblank_get(struct drm_device *dev, int crtc);
 extern void drm_vblank_put(struct drm_device *dev, int crtc);
 extern int drm_crtc_vblank_get(struct drm_crtc *crtc);
 extern void drm_crtc_vblank_put(struct drm_crtc *crtc);
+extern void drm_vblank_wait(struct drm_device *dev, int crtc);
+extern void drm_crtc_vblank_wait(struct drm_crtc *crtc);
 extern void drm_vblank_off(struct drm_device *dev, int crtc);
 extern void drm_vblank_on(struct drm_device *dev, int crtc);
 extern void drm_crtc_vblank_off(struct drm_crtc *crtc);
-- 
2.0.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Michel Dänzer]

On 30.07.2014 06:32, Daniel Vetter wrote:
> As usual in both a crtc index and a struct drm_crtc * version.
> 
> The function assumes that no one drivers their display below 10Hz, and
> it will complain if the vblank wait takes longer than that.
> 
> v2: Also check dev->max_vblank_counter since some drivers register a
> fake get_vblank_counter function.

What does that refer to? Can't find any other reference to
max_vblank_counter in the patch.


> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 0de123afdb34..76024fdde452 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -999,6 +999,51 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc)
>  EXPORT_SYMBOL(drm_crtc_vblank_put);
>  
>  /**
> + * drm_vblank_wait - wait for one vblank
> + * @dev: DRM device
> + * @crtc: crtc index
> + *
> + * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
> + * It is a failure to call this when the vblank irq for @crtc is disable, e.g.

Spelling: 'disabled'


> + * due to lack of driver support or because the crtc is off.
> + */
> +void drm_vblank_wait(struct drm_device *dev, int crtc)
> +{
> +	int ret;
> +	u32 last;
> +
> +	ret = drm_vblank_get(dev, crtc);
> +	if (WARN_ON(ret))
> +		return;
> +
> +	last = drm_vblank_count(dev, crtc);
> +
> +#define C (last != drm_vblank_count(dev, crtc))
> +	ret = wait_event_timeout(dev->vblank[crtc].queue,
> +				 C, msecs_to_jiffies(100));
> +
> +	WARN_ON(ret == 0);
> +#undef C

What's the point of the C macro?


> +	drm_vblank_put(dev, crtc);
> +}
> +EXPORT_SYMBOL(drm_vblank_wait);
> +
> +/**
> + * drm_crtc_vblank_wait - wait for one vblank
> + * @crtc: DRM crtc
> + *
> + * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
> + * It is a failure to call this when the vblank irq for @crtc is disable, e.g.

Same typo as above.


> + * due to lack of driver support or because the crtc is off.
> + */
> +void drm_crtc_vblank_wait(struct drm_crtc *crtc)
> +{
> +	drm_vblank_wait(crtc->dev, drm_crtc_index(crtc));
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_wait);
> +
> +/**

Maybe the function names should be *_vblank_wait_next() or something to
clarify the purpose and reduce potential confusion versus drm_wait_vblank().


Looks good to me other than that.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Daniel Vetter]

On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:
> On 30.07.2014 06:32, Daniel Vetter wrote:
> > As usual in both a crtc index and a struct drm_crtc * version.
> > 
> > The function assumes that no one drivers their display below 10Hz, and
> > it will complain if the vblank wait takes longer than that.
> > 
> > v2: Also check dev->max_vblank_counter since some drivers register a
> > fake get_vblank_counter function.
> 
> What does that refer to? Can't find any other reference to
> max_vblank_counter in the patch.

Oops, that was from an intermediate version. The v3: text somehow got lost
where I've switched the code from directly calling the ->get_counter
callback to using drm_vblank_counter, which will dtrt even when there's
not hw counter available. Will augment the commit message.

> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 0de123afdb34..76024fdde452 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -999,6 +999,51 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc)
> >  EXPORT_SYMBOL(drm_crtc_vblank_put);
> >  
> >  /**
> > + * drm_vblank_wait - wait for one vblank
> > + * @dev: DRM device
> > + * @crtc: crtc index
> > + *
> > + * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
> > + * It is a failure to call this when the vblank irq for @crtc is disable, e.g.
> 
> Spelling: 'disabled'
> 
> 
> > + * due to lack of driver support or because the crtc is off.
> > + */
> > +void drm_vblank_wait(struct drm_device *dev, int crtc)
> > +{
> > +	int ret;
> > +	u32 last;
> > +
> > +	ret = drm_vblank_get(dev, crtc);
> > +	if (WARN_ON(ret))
> > +		return;
> > +
> > +	last = drm_vblank_count(dev, crtc);
> > +
> > +#define C (last != drm_vblank_count(dev, crtc))
> > +	ret = wait_event_timeout(dev->vblank[crtc].queue,
> > +				 C, msecs_to_jiffies(100));
> > +
> > +	WARN_ON(ret == 0);
> > +#undef C
> 
> What's the point of the C macro?

Usually the conditions tend to overflow the 80 char limit, so I've adopted
that pattern of extracting it into a local #define. I think it'll fit
here, so will roll in.

> 
> 
> > +	drm_vblank_put(dev, crtc);
> > +}
> > +EXPORT_SYMBOL(drm_vblank_wait);
> > +
> > +/**
> > + * drm_crtc_vblank_wait - wait for one vblank
> > + * @crtc: DRM crtc
> > + *
> > + * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
> > + * It is a failure to call this when the vblank irq for @crtc is disable, e.g.
> 
> Same typo as above.
> 
> 
> > + * due to lack of driver support or because the crtc is off.
> > + */
> > +void drm_crtc_vblank_wait(struct drm_crtc *crtc)
> > +{
> > +	drm_vblank_wait(crtc->dev, drm_crtc_index(crtc));
> > +}
> > +EXPORT_SYMBOL(drm_crtc_vblank_wait);
> > +
> > +/**
> 
> Maybe the function names should be *_vblank_wait_next() or something to
> clarify the purpose and reduce potential confusion versus drm_wait_vblank().

Yeah that name is just transferred from the i915 driver. What about
drm_wait_one_vblank()/drm_crtc_wait_one_vblank()? At least to my ear
vblank_wait_next sounds backwards.

> Looks good to me other than that.

If you're ok with the name suggestion I'll polish & resend, thanks for the
comments.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Michel Dänzer]

On 30.07.2014 17:22, Daniel Vetter wrote:
> On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:
>> On 30.07.2014 06:32, Daniel Vetter wrote:
>>> + * due to lack of driver support or because the crtc is off.
>>> + */
>>> +void drm_crtc_vblank_wait(struct drm_crtc *crtc)
>>> +{
>>> +	drm_vblank_wait(crtc->dev, drm_crtc_index(crtc));
>>> +}
>>> +EXPORT_SYMBOL(drm_crtc_vblank_wait);
>>> +
>>> +/**
>>
>> Maybe the function names should be *_vblank_wait_next() or something to
>> clarify the purpose and reduce potential confusion versus drm_wait_vblank().
> 
> Yeah that name is just transferred from the i915 driver. What about
> drm_wait_one_vblank()/drm_crtc_wait_one_vblank()?

I don't care that much :), go ahead.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer

Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1760 bytes --]

On Wed, Jul 30, 2014 at 05:32:28PM +0900, Michel Dänzer wrote:
> On 30.07.2014 17:22, Daniel Vetter wrote:
> > On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:
> >> On 30.07.2014 06:32, Daniel Vetter wrote:
> >>> + * due to lack of driver support or because the crtc is off.
> >>> + */
> >>> +void drm_crtc_vblank_wait(struct drm_crtc *crtc)
> >>> +{
> >>> +	drm_vblank_wait(crtc->dev, drm_crtc_index(crtc));
> >>> +}
> >>> +EXPORT_SYMBOL(drm_crtc_vblank_wait);
> >>> +
> >>> +/**
> >>
> >> Maybe the function names should be *_vblank_wait_next() or something to
> >> clarify the purpose and reduce potential confusion versus drm_wait_vblank().
> > 
> > Yeah that name is just transferred from the i915 driver. What about
> > drm_wait_one_vblank()/drm_crtc_wait_one_vblank()?
> 
> I don't care that much :), go ahead.

Just my two cents: our downstream kernel has a helper somewhat like this
which waits for a specified number of frames (apparently this is useful
for some panels that require up to 5 or 6 frames before they display the
correct image on screen). So perhaps something like this could work:

	void drm_wait_vblank_count(struct drm_device *dev, unsigned int crtc,
				   unsigned int count)
	{
		u32 last;
		int ret;

		ret = drm_vblank_get(dev, crtc);
		if (WARN_ON(ret))
			return;

		while (count--) {
			last = drm_vblank_count(dev, crtc);

			...
		}

		drm_vblank_put(dev, crtc);
	}

Then implement drm_wait_vblank() (or drm_wait_one_vblank()) on top of
that as a special case. Of course one could equally well implement
drm_wait_vblank_count() on top of your drm_wait_{one_,}vblank().

I couldn't think of a safe way to make the above work without the
loop...

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Ville Syrjälä]

On Wed, Jul 30, 2014 at 04:20:25PM +0200, Thierry Reding wrote:
> On Wed, Jul 30, 2014 at 05:32:28PM +0900, Michel Dänzer wrote:
> > On 30.07.2014 17:22, Daniel Vetter wrote:
> > > On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:
> > >> On 30.07.2014 06:32, Daniel Vetter wrote:
> > >>> + * due to lack of driver support or because the crtc is off.
> > >>> + */
> > >>> +void drm_crtc_vblank_wait(struct drm_crtc *crtc)
> > >>> +{
> > >>> +	drm_vblank_wait(crtc->dev, drm_crtc_index(crtc));
> > >>> +}
> > >>> +EXPORT_SYMBOL(drm_crtc_vblank_wait);
> > >>> +
> > >>> +/**
> > >>
> > >> Maybe the function names should be *_vblank_wait_next() or something to
> > >> clarify the purpose and reduce potential confusion versus drm_wait_vblank().
> > > 
> > > Yeah that name is just transferred from the i915 driver. What about
> > > drm_wait_one_vblank()/drm_crtc_wait_one_vblank()?
> > 
> > I don't care that much :), go ahead.
> 
> Just my two cents: our downstream kernel has a helper somewhat like this
> which waits for a specified number of frames (apparently this is useful
> for some panels that require up to 5 or 6 frames before they display the
> correct image on screen). So perhaps something like this could work:
> 
> 	void drm_wait_vblank_count(struct drm_device *dev, unsigned int crtc,
> 				   unsigned int count)
> 	{
> 		u32 last;
> 		int ret;
> 
> 		ret = drm_vblank_get(dev, crtc);
> 		if (WARN_ON(ret))
> 			return;
> 
> 		while (count--) {
> 			last = drm_vblank_count(dev, crtc);
> 
> 			...
> 		}
> 
> 		drm_vblank_put(dev, crtc);
> 	}

Would be nicer to wait for an absolute vblank count instead IMO. Or
if you want to pass a relative count in just convert it to an absolute
count first and wait for it (taking wraparound into account obviously).

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Daniel Vetter]

On Wed, Jul 30, 2014 at 05:36:21PM +0300, Ville Syrjälä wrote:
> On Wed, Jul 30, 2014 at 04:20:25PM +0200, Thierry Reding wrote:
> > On Wed, Jul 30, 2014 at 05:32:28PM +0900, Michel Dänzer wrote:
> > > On 30.07.2014 17:22, Daniel Vetter wrote:
> > > > On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:
> > > >> On 30.07.2014 06:32, Daniel Vetter wrote:
> > > >>> + * due to lack of driver support or because the crtc is off.
> > > >>> + */
> > > >>> +void drm_crtc_vblank_wait(struct drm_crtc *crtc)
> > > >>> +{
> > > >>> +	drm_vblank_wait(crtc->dev, drm_crtc_index(crtc));
> > > >>> +}
> > > >>> +EXPORT_SYMBOL(drm_crtc_vblank_wait);
> > > >>> +
> > > >>> +/**
> > > >>
> > > >> Maybe the function names should be *_vblank_wait_next() or something to
> > > >> clarify the purpose and reduce potential confusion versus drm_wait_vblank().
> > > > 
> > > > Yeah that name is just transferred from the i915 driver. What about
> > > > drm_wait_one_vblank()/drm_crtc_wait_one_vblank()?
> > > 
> > > I don't care that much :), go ahead.
> > 
> > Just my two cents: our downstream kernel has a helper somewhat like this
> > which waits for a specified number of frames (apparently this is useful
> > for some panels that require up to 5 or 6 frames before they display the
> > correct image on screen). So perhaps something like this could work:
> > 
> > 	void drm_wait_vblank_count(struct drm_device *dev, unsigned int crtc,
> > 				   unsigned int count)
> > 	{
> > 		u32 last;
> > 		int ret;
> > 
> > 		ret = drm_vblank_get(dev, crtc);
> > 		if (WARN_ON(ret))
> > 			return;
> > 
> > 		while (count--) {
> > 			last = drm_vblank_count(dev, crtc);
> > 
> > 			...
> > 		}
> > 
> > 		drm_vblank_put(dev, crtc);
> > 	}
> 
> Would be nicer to wait for an absolute vblank count instead IMO. Or
> if you want to pass a relative count in just convert it to an absolute
> count first and wait for it (taking wraparound into account obviously).

Yeah I've conisidered to to a generic version, but don't though about all
the ways I'll get wrap-around wrong and decided that we better postpone
this until there's a real need. We can easily extract it later on ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2296 bytes --]

On Wed, Jul 30, 2014 at 05:36:21PM +0300, Ville Syrjälä wrote:
> On Wed, Jul 30, 2014 at 04:20:25PM +0200, Thierry Reding wrote:
> > On Wed, Jul 30, 2014 at 05:32:28PM +0900, Michel Dänzer wrote:
> > > On 30.07.2014 17:22, Daniel Vetter wrote:
> > > > On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:
> > > >> On 30.07.2014 06:32, Daniel Vetter wrote:
> > > >>> + * due to lack of driver support or because the crtc is off.
> > > >>> + */
> > > >>> +void drm_crtc_vblank_wait(struct drm_crtc *crtc)
> > > >>> +{
> > > >>> +	drm_vblank_wait(crtc->dev, drm_crtc_index(crtc));
> > > >>> +}
> > > >>> +EXPORT_SYMBOL(drm_crtc_vblank_wait);
> > > >>> +
> > > >>> +/**
> > > >>
> > > >> Maybe the function names should be *_vblank_wait_next() or something to
> > > >> clarify the purpose and reduce potential confusion versus drm_wait_vblank().
> > > > 
> > > > Yeah that name is just transferred from the i915 driver. What about
> > > > drm_wait_one_vblank()/drm_crtc_wait_one_vblank()?
> > > 
> > > I don't care that much :), go ahead.
> > 
> > Just my two cents: our downstream kernel has a helper somewhat like this
> > which waits for a specified number of frames (apparently this is useful
> > for some panels that require up to 5 or 6 frames before they display the
> > correct image on screen). So perhaps something like this could work:
> > 
> > 	void drm_wait_vblank_count(struct drm_device *dev, unsigned int crtc,
> > 				   unsigned int count)
> > 	{
> > 		u32 last;
> > 		int ret;
> > 
> > 		ret = drm_vblank_get(dev, crtc);
> > 		if (WARN_ON(ret))
> > 			return;
> > 
> > 		while (count--) {
> > 			last = drm_vblank_count(dev, crtc);
> > 
> > 			...
> > 		}
> > 
> > 		drm_vblank_put(dev, crtc);
> > 	}
> 
> Would be nicer to wait for an absolute vblank count instead IMO. Or
> if you want to pass a relative count in just convert it to an absolute
> count first and wait for it (taking wraparound into account obviously).

Hmm... would something like this work?

	target = drm_vblank_count(dev, crtc) + count;

	ret = wait_event_timeout(...,
				 drm_vblank_count(dev, crtc) == target,
				 ...);

That should properly take into account wrap-around given that both sites
use drm_vblank_count().

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Michel Dänzer]

[-- Attachment #1.1: Type: text/plain, Size: 2554 bytes --]

On 31.07.2014 00:21, Thierry Reding wrote:
> On Wed, Jul 30, 2014 at 05:36:21PM +0300, Ville Syrjälä wrote:
>> On Wed, Jul 30, 2014 at 04:20:25PM +0200, Thierry Reding wrote:
>>> On Wed, Jul 30, 2014 at 05:32:28PM +0900, Michel Dänzer wrote:
>>>> On 30.07.2014 17:22, Daniel Vetter wrote:
>>>>> On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:
>>>>>> On 30.07.2014 06:32, Daniel Vetter wrote:
>>>>>>> + * due to lack of driver support or because the crtc is off.
>>>>>>> + */
>>>>>>> +void drm_crtc_vblank_wait(struct drm_crtc *crtc)
>>>>>>> +{
>>>>>>> +	drm_vblank_wait(crtc->dev, drm_crtc_index(crtc));
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(drm_crtc_vblank_wait);
>>>>>>> +
>>>>>>> +/**
>>>>>>
>>>>>> Maybe the function names should be *_vblank_wait_next() or something to
>>>>>> clarify the purpose and reduce potential confusion versus drm_wait_vblank().
>>>>>
>>>>> Yeah that name is just transferred from the i915 driver. What about
>>>>> drm_wait_one_vblank()/drm_crtc_wait_one_vblank()?
>>>>
>>>> I don't care that much :), go ahead.
>>>
>>> Just my two cents: our downstream kernel has a helper somewhat like this
>>> which waits for a specified number of frames (apparently this is useful
>>> for some panels that require up to 5 or 6 frames before they display the
>>> correct image on screen). So perhaps something like this could work:
>>>
>>> 	void drm_wait_vblank_count(struct drm_device *dev, unsigned int crtc,
>>> 				   unsigned int count)
>>> 	{
>>> 		u32 last;
>>> 		int ret;
>>>
>>> 		ret = drm_vblank_get(dev, crtc);
>>> 		if (WARN_ON(ret))
>>> 			return;
>>>
>>> 		while (count--) {
>>> 			last = drm_vblank_count(dev, crtc);
>>>
>>> 			...
>>> 		}
>>>
>>> 		drm_vblank_put(dev, crtc);
>>> 	}
>>
>> Would be nicer to wait for an absolute vblank count instead IMO. Or
>> if you want to pass a relative count in just convert it to an absolute
>> count first and wait for it (taking wraparound into account obviously).
> 
> Hmm... would something like this work?
> 
> 	target = drm_vblank_count(dev, crtc) + count;
> 
> 	ret = wait_event_timeout(...,
> 				 drm_vblank_count(dev, crtc) == target,
> 				 ...);
> 
> That should properly take into account wrap-around given that both sites
> use drm_vblank_count().

I think it would be better to refactor drm_wait_vblank() than to
reinvent it.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 234 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Daniel Vetter]

On Thu, Jul 31, 2014 at 3:14 AM, Michel Dänzer <michel@daenzer.net> wrote:
> I think it would be better to refactor drm_wait_vblank() than to
> reinvent it.

That's the ioctl implementation which spends most of its time decoding
ioctl structures. If we take that out then there's about half a line
which would be shared (since a lot of the stuff in there is ums gunk
that we don't want to carry over to a kms-only internal interface). So
imo that doesn't make sense.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Michel Dänzer]

On 31.07.2014 16:54, Daniel Vetter wrote:
> On Thu, Jul 31, 2014 at 3:14 AM, Michel Dänzer <michel@daenzer.net> wrote:
>> I think it would be better to refactor drm_wait_vblank() than to
>> reinvent it.
> 
> That's the ioctl implementation which spends most of its time decoding
> ioctl structures. If we take that out then there's about half a line
> which would be shared (since a lot of the stuff in there is ums gunk
> that we don't want to carry over to a kms-only internal interface). So
> imo that doesn't make sense.

I'm referring to the core logic of waiting for a number of vblank
periods or until the vblank period with a given sequence number, dealing
with wraparound etc. The issues you guys were discussing for a new
function were ironed out there long ago.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Daniel Vetter]

On Thu, Jul 31, 2014 at 10:56 AM, Michel Dänzer <michel@daenzer.net> wrote:
> On 31.07.2014 16:54, Daniel Vetter wrote:
>> On Thu, Jul 31, 2014 at 3:14 AM, Michel Dänzer <michel@daenzer.net> wrote:
>>> I think it would be better to refactor drm_wait_vblank() than to
>>> reinvent it.
>>
>> That's the ioctl implementation which spends most of its time decoding
>> ioctl structures. If we take that out then there's about half a line
>> which would be shared (since a lot of the stuff in there is ums gunk
>> that we don't want to carry over to a kms-only internal interface). So
>> imo that doesn't make sense.
>
> I'm referring to the core logic of waiting for a number of vblank
> periods or until the vblank period with a given sequence number, dealing
> with wraparound etc. The issues you guys were discussing for a new
> function were ironed out there long ago.

I'm referering to the same, but that logic is gunked up with
special-cases for UMS and absolute vblank waits and all kinds of other
stuff, so that sharing this with a kms helper to wait a few vblanks
(so relative only) really doesn't buy us all that much. Actually I
think you'll be left with nothing shared since for the kms
driver-internal functions really shouldn't have all these hacks to
paper over races and other issues (like ums shutting down the pipe
while we didn't look).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/irq: Implement a generic vblank_wait function

[Daniel Vetter]

As usual in both a crtc index and a struct drm_crtc * version.

The function assumes that no one drivers their display below 10Hz, and
it will complain if the vblank wait takes longer than that.

v2: Also check dev->max_vblank_counter since some drivers register a
fake get_vblank_counter function.

v3: Use drm_vblank_count instead of calling the low-level
->get_vblank_counter callback. That way we'll get the sw-cooked
counter for platforms without proper vblank support and so can ditch
the max_vblank_counter check again.

v4: Review from Michel Dänzer:
- Restore lost notes about v3:
- Spelling in kerneldoc.
- Inline wait_event condition.
- s/vblank_wait/wait_one_vblank/

Cc: Michel Dänzer <michel@daenzer.net>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h        |  2 ++
 2 files changed, 46 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 0de123afdb34..2605a5923fdb 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -999,6 +999,50 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc)
 EXPORT_SYMBOL(drm_crtc_vblank_put);
 
 /**
+ * drm_wait_one_vblank - wait for one vblank
+ * @dev: DRM device
+ * @crtc: crtc index
+ *
+ * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
+ * It is a failure to call this when the vblank irq for @crtc is disabled, e.g.
+ * due to lack of driver support or because the crtc is off.
+ */
+void drm_wait_one_vblank(struct drm_device *dev, int crtc)
+{
+	int ret;
+	u32 last;
+
+	ret = drm_vblank_get(dev, crtc);
+	if (WARN_ON(ret))
+		return;
+
+	last = drm_vblank_count(dev, crtc);
+
+	ret = wait_event_timeout(dev->vblank[crtc].queue,
+				 last != drm_vblank_count(dev, crtc),
+				 msecs_to_jiffies(100));
+
+	WARN_ON(ret == 0);
+
+	drm_vblank_put(dev, crtc);
+}
+EXPORT_SYMBOL(drm_wait_one_vblank);
+
+/**
+ * drm_crtc_wait_one_vblank - wait for one vblank
+ * @crtc: DRM crtc
+ *
+ * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
+ * It is a failure to call this when the vblank irq for @crtc is disabled, e.g.
+ * due to lack of driver support or because the crtc is off.
+ */
+void drm_crtc_wait_one_vblank(struct drm_crtc *crtc)
+{
+	drm_wait_one_vblank(crtc->dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_wait_one_vblank);
+
+/**
  * drm_vblank_off - disable vblank events on a CRTC
  * @dev: DRM device
  * @crtc: CRTC in question
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 06a673894c47..ee4c321455e8 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1355,6 +1355,8 @@ extern int drm_vblank_get(struct drm_device *dev, int crtc);
 extern void drm_vblank_put(struct drm_device *dev, int crtc);
 extern int drm_crtc_vblank_get(struct drm_crtc *crtc);
 extern void drm_crtc_vblank_put(struct drm_crtc *crtc);
+extern void drm_wait_one_vblank(struct drm_device *dev, int crtc);
+extern void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
 extern void drm_vblank_off(struct drm_device *dev, int crtc);
 extern void drm_vblank_on(struct drm_device *dev, int crtc);
 extern void drm_crtc_vblank_off(struct drm_crtc *crtc);
-- 
2.0.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/irq: Implement a generic vblank_wait function

[Michel Dänzer]

On 30.07.2014 18:25, Daniel Vetter wrote:
> As usual in both a crtc index and a struct drm_crtc * version.
> 
> The function assumes that no one drivers their display below 10Hz, and
> it will complain if the vblank wait takes longer than that.
> 
> v2: Also check dev->max_vblank_counter since some drivers register a
> fake get_vblank_counter function.
> 
> v3: Use drm_vblank_count instead of calling the low-level
> ->get_vblank_counter callback. That way we'll get the sw-cooked
> counter for platforms without proper vblank support and so can ditch
> the max_vblank_counter check again.
> 
> v4: Review from Michel Dänzer:
> - Restore lost notes about v3:
> - Spelling in kerneldoc.
> - Inline wait_event condition.
> - s/vblank_wait/wait_one_vblank/
> 
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 453 bytes --]

On Tue, Jul 29, 2014 at 11:32:22PM +0200, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
[...]
> +	ret = wait_event_timeout(dev->vblank[crtc].queue,
> +				 C, msecs_to_jiffies(100));

100 milliseconds looks like a very arbitrary choice. Theoretically we
support things like crtc->mode.vrefresh = 5, so I wonder if we should
parameterize this on the refresh rate, or simply increase it further?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Daniel Vetter]

On Wed, Jul 30, 2014 at 04:24:06PM +0200, Thierry Reding wrote:
> On Tue, Jul 29, 2014 at 11:32:22PM +0200, Daniel Vetter wrote:
> [...]
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> [...]
> > +	ret = wait_event_timeout(dev->vblank[crtc].queue,
> > +				 C, msecs_to_jiffies(100));
> 
> 100 milliseconds looks like a very arbitrary choice. Theoretically we
> support things like crtc->mode.vrefresh = 5, so I wonder if we should
> parameterize this on the refresh rate, or simply increase it further?

Once we have 10Hz panels I think we can pump/fix this. In i915 we've used
50ms everywhere, and never hit this. Except when driver bugs, ofc.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 809 bytes --]

On Wed, Jul 30, 2014 at 05:06:38PM +0200, Daniel Vetter wrote:
> On Wed, Jul 30, 2014 at 04:24:06PM +0200, Thierry Reding wrote:
> > On Tue, Jul 29, 2014 at 11:32:22PM +0200, Daniel Vetter wrote:
> > [...]
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > [...]
> > > +	ret = wait_event_timeout(dev->vblank[crtc].queue,
> > > +				 C, msecs_to_jiffies(100));
> > 
> > 100 milliseconds looks like a very arbitrary choice. Theoretically we
> > support things like crtc->mode.vrefresh = 5, so I wonder if we should
> > parameterize this on the refresh rate, or simply increase it further?
> 
> Once we have 10Hz panels I think we can pump/fix this. In i915 we've used
> 50ms everywhere, and never hit this. Except when driver bugs, ofc.

Okay, fair enough.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 8/8] drm/i915: Use generic vblank wait

[Daniel Vetter]

This has the upside that it will no longer steal interrupts from the
interrutp handler on pre-g4x. Furthermore this will now scream properly
on all platforms if we don't have hw counters enabled.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 41 +-----------------------------------
 1 file changed, 1 insertion(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1edfd1ae5b37..13bcf1348fc3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -891,17 +891,6 @@ enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
 	return intel_crtc->config.cpu_transcoder;
 }
 
-static void g4x_wait_for_vblank(struct drm_device *dev, int pipe)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 frame, frame_reg = PIPE_FRMCOUNT_GM45(pipe);
-
-	frame = I915_READ(frame_reg);
-
-	if (wait_for(I915_READ_NOTRACE(frame_reg) != frame, 50))
-		WARN(1, "vblank wait timed out\n");
-}
-
 /**
  * intel_wait_for_vblank - wait for vblank on a given pipe
  * @dev: drm device
@@ -912,35 +901,7 @@ static void g4x_wait_for_vblank(struct drm_device *dev, int pipe)
  */
 void intel_wait_for_vblank(struct drm_device *dev, int pipe)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int pipestat_reg = PIPESTAT(pipe);
-
-	if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
-		g4x_wait_for_vblank(dev, pipe);
-		return;
-	}
-
-	/* Clear existing vblank status. Note this will clear any other
-	 * sticky status fields as well.
-	 *
-	 * This races with i915_driver_irq_handler() with the result
-	 * that either function could miss a vblank event.  Here it is not
-	 * fatal, as we will either wait upon the next vblank interrupt or
-	 * timeout.  Generally speaking intel_wait_for_vblank() is only
-	 * called during modeset at which time the GPU should be idle and
-	 * should *not* be performing page flips and thus not waiting on
-	 * vblanks...
-	 * Currently, the result of us stealing a vblank from the irq
-	 * handler is that a single frame will be skipped during swapbuffers.
-	 */
-	I915_WRITE(pipestat_reg,
-		   I915_READ(pipestat_reg) | PIPE_VBLANK_INTERRUPT_STATUS);
-
-	/* Wait for vblank interrupt bit to set */
-	if (wait_for(I915_READ(pipestat_reg) &
-		     PIPE_VBLANK_INTERRUPT_STATUS,
-		     50))
-		DRM_DEBUG_KMS("vblank wait timed out\n");
+	drm_vblank_wait(dev, pipe);
 }
 
 static bool pipe_dsl_stopped(struct drm_device *dev, enum pipe pipe)
-- 
2.0.1

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

[PATCH] drm/i915: Use generic vblank wait

[Daniel Vetter]

This has the upside that it will no longer steal interrupts from the
interrutp handler on pre-g4x. Furthermore this will now scream properly
on all platforms if we don't have hw counters enabled.

v2: Adjust to the new names.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 41 +-----------------------------------
 1 file changed, 1 insertion(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1edfd1ae5b37..540fc762179f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -891,17 +891,6 @@ enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
 	return intel_crtc->config.cpu_transcoder;
 }
 
-static void g4x_wait_for_vblank(struct drm_device *dev, int pipe)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 frame, frame_reg = PIPE_FRMCOUNT_GM45(pipe);
-
-	frame = I915_READ(frame_reg);
-
-	if (wait_for(I915_READ_NOTRACE(frame_reg) != frame, 50))
-		WARN(1, "vblank wait timed out\n");
-}
-
 /**
  * intel_wait_for_vblank - wait for vblank on a given pipe
  * @dev: drm device
@@ -912,35 +901,7 @@ static void g4x_wait_for_vblank(struct drm_device *dev, int pipe)
  */
 void intel_wait_for_vblank(struct drm_device *dev, int pipe)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int pipestat_reg = PIPESTAT(pipe);
-
-	if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
-		g4x_wait_for_vblank(dev, pipe);
-		return;
-	}
-
-	/* Clear existing vblank status. Note this will clear any other
-	 * sticky status fields as well.
-	 *
-	 * This races with i915_driver_irq_handler() with the result
-	 * that either function could miss a vblank event.  Here it is not
-	 * fatal, as we will either wait upon the next vblank interrupt or
-	 * timeout.  Generally speaking intel_wait_for_vblank() is only
-	 * called during modeset at which time the GPU should be idle and
-	 * should *not* be performing page flips and thus not waiting on
-	 * vblanks...
-	 * Currently, the result of us stealing a vblank from the irq
-	 * handler is that a single frame will be skipped during swapbuffers.
-	 */
-	I915_WRITE(pipestat_reg,
-		   I915_READ(pipestat_reg) | PIPE_VBLANK_INTERRUPT_STATUS);
-
-	/* Wait for vblank interrupt bit to set */
-	if (wait_for(I915_READ(pipestat_reg) &
-		     PIPE_VBLANK_INTERRUPT_STATUS,
-		     50))
-		DRM_DEBUG_KMS("vblank wait timed out\n");
+	drm_wait_one_vblank(dev, pipe);
 }
 
 static bool pipe_dsl_stopped(struct drm_device *dev, enum pipe pipe)
-- 
2.0.1

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

[PATCH] drm: Docbook fixes

[Daniel Vetter]

Bunch of small leftovers spotted by looking at the make htmldocs output.

I've left out dp mst, there's too much amiss there.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c         | 5 +++--
 drivers/gpu/drm/drm_edid.c         | 2 +-
 drivers/gpu/drm/drm_modeset_lock.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 315770b7e65f..d4a1fab92562 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3425,9 +3425,10 @@ EXPORT_SYMBOL(drm_property_create_enum);
  * @flags: flags specifying the property type
  * @name: name of the property
  * @props: enumeration lists with property bitflags
- * @num_values: number of pre-defined values
+ * @num_props: size of the @props array
+ * @supported_bits: bitmask of all supported enumeration values
  *
- * This creates a new generic drm property which can then be attached to a drm
+ * This creates a new bitmask drm property which can then be attached to a drm
  * object with drm_object_attach_property. The returned property object must be
  * freed with drm_property_destroy.
  *
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1dbf3bc4c6a3..f905c63c0f68 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3433,10 +3433,10 @@ EXPORT_SYMBOL(drm_rgb_quant_range_selectable);
 /**
  * drm_assign_hdmi_deep_color_info - detect whether monitor supports
  * hdmi deep color modes and update drm_display_info if so.
- *
  * @edid: monitor EDID information
  * @info: Updated with maximum supported deep color bpc and color format
  *        if deep color supported.
+ * @connector: DRM connector, used only for debug output
  *
  * Parse the CEA extension according to CEA-861-B.
  * Return true if HDMI deep color supported, false if not or unknown.
diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
index 5280b64a0230..8749fc06570e 100644
--- a/drivers/gpu/drm/drm_modeset_lock.c
+++ b/drivers/gpu/drm/drm_modeset_lock.c
@@ -199,7 +199,7 @@ EXPORT_SYMBOL(drm_modeset_lock_crtc);
 
 /**
  * drm_modeset_legacy_acquire_ctx - find acquire ctx for legacy ioctls
- * crtc: drm crtc
+ * @crtc: drm crtc
  *
  * Legacy ioctl operations like cursor updates or page flips only have per-crtc
  * locking, and store the acquire ctx in the corresponding crtc. All other
-- 
2.0.1

Re: [Intel-gfx] [PATCH] drm: Docbook fixes

[Dave Airlie]

On 30 July 2014 23:36, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Bunch of small leftovers spotted by looking at the make htmldocs output.
>
> I've left out dp mst, there's too much amiss there.

(btw this patch doesn't apply cleanly - modeset_lock seems different).

Just spotted this comment!
I do wonder if I should really be documenting the internals of that
struct in docbook,

The main reason its there is to embed in the driver, I don't think
drivers should be
looking inside it too often, and we certainly shouldn't be generating info about
most of the members in the interface docs for driver writers.

Dave.

Re: [PATCH] drm: Docbook fixes

[Daniel Vetter]

On Tue, Aug 5, 2014 at 4:51 AM, Dave Airlie <airlied@gmail.com> wrote:
> On 30 July 2014 23:36, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> Bunch of small leftovers spotted by looking at the make htmldocs output.
>>
>> I've left out dp mst, there's too much amiss there.
>
> (btw this patch doesn't apply cleanly - modeset_lock seems different).

It is on top of the atomic prep patches, so follows the code move for
modeset_lock_all stuff.

> Just spotted this comment!
> I do wonder if I should really be documenting the internals of that
> struct in docbook,
>
> The main reason its there is to embed in the driver, I don't think
> drivers should be
> looking inside it too often, and we certainly shouldn't be generating info about
> most of the members in the interface docs for driver writers.

Hm right, besides the header structs it's just one parameter that's
missing. I'll augment the patch. On a quick look documentation for
return value semantics is missing a bit, but I'm too lazy to add that
everywhere. Maybe when I have an excuse to read through the dp mst
code ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch