| Message ID | 20180502215851.30736-1-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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. > > 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> yes I verified this against the display modset sequence mentioned in the spec and this is correct order. Reviewed-by: Manasi Navare <manasi.d.navare@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
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;
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. 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> --- 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.