[PATCH 1/7] drm/i915: Fix deadlock witha the pipe A quirk during resume

[PATCH 1/7] drm/i915: Fix deadlock witha the pipe A quirk during resume

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pass down the correct acquire context to the pipe A quirk load detect
hack during display resume. Avoids deadlocking the entire thing.

Cc: stable@vger.kernel.org
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ed41b59ee8e3..2f76a63efe8c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -120,7 +120,8 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
 static void skylake_pfit_enable(struct intel_crtc *crtc);
 static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
 static void ironlake_pfit_enable(struct intel_crtc *crtc);
-static void intel_modeset_setup_hw_state(struct drm_device *dev);
+static void intel_modeset_setup_hw_state(struct drm_device *dev,
+					 struct drm_modeset_acquire_ctx *ctx);
 static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
 
 struct intel_limit {
@@ -3461,7 +3462,7 @@ __intel_display_resume(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	int i, ret;
 
-	intel_modeset_setup_hw_state(dev);
+	intel_modeset_setup_hw_state(dev, ctx);
 	i915_redisable_vga(to_i915(dev));
 
 	if (!state)
@@ -15094,7 +15095,7 @@ int intel_modeset_init(struct drm_device *dev)
 	intel_setup_outputs(dev_priv);
 
 	drm_modeset_lock_all(dev);
-	intel_modeset_setup_hw_state(dev);
+	intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
 	drm_modeset_unlock_all(dev);
 
 	for_each_intel_crtc(dev, crtc) {
@@ -15131,13 +15132,13 @@ int intel_modeset_init(struct drm_device *dev)
 	return 0;
 }
 
-static void intel_enable_pipe_a(struct drm_device *dev)
+static void intel_enable_pipe_a(struct drm_device *dev,
+				struct drm_modeset_acquire_ctx *ctx)
 {
 	struct intel_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 	struct drm_connector *crt = NULL;
 	struct intel_load_detect_pipe load_detect_temp;
-	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
 	int ret;
 
 	/* We can't just switch on the pipe A, we need to set things up with a
@@ -15209,7 +15210,8 @@ static bool has_pch_trancoder(struct drm_i915_private *dev_priv,
 		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
 }
 
-static void intel_sanitize_crtc(struct intel_crtc *crtc)
+static void intel_sanitize_crtc(struct intel_crtc *crtc,
+				struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -15265,7 +15267,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 * resume. Force-enable the pipe to fix this, the update_dpms
 		 * call below we restore the pipe to the right state, but leave
 		 * the required bits on. */
-		intel_enable_pipe_a(dev);
+		intel_enable_pipe_a(dev, ctx);
 	}
 
 	/* Adjust the state of the output pipe according to whether we
@@ -15568,7 +15570,8 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
  * and sanitizes it to the current state
  */
 static void
-intel_modeset_setup_hw_state(struct drm_device *dev)
+intel_modeset_setup_hw_state(struct drm_device *dev,
+			     struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	enum pipe pipe;
@@ -15588,7 +15591,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 	for_each_pipe(dev_priv, pipe) {
 		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
 
-		intel_sanitize_crtc(crtc);
+		intel_sanitize_crtc(crtc, ctx);
 		intel_dump_pipe_config(crtc, crtc->config,
 				       "[setup_hw_state]");
 	}
-- 
2.10.2

[PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If intel_crtc_disable_noatomic() were to ever get called during resume
e'd end up deadlocking since resume has its own acqcuire_ctx but
intel_crtc_disable_noatomic() still tries to use the
mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.

Cc: stable@vger.kernel.org
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2f76a63efe8c..4e3c64ed4131 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
 		intel_update_watermarks(intel_crtc);
 }
 
-static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
+static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
+					struct drm_modeset_acquire_ctx *ctx)
 {
 	struct intel_encoder *encoder;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 		return;
 	}
 
-	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
+	state->acquire_ctx = ctx;
 
 	/* Everything's already locked, -EDEADLK can't happen. */
 	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
@@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 		plane = crtc->plane;
 		crtc->base.primary->state->visible = true;
 		crtc->plane = !plane;
-		intel_crtc_disable_noatomic(&crtc->base);
+		intel_crtc_disable_noatomic(&crtc->base, ctx);
 		crtc->plane = plane;
 	}
 
@@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 	/* Adjust the state of the output pipe according to whether we
 	 * have active connectors/encoders. */
 	if (crtc->active && !intel_crtc_has_encoders(crtc))
-		intel_crtc_disable_noatomic(&crtc->base);
+		intel_crtc_disable_noatomic(&crtc->base, ctx);
 
 	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
 		/*
-- 
2.10.2

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Fix deadlock witha the pipe A quirk during resume

[Jani Nikula]

On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Pass down the correct acquire context to the pipe A quirk load detect
> hack during display resume. Avoids deadlocking the entire thing.

Have we seen this in the wild? References?

BR,
Jani.

>
> Cc: stable@vger.kernel.org
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ed41b59ee8e3..2f76a63efe8c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -120,7 +120,8 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
>  static void skylake_pfit_enable(struct intel_crtc *crtc);
>  static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
>  static void ironlake_pfit_enable(struct intel_crtc *crtc);
> -static void intel_modeset_setup_hw_state(struct drm_device *dev);
> +static void intel_modeset_setup_hw_state(struct drm_device *dev,
> +					 struct drm_modeset_acquire_ctx *ctx);
>  static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
>  
>  struct intel_limit {
> @@ -3461,7 +3462,7 @@ __intel_display_resume(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	int i, ret;
>  
> -	intel_modeset_setup_hw_state(dev);
> +	intel_modeset_setup_hw_state(dev, ctx);
>  	i915_redisable_vga(to_i915(dev));
>  
>  	if (!state)
> @@ -15094,7 +15095,7 @@ int intel_modeset_init(struct drm_device *dev)
>  	intel_setup_outputs(dev_priv);
>  
>  	drm_modeset_lock_all(dev);
> -	intel_modeset_setup_hw_state(dev);
> +	intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
>  	drm_modeset_unlock_all(dev);
>  
>  	for_each_intel_crtc(dev, crtc) {
> @@ -15131,13 +15132,13 @@ int intel_modeset_init(struct drm_device *dev)
>  	return 0;
>  }
>  
> -static void intel_enable_pipe_a(struct drm_device *dev)
> +static void intel_enable_pipe_a(struct drm_device *dev,
> +				struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct intel_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  	struct drm_connector *crt = NULL;
>  	struct intel_load_detect_pipe load_detect_temp;
> -	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
>  	int ret;
>  
>  	/* We can't just switch on the pipe A, we need to set things up with a
> @@ -15209,7 +15210,8 @@ static bool has_pch_trancoder(struct drm_i915_private *dev_priv,
>  		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
>  }
>  
> -static void intel_sanitize_crtc(struct intel_crtc *crtc)
> +static void intel_sanitize_crtc(struct intel_crtc *crtc,
> +				struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -15265,7 +15267,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		 * resume. Force-enable the pipe to fix this, the update_dpms
>  		 * call below we restore the pipe to the right state, but leave
>  		 * the required bits on. */
> -		intel_enable_pipe_a(dev);
> +		intel_enable_pipe_a(dev, ctx);
>  	}
>  
>  	/* Adjust the state of the output pipe according to whether we
> @@ -15568,7 +15570,8 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
>   * and sanitizes it to the current state
>   */
>  static void
> -intel_modeset_setup_hw_state(struct drm_device *dev)
> +intel_modeset_setup_hw_state(struct drm_device *dev,
> +			     struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum pipe pipe;
> @@ -15588,7 +15591,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
>  	for_each_pipe(dev_priv, pipe) {
>  		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>  
> -		intel_sanitize_crtc(crtc);
> +		intel_sanitize_crtc(crtc, ctx);
>  		intel_dump_pipe_config(crtc, crtc->config,
>  				       "[setup_hw_state]");
>  	}

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

[Jani Nikula]

On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> If intel_crtc_disable_noatomic() were to ever get called during resume
> e'd end up deadlocking since resume has its own acqcuire_ctx but
> intel_crtc_disable_noatomic() still tries to use the
> mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.

Same here, have we seen this, or "just" theoretical?

BR,
Jani.

>
> Cc: stable@vger.kernel.org
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2f76a63efe8c..4e3c64ed4131 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
>  		intel_update_watermarks(intel_crtc);
>  }
>  
> -static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> +static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> +					struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct intel_encoder *encoder;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>  		return;
>  	}
>  
> -	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
> +	state->acquire_ctx = ctx;
>  
>  	/* Everything's already locked, -EDEADLK can't happen. */
>  	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> @@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  		plane = crtc->plane;
>  		crtc->base.primary->state->visible = true;
>  		crtc->plane = !plane;
> -		intel_crtc_disable_noatomic(&crtc->base);
> +		intel_crtc_disable_noatomic(&crtc->base, ctx);
>  		crtc->plane = plane;
>  	}
>  
> @@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  	/* Adjust the state of the output pipe according to whether we
>  	 * have active connectors/encoders. */
>  	if (crtc->active && !intel_crtc_has_encoders(crtc))
> -		intel_crtc_disable_noatomic(&crtc->base);
> +		intel_crtc_disable_noatomic(&crtc->base, ctx);
>  
>  	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
>  		/*

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Fix deadlock witha the pipe A quirk during resume

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 05:47:45PM +0300, Jani Nikula wrote:
> On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > Pass down the correct acquire context to the pipe A quirk load detect
> > hack during display resume. Avoids deadlocking the entire thing.
> 
> Have we seen this in the wild? References?

My 830.

> 
> BR,
> Jani.
> 
> >
> > Cc: stable@vger.kernel.org
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ed41b59ee8e3..2f76a63efe8c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -120,7 +120,8 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
> >  static void skylake_pfit_enable(struct intel_crtc *crtc);
> >  static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
> >  static void ironlake_pfit_enable(struct intel_crtc *crtc);
> > -static void intel_modeset_setup_hw_state(struct drm_device *dev);
> > +static void intel_modeset_setup_hw_state(struct drm_device *dev,
> > +					 struct drm_modeset_acquire_ctx *ctx);
> >  static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
> >  
> >  struct intel_limit {
> > @@ -3461,7 +3462,7 @@ __intel_display_resume(struct drm_device *dev,
> >  	struct drm_crtc *crtc;
> >  	int i, ret;
> >  
> > -	intel_modeset_setup_hw_state(dev);
> > +	intel_modeset_setup_hw_state(dev, ctx);
> >  	i915_redisable_vga(to_i915(dev));
> >  
> >  	if (!state)
> > @@ -15094,7 +15095,7 @@ int intel_modeset_init(struct drm_device *dev)
> >  	intel_setup_outputs(dev_priv);
> >  
> >  	drm_modeset_lock_all(dev);
> > -	intel_modeset_setup_hw_state(dev);
> > +	intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
> >  	drm_modeset_unlock_all(dev);
> >  
> >  	for_each_intel_crtc(dev, crtc) {
> > @@ -15131,13 +15132,13 @@ int intel_modeset_init(struct drm_device *dev)
> >  	return 0;
> >  }
> >  
> > -static void intel_enable_pipe_a(struct drm_device *dev)
> > +static void intel_enable_pipe_a(struct drm_device *dev,
> > +				struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct intel_connector *connector;
> >  	struct drm_connector_list_iter conn_iter;
> >  	struct drm_connector *crt = NULL;
> >  	struct intel_load_detect_pipe load_detect_temp;
> > -	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
> >  	int ret;
> >  
> >  	/* We can't just switch on the pipe A, we need to set things up with a
> > @@ -15209,7 +15210,8 @@ static bool has_pch_trancoder(struct drm_i915_private *dev_priv,
> >  		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
> >  }
> >  
> > -static void intel_sanitize_crtc(struct intel_crtc *crtc)
> > +static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > +				struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -15265,7 +15267,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> >  		 * resume. Force-enable the pipe to fix this, the update_dpms
> >  		 * call below we restore the pipe to the right state, but leave
> >  		 * the required bits on. */
> > -		intel_enable_pipe_a(dev);
> > +		intel_enable_pipe_a(dev, ctx);
> >  	}
> >  
> >  	/* Adjust the state of the output pipe according to whether we
> > @@ -15568,7 +15570,8 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
> >   * and sanitizes it to the current state
> >   */
> >  static void
> > -intel_modeset_setup_hw_state(struct drm_device *dev)
> > +intel_modeset_setup_hw_state(struct drm_device *dev,
> > +			     struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	enum pipe pipe;
> > @@ -15588,7 +15591,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
> >  	for_each_pipe(dev_priv, pipe) {
> >  		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> >  
> > -		intel_sanitize_crtc(crtc);
> > +		intel_sanitize_crtc(crtc, ctx);
> >  		intel_dump_pipe_config(crtc, crtc->config,
> >  				       "[setup_hw_state]");
> >  	}
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrj�l�
Intel OTC

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 05:48:16PM +0300, Jani Nikula wrote:
> On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > If intel_crtc_disable_noatomic() were to ever get called during resume
> > e'd end up deadlocking since resume has its own acqcuire_ctx but
> > intel_crtc_disable_noatomic() still tries to use the
> > mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.
> 
> Same here, have we seen this, or "just" theoretical?

Happens on my 830 after adding the pipes powerwell. Before that the
machine resumed with both pipes off and the pipe A quirk left
crtc->active as false so intel_crtc_disable_noatomic() didn't get
called.

Hmm. Actually, now that I think about it more, I believe this should
happen for S4 even without the new powerwell since both pipes should
have been enabled by the BIOS when resuming from S4. I'll have to
run a proper to test to verify that.

> 
> BR,
> Jani.
> 
> >
> > Cc: stable@vger.kernel.org
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2f76a63efe8c..4e3c64ed4131 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
> >  		intel_update_watermarks(intel_crtc);
> >  }
> >  
> > -static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> > +static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> > +					struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct intel_encoder *encoder;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > @@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> >  		return;
> >  	}
> >  
> > -	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
> > +	state->acquire_ctx = ctx;
> >  
> >  	/* Everything's already locked, -EDEADLK can't happen. */
> >  	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> > @@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >  		plane = crtc->plane;
> >  		crtc->base.primary->state->visible = true;
> >  		crtc->plane = !plane;
> > -		intel_crtc_disable_noatomic(&crtc->base);
> > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> >  		crtc->plane = plane;
> >  	}
> >  
> > @@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >  	/* Adjust the state of the output pipe according to whether we
> >  	 * have active connectors/encoders. */
> >  	if (crtc->active && !intel_crtc_has_encoders(crtc))
> > -		intel_crtc_disable_noatomic(&crtc->base);
> > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> >  
> >  	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
> >  		/*
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrj�l�
Intel OTC

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Plumb the correct acquire ctx into intel_crtc_disable_noatomic()

[Ville Syrjälä]

On Thu, Jun 01, 2017 at 07:05:41PM +0300, Ville Syrj�l� wrote:
> On Thu, Jun 01, 2017 at 05:48:16PM +0300, Jani Nikula wrote:
> > On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > >
> > > If intel_crtc_disable_noatomic() were to ever get called during resume
> > > e'd end up deadlocking since resume has its own acqcuire_ctx but
> > > intel_crtc_disable_noatomic() still tries to use the
> > > mode_config.acquire_ctx. Pass down the correct acquire ctx from the top.
> > 
> > Same here, have we seen this, or "just" theoretical?
> 
> Happens on my 830 after adding the pipes powerwell. Before that the
> machine resumed with both pipes off and the pipe A quirk left
> crtc->active as false so intel_crtc_disable_noatomic() didn't get
> called.
> 
> Hmm. Actually, now that I think about it more, I believe this should
> happen for S4 even without the new powerwell since both pipes should
> have been enabled by the BIOS when resuming from S4. I'll have to
> run a proper to test to verify that.

Indeed. In fact it deadlocks already during the thaw phase between
creating the hibernation image and writing to it disk.

> 
> > 
> > BR,
> > Jani.
> > 
> > >
> > > Cc: stable@vger.kernel.org
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 2f76a63efe8c..4e3c64ed4131 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5839,7 +5839,8 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
> > >  		intel_update_watermarks(intel_crtc);
> > >  }
> > >  
> > > -static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> > > +static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> > > +					struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > >  	struct intel_encoder *encoder;
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > @@ -5869,7 +5870,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> > >  		return;
> > >  	}
> > >  
> > > -	state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
> > > +	state->acquire_ctx = ctx;
> > >  
> > >  	/* Everything's already locked, -EDEADLK can't happen. */
> > >  	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> > > @@ -15257,7 +15258,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > >  		plane = crtc->plane;
> > >  		crtc->base.primary->state->visible = true;
> > >  		crtc->plane = !plane;
> > > -		intel_crtc_disable_noatomic(&crtc->base);
> > > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> > >  		crtc->plane = plane;
> > >  	}
> > >  
> > > @@ -15273,7 +15274,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > >  	/* Adjust the state of the output pipe according to whether we
> > >  	 * have active connectors/encoders. */
> > >  	if (crtc->active && !intel_crtc_has_encoders(crtc))
> > > -		intel_crtc_disable_noatomic(&crtc->base);
> > > +		intel_crtc_disable_noatomic(&crtc->base, ctx);
> > >  
> > >  	if (crtc->active || HAS_GMCH_DISPLAY(dev_priv)) {
> > >  		/*
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Ville Syrj�l�
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrj�l�
Intel OTC

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Fix deadlock witha the pipe A quirk during resume

[Jani Nikula]

On Thu, 01 Jun 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Jun 01, 2017 at 05:47:45PM +0300, Jani Nikula wrote:
>> On Thu, 01 Jun 2017, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Pass down the correct acquire context to the pipe A quirk load detect
>> > hack during display resume. Avoids deadlocking the entire thing.
>> 
>> Have we seen this in the wild? References?
>
> My 830.

Okay, just checking if the cc: stable was really warranted in this one
and the next patch. Carry on! :)

BR,
Jani.

>
>> 
>> BR,
>> Jani.
>> 
>> >
>> > Cc: stable@vger.kernel.org
>> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++---------
>> >  1 file changed, 12 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index ed41b59ee8e3..2f76a63efe8c 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -120,7 +120,8 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
>> >  static void skylake_pfit_enable(struct intel_crtc *crtc);
>> >  static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force);
>> >  static void ironlake_pfit_enable(struct intel_crtc *crtc);
>> > -static void intel_modeset_setup_hw_state(struct drm_device *dev);
>> > +static void intel_modeset_setup_hw_state(struct drm_device *dev,
>> > +					 struct drm_modeset_acquire_ctx *ctx);
>> >  static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
>> >  
>> >  struct intel_limit {
>> > @@ -3461,7 +3462,7 @@ __intel_display_resume(struct drm_device *dev,
>> >  	struct drm_crtc *crtc;
>> >  	int i, ret;
>> >  
>> > -	intel_modeset_setup_hw_state(dev);
>> > +	intel_modeset_setup_hw_state(dev, ctx);
>> >  	i915_redisable_vga(to_i915(dev));
>> >  
>> >  	if (!state)
>> > @@ -15094,7 +15095,7 @@ int intel_modeset_init(struct drm_device *dev)
>> >  	intel_setup_outputs(dev_priv);
>> >  
>> >  	drm_modeset_lock_all(dev);
>> > -	intel_modeset_setup_hw_state(dev);
>> > +	intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
>> >  	drm_modeset_unlock_all(dev);
>> >  
>> >  	for_each_intel_crtc(dev, crtc) {
>> > @@ -15131,13 +15132,13 @@ int intel_modeset_init(struct drm_device *dev)
>> >  	return 0;
>> >  }
>> >  
>> > -static void intel_enable_pipe_a(struct drm_device *dev)
>> > +static void intel_enable_pipe_a(struct drm_device *dev,
>> > +				struct drm_modeset_acquire_ctx *ctx)
>> >  {
>> >  	struct intel_connector *connector;
>> >  	struct drm_connector_list_iter conn_iter;
>> >  	struct drm_connector *crt = NULL;
>> >  	struct intel_load_detect_pipe load_detect_temp;
>> > -	struct drm_modeset_acquire_ctx *ctx = dev->mode_config.acquire_ctx;
>> >  	int ret;
>> >  
>> >  	/* We can't just switch on the pipe A, we need to set things up with a
>> > @@ -15209,7 +15210,8 @@ static bool has_pch_trancoder(struct drm_i915_private *dev_priv,
>> >  		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
>> >  }
>> >  
>> > -static void intel_sanitize_crtc(struct intel_crtc *crtc)
>> > +static void intel_sanitize_crtc(struct intel_crtc *crtc,
>> > +				struct drm_modeset_acquire_ctx *ctx)
>> >  {
>> >  	struct drm_device *dev = crtc->base.dev;
>> >  	struct drm_i915_private *dev_priv = to_i915(dev);
>> > @@ -15265,7 +15267,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>> >  		 * resume. Force-enable the pipe to fix this, the update_dpms
>> >  		 * call below we restore the pipe to the right state, but leave
>> >  		 * the required bits on. */
>> > -		intel_enable_pipe_a(dev);
>> > +		intel_enable_pipe_a(dev, ctx);
>> >  	}
>> >  
>> >  	/* Adjust the state of the output pipe according to whether we
>> > @@ -15568,7 +15570,8 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
>> >   * and sanitizes it to the current state
>> >   */
>> >  static void
>> > -intel_modeset_setup_hw_state(struct drm_device *dev)
>> > +intel_modeset_setup_hw_state(struct drm_device *dev,
>> > +			     struct drm_modeset_acquire_ctx *ctx)
>> >  {
>> >  	struct drm_i915_private *dev_priv = to_i915(dev);
>> >  	enum pipe pipe;
>> > @@ -15588,7 +15591,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
>> >  	for_each_pipe(dev_priv, pipe) {
>> >  		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>> >  
>> > -		intel_sanitize_crtc(crtc);
>> > +		intel_sanitize_crtc(crtc, ctx);
>> >  		intel_dump_pipe_config(crtc, crtc->config,
>> >  				       "[setup_hw_state]");
>> >  	}
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center