Make init and mode set more asynchronous

Make init and mode set more asynchronous

[Jesse Barnes]

I'm worried about the locking in this...  I've also commented out the
state checker, but that can be re-added as a check after any queued CRTC
changes as another queued item, so should be easy to fix.

This set drastically improves the init time of the i915 module (based on
initcall_debug timing), and should allow suspend and resume to speed up
significantly too, but I'm still working on that.  Open items on the
suspend/resume path include:
  - synchronizing CRTC state against DP up/down (right now I synchronize
    which could be put off)
  - speeding up our DP hot plug on resume, it currently takes forever
    mostly due to VDD toggling.  I think Paulo's stuff should allow us
    to make this faster, but we still have some big delays here
  - get rid of the CRTC sync in the mode_set prepare_pipes; the sync
    just needs to happen before the enable CRTCs are actually enabled...
    so maybe queuing things in order is sufficient

Anyway I'd appreciate some eyes and feedback on this.

Thanks,
Jesse

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 08052f3d..29cc079 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -456,7 +456,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);
+			dev_priv->display._crtc_disable(crtc);
 		mutex_unlock(&dev->mode_config.mutex);
 
 		intel_modeset_suspend_hw(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 42f83f2..4c39bb5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -438,8 +438,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 46ce940..c066a7d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1747,6 +1747,63 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
 	I915_WRITE(_TRANSA_CHICKEN2, val);
 }
 
+static void intel_sync_crtc(struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_device *dev = crtc->dev;
+
+	WARN(!mutex_is_locked(&intel_crtc->base.mutex), "need crtc mutex\n");
+
+	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&intel_crtc->base.mutex);
+	flush_work(&intel_crtc->disable_work);
+	flush_work(&intel_crtc->enable_work);
+	mutex_lock(&intel_crtc->base.mutex);
+	mutex_lock(&dev->mode_config.mutex);
+}
+
+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;
+	struct drm_device *dev = dev_priv->dev;
+
+	mutex_lock(&dev->mode_config.mutex);
+	mutex_lock(&intel_crtc->base.mutex);
+	dev_priv->display._crtc_disable(&intel_crtc->base);
+	mutex_unlock(&intel_crtc->base.mutex);
+	mutex_unlock(&dev->mode_config.mutex);
+}
+
+void intel_queue_crtc_disable(struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	schedule_work(&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);
+	struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private;
+	struct drm_device *dev = dev_priv->dev;
+
+	mutex_lock(&dev->mode_config.mutex);
+	mutex_lock(&intel_crtc->base.mutex);
+	dev_priv->display._crtc_enable(&intel_crtc->base);
+	mutex_unlock(&intel_crtc->base.mutex);
+	mutex_unlock(&dev->mode_config.mutex);
+}
+
+static void intel_queue_crtc_enable(struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	schedule_work(&intel_crtc->enable_work);
+}
+
 /**
  * intel_enable_pipe - enable a pipe, asserting requirements
  * @crtc: crtc responsible for the pipe
@@ -4309,7 +4366,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;
 
@@ -4317,9 +4373,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);
 }
@@ -4334,7 +4390,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);
@@ -7554,6 +7610,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 (!INTEL_INFO(dev)->cursor_needs_physical) {
@@ -7639,6 +7697,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);
 
@@ -8682,6 +8742,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (work == NULL)
 		return -ENOMEM;
 
+	intel_sync_crtc(crtc);
+
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
@@ -9677,8 +9739,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
@@ -9719,7 +9783,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);
 
 	/* FIXME: add subpixel order */
 done:
@@ -9740,8 +9804,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: check after async crtc enable/disable */
+//	if (ret == 0)
+//		intel_modeset_check_state(crtc->dev);
 
 	return ret;
 }
@@ -10306,6 +10371,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)
@@ -10716,29 +10784,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;
 	}
@@ -11142,7 +11210,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. */
@@ -11417,7 +11485,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		intel_modeset_update_staged_output_state(dev);
 	}
 
-	intel_modeset_check_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 a4ffc02..b90f1f5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -384,6 +384,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 {
@@ -736,6 +739,7 @@ void intel_display_set_init_power(struct drm_device *dev, bool enable);
 int valleyview_get_vco(struct drm_i915_private *dev_priv);
 void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_config *pipe_config);
+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

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

[Imre Deak]

On Wed, 2014-03-05 at 14:48 -0800, Jesse Barnes wrote:
> 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.

IMHO this is a good move towards a full command queue based solution for
kms commands, where eventually we have to think less of concurrency.
That is if we can queue all the other kms commands too (flip,
set_plane). But I don't see why that wouldn't be possible.

Btw, why do you have a separate disable and enable queue?

--Imre

> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |   2 +-
>  drivers/gpu/drm/i915/i915_drv.h      |   4 +-
>  drivers/gpu/drm/i915/intel_display.c | 106 ++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_drv.h     |   4 ++
>  4 files changed, 94 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 08052f3d..29cc079 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -456,7 +456,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);
> +			dev_priv->display._crtc_disable(crtc);
>  		mutex_unlock(&dev->mode_config.mutex);
>  
>  		intel_modeset_suspend_hw(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 42f83f2..4c39bb5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -438,8 +438,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 46ce940..c066a7d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1747,6 +1747,63 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
>  	I915_WRITE(_TRANSA_CHICKEN2, val);
>  }
>  
> +static void intel_sync_crtc(struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_device *dev = crtc->dev;
> +
> +	WARN(!mutex_is_locked(&intel_crtc->base.mutex), "need crtc mutex\n");
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +	mutex_unlock(&intel_crtc->base.mutex);
> +	flush_work(&intel_crtc->disable_work);
> +	flush_work(&intel_crtc->enable_work);
> +	mutex_lock(&intel_crtc->base.mutex);
> +	mutex_lock(&dev->mode_config.mutex);
> +}
> +
> +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;
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +	mutex_lock(&intel_crtc->base.mutex);
> +	dev_priv->display._crtc_disable(&intel_crtc->base);
> +	mutex_unlock(&intel_crtc->base.mutex);
> +	mutex_unlock(&dev->mode_config.mutex);
> +}
> +
> +void intel_queue_crtc_disable(struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	schedule_work(&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);
> +	struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private;
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +	mutex_lock(&intel_crtc->base.mutex);
> +	dev_priv->display._crtc_enable(&intel_crtc->base);
> +	mutex_unlock(&intel_crtc->base.mutex);
> +	mutex_unlock(&dev->mode_config.mutex);
> +}
> +
> +static void intel_queue_crtc_enable(struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	schedule_work(&intel_crtc->enable_work);
> +}
> +
>  /**
>   * intel_enable_pipe - enable a pipe, asserting requirements
>   * @crtc: crtc responsible for the pipe
> @@ -4309,7 +4366,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;
>  
> @@ -4317,9 +4373,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);
>  }
> @@ -4334,7 +4390,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);
> @@ -7554,6 +7610,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 (!INTEL_INFO(dev)->cursor_needs_physical) {
> @@ -7639,6 +7697,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);
>  
> @@ -8682,6 +8742,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (work == NULL)
>  		return -ENOMEM;
>  
> +	intel_sync_crtc(crtc);
> +
>  	work->event = event;
>  	work->crtc = crtc;
>  	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
> @@ -9677,8 +9739,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
> @@ -9719,7 +9783,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);
>  
>  	/* FIXME: add subpixel order */
>  done:
> @@ -9740,8 +9804,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: check after async crtc enable/disable */
> +//	if (ret == 0)
> +//		intel_modeset_check_state(crtc->dev);
>  
>  	return ret;
>  }
> @@ -10306,6 +10371,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)
> @@ -10716,29 +10784,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;
>  	}
> @@ -11142,7 +11210,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. */
> @@ -11417,7 +11485,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		intel_modeset_update_staged_output_state(dev);
>  	}
>  
> -	intel_modeset_check_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 a4ffc02..b90f1f5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -384,6 +384,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 {
> @@ -736,6 +739,7 @@ void intel_display_set_init_power(struct drm_device *dev, bool enable);
>  int valleyview_get_vco(struct drm_i915_private *dev_priv);
>  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_config *pipe_config);
> +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);

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

[Jesse Barnes]

On Thu, 06 Mar 2014 01:29:14 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On Wed, 2014-03-05 at 14:48 -0800, Jesse Barnes wrote:
> > 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.
> 
> IMHO this is a good move towards a full command queue based solution for
> kms commands, where eventually we have to think less of concurrency.
> That is if we can queue all the other kms commands too (flip,
> set_plane). But I don't see why that wouldn't be possible.
> 
> Btw, why do you have a separate disable and enable queue?

As opposed to a dedicated work queue for both combined?  I had a
separate queue in an earlier patch, but dropped it while debugging some
other stuff.  We should bring it back to ensure ordering.  That would
remove the need for a few of the syncs, and would also let us queue a
check at the appropriate time on the same queue.

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Imre Deak]

On Wed, 2014-03-05 at 15:39 -0800, Jesse Barnes wrote:
> On Thu, 06 Mar 2014 01:29:14 +0200
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > On Wed, 2014-03-05 at 14:48 -0800, Jesse Barnes wrote:
> > > 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.
> > 
> > IMHO this is a good move towards a full command queue based solution for
> > kms commands, where eventually we have to think less of concurrency.
> > That is if we can queue all the other kms commands too (flip,
> > set_plane). But I don't see why that wouldn't be possible.
> > 
> > Btw, why do you have a separate disable and enable queue?
> 
> As opposed to a dedicated work queue for both combined?  I had a
> separate queue in an earlier patch, but dropped it while debugging some
> other stuff.  We should bring it back to ensure ordering.  That would
> remove the need for a few of the syncs, and would also let us queue a
> check at the appropriate time on the same queue.

Ah, sorry I confused myself. I didn't notice you used the global
workqueue. I'm not sure about the ordering guarantees of the global
workqueue, but if that's not given then yea a dedicated ordered
workqueue would seem better.

In any case if this can move towards a more generic command queue
solution then it's great.

--Imre

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

[Chris Wilson]

On Wed, Mar 05, 2014 at 02:48:26PM -0800, Jesse Barnes wrote:
> @@ -7554,6 +7610,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 (!INTEL_INFO(dev)->cursor_needs_physical) {
> @@ -7639,6 +7697,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);
>  

Hmm. Would be much nicer if touching the cursor didn't incur a delay.
And it would seem to quite easy to capture the state change and queue it
for when the CRTC is re-enabled.

> @@ -8682,6 +8742,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (work == NULL)
>  		return -ENOMEM;
>  
> +	intel_sync_crtc(crtc);
> +
>  	work->event = event;
>  	work->crtc = crtc;
>  	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
> @@ -9677,8 +9739,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
> @@ -9719,7 +9783,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);
>  
>  	/* FIXME: add subpixel order */
>  done:
> @@ -9740,8 +9804,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: check after async crtc enable/disable */
> +//	if (ret == 0)
> +//		intel_modeset_check_state(crtc->dev);
>  
>  	return ret;
>  }
> @@ -10306,6 +10371,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);

I feel using independent work items (remember the global wq is really a
pool of many wq) is horribly prone to deadlocks.

We have the usual caveat that this has an implicit API change in that
setcrtc can now return before the change is complete - and so userspace
may write to a still currently visible scanout. Its not a huge issue
(and is a change I am in favour of), it is just a change in behaviour we
have to be wary of (which also means stating it in the changelog for future
reference).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jesse Barnes]

On Thu, 6 Mar 2014 09:35:23 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Wed, Mar 05, 2014 at 02:48:26PM -0800, Jesse Barnes wrote:
> > @@ -7554,6 +7610,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 (!INTEL_INFO(dev)->cursor_needs_physical) {
> > @@ -7639,6 +7697,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);
> >  
> 
> Hmm. Would be much nicer if touching the cursor didn't incur a delay.
> And it would seem to quite easy to capture the state change and queue it
> for when the CRTC is re-enabled.

Do you think that's worthwhile?  I guess we'll block userspace a bit
here, but presumably the cursor won't be visible until the mode set
completes anyway...

But queuing this stuff is another option.

> 
> > @@ -8682,6 +8742,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  	if (work == NULL)
> >  		return -ENOMEM;
> >  
> > +	intel_sync_crtc(crtc);
> > +
> >  	work->event = event;
> >  	work->crtc = crtc;
> >  	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
> > @@ -9677,8 +9739,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
> > @@ -9719,7 +9783,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);
> >  
> >  	/* FIXME: add subpixel order */
> >  done:
> > @@ -9740,8 +9804,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: check after async crtc enable/disable */
> > +//	if (ret == 0)
> > +//		intel_modeset_check_state(crtc->dev);
> >  
> >  	return ret;
> >  }
> > @@ -10306,6 +10371,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);
> 
> I feel using independent work items (remember the global wq is really a
> pool of many wq) is horribly prone to deadlocks.
> 
> We have the usual caveat that this has an implicit API change in that
> setcrtc can now return before the change is complete - and so userspace
> may write to a still currently visible scanout. Its not a huge issue
> (and is a change I am in favour of), it is just a change in behaviour we
> have to be wary of (which also means stating it in the changelog for future
> reference).

Yeah that's a good point, and if we're not careful it could result in
some visible ugliness.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 2/6] drm/i915: make fbdev initialization asynchronous

[Jesse Barnes]

This gets us out of our init code and out to userspace quite a bit
faster, but does open us up to some bugs given the state of our init
time locking.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_dma.c    | 3 ++-
 drivers/gpu/drm/i915/i915_drv.h    | 1 +
 drivers/gpu/drm/i915/intel_drv.h   | 4 ++--
 drivers/gpu/drm/i915/intel_fbdev.c | 6 ++++--
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7688abc..7c0a61a 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1364,7 +1364,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	 * scanning against hotplug events. Hence do this first and ignore the
 	 * tiny window where we will loose hotplug notifactions.
 	 */
-	intel_fbdev_initial_config(dev);
+	INIT_WORK(&dev_priv->fbdev_work, intel_fbdev_initial_config);
+	schedule_work(&dev_priv->fbdev_work);
 
 	/* Only enable hotplug handling once the fbdev is fully set up. */
 	dev_priv->enable_hotplug_processing = true;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4c39bb5..9024809 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1551,6 +1551,7 @@ typedef struct drm_i915_private {
 	/* list of fbdev register on this device */
 	struct intel_fbdev *fbdev;
 #endif
+	struct work_struct fbdev_work;
 
 	/*
 	 * The console may be contended at resume, but we don't
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b90f1f5..5eeca0f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -775,7 +775,7 @@ void intel_dvo_init(struct drm_device *dev);
 /* legacy fbdev emulation in intel_fbdev.c */
 #ifdef CONFIG_DRM_I915_FBDEV
 extern int intel_fbdev_init(struct drm_device *dev);
-extern void intel_fbdev_initial_config(struct drm_device *dev);
+extern void intel_fbdev_initial_config(struct work_struct *work);
 extern void intel_fbdev_fini(struct drm_device *dev);
 extern void intel_fbdev_set_suspend(struct drm_device *dev, int state);
 extern void intel_fbdev_output_poll_changed(struct drm_device *dev);
@@ -786,7 +786,7 @@ static inline int intel_fbdev_init(struct drm_device *dev)
 	return 0;
 }
 
-static inline void intel_fbdev_initial_config(struct drm_device *dev)
+static inline void intel_fbdev_initial_config(struct work_struct *work)
 {
 }
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 4e4b461..4418c8a 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -457,9 +457,10 @@ int intel_fbdev_init(struct drm_device *dev)
 	return 0;
 }
 
-void intel_fbdev_initial_config(struct drm_device *dev)
+void intel_fbdev_initial_config(struct work_struct *work)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private, fbdev_work);
 
 	/* Due to peculiar init order wrt to hpd handling this is separate. */
 	drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
@@ -471,6 +472,7 @@ void intel_fbdev_fini(struct drm_device *dev)
 	if (!dev_priv->fbdev)
 		return;
 
+	cancel_work_sync(&dev_priv->fbdev_work);
 	intel_fbdev_destroy(dev, dev_priv->fbdev);
 	kfree(dev_priv->fbdev);
 	dev_priv->fbdev = NULL;
-- 
1.8.4.2

Re: [PATCH 2/6] drm/i915: make fbdev initialization asynchronous

[Chris Wilson]

On Wed, Mar 05, 2014 at 02:48:27PM -0800, Jesse Barnes wrote:
> This gets us out of our init code and out to userspace quite a bit
> faster, but does open us up to some bugs given the state of our init
> time locking.

Why are we hand rolling an async task for this? See
http://lists.freedesktop.org/archives/intel-gfx/2010-August/007642.html

And the locking issue was the main reason why we haven't been able to
proceed so far...
-Chris
> 

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/6] drm/i915: make fbdev initialization asynchronous

[Jesse Barnes]

On Thu, 6 Mar 2014 09:12:40 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Wed, Mar 05, 2014 at 02:48:27PM -0800, Jesse Barnes wrote:
> > This gets us out of our init code and out to userspace quite a bit
> > faster, but does open us up to some bugs given the state of our init
> > time locking.
> 
> Why are we hand rolling an async task for this? See
> http://lists.freedesktop.org/archives/intel-gfx/2010-August/007642.html
> 
> And the locking issue was the main reason why we haven't been able to
> proceed so far...

In looking at the async domains it didn't appear that they would
actually save me much if any code in most of these cases.

The locking is worrisome, but I added some extra WARNs and things are
solid across multiple boots, reloads, and suspend/resume cycles.  I
haven't tried lockdep though...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/6] drm/i915: make fbdev initialization asynchronous

[Chris Wilson]

On Thu, Mar 06, 2014 at 08:17:51AM -0800, Jesse Barnes wrote:
> On Thu, 6 Mar 2014 09:12:40 +0000
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Wed, Mar 05, 2014 at 02:48:27PM -0800, Jesse Barnes wrote:
> > > This gets us out of our init code and out to userspace quite a bit
> > > faster, but does open us up to some bugs given the state of our init
> > > time locking.
> > 
> > Why are we hand rolling an async task for this? See
> > http://lists.freedesktop.org/archives/intel-gfx/2010-August/007642.html
> > 
> > And the locking issue was the main reason why we haven't been able to
> > proceed so far...
> 
> In looking at the async domains it didn't appear that they would
> actually save me much if any code in most of these cases.

I agree that it may not save actual lines of code. I am more thinking of
the need to try and integrate with existing work along the same paths. I
presume that the async init debug code is more refined (or at least
could be) etc.
 
> The locking is worrisome, but I added some extra WARNs and things are
> solid across multiple boots, reloads, and suspend/resume cycles.  I
> haven't tried lockdep though...

Things have changed wrt locking, but I still expect that to make lockdep
happy will require some ugly reworking.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/6] drm/i915/dp: put power sequence info into intel_dp

[Jesse Barnes]

Reduces params in a few places and makes workqueueing the eDP caching work
easier.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c  | 23 +++++++++--------------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c512d78..0d5a311 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -246,12 +246,10 @@ intel_hrawclk(struct drm_device *dev)
 
 static void
 intel_dp_init_panel_power_sequencer(struct drm_device *dev,
-				    struct intel_dp *intel_dp,
-				    struct edp_power_seq *out);
+				    struct intel_dp *intel_dp);
 static void
 intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
-					      struct intel_dp *intel_dp,
-					      struct edp_power_seq *out);
+					      struct intel_dp *intel_dp);
 
 static enum pipe
 vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
@@ -1942,7 +1940,6 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
 	enum dpio_channel port = vlv_dport_to_channel(dport);
 	int pipe = intel_crtc->pipe;
-	struct edp_power_seq power_seq;
 	u32 val;
 
 	mutex_lock(&dev_priv->dpio_lock);
@@ -1962,9 +1959,8 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
 
 	if (is_edp(intel_dp)) {
 		/* init power sequencer on this pipe and port */
-		intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
-		intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
-							      &power_seq);
+		intel_dp_init_panel_power_sequencer(dev, intel_dp);
+		intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
 	}
 
 	intel_enable_dp(encoder);
@@ -3529,11 +3525,11 @@ static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
 
 static void
 intel_dp_init_panel_power_sequencer(struct drm_device *dev,
-				    struct intel_dp *intel_dp,
-				    struct edp_power_seq *out)
+				    struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct edp_power_seq cur, vbt, spec, final;
+	struct edp_power_seq *out = &intel_dp->power_seq;
 	u32 pp_on, pp_off, pp_div, pp;
 	int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
 
@@ -3629,10 +3625,10 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
 
 static void
 intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
-					      struct intel_dp *intel_dp,
-					      struct edp_power_seq *seq)
+					      struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct edp_power_seq *seq = &intel_dp->power_seq;
 	u32 pp_on, pp_off, pp_div, port_sel = 0;
 	int div = HAS_PCH_SPLIT(dev) ? intel_pch_rawclk(dev) : intel_hrawclk(dev);
 	int pp_on_reg, pp_off_reg, pp_div_reg;
@@ -3726,7 +3722,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	}
 
 	/* We now know it's not a ghost, init power sequence regs. */
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
+	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
 
 	edid = drm_get_edid(connector, &intel_dp->adapter);
 	if (edid) {
@@ -3775,7 +3771,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum port port = intel_dig_port->port;
-	struct edp_power_seq power_seq = { 0 };
 	const char *name = NULL;
 	int type, error;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5eeca0f..a01fcf0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -504,6 +504,7 @@ struct intel_dp {
 	bool psr_setup_done;
 	bool use_tps3;
 	struct intel_connector *attached_connector;
+	struct edp_power_seq power_seq;
 
 	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
 	/*
-- 
1.8.4.2

[PATCH 4/6] drm: take modeset locks around initial fb helper probing

[Jesse Barnes]

Drivers ought to complain otherwise.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/drm_fb_helper.c  | 2 ++
 drivers/gpu/drm/i915/intel_dp.c  | 4 ++++
 drivers/gpu/drm/i915/intel_drv.h | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index ca100d6..b946217 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1533,9 +1533,11 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 
 	drm_fb_helper_parse_command_line(fb_helper);
 
+	drm_modeset_lock_all(dev);
 	count = drm_fb_helper_probe_connector_modes(fb_helper,
 						    dev->mode_config.max_width,
 						    dev->mode_config.max_height);
+	drm_modeset_unlock_all(dev);
 	/*
 	 * we shouldn't end up with no modes here.
 	 */
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0d5a311..738c4e6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2880,6 +2880,10 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 
 	char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3];
 
+	/* We cache the DPCD for eDP panels */
+	if (intel_dp->dpcd_valid)
+		return true;
+
 	if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd,
 					   sizeof(intel_dp->dpcd)) == 0)
 		return false; /* aux transfer failed */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a01fcf0..9ee412d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -503,8 +503,11 @@ struct intel_dp {
 	unsigned long last_backlight_off;
 	bool psr_setup_done;
 	bool use_tps3;
+	bool dpcd_valid; /* for eDP DPCD caching */
 	struct intel_connector *attached_connector;
+	struct work_struct edp_cache_work;
 	struct edp_power_seq power_seq;
+	const char *i2c_name;
 
 	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
 	/*
-- 
1.8.4.2

Re: [PATCH 4/6] drm: take modeset locks around initial fb helper probing

[Chris Wilson]

On Wed, Mar 05, 2014 at 02:48:29PM -0800, Jesse Barnes wrote:
> Drivers ought to complain otherwise.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/drm_fb_helper.c  | 2 ++
>  drivers/gpu/drm/i915/intel_dp.c  | 4 ++++
>  drivers/gpu/drm/i915/intel_drv.h | 3 +++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index ca100d6..b946217 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1533,9 +1533,11 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>  
>  	drm_fb_helper_parse_command_line(fb_helper);
>  
> +	drm_modeset_lock_all(dev);
>  	count = drm_fb_helper_probe_connector_modes(fb_helper,
>  						    dev->mode_config.max_width,
>  						    dev->mode_config.max_height);
> +	drm_modeset_unlock_all(dev);
>  	/*
>  	 * we shouldn't end up with no modes here.
>  	 */

Spurious chunks below?

> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0d5a311..738c4e6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2880,6 +2880,10 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  
>  	char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3];
>  
> +	/* We cache the DPCD for eDP panels */
> +	if (intel_dp->dpcd_valid)
> +		return true;
> +
>  	if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd,
>  					   sizeof(intel_dp->dpcd)) == 0)
>  		return false; /* aux transfer failed */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a01fcf0..9ee412d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -503,8 +503,11 @@ struct intel_dp {
>  	unsigned long last_backlight_off;
>  	bool psr_setup_done;
>  	bool use_tps3;
> +	bool dpcd_valid; /* for eDP DPCD caching */
>  	struct intel_connector *attached_connector;
> +	struct work_struct edp_cache_work;
>  	struct edp_power_seq power_seq;
> +	const char *i2c_name;
>  
>  	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
>  	/*

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 5/6] drm/i915/dp: push eDP caching out into a work queue

[Jesse Barnes]

It takes awhile to fetch the DPCD and EDID for caching, so take it out
of the critical path to improve init time.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c | 113 +++++++++++++++++++++++++++++-----------
 1 file changed, 82 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 738c4e6..763f235 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3001,6 +3001,20 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, DP_TEST_NAK);
 }
 
+static void intel_flush_edp_cache_work(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp->attached_connector->base.dev;
+
+	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+
+	if (!is_edp(intel_dp))
+		return;
+
+	mutex_unlock(&dev->mode_config.mutex);
+	flush_work(&intel_dp->edp_cache_work);
+	mutex_lock(&dev->mode_config.mutex);
+}
+
 /*
  * According to DP spec
  * 5.1.2:
@@ -3028,6 +3042,8 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 	}
 
+	intel_flush_edp_cache_work(intel_dp);
+
 	/* Now read the DPCD to see if it's actually running */
 	if (!intel_dp_get_dpcd(intel_dp)) {
 		return;
@@ -3063,6 +3079,8 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	uint8_t *dpcd = intel_dp->dpcd;
 	uint8_t type;
 
+	intel_flush_edp_cache_work(intel_dp);
+
 	if (!intel_dp_get_dpcd(intel_dp))
 		return connector_status_disconnected;
 
@@ -3180,13 +3198,23 @@ g4x_dp_detect(struct intel_dp *intel_dp)
 	return intel_dp_detect_dpcd(intel_dp);
 }
 
+static bool intel_connector_has_edid(struct drm_connector *connector)
+{
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+
+	intel_flush_edp_cache_work(intel_dp);
+
+	return intel_connector->edid != NULL;
+}
+
 static struct edid *
 intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 
 	/* use cached edid if we have one */
-	if (intel_connector->edid) {
+	if (intel_connector_has_edid(connector)) {
 		/* invalid edid */
 		if (IS_ERR(intel_connector->edid))
 			return NULL;
@@ -3203,7 +3231,7 @@ intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *ada
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 
 	/* use cached edid if we have one */
-	if (intel_connector->edid) {
+	if (intel_connector_has_edid(connector)) {
 		/* invalid edid */
 		if (IS_ERR(intel_connector->edid))
 			return 0;
@@ -3226,6 +3254,8 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	enum drm_connector_status status;
 	struct edid *edid = NULL;
 
+	intel_flush_edp_cache_work(intel_dp);
+
 	intel_runtime_pm_get(dev_priv);
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
@@ -3420,6 +3450,7 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	drm_encoder_cleanup(encoder);
 	if (is_edp(intel_dp)) {
 		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
+		cancel_work_sync(&intel_dp->edp_cache_work);
 		mutex_lock(&dev->mode_config.mutex);
 		edp_panel_vdd_off_sync(intel_dp);
 		mutex_unlock(&dev->mode_config.mutex);
@@ -3693,21 +3724,30 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 		      I915_READ(pp_div_reg));
 }
 
-static bool intel_edp_init_connector(struct intel_dp *intel_dp,
-				     struct intel_connector *intel_connector,
-				     struct edp_power_seq *power_seq)
+static void intel_edp_cache_work(struct work_struct *work)
 {
+	struct intel_dp *intel_dp = container_of(work, struct intel_dp,
+						 edp_cache_work);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct drm_connector *connector = &intel_connector->base;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *fixed_mode = NULL;
+	enum port port = intel_dig_port->port;
 	bool has_dpcd;
 	struct drm_display_mode *scan;
 	struct edid *edid;
+	int error;
 
-	if (!is_edp(intel_dp))
-		return true;
+	mutex_lock(&dev->mode_config.mutex);
+
+	error = intel_dp_i2c_init(intel_dp, intel_connector,
+				  intel_dp->i2c_name);
+	WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
+	     error, port_name(port));
+
+	intel_dp->psr_setup_done = false;
 
 	/* Cache DPCD and EDID for edp. */
 	edp_panel_vdd_on(intel_dp);
@@ -3719,10 +3759,15 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 			dev_priv->no_aux_handshake =
 				intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
 				DP_NO_AUX_HANDSHAKE_LINK_TRAINING;
+		intel_dp->dpcd_valid = true;
 	} else {
-		/* if this fails, presume the device is a ghost */
-		DRM_INFO("failed to retrieve link info, disabling eDP\n");
-		return false;
+		i2c_del_adapter(&intel_dp->adapter);
+		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
+		edp_panel_vdd_off_sync(intel_dp);
+		drm_sysfs_connector_remove(connector);
+		drm_connector_cleanup(connector);
+		mutex_unlock(&dev->mode_config.mutex);
+		return;
 	}
 
 	/* We now know it's not a ghost, init power sequence regs. */
@@ -3750,7 +3795,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 			break;
 		}
 	}
-
 	/* fallback to VBT if available for eDP */
 	if (!fixed_mode && dev_priv->vbt.lfp_lvds_vbt_mode) {
 		fixed_mode = drm_mode_duplicate(dev,
@@ -3760,9 +3804,23 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	}
 
 	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
-	intel_panel_setup_backlight(connector);
 
-	return true;
+	mutex_unlock(&dev->mode_config.mutex);
+}
+
+static void intel_edp_init_connector(struct intel_dp *intel_dp,
+				     struct intel_connector *intel_connector)
+{
+	struct drm_connector *connector = &intel_connector->base;
+
+	if (!is_edp(intel_dp))
+		return;
+
+	INIT_WORK(&intel_dp->edp_cache_work, intel_edp_cache_work);
+
+	schedule_work(&intel_dp->edp_cache_work);
+
+	intel_panel_setup_backlight(connector);
 }
 
 bool
@@ -3817,8 +3875,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	connector->interlace_allowed = true;
 	connector->doublescan_allowed = 0;
 
-	INIT_DELAYED_WORK(&intel_dp->panel_vdd_work,
-			  edp_panel_vdd_work);
+	INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, edp_panel_vdd_work);
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
 	drm_sysfs_connector_add(connector);
