Re: [PATCH] drm/i915: enable the pipe/transcoder/planes later on HSW+

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, May 02, 2018 at 02:58:51PM -0700, Paulo Zanoni wrote:
> For all platforms that run haswell_crtc_enable, our spec tells us to
> configure the transcoder clocks and do link training before it tells
> us to set pipeconf and the other pipe/transcoder/plane registers.

oh! I remember this order years ago when trying to figure out where
to enable PSR... but I never noticed we weren't fully respecting it.

> 
> Starting from Icelake, we get machine hangs if we try to touch the
> pipe/transcoder registers without having the clocks configured and not
> having some chicken bits set. So this patch changes
> haswell_crtc_enable() to issue the calls at the appropriate order
> mandated by the spec.
> 
> While setting the appropriate chicken bits would also work here, it's
> better if we actually program the hardware the way it is intended to
> be programmed. And the chicken bit also has some theoretical downsides
> that may or may not affect us. Also, correctly programming the
> hardware does not prevent us from setting the chicken bits in a later
> patch in case we decide to.
> 
> v2: Don't forget link training (Ville).
> 
> Cc: Arthur J Runyan <arthur.j.runyan@intel.com>
> Cc: James Ausmus <james.ausmus@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

makes sense for me.... let's now just see CI is happy as well! ;)

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Again, I didn't test this patch on every affected platform. Let's see
> what the CI system says about it.
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1087358f6364..f566c9e56cf6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5559,6 +5559,11 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>  	if (intel_crtc->config->shared_dpll)
>  		intel_enable_shared_dpll(intel_crtc);
>  
> +	intel_encoders_pre_enable(crtc, pipe_config, old_state);
> +
> +	if (!transcoder_is_dsi(cpu_transcoder))
> +		intel_ddi_enable_pipe_clock(pipe_config);
> +
>  	if (intel_crtc_has_dp_encoder(intel_crtc->config))
>  		intel_dp_set_m_n(intel_crtc, M1_N1);
>  
> @@ -5587,11 +5592,6 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>  
>  	intel_crtc->active = true;
>  
> -	intel_encoders_pre_enable(crtc, pipe_config, old_state);
> -
> -	if (!transcoder_is_dsi(cpu_transcoder))
> -		intel_ddi_enable_pipe_clock(pipe_config);
> -
>  	/* Display WA #1180: WaDisableScalarClockGating: glk, cnl */
>  	psl_clkgate_wa = (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) &&
>  			 intel_crtc->config->pch_pfit.enabled;
> -- 
> 2.14.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx