[RFC] async CRTC enable/disable hack

[RFC] async CRTC enable/disable hack

[Jesse Barnes]

Obviously still need a lot of work (and I didn't quite get the patch
split correctly, I meant commit the first bits earlier).

I think the approach may be sound though, and is actually not too hard
to get right with all the cross checking we have in place.  Things to
fix:
  - modeset cross check - we don't want to sync after a mode set just to
    check, maybe we could put this off until the actual enable happens
  - split flushing of disable and enable to actually make full mode sets
    fast

Anyway this is just a sketch of something we might do, definitely not
intended for upstream anytime soon.

Thanks,
Jesse

[PATCH 1/2] drm/i915: wrap crtc enable/disable

[Jesse Barnes]

This allows us to hide queuing of enable/disable later.
---
 drivers/gpu/drm/i915/i915_drv.c      |  2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  4 +-
 drivers/gpu/drm/i915/intel_display.c | 83 +++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 4 files changed, 71 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index af9fe02..8438e4e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -543,7 +543,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 		 */
 		mutex_lock(&dev->mode_config.mutex);
 		list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
-			dev_priv->display.crtc_disable(crtc);
+			intel_queue_crtc_disable(crtc);
 		mutex_unlock(&dev->mode_config.mutex);
 
 		DRM_ERROR("disabled crtcs\n");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index efc57fe..a1088fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -407,8 +407,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,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e5c1850..006ec10 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1725,6 +1725,46 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
 	I915_WRITE(_TRANSA_CHICKEN2, val);
 }
 
+static void intel_crtc_disable_work(struct work_struct *work)
+{
+	struct intel_crtc *intel_crtc = container_of(work, struct intel_crtc,
+						     disable_work);
+}
+
+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);
+
+	queue_work(dev_priv->wq, &intel_crtc->disable_work);
+}
+
+static void intel_crtc_enable_work(struct work_struct *work)
+{
+	struct intel_crtc *intel_crtc = container_of(work, struct intel_crtc,
+						     enable_work);
+}
+
+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);
+
+	queue_work(dev_priv->wq, &intel_crtc->enable_work);
+}
+
+static void intel_sync_crtc(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);
+
+	flush_work(&intel_crtc->disable_work);
+	flush_work(&intel_crtc->enable_work);
+}
+
 /**
  * intel_enable_pipe - enable a pipe, asserting requirements
  * @dev_priv: i915 private structure
@@ -4288,7 +4328,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;
 
@@ -4296,9 +4335,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);
 }
@@ -4313,7 +4352,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->eld_vld = false;
 	intel_crtc_update_sarea(crtc, false);
 	dev_priv->display.off(crtc);
@@ -7519,6 +7558,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 		goto fail;
 	}
 
+	intel_sync_crtc(crtc);
+
 	/* we only need to pin inside GTT if cursor is non-phy */
 	mutex_lock(&dev->struct_mutex);
 	if (!dev_priv->info->cursor_needs_physical) {
@@ -7604,6 +7645,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_crtc(crtc);
+
 	if (intel_crtc->active)
 		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
 
@@ -8602,6 +8645,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (work == NULL)
 		return -ENOMEM;
 
+	/* Complete any pending CRTC enable/disable */
+	intel_sync_crtc(crtc);
+
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
@@ -9608,8 +9654,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 		intel_crtc_disable(&intel_crtc->base);
 
 	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
-		if (intel_crtc->base.enabled)
-			dev_priv->display.crtc_disable(&intel_crtc->base);
+		if (intel_crtc->base.enabled) {
+			intel_queue_crtc_disable(&intel_crtc->base);
+			intel_sync_crtc(&intel_crtc->base);
+		}
 	}
 
 	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
@@ -9641,7 +9689,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
-		dev_priv->display.crtc_enable(&intel_crtc->base);
+		intel_queue_crtc_enable(&intel_crtc->base);
 
 	if (modeset_pipes) {
 		/* Store real post-adjustment hardware mode. */
@@ -10145,6 +10193,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
+
+	INIT_WORK(intel_crtc->enable_work, intel_crtc_enable_work);
+	INIT_WORK(intel_crtc->disable_work, intel_crtc_disable_work);
 }
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
@@ -10540,29 +10591,29 @@ static void intel_init_display(struct drm_device *dev)
 	if (HAS_DDI(dev)) {
 		dev_priv->display.get_pipe_config = haswell_get_pipe_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_plane = ironlake_update_plane;
 	} else if (HAS_PCH_SPLIT(dev)) {
 		dev_priv->display.get_pipe_config = ironlake_get_pipe_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_plane = ironlake_update_plane;
 	} else if (IS_VALLEYVIEW(dev)) {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_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_plane = i9xx_update_plane;
 	} else {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_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_plane = i9xx_update_plane;
 	}
@@ -10953,7 +11004,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. */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ea62673..bfae2ef 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -707,6 +707,7 @@ void hsw_enable_ips(struct intel_crtc *crtc);
 void hsw_disable_ips(struct intel_crtc *crtc);
 void intel_display_set_init_power(struct drm_device *dev, bool enable);
 int valleyview_get_vco(struct drm_i915_private *dev_priv);
+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.8.4.2

[PATCH 2/2] drm/i915: make crtc enable/disable asynchronous

[Jesse Barnes]

The intent is to get back to userspace as quickly as possible so it can
start doing drawing or whatever.  It should also allow our
suspend/resume/init time to improve a lot.
---
 drivers/gpu/drm/i915/i915_irq.c      | 10 +++++++++-
 drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
 3 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 74918e7..9490d32 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -897,8 +897,16 @@ static void i915_hotplug_work_func(struct work_struct *work)
 		intel_connector = to_intel_connector(connector);
 		intel_encoder = intel_connector->encoder;
 		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
-			if (intel_encoder->hot_plug)
+			if (intel_encoder->hot_plug) {
+				struct drm_crtc *crtc =
+					intel_encoder->base.crtc;
+				if (crtc) {
+					mutex_lock(&crtc->mutex);
+					intel_sync_crtc(intel_encoder->base.crtc);
+					mutex_unlock(&crtc->mutex);
+				}
 				intel_encoder->hot_plug(intel_encoder);
+			}
 			if (intel_hpd_irq_event(dev, connector))
 				changed = true;
 		}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 006ec10..302aae1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1729,6 +1729,11 @@ static void intel_crtc_disable_work(struct work_struct *work)
 {
 	struct intel_crtc *intel_crtc = container_of(work, struct intel_crtc,
 						     disable_work);
+	struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private;
+
+	mutex_lock(&intel_crtc->base.mutex);
+	dev_priv->display._crtc_disable(&intel_crtc->base);
+	mutex_unlock(&intel_crtc->base.mutex);
 }
 
 void intel_queue_crtc_disable(struct drm_crtc *crtc)
@@ -1744,6 +1749,11 @@ static void intel_crtc_enable_work(struct work_struct *work)
 {
 	struct intel_crtc *intel_crtc = container_of(work, struct intel_crtc,
 						     enable_work);
+	struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private;
+
+	mutex_lock(&intel_crtc->base.mutex);
+	dev_priv->display._crtc_enable(&intel_crtc->base);
+	mutex_unlock(&intel_crtc->base.mutex);
 }
 
 static void intel_queue_crtc_enable(struct drm_crtc *crtc)
@@ -1755,14 +1765,16 @@ static void intel_queue_crtc_enable(struct drm_crtc *crtc)
 	queue_work(dev_priv->wq, &intel_crtc->enable_work);
 }
 
-static void intel_sync_crtc(struct drm_crtc *crtc)
+void intel_sync_crtc(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);
 
+	WARN(!mutex_is_locked(&intel_crtc->base.mutex), "need crtc mutex\n");
+
+	mutex_unlock(&intel_crtc->base.mutex);
 	flush_work(&intel_crtc->disable_work);
 	flush_work(&intel_crtc->enable_work);
+	mutex_lock(&intel_crtc->base.mutex);
 }
 
 /**
@@ -9723,8 +9735,9 @@ static int intel_set_mode(struct drm_crtc *crtc,
 
 	ret = __intel_set_mode(crtc, mode, x, y, fb);
 
-	if (ret == 0)
-		intel_modeset_check_state(crtc->dev);
+	/* FIXME: need to check after the CRTC changes have been applied */
+//	if (ret == 0)
+//		intel_modeset_check_state(crtc->dev);
 
 	return ret;
 }
@@ -10194,8 +10207,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
 
-	INIT_WORK(intel_crtc->enable_work, intel_crtc_enable_work);
-	INIT_WORK(intel_crtc->disable_work, intel_crtc_disable_work);
+	INIT_WORK(&intel_crtc->enable_work, intel_crtc_enable_work);
+	INIT_WORK(&intel_crtc->disable_work, intel_crtc_disable_work);
 }
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bfae2ef..b187c51 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -373,6 +373,9 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	struct work_struct enable_work;
+	struct work_struct disable_work;
 };
 
 struct intel_plane_wm_parameters {
@@ -708,6 +711,7 @@ void hsw_disable_ips(struct intel_crtc *crtc);
 void intel_display_set_init_power(struct drm_device *dev, bool enable);
 int valleyview_get_vco(struct drm_i915_private *dev_priv);
 void intel_queue_crtc_disable(struct drm_crtc *crtc);
+void intel_sync_crtc(struct drm_crtc *crtc);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
-- 
1.8.4.2

Re: [RFC] async CRTC enable/disable hack

[Daniel Vetter]

On Fri, Dec 13, 2013 at 8:26 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Obviously still need a lot of work (and I didn't quite get the patch
> split correctly, I meant commit the first bits earlier).
>
> I think the approach may be sound though, and is actually not too hard
> to get right with all the cross checking we have in place.  Things to
> fix:
>   - modeset cross check - we don't want to sync after a mode set just to
>     check, maybe we could put this off until the actual enable happens
>   - split flushing of disable and enable to actually make full mode sets
>     fast

When I've last pondered this async_domains looked like a suitable
thing. We can create as many of them as we want or reuse them, and
they have a nice primitive for syncing up with all previously launched
async work items. I haven't checked how well they take being called
from different processes (since we still want to do the very last sync
from a work item to fully asynchronously launch the the state
checker). But my impression was that they fit the bill or are at least
fairly close.

We could also try to abuse them in the driver load/suspend/resume code
and e.g. run the gem restore in parallel with early modeset stuff. I'm
not sure whether the syncing would be flexible enough though. But
worth a shot imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch