| Message ID | 1449260459-14347-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-12-04 18:20 GMT-02:00 <ville.syrjala@linux.intel.com>: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Bspec modeset sequence tells us to disable the PCH transcoder and > FDI after the CRT port on LPT-H, so let's do that. And the CRT port > should be disabled after the pipe, as we do on other PCH platforms > too since > commit 1ea56e269e13 ("drm/i915: Disable CRT port after pipe on PCH platforms") > > commit 00490c22b1b5 ("drm/i915: Consider SPLL as another shared pll, v2.") > moved the SPLL disaable from the .post_disable() hook to some upper disaable > laeve code, so we can just move the CRT port disabling into the laeve > .post_disable() hook. If we still had the non-shared SPLL, it would have > needed to be moved into the .post_pll_disable() hook. > > v2: Actually move the CRT port disable to the .post_disable() hook, > and amend the commit message with more details (Paulo) Since I seem to recall that the DDI disable sequence for CRT was correct at some point in a distant past, I suppose your commit is a regression fix, and maybe we'd like a backportable version. Maybe we could also provide explicit information on the first bad commit. Anyway, now it looks correct: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Paulo Zanoni <przanoni@gmail.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_crt.c | 2 +- > drivers/gpu/drm/i915/intel_display.c | 11 +++++------ > 2 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > index 12008af797bd..cef359958c73 100644 > --- a/drivers/gpu/drm/i915/intel_crt.c > +++ b/drivers/gpu/drm/i915/intel_crt.c > @@ -844,7 +844,7 @@ void intel_crt_init(struct drm_device *dev) > crt->adpa_reg = adpa_reg; > > crt->base.compute_config = intel_crt_compute_config; > - if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev)) { > + if (HAS_PCH_SPLIT(dev)) { > crt->base.disable = pch_disable_crt; > crt->base.post_disable = pch_post_disable_crt; > } else { > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3391ab0ca752..42ed799390e5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5166,18 +5166,17 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) > if (!intel_crtc->config->has_dsi_encoder) > intel_ddi_disable_pipe_clock(intel_crtc); > > - if (intel_crtc->config->has_pch_encoder) { > - lpt_disable_pch_transcoder(dev_priv); > - intel_ddi_fdi_disable(crtc); > - } > - > for_each_encoder_on_crtc(dev, crtc, encoder) > if (encoder->post_disable) > encoder->post_disable(encoder); > > - if (intel_crtc->config->has_pch_encoder) > + if (intel_crtc->config->has_pch_encoder) { > + lpt_disable_pch_transcoder(dev_priv); > + intel_ddi_fdi_disable(crtc); > + > intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A, > true); > + } > > intel_fbc_disable_crtc(intel_crtc); > } > -- > 2.4.10 >
On Mon, Dec 07, 2015 at 03:51:06PM -0200, Paulo Zanoni wrote: > 2015-12-04 18:20 GMT-02:00 <ville.syrjala@linux.intel.com>: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Bspec modeset sequence tells us to disable the PCH transcoder and > > FDI after the CRT port on LPT-H, so let's do that. And the CRT port > > should be disabled after the pipe, as we do on other PCH platforms > > too since > > commit 1ea56e269e13 ("drm/i915: Disable CRT port after pipe on PCH platforms") > > > > commit 00490c22b1b5 ("drm/i915: Consider SPLL as another shared pll, v2.") > > moved the SPLL disaable from the .post_disable() hook to some upper > > disaable > > > laeve code, so we can just move the CRT port disabling into the > > laeve > > > .post_disable() hook. If we still had the non-shared SPLL, it would have > > needed to be moved into the .post_pll_disable() hook. > > > > v2: Actually move the CRT port disable to the .post_disable() hook, > > and amend the commit message with more details (Paulo) > > Since I seem to recall that the DDI disable sequence for CRT was > correct at some point in a distant past, I suppose your commit is a > regression fix, and maybe we'd like a backportable version. Maybe we > could also provide explicit information on the first bad commit. Well, even the spec has changed since the early days, at least w.r.t. the FDI RX disable, so not sure if something else has changed in there as well. I didn't go digging through history to see if we accidentally changed the order of some steps at some point. > > Anyway, now it looks correct: > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > Cc: Paulo Zanoni <przanoni@gmail.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_crt.c | 2 +- > > drivers/gpu/drm/i915/intel_display.c | 11 +++++------ > > 2 files changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > > index 12008af797bd..cef359958c73 100644 > > --- a/drivers/gpu/drm/i915/intel_crt.c > > +++ b/drivers/gpu/drm/i915/intel_crt.c > > @@ -844,7 +844,7 @@ void intel_crt_init(struct drm_device *dev) > > crt->adpa_reg = adpa_reg; > > > > crt->base.compute_config = intel_crt_compute_config; > > - if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev)) { > > + if (HAS_PCH_SPLIT(dev)) { > > crt->base.disable = pch_disable_crt; > > crt->base.post_disable = pch_post_disable_crt; > > } else { > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 3391ab0ca752..42ed799390e5 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5166,18 +5166,17 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) > > if (!intel_crtc->config->has_dsi_encoder) > > intel_ddi_disable_pipe_clock(intel_crtc); > > > > - if (intel_crtc->config->has_pch_encoder) { > > - lpt_disable_pch_transcoder(dev_priv); > > - intel_ddi_fdi_disable(crtc); > > - } > > - > > for_each_encoder_on_crtc(dev, crtc, encoder) > > if (encoder->post_disable) > > encoder->post_disable(encoder); > > > > - if (intel_crtc->config->has_pch_encoder) > > + if (intel_crtc->config->has_pch_encoder) { > > + lpt_disable_pch_transcoder(dev_priv); > > + intel_ddi_fdi_disable(crtc); > > + > > intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A, > > true); > > + } > > > > intel_fbc_disable_crtc(intel_crtc); > > } > > -- > > 2.4.10 > > > > > > -- > Paulo Zanoni
On Mon, Dec 07, 2015 at 08:57:59PM +0200, Ville Syrjälä wrote: > On Mon, Dec 07, 2015 at 03:51:06PM -0200, Paulo Zanoni wrote: > > 2015-12-04 18:20 GMT-02:00 <ville.syrjala@linux.intel.com>: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Bspec modeset sequence tells us to disable the PCH transcoder and > > > FDI after the CRT port on LPT-H, so let's do that. And the CRT port > > > should be disabled after the pipe, as we do on other PCH platforms > > > too since > > > commit 1ea56e269e13 ("drm/i915: Disable CRT port after pipe on PCH platforms") > > > > > > commit 00490c22b1b5 ("drm/i915: Consider SPLL as another shared pll, v2.") > > > moved the SPLL disaable from the .post_disable() hook to some upper > > > > disaable > > > > > laeve code, so we can just move the CRT port disabling into the > > > > laeve > > > > > .post_disable() hook. If we still had the non-shared SPLL, it would have > > > needed to be moved into the .post_pll_disable() hook. > > > > > > v2: Actually move the CRT port disable to the .post_disable() hook, > > > and amend the commit message with more details (Paulo) > > > > Since I seem to recall that the DDI disable sequence for CRT was > > correct at some point in a distant past, I suppose your commit is a > > regression fix, and maybe we'd like a backportable version. Maybe we > > could also provide explicit information on the first bad commit. > > Well, even the spec has changed since the early days, at least w.r.t. > the FDI RX disable, so not sure if something else has changed in there > as well. I didn't go digging through history to see if we accidentally > changed the order of some steps at some point. I tried to look through the hisrtory a bit, but didn't spot any place where any significant reordering took place (ie. we always seemed to disable the port before the pipe). So I'm leaning towards the "spec got changed" theory here, or perhaps the "copy paste" theory since we did get this order wrong for all earlier PCH platforms as well. > > > > > Anyway, now it looks correct: > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > > > Cc: Paulo Zanoni <przanoni@gmail.com> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_crt.c | 2 +- > > > drivers/gpu/drm/i915/intel_display.c | 11 +++++------ > > > 2 files changed, 6 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > > > index 12008af797bd..cef359958c73 100644 > > > --- a/drivers/gpu/drm/i915/intel_crt.c > > > +++ b/drivers/gpu/drm/i915/intel_crt.c > > > @@ -844,7 +844,7 @@ void intel_crt_init(struct drm_device *dev) > > > crt->adpa_reg = adpa_reg; > > > > > > crt->base.compute_config = intel_crt_compute_config; > > > - if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev)) { > > > + if (HAS_PCH_SPLIT(dev)) { > > > crt->base.disable = pch_disable_crt; > > > crt->base.post_disable = pch_post_disable_crt; > > > } else { > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 3391ab0ca752..42ed799390e5 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -5166,18 +5166,17 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) > > > if (!intel_crtc->config->has_dsi_encoder) > > > intel_ddi_disable_pipe_clock(intel_crtc); > > > > > > - if (intel_crtc->config->has_pch_encoder) { > > > - lpt_disable_pch_transcoder(dev_priv); > > > - intel_ddi_fdi_disable(crtc); > > > - } > > > - > > > for_each_encoder_on_crtc(dev, crtc, encoder) > > > if (encoder->post_disable) > > > encoder->post_disable(encoder); > > > > > > - if (intel_crtc->config->has_pch_encoder) > > > + if (intel_crtc->config->has_pch_encoder) { > > > + lpt_disable_pch_transcoder(dev_priv); > > > + intel_ddi_fdi_disable(crtc); > > > + > > > intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A, > > > true); > > > + } > > > > > > intel_fbc_disable_crtc(intel_crtc); > > > } > > > -- > > > 2.4.10 > > > > > > > > > > > -- > > Paulo Zanoni > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 12008af797bd..cef359958c73 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -844,7 +844,7 @@ void intel_crt_init(struct drm_device *dev) crt->adpa_reg = adpa_reg; crt->base.compute_config = intel_crt_compute_config; - if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev)) { + if (HAS_PCH_SPLIT(dev)) { crt->base.disable = pch_disable_crt; crt->base.post_disable = pch_post_disable_crt; } else { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3391ab0ca752..42ed799390e5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5166,18 +5166,17 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) if (!intel_crtc->config->has_dsi_encoder) intel_ddi_disable_pipe_clock(intel_crtc); - if (intel_crtc->config->has_pch_encoder) { - lpt_disable_pch_transcoder(dev_priv); - intel_ddi_fdi_disable(crtc); - } - for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder->post_disable) encoder->post_disable(encoder); - if (intel_crtc->config->has_pch_encoder) + if (intel_crtc->config->has_pch_encoder) { + lpt_disable_pch_transcoder(dev_priv); + intel_ddi_fdi_disable(crtc); + intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A, true); + } intel_fbc_disable_crtc(intel_crtc); }