@@ -3873,27 +3930,21 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 
 	if (is_edp(intel_dp)) {
 		intel_dp_init_panel_power_timestamps(intel_dp);
-		intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
+		intel_dp_init_panel_power_sequencer(dev, intel_dp);
 	}
 
-	error = intel_dp_i2c_init(intel_dp, intel_connector, name);
-	WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
-	     error, port_name(port));
+	intel_dp->i2c_name = name;
+
+	if (!is_edp(intel_dp)) {
+		error = intel_dp_i2c_init(intel_dp, intel_connector, name);
+		WARN(error,
+		     "intel_dp_i2c_init failed with error %d for port %c\n",
+		     error, port_name(port));
+	}
 
 	intel_dp->psr_setup_done = false;
 
-	if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) {
-		i2c_del_adapter(&intel_dp->adapter);
-		if (is_edp(intel_dp)) {
-			cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
-			mutex_lock(&dev->mode_config.mutex);
-			edp_panel_vdd_off_sync(intel_dp);
-			mutex_unlock(&dev->mode_config.mutex);
-		}
-		drm_sysfs_connector_remove(connector);
-		drm_connector_cleanup(connector);
-		return false;
-	}
+	intel_edp_init_connector(intel_dp, intel_connector);
 
 	intel_dp_add_properties(intel_dp, connector);
 
-- 
1.8.4.2

Re: [PATCH 5/6] drm/i915/dp: push eDP caching out into a work queue

[Ville Syrjälä]

On Wed, Mar 05, 2014 at 02:48:30PM -0800, Jesse Barnes wrote:
> It takes awhile to fetch the DPCD and EDID for caching, so take it out
> of the critical path to improve init time.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 113 +++++++++++++++++++++++++++++-----------
>  1 file changed, 82 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 738c4e6..763f235 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3001,6 +3001,20 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  	intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, DP_TEST_NAK);
>  }
>  
> +static void intel_flush_edp_cache_work(struct intel_dp *intel_dp)
> +{
> +	struct drm_device *dev = intel_dp->attached_connector->base.dev;
> +
> +	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> +
> +	if (!is_edp(intel_dp))
> +		return;
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +	flush_work(&intel_dp->edp_cache_work);
> +	mutex_lock(&dev->mode_config.mutex);

This feels like deadlock land to me. If we drop mode_config.mutex
someone else might grab it and then get blocked on the crtc mutex
we're already holding, and then we try to re-grab mode_config.mutex...

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 5/6] drm/i915/dp: push eDP caching out into a work queue

[Jesse Barnes]

On Thu, 6 Mar 2014 11:28:13 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Wed, Mar 05, 2014 at 02:48:30PM -0800, Jesse Barnes wrote:
> > It takes awhile to fetch the DPCD and EDID for caching, so take it out
> > of the critical path to improve init time.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 113 +++++++++++++++++++++++++++++-----------
> >  1 file changed, 82 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 738c4e6..763f235 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3001,6 +3001,20 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  	intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, DP_TEST_NAK);
> >  }
> >  
> > +static void intel_flush_edp_cache_work(struct intel_dp *intel_dp)
> > +{
> > +	struct drm_device *dev = intel_dp->attached_connector->base.dev;
> > +
> > +	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> > +
> > +	if (!is_edp(intel_dp))
> > +		return;
> > +
> > +	mutex_unlock(&dev->mode_config.mutex);
> > +	flush_work(&intel_dp->edp_cache_work);
> > +	mutex_lock(&dev->mode_config.mutex);
> 
> This feels like deadlock land to me. If we drop mode_config.mutex
> someone else might grab it and then get blocked on the crtc mutex
> we're already holding, and then we try to re-grab mode_config.mutex...

Yeah I could use unlock_all here instead, or be more careful about
dropping the specific crtc mutex we need.  I did that on the crtc side
of things but obviously missed it here for cases where we'll hold the
crtc lock in this path.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/6] drm/i915/dp: make sure VDD is on around link status checking

[Jesse Barnes]

In the hotplug case, nothing was grabbing VDD, leading to some warnings.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 763f235..78c883e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3037,10 +3037,13 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (WARN_ON(!intel_encoder->base.crtc))
 		return;
 
+	edp_panel_vdd_on(intel_dp);
 	/* Try to read receiver status if the link appears to be up */
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		edp_panel_vdd_off(intel_dp, false);
 		return;
 	}
+	edp_panel_vdd_off(intel_dp, false);
 
 	intel_flush_edp_cache_work(intel_dp);
 
-- 
1.8.4.2