[PATCH 1/2] drm/i915: make CRTC enable/disable asynchronous v2

[PATCH 1/2] drm/i915: make CRTC enable/disable asynchronous v2

[Jesse Barnes]

This lets us return to userspace more quickly and should improve init
and suspend/resume times as well, allowing us to return to userspace
sooner.

This was initially motivated by slow resume time on some machines with
very long panel power sequencing times, and it should also improve boot
time when a full mode set is required.

v2: use a single enable/disable queue (Jesse/Chris)
    fixup locking, test with lockdep (Jesse)
    move hw state checks to sync_crtcs (Jesse)
    make userspace initiated mode sets stay synchronous (Chris)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c      |   2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  12 ++-
 drivers/gpu/drm/i915/intel_display.c | 170 +++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |   2 +-
 4 files changed, 165 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e2bfdda..e7fa84f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -530,7 +530,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 		mutex_lock(&dev->mode_config.mutex);
 		for_each_crtc(dev, crtc) {
 			mutex_lock(&crtc->mutex);
-			dev_priv->display.crtc_disable(crtc);
+			dev_priv->display._crtc_disable(crtc);
 			mutex_unlock(&crtc->mutex);
 		}
 		mutex_unlock(&dev->mode_config.mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bea9ab40..bbfe402 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -447,8 +447,8 @@ struct drm_i915_display_funcs {
 	int (*crtc_mode_set)(struct drm_crtc *crtc,
 			     int x, int y,
 			     struct drm_framebuffer *old_fb);
-	void (*crtc_enable)(struct drm_crtc *crtc);
-	void (*crtc_disable)(struct drm_crtc *crtc);
+	void (*_crtc_enable)(struct drm_crtc *crtc);
+	void (*_crtc_disable)(struct drm_crtc *crtc);
 	void (*off)(struct drm_crtc *crtc);
 	void (*write_eld)(struct drm_connector *connector,
 			  struct drm_crtc *crtc,
@@ -1432,6 +1432,14 @@ struct drm_i915_private {
 	/* Display functions */
 	struct drm_i915_display_funcs display;
 
+	/**
+	 * CRTC work queue handling.  Enable/disable calls are queued
+	 * into the list and processed by the CRTC work function at some
+	 * later time, or inline by a call to intel_sync_crtcs().
+	 */
+	struct list_head crtc_work_queue;
+	struct work_struct crtc_work;
+
 	/* PCH chipset type */
 	enum intel_pch pch_type;
 	unsigned short pch_id;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 731cd01..8c52038 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1973,6 +1973,135 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
 	I915_WRITE(_TRANSA_CHICKEN2, val);
 }
 
+struct intel_crtc_work {
+	/**
+	 * Whether to enable or disable the given CRTC
+	 */
+	bool enable;
+	/**
+	 * CRTC to operate on
+	 */
+	struct intel_crtc *intel_crtc;
+	/**
+	 * Used to link to dev_priv->crtc_work_queue, protected
+	 * by mode_config mutex.
+	 */
+	struct list_head head;
+};
+
+/**
+ * intel_sync_crtcs - complete any pending CRTC enable/disable calls
+ * @dev_priv: i915 driver struct
+ *
+ * Walk the CRTC work queue and perform the enable/disable calls in the
+ * order they were added.
+ *
+ * This function will return when the enable/disable calls have been completed,
+ * and so may take many milliseconds before returning.
+ */
+static void intel_sync_crtcs(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_crtc_work *crtc_work, *tmp;
+
+	WARN(!mutex_is_locked(&dev->mode_config.mutex),
+	     "need mode_config mutex\n");
+
+	list_for_each_entry_safe(crtc_work, tmp, &dev_priv->crtc_work_queue,
+				 head) {
+		struct drm_crtc *crtc = &crtc_work->intel_crtc->base;
+
+		if (crtc_work->enable)
+			dev_priv->display._crtc_enable(crtc);
+		else
+			dev_priv->display._crtc_disable(crtc);
+		list_del(&crtc_work->head);
+		kfree(crtc_work);
+	}
+
+	intel_modeset_check_state(dev);
+}
+
+/**
+ * intel_crtc_work - CRTC queue processing function
+ * @work: crtc_work struct from drm_i915_private
+ *
+ * Just calls intel_sync_crtcs() to take the lock and process the list if any
+ * entries are present.
+ */
+static void intel_crtc_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private, crtc_work);
+	struct drm_device *dev = dev_priv->dev;
+
+	mutex_lock(&dev->mode_config.mutex);
+	intel_sync_crtcs(dev_priv);
+	mutex_unlock(&dev->mode_config.mutex);
+}
+
+/**
+ * intel_queue_crtc_disable - queue a disable on a given crtc
+ * @crtc: drm CRTC to disable
+ *
+ * Allocates an intel_crtc_work struct and adds it to the crtc_work_queue
+ * for later processing by the worker thread or an intel_sync_crtcs() call.
+ */
+void intel_queue_crtc_disable(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_work *work;
+
+	WARN(!mutex_is_locked(&dev->mode_config.mutex),
+	     "need mode_config mutex\n");
+
+	work = kmalloc(sizeof(*work), GFP_KERNEL);
+	if (!work) {
+		dev_priv->display._crtc_disable(&intel_crtc->base);
+		return;
+	}
+
+	work->enable = false;
+	work->intel_crtc = intel_crtc;
+	INIT_LIST_HEAD(&work->head);
+
+	list_add_tail(&dev_priv->crtc_work_queue, &work->head);
+	schedule_work(&dev_priv->crtc_work);
+}
+
+/**
+ * intel_queue_crtc_enable - queue an enable on a given crtc
+ * @crtc: drm CRTC to enable
+ *
+ * Allocates an intel_crtc_work struct and adds it to the crtc_work_queue
+ * for later processing by the worker thread or an intel_sync_crtcs() call.
+ */
+static void intel_queue_crtc_enable(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_work *work;
+
+	WARN(!mutex_is_locked(&dev->mode_config.mutex),
+	     "need mode_config mutex\n");
+
+	work = kmalloc(sizeof(*work), GFP_KERNEL);
+	if (!work) {
+		dev_priv->display._crtc_disable(&intel_crtc->base);
+		return;
+	}
+
+	work->enable = true;
+	work->intel_crtc = intel_crtc;
+	INIT_LIST_HEAD(&work->head);
+
+	list_add_tail(&dev_priv->crtc_work_queue, &work->head);
+	schedule_work(&dev_priv->crtc_work);
+}
+
 /**
  * intel_enable_pipe - enable a pipe, asserting requirements
  * @crtc: crtc responsible for the pipe
@@ -4845,7 +4974,6 @@ static void intel_crtc_update_sarea(struct drm_crtc *crtc,
 void intel_crtc_update_dpms(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *intel_encoder;
 	bool enable = false;
 
@@ -4853,9 +4981,9 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
 		enable |= intel_encoder->connectors_active;
 
 	if (enable)
-		dev_priv->display.crtc_enable(crtc);
+		intel_queue_crtc_enable(crtc);
 	else
-		dev_priv->display.crtc_disable(crtc);
+		intel_queue_crtc_disable(crtc);
 
 	intel_crtc_update_sarea(crtc, enable);
 }
@@ -4869,7 +4997,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	/* crtc should still be enabled when we disable it. */
 	WARN_ON(!crtc->enabled);
 
-	dev_priv->display.crtc_disable(crtc);
+	dev_priv->display._crtc_disable(crtc);
 	intel_crtc_update_sarea(crtc, false);
 	dev_priv->display.off(crtc);
 
@@ -8081,6 +8209,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 		goto fail;
 	}
 
+	intel_sync_crtcs(dev_priv);
+
 	/* we only need to pin inside GTT if cursor is non-phy */
 	mutex_lock(&dev->struct_mutex);
 	if (!INTEL_INFO(dev)->cursor_needs_physical) {
@@ -8166,6 +8296,8 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 	intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX);
 	intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX);
 
+	intel_sync_crtcs(crtc->dev->dev_private);
+
 	if (intel_crtc->active)
 		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
 
@@ -9228,6 +9360,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (work == NULL)
 		return -ENOMEM;
 
+	intel_sync_crtcs(dev_priv);
+
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
@@ -10326,7 +10460,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 
 	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
 		if (intel_crtc->base.enabled)
-			dev_priv->display.crtc_disable(&intel_crtc->base);
+			intel_queue_crtc_disable(&intel_crtc->base);
 	}
 
 	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
@@ -10389,7 +10523,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
 		update_scanline_offset(intel_crtc);
 
-		dev_priv->display.crtc_enable(&intel_crtc->base);
+		intel_queue_crtc_enable(&intel_crtc->base);
 	}
 
 	/* FIXME: add subpixel order */
@@ -10411,6 +10545,7 @@ static int intel_set_mode(struct drm_crtc *crtc,
 
 	ret = __intel_set_mode(crtc, mode, x, y, fb);
 
+	intel_sync_crtcs(crtc->dev->dev_private);
 	if (ret == 0)
 		intel_modeset_check_state(crtc->dev);
 
@@ -11397,8 +11532,8 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
 		dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
-		dev_priv->display.crtc_enable = haswell_crtc_enable;
-		dev_priv->display.crtc_disable = haswell_crtc_disable;
+		dev_priv->display._crtc_enable = haswell_crtc_enable;
+		dev_priv->display._crtc_disable = haswell_crtc_disable;
 		dev_priv->display.off = haswell_crtc_off;
 		dev_priv->display.update_primary_plane =
 			ironlake_update_primary_plane;
@@ -11406,8 +11541,8 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
 		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
-		dev_priv->display.crtc_enable = ironlake_crtc_enable;
-		dev_priv->display.crtc_disable = ironlake_crtc_disable;
+		dev_priv->display._crtc_enable = ironlake_crtc_enable;
+		dev_priv->display._crtc_disable = ironlake_crtc_disable;
 		dev_priv->display.off = ironlake_crtc_off;
 		dev_priv->display.update_primary_plane =
 			ironlake_update_primary_plane;
@@ -11415,8 +11550,8 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
-		dev_priv->display.crtc_enable = valleyview_crtc_enable;
-		dev_priv->display.crtc_disable = i9xx_crtc_disable;
+		dev_priv->display._crtc_enable = valleyview_crtc_enable;
+		dev_priv->display._crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			i9xx_update_primary_plane;
@@ -11424,8 +11559,8 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
-		dev_priv->display.crtc_enable = i9xx_crtc_enable;
-		dev_priv->display.crtc_disable = i9xx_crtc_disable;
+		dev_priv->display._crtc_enable = i9xx_crtc_enable;
+		dev_priv->display._crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			i9xx_update_primary_plane;
@@ -11732,6 +11867,9 @@ void intel_modeset_init(struct drm_device *dev)
 		      INTEL_INFO(dev)->num_pipes,
 		      INTEL_INFO(dev)->num_pipes > 1 ? "s" : "");
 
+	INIT_WORK(&dev_priv->crtc_work, intel_crtc_work);
+	INIT_LIST_HEAD(&dev_priv->crtc_work_queue);
+
 	for_each_pipe(pipe) {
 		intel_crtc_init(dev, pipe);
 		for_each_sprite(pipe, sprite) {
@@ -11860,7 +11998,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 * ...  */
 		plane = crtc->plane;
 		crtc->plane = !plane;
-		dev_priv->display.crtc_disable(&crtc->base);
+		intel_queue_crtc_disable(&crtc->base);
 		crtc->plane = plane;
 
 		/* ... and break all links. */
@@ -12180,8 +12318,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	} else {
 		intel_modeset_update_staged_output_state(dev);
 	}
-
-	intel_modeset_check_state(dev);
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c597b0d..5ef8bdb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -804,7 +804,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_config *pipe_config);
 int intel_format_to_fourcc(int format);
 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
-
+void intel_queue_crtc_disable(struct drm_crtc *crtc);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
-- 
1.9.1

[PATCH 2/2] drm/i915: make userspace mode sets asynchronous

[Jesse Barnes]

Now that we can queue CRTC enable/disable calls for later, we can allow
userspace mode sets to return immediately.  This may mean that userspace
will draw into a buffer that's not yet displayed (which is fine) or that
it may draw into a buffer it thinks is no longer displayed (which could
lead to some visual artifacts until the mode set completes, but is
otherwise harmless).  Page flip and cursor activity will synchronize
with any outstanding activity to avoid problems with the display being
off for those operations.

It should be possible to queue those ops as well though and further
de-couple driver updates of the hw state from userspace queueing of
commands.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8c52038..74310b5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10545,10 +10545,6 @@ static int intel_set_mode(struct drm_crtc *crtc,
 
 	ret = __intel_set_mode(crtc, mode, x, y, fb);
 
-	intel_sync_crtcs(crtc->dev->dev_private);
-	if (ret == 0)
-		intel_modeset_check_state(crtc->dev);
-
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH 1/2] drm/i915: make CRTC enable/disable asynchronous v2

[Chris Wilson]

On Fri, May 30, 2014 at 11:05:21AM -0700, Jesse Barnes wrote:
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e2bfdda..e7fa84f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -530,7 +530,7 @@ static int i915_drm_freeze(struct drm_device *dev)
>  		mutex_lock(&dev->mode_config.mutex);
>  		for_each_crtc(dev, crtc) {
>  			mutex_lock(&crtc->mutex);
> -			dev_priv->display.crtc_disable(crtc);
> +			dev_priv->display._crtc_disable(crtc);

Don't you want to cancel the pending work here or it will be run on
resume - but on resume, we just want to send the hotplug event and let
userspace set itself up in the new configuration.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: make CRTC enable/disable asynchronous v2

[Jesse Barnes]

On Fri, 30 May 2014 19:47:56 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, May 30, 2014 at 11:05:21AM -0700, Jesse Barnes wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index e2bfdda..e7fa84f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -530,7 +530,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  		mutex_lock(&dev->mode_config.mutex);
> >  		for_each_crtc(dev, crtc) {
> >  			mutex_lock(&crtc->mutex);
> > -			dev_priv->display.crtc_disable(crtc);
> > +			dev_priv->display._crtc_disable(crtc);
> 
> Don't you want to cancel the pending work here or it will be run on
> resume - but on resume, we just want to send the hotplug event and let
> userspace set itself up in the new configuration.

No you're right I need to cancel it here and in unload, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: make CRTC enable/disable asynchronous v2

[Chris Wilson]

On Fri, May 30, 2014 at 11:05:21AM -0700, Jesse Barnes wrote:
> @@ -8166,6 +8296,8 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  	intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX);
>  	intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX);
>  
> +	intel_sync_crtcs(crtc->dev->dev_private);
> +
>  	if (intel_crtc->active)
>  		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
>  

Since the pending CRTC enable/disable will set the cursor anyway, this
sync could be avoided if intel_crtc->active was accurate.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: make CRTC enable/disable asynchronous v2

[Chris Wilson]

On Fri, May 30, 2014 at 11:05:21AM -0700, Jesse Barnes wrote:
> +static void intel_queue_crtc_enable(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_work *work;
> +
> +	WARN(!mutex_is_locked(&dev->mode_config.mutex),
> +	     "need mode_config mutex\n");
> +
> +	work = kmalloc(sizeof(*work), GFP_KERNEL);
> +	if (!work) {
> +		dev_priv->display._crtc_disable(&intel_crtc->base);
> +		return;
> +	}
> +
> +	work->enable = true;
> +	work->intel_crtc = intel_crtc;
> +	INIT_LIST_HEAD(&work->head);
(redundant, list_add doesn't care)

> +
> +	list_add_tail(&dev_priv->crtc_work_queue, &work->head);
> +	schedule_work(&dev_priv->crtc_work);
> +}

If we tracked one queued item per crtc, we could avoid the allocation
and allow for elision of pending operations.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: make CRTC enable/disable asynchronous v2

[Jesse Barnes]

On Fri, 30 May 2014 19:56:22 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, May 30, 2014 at 11:05:21AM -0700, Jesse Barnes wrote:
> > +static void intel_queue_crtc_enable(struct drm_crtc *crtc)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_crtc_work *work;
> > +
> > +	WARN(!mutex_is_locked(&dev->mode_config.mutex),
> > +	     "need mode_config mutex\n");
> > +
> > +	work = kmalloc(sizeof(*work), GFP_KERNEL);
> > +	if (!work) {
> > +		dev_priv->display._crtc_disable(&intel_crtc->base);
> > +		return;
> > +	}
> > +
> > +	work->enable = true;
> > +	work->intel_crtc = intel_crtc;
> > +	INIT_LIST_HEAD(&work->head);
> (redundant, list_add doesn't care)

Will fix.

> > +
> > +	list_add_tail(&dev_priv->crtc_work_queue, &work->head);
> > +	schedule_work(&dev_priv->crtc_work);
> > +}
> 
> If we tracked one queued item per crtc, we could avoid the allocation
> and allow for elision of pending operations.

Yeah I thought about that too, might make for a good optimization, but
I figured this was simplest to start with.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] drm/i915: make CRTC enable/disable asynchronous v3

[Jesse Barnes]

This lets us return to userspace more quickly and should improve init
and suspend/resume times as well, allowing us to return to userspace
sooner.

This was initially motivated by slow resume time on some machines with
very long panel power sequencing times, and it should also improve boot
time when a full mode set is required.

v2: use a single enable/disable queue (Jesse/Chris)
    fixup locking, test with lockdep (Jesse)
    move hw state checks to sync_crtcs (Jesse)
    make userspace initiated mode sets stay synchronous (Chris)
v3: take crtc lock around enable/disable (Jesse)
    cancel work on suspend & unload (Chris)
    complete work if alloc fails (Jesse)
    drop unneeded list head init (Chris)
    drop unneeded sync in cusor movement, rely on intel_crtc->active (Chris)
    take mode_config mutex around cursor_set sync call (Jesse)
    fix order of list_add_tail parameters (Jesse)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

fix order of list add
take mutex around sync in cursor_set
---
 drivers/gpu/drm/i915/i915_drv.c      |   3 +-
 drivers/gpu/drm/i915/i915_drv.h      |  12 ++-
 drivers/gpu/drm/i915/intel_display.c | 177 +++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |   2 +-
 4 files changed, 173 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e2bfdda..59a583f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -527,10 +527,11 @@ static int i915_drm_freeze(struct drm_device *dev)
 		 * Disable CRTCs directly since we want to preserve sw state
 		 * for _thaw.
 		 */
+		cancel_work_sync(&dev_priv->crtc_work);
 		mutex_lock(&dev->mode_config.mutex);
 		for_each_crtc(dev, crtc) {
 			mutex_lock(&crtc->mutex);
-			dev_priv->display.crtc_disable(crtc);
+			dev_priv->display._crtc_disable(crtc);
 			mutex_unlock(&crtc->mutex);
 		}
 		mutex_unlock(&dev->mode_config.mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bea9ab40..bbfe402 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -447,8 +447,8 @@ struct drm_i915_display_funcs {
 	int (*crtc_mode_set)(struct drm_crtc *crtc,
 			     int x, int y,
 			     struct drm_framebuffer *old_fb);
-	void (*crtc_enable)(struct drm_crtc *crtc);
-	void (*crtc_disable)(struct drm_crtc *crtc);
+	void (*_crtc_enable)(struct drm_crtc *crtc);
+	void (*_crtc_disable)(struct drm_crtc *crtc);
 	void (*off)(struct drm_crtc *crtc);
 	void (*write_eld)(struct drm_connector *connector,
 			  struct drm_crtc *crtc,
@@ -1432,6 +1432,14 @@ struct drm_i915_private {
 	/* Display functions */
 	struct drm_i915_display_funcs display;
 
+	/**
+	 * CRTC work queue handling.  Enable/disable calls are queued
+	 * into the list and processed by the CRTC work function at some
+	 * later time, or inline by a call to intel_sync_crtcs().
+	 */
+	struct list_head crtc_work_queue;
+	struct work_struct crtc_work;
+
 	/* PCH chipset type */
 	enum intel_pch pch_type;
 	unsigned short pch_id;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 731cd01..d9e5d36 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1973,6 +1973,141 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
 	I915_WRITE(_TRANSA_CHICKEN2, val);
 }
 
+struct intel_crtc_work {
+	/**
+	 * Whether to enable or disable the given CRTC
+	 */
+	bool enable;
+	/**
+	 * CRTC to operate on
+	 */
+	struct intel_crtc *intel_crtc;
+	/**
+	 * Used to link to dev_priv->crtc_work_queue, protected
+	 * by mode_config mutex.
+	 */
+	struct list_head head;
+};
+
+/**
+ * intel_sync_crtcs - complete any pending CRTC enable/disable calls
+ * @dev_priv: i915 driver struct
+ *
+ * Walk the CRTC work queue and perform the enable/disable calls in the
+ * order they were added.
+ *
+ * This function will return when the enable/disable calls have been completed,
+ * and so may take many milliseconds before returning.
+ */
+static void intel_sync_crtcs(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_crtc_work *crtc_work, *tmp;
+
+	WARN(!mutex_is_locked(&dev->mode_config.mutex),
+	     "need mode_config mutex\n");
+
+	list_for_each_entry_safe(crtc_work, tmp, &dev_priv->crtc_work_queue,
+				 head) {
+		struct drm_crtc *crtc = &crtc_work->intel_crtc->base;
+
+		mutex_lock(&crtc->mutex);
+		if (crtc_work->enable)
+			dev_priv->display._crtc_enable(crtc);
+		else
+			dev_priv->display._crtc_disable(crtc);
+		mutex_unlock(&crtc->mutex);
+		list_del(&crtc_work->head);
+		kfree(crtc_work);
+	}
+
+	intel_modeset_check_state(dev);
+}
+
+/**
+ * intel_crtc_work - CRTC queue processing function
+ * @work: crtc_work struct from drm_i915_private
+ *
+ * Just calls intel_sync_crtcs() to take the lock and process the list if any
+ * entries are present.
+ */
+static void intel_crtc_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private, crtc_work);
+	struct drm_device *dev = dev_priv->dev;
+
+	mutex_lock(&dev->mode_config.mutex);
+	intel_sync_crtcs(dev_priv);
+	mutex_unlock(&dev->mode_config.mutex);
+}
+
+/**
+ * intel_queue_crtc_disable - queue a disable on a given crtc
+ * @crtc: drm CRTC to disable
+ *
+ * Allocates an intel_crtc_work struct and adds it to the crtc_work_queue
+ * for later processing by the worker thread or an intel_sync_crtcs() call.
+ */
+void intel_queue_crtc_disable(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_work *work;
+
+	WARN(!mutex_is_locked(&dev->mode_config.mutex),
+	     "need mode_config mutex\n");
+
+	work = kmalloc(sizeof(*work), GFP_KERNEL);
+	if (!work) {
+		intel_sync_crtcs(dev_priv);
+		mutex_lock(&crtc->mutex);
+		dev_priv->display._crtc_disable(&intel_crtc->base);
+		mutex_unlock(&crtc->mutex);
+		return;
+	}
+
+	work->enable = false;
+	work->intel_crtc = intel_crtc;
+
+	list_add_tail(&work->head, &dev_priv->crtc_work_queue);
+	schedule_work(&dev_priv->crtc_work);
+}
+
+/**
+ * intel_queue_crtc_enable - queue an enable on a given crtc
+ * @crtc: drm CRTC to enable
+ *
+ * Allocates an intel_crtc_work struct and adds it to the crtc_work_queue
+ * for later processing by the worker thread or an intel_sync_crtcs() call.
+ */
+static void intel_queue_crtc_enable(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_work *work;
+
+	WARN(!mutex_is_locked(&dev->mode_config.mutex),
+	     "need mode_config mutex\n");
+
+	work = kmalloc(sizeof(*work), GFP_KERNEL);
+	if (!work) {
+		intel_sync_crtcs(dev_priv);
+		mutex_lock(&crtc->mutex);
+		dev_priv->display._crtc_enable(&intel_crtc->base);
+		mutex_unlock(&crtc->mutex);
+		return;
+	}
+
+	work->enable = true;
+	work->intel_crtc = intel_crtc;
+
+	list_add_tail(&work->head, &dev_priv->crtc_work_queue);
+	schedule_work(&dev_priv->crtc_work);
+}
+
 /**
  * intel_enable_pipe - enable a pipe, asserting requirements
  * @crtc: crtc responsible for the pipe
@@ -4845,7 +4980,6 @@ static void intel_crtc_update_sarea(struct drm_crtc *crtc,
 void intel_crtc_update_dpms(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *intel_encoder;
 	bool enable = false;
 
@@ -4853,9 +4987,9 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
 		enable |= intel_encoder->connectors_active;
 
 	if (enable)
-		dev_priv->display.crtc_enable(crtc);
+		intel_queue_crtc_enable(crtc);
 	else
-		dev_priv->display.crtc_disable(crtc);
+		intel_queue_crtc_disable(crtc);
 
 	intel_crtc_update_sarea(crtc, enable);
 }
@@ -4869,7 +5003,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	/* crtc should still be enabled when we disable it. */
 	WARN_ON(!crtc->enabled);
 
-	dev_priv->display.crtc_disable(crtc);
+	dev_priv->display._crtc_disable(crtc);
 	intel_crtc_update_sarea(crtc, false);
 	dev_priv->display.off(crtc);
 
@@ -8081,6 +8215,10 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 		goto fail;
 	}
 
+	mutex_lock(&dev->mode_config.mutex);
+	intel_sync_crtcs(dev_priv);
+	mutex_unlock(&dev->mode_config.mutex);
+
 	/* we only need to pin inside GTT if cursor is non-phy */
 	mutex_lock(&dev->struct_mutex);
 	if (!INTEL_INFO(dev)->cursor_needs_physical) {
@@ -9228,6 +9366,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (work == NULL)
 		return -ENOMEM;
 
+	intel_sync_crtcs(dev_priv);
+
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
@@ -10326,7 +10466,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 
 	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
 		if (intel_crtc->base.enabled)
-			dev_priv->display.crtc_disable(&intel_crtc->base);
+			intel_queue_crtc_disable(&intel_crtc->base);
 	}
 
 	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
@@ -10389,7 +10529,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
 		update_scanline_offset(intel_crtc);
 
-		dev_priv->display.crtc_enable(&intel_crtc->base);
+		intel_queue_crtc_enable(&intel_crtc->base);
 	}
 
 	/* FIXME: add subpixel order */
@@ -10411,6 +10551,7 @@ static int intel_set_mode(struct drm_crtc *crtc,
 
 	ret = __intel_set_mode(crtc, mode, x, y, fb);
 
+	intel_sync_crtcs(crtc->dev->dev_private);
 	if (ret == 0)
 		intel_modeset_check_state(crtc->dev);
 
@@ -11397,8 +11538,8 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
 		dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
-		dev_priv->display.crtc_enable = haswell_crtc_enable;
-		dev_priv->display.crtc_disable = haswell_crtc_disable;
+		dev_priv->display._crtc_enable = haswell_crtc_enable;
+		dev_priv->display._crtc_disable = haswell_crtc_disable;
 		dev_priv->display.off = haswell_crtc_off;
 		dev_priv->display.update_primary_plane =
 			ironlake_update_primary_plane;
@@ -11406,8 +11547,8 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
 		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
-		dev_priv->display.crtc_enable = ironlake_crtc_enable;
-		dev_priv->display.crtc_disable = ironlake_crtc_disable;
+		dev_priv->display._crtc_enable = ironlake_crtc_enable;
+		dev_priv->display._crtc_disable = ironlake_crtc_disable;
 		dev_priv->display.off = ironlake_crtc_off;
 		dev_priv->display.update_primary_plane =
 			ironlake_update_primary_plane;
@@ -11415,8 +11556,8 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
-		dev_priv->display.crtc_enable = valleyview_crtc_enable;
-		dev_priv->display.crtc_disable = i9xx_crtc_disable;
+		dev_priv->display._crtc_enable = valleyview_crtc_enable;
+		dev_priv->display._crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			i9xx_update_primary_plane;
@@ -11424,8 +11565,8 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
-		dev_priv->display.crtc_enable = i9xx_crtc_enable;
-		dev_priv->display.crtc_disable = i9xx_crtc_disable;
+		dev_priv->display._crtc_enable = i9xx_crtc_enable;
+		dev_priv->display._crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_primary_plane =
 			i9xx_update_primary_plane;
@@ -11732,6 +11873,9 @@ void intel_modeset_init(struct drm_device *dev)
 		      INTEL_INFO(dev)->num_pipes,
 		      INTEL_INFO(dev)->num_pipes > 1 ? "s" : "");
 
+	INIT_WORK(&dev_priv->crtc_work, intel_crtc_work);
+	INIT_LIST_HEAD(&dev_priv->crtc_work_queue);
+
 	for_each_pipe(pipe) {
 		intel_crtc_init(dev, pipe);
 		for_each_sprite(pipe, sprite) {
@@ -11860,7 +12004,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 * ...  */
 		plane = crtc->plane;
 		crtc->plane = !plane;
-		dev_priv->display.crtc_disable(&crtc->base);
+		intel_queue_crtc_disable(&crtc->base);
 		crtc->plane = plane;
 
 		/* ... and break all links. */
@@ -12180,8 +12324,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	} else {
 		intel_modeset_update_staged_output_state(dev);
 	}
-
-	intel_modeset_check_state(dev);
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)
@@ -12239,6 +12381,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	 */
 	drm_irq_uninstall(dev);
 	cancel_work_sync(&dev_priv->hotplug_work);
+	cancel_work_sync(&dev_priv->crtc_work);
 	/*
 	 * Due to the hpd irq storm handling the hotplug work can re-arm the
 	 * poll handlers. Hence disable polling after hpd handling is shut down.
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c597b0d..5ef8bdb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -804,7 +804,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_config *pipe_config);
 int intel_format_to_fourcc(int format);
 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
-
+void intel_queue_crtc_disable(struct drm_crtc *crtc);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
-- 
1.9.1

Re: [PATCH] drm/i915: make CRTC enable/disable asynchronous v3

[Chris Wilson]

On Fri, May 30, 2014 at 02:28:52PM -0700, Jesse Barnes wrote:
> @@ -10326,7 +10466,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  
>  	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
>  		if (intel_crtc->base.enabled)
> -			dev_priv->display.crtc_disable(&intel_crtc->base);
> +			intel_queue_crtc_disable(&intel_crtc->base);
>  	}

This one looks odd. prepare_pipes are the pipes we have to turn off in
order to perform the modeset - which needs to be synchronous.

intel_sync_crtcs(prepare_pipes) ?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: make CRTC enable/disable asynchronous v3

[Jesse Barnes]

On Fri, 30 May 2014 23:02:18 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, May 30, 2014 at 02:28:52PM -0700, Jesse Barnes wrote:
> > @@ -10326,7 +10466,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> >  
> >  	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> >  		if (intel_crtc->base.enabled)
> > -			dev_priv->display.crtc_disable(&intel_crtc->base);
> > +			intel_queue_crtc_disable(&intel_crtc->base);
> >  	}
> 
> This one looks odd. prepare_pipes are the pipes we have to turn off in
> order to perform the modeset - which needs to be synchronous.
> 
> intel_sync_crtcs(prepare_pipes) ?

Well, they'll happen in order.  But I was just looking at this code and
in cases where ->mode_set messes with regs rather than staging it for
->crtc_enable we'll lose stuff here.

Until we have that, doing the disables for the prepare_pipe
synchronously is probably the right thing to do.

-- 
Jesse Barnes, Intel Open Source Technology Center