| Message ID | 20181108143635.9556-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Sanitize PCH port transcoder select on IBX | expand |
On Thu, Nov 08, 2018 at 03:08:25PM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Sanitize PCH port transcoder select on IBX > URL : https://patchwork.freedesktop.org/series/52239/ > State : success > > == Summary == > > = CI Bug Log - changes from CI_DRM_5105 -> Patchwork_10775 = > > == Summary - SUCCESS == > > No regressions found. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/52239/revisions/1/mbox/ > > == Known issues == > > Here are the changes found in Patchwork_10775 that come from known issues: Unfortunately CI still doesn't check for errors/warns during the initial driver load, but looking at the logs we see that fi-ilk-650 is now happier: - <4>[ 3.915931] ------------[ cut here ]------------ - <4>[ 3.915934] IBX PCH DP C still using transcoder B - <4>[ 3.916028] WARNING: CPU: 1 PID: 174 at drivers/gpu/drm/i915/intel_display.c:1279 assert_pch_dp_disabled+0x9e/0xb0 [i915] - ... + <7>[ 3.765439] [drm:ibx_sanitize_pch_dp_port [i915]] Sanitizing transcoder select for DP C > > === IGT changes === > > ==== Issues hit ==== > > igt@gem_exec_suspend@basic-s3: > fi-byt-clapper: PASS -> INCOMPLETE (fdo#102657) > > igt@kms_chamelium@dp-edid-read: > fi-kbl-7500u: PASS -> WARN (fdo#102672) > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c: > fi-icl-u: PASS -> INCOMPLETE (fdo#107713) > > > ==== Possible fixes ==== > > igt@kms_chamelium@common-hpd-after-suspend: > fi-skl-6700k2: FAIL (fdo#103841) -> PASS > > > fdo#102657 https://bugs.freedesktop.org/show_bug.cgi?id=102657 > fdo#102672 https://bugs.freedesktop.org/show_bug.cgi?id=102672 > fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841 > fdo#107713 https://bugs.freedesktop.org/show_bug.cgi?id=107713 > > > == Participating hosts (51 -> 45) == > > Additional (2): fi-byt-j1900 fi-glk-dsi > Missing (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-snb-2520m fi-glk-j4005 > > > == Build changes == > > * Linux: CI_DRM_5105 -> Patchwork_10775 > > CI_DRM_5105: e44a1cc644d1719d195bd0afb9e319ae555059e1 @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4712: a3ede1b535ac8137f6949c468edd7054453d5dae @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_10775: 56d080634d63c6015827c7da5d35e599be173e25 @ git://anongit.freedesktop.org/gfx-ci/linux > > > == Linux commits == > > 56d080634d63 drm/i915: Sanitize PCH port transcoder select on IBX > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10775/issues.html
Quoting Ville Syrjala (2018-11-08 14:36:35) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > IBX has a documented workaround which states that when we disable the > port we must change its transcoder select to A, otherwise it will > prevent the other port (DP vs. HDMI/SDVO) from using transcoder A. > We implement the workaround during encoder disable, but looks like > some BIOSen leave transcoder B selected even when the port wasn't > actually enabled by the BIOS. That will trip up our asserts > that attempt to make sure we never forget this w/a. > > Sanitize the transcoder select to A for all disabled PCH > DP/HDMI/SDVO ports. We assume that the port was never enabled > by the BIOS on transcoder B, because if it had we'd actually have > to toggle the port on and back off to properly switch it back to > transcoder A. That would cause some display flicker if transcoder A > is already enabled on some other port, so it's better not to do it > unless absolutely necessary. Since we have no indication that the > transcoder select is misbehaving on the affected machines we can > assume the port was never actually enabled by the BIOS. > > This cures warning like this during driver load: > IBX PCH DP C still using transcoder B > WARNING: CPU: 2 PID: 172 at drivers/gpu/drm/i915/intel_display.c:1279 assert_pch_dp_disabled+0x9e/0xb0 [i915] > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index ae6d58dbf1ed..71b7bff85e52 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15653,6 +15653,64 @@ static void intel_early_display_was(struct drm_i915_private *dev_priv) > } > } > > +static void ibx_sanitize_pch_hdmi_port(struct drm_i915_private *dev_priv, > + enum port port, i915_reg_t hdmi_reg) > +{ > + u32 val = I915_READ(hdmi_reg); > + > + if (val & SDVO_ENABLE || > + (val & SDVO_PIPE_SEL_MASK) == SDVO_PIPE_SEL(PIPE_A)) > + return; Hmm, the w/a is also applied in intel_sdvo.c. Do we need to worry about those as well? Oh my, it's perfectly obvious SDVO is aliased to HDMI on ibx. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> I would have s/hdmi/svdo/ here so that the labelling was all consistent and added a reminder in the comment below that hdmi is aliased to sdvo. -Chris
On Thu, Nov 08, 2018 at 04:23:48PM +0000, Chris Wilson wrote: > Quoting Ville Syrjala (2018-11-08 14:36:35) > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > IBX has a documented workaround which states that when we disable the > > port we must change its transcoder select to A, otherwise it will > > prevent the other port (DP vs. HDMI/SDVO) from using transcoder A. > > We implement the workaround during encoder disable, but looks like > > some BIOSen leave transcoder B selected even when the port wasn't > > actually enabled by the BIOS. That will trip up our asserts > > that attempt to make sure we never forget this w/a. > > > > Sanitize the transcoder select to A for all disabled PCH > > DP/HDMI/SDVO ports. We assume that the port was never enabled > > by the BIOS on transcoder B, because if it had we'd actually have > > to toggle the port on and back off to properly switch it back to > > transcoder A. That would cause some display flicker if transcoder A > > is already enabled on some other port, so it's better not to do it > > unless absolutely necessary. Since we have no indication that the > > transcoder select is misbehaving on the affected machines we can > > assume the port was never actually enabled by the BIOS. > > > > This cures warning like this during driver load: > > IBX PCH DP C still using transcoder B > > WARNING: CPU: 2 PID: 172 at drivers/gpu/drm/i915/intel_display.c:1279 assert_pch_dp_disabled+0x9e/0xb0 [i915] > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index ae6d58dbf1ed..71b7bff85e52 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -15653,6 +15653,64 @@ static void intel_early_display_was(struct drm_i915_private *dev_priv) > > } > > } > > > > +static void ibx_sanitize_pch_hdmi_port(struct drm_i915_private *dev_priv, > > + enum port port, i915_reg_t hdmi_reg) > > +{ > > + u32 val = I915_READ(hdmi_reg); > > + > > + if (val & SDVO_ENABLE || > > + (val & SDVO_PIPE_SEL_MASK) == SDVO_PIPE_SEL(PIPE_A)) > > + return; > > Hmm, the w/a is also applied in intel_sdvo.c. Do we need to worry about > those as well? > > Oh my, it's perfectly obvious SDVO is aliased to HDMI on ibx. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > I would have s/hdmi/svdo/ here so that the labelling was all consistent > and added a reminder in the comment below that hdmi is aliased to sdvo. I wanted to be consistent with assert_pch_hdmi_disabled(). I think only port B can be SDVO actually, or something along those lines. Not sure we want to rename all of this for that reason. But a comment seems like a very good idea at least. I'll add one.
On Thu, Nov 08, 2018 at 06:36:15PM +0200, Ville Syrjälä wrote: > On Thu, Nov 08, 2018 at 04:23:48PM +0000, Chris Wilson wrote: > > Quoting Ville Syrjala (2018-11-08 14:36:35) > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > IBX has a documented workaround which states that when we disable the > > > port we must change its transcoder select to A, otherwise it will > > > prevent the other port (DP vs. HDMI/SDVO) from using transcoder A. > > > We implement the workaround during encoder disable, but looks like > > > some BIOSen leave transcoder B selected even when the port wasn't > > > actually enabled by the BIOS. That will trip up our asserts > > > that attempt to make sure we never forget this w/a. > > > > > > Sanitize the transcoder select to A for all disabled PCH > > > DP/HDMI/SDVO ports. We assume that the port was never enabled > > > by the BIOS on transcoder B, because if it had we'd actually have > > > to toggle the port on and back off to properly switch it back to > > > transcoder A. That would cause some display flicker if transcoder A > > > is already enabled on some other port, so it's better not to do it > > > unless absolutely necessary. Since we have no indication that the > > > transcoder select is misbehaving on the affected machines we can > > > assume the port was never actually enabled by the BIOS. > > > > > > This cures warning like this during driver load: > > > IBX PCH DP C still using transcoder B > > > WARNING: CPU: 2 PID: 172 at drivers/gpu/drm/i915/intel_display.c:1279 assert_pch_dp_disabled+0x9e/0xb0 [i915] > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++++++ > > > 1 file changed, 61 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index ae6d58dbf1ed..71b7bff85e52 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -15653,6 +15653,64 @@ static void intel_early_display_was(struct drm_i915_private *dev_priv) > > > } > > > } > > > > > > +static void ibx_sanitize_pch_hdmi_port(struct drm_i915_private *dev_priv, > > > + enum port port, i915_reg_t hdmi_reg) > > > +{ > > > + u32 val = I915_READ(hdmi_reg); > > > + > > > + if (val & SDVO_ENABLE || > > > + (val & SDVO_PIPE_SEL_MASK) == SDVO_PIPE_SEL(PIPE_A)) > > > + return; > > > > Hmm, the w/a is also applied in intel_sdvo.c. Do we need to worry about > > those as well? > > > > Oh my, it's perfectly obvious SDVO is aliased to HDMI on ibx. > > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > I would have s/hdmi/svdo/ here so that the labelling was all consistent > > and added a reminder in the comment below that hdmi is aliased to sdvo. > > I wanted to be consistent with assert_pch_hdmi_disabled(). I think only > port B can be SDVO actually, or something along those lines. Not sure > we want to rename all of this for that reason. But a comment seems like > a very good idea at least. I'll add one. Tossed in a few comments about SDVOB being an alias for HDMIB and pushed to dinq. Thanks for the review.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ae6d58dbf1ed..71b7bff85e52 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15653,6 +15653,64 @@ static void intel_early_display_was(struct drm_i915_private *dev_priv) } } +static void ibx_sanitize_pch_hdmi_port(struct drm_i915_private *dev_priv, + enum port port, i915_reg_t hdmi_reg) +{ + u32 val = I915_READ(hdmi_reg); + + if (val & SDVO_ENABLE || + (val & SDVO_PIPE_SEL_MASK) == SDVO_PIPE_SEL(PIPE_A)) + return; + + DRM_DEBUG_KMS("Sanitizing transcoder select for HDMI %c\n", + port_name(port)); + + val &= ~SDVO_PIPE_SEL_MASK; + val |= SDVO_PIPE_SEL(PIPE_A); + + I915_WRITE(hdmi_reg, val); +} + +static void ibx_sanitize_pch_dp_port(struct drm_i915_private *dev_priv, + enum port port, i915_reg_t dp_reg) +{ + u32 val = I915_READ(dp_reg); + + if (val & DP_PORT_EN || + (val & DP_PIPE_SEL_MASK) == DP_PIPE_SEL(PIPE_A)) + return; + + DRM_DEBUG_KMS("Sanitizing transcoder select for DP %c\n", + port_name(port)); + + val &= ~DP_PIPE_SEL_MASK; + val |= DP_PIPE_SEL(PIPE_A); + + I915_WRITE(dp_reg, val); +} + +static void ibx_sanitize_pch_ports(struct drm_i915_private *dev_priv) +{ + /* + * The BIOS may select transcoder B on some of the PCH + * ports even it doesn't enable the port. This would trip + * assert_pch_dp_disabled() and assert_pch_hdmi_disabled(). + * Sanitize the transcoder select bits to prevent that. We + * assume that the BIOS never actually enabled the port, + * because if it did we'd actually have to toggle the port + * on and back off to make the transcoder A select stick + * (see. intel_dp_link_down(), intel_disable_hdmi(), + * intel_disable_sdvo()). + */ + ibx_sanitize_pch_dp_port(dev_priv, PORT_B, PCH_DP_B); + ibx_sanitize_pch_dp_port(dev_priv, PORT_C, PCH_DP_C); + ibx_sanitize_pch_dp_port(dev_priv, PORT_D, PCH_DP_D); + + ibx_sanitize_pch_hdmi_port(dev_priv, PORT_B, PCH_HDMIB); + ibx_sanitize_pch_hdmi_port(dev_priv, PORT_C, PCH_HDMIC); + ibx_sanitize_pch_hdmi_port(dev_priv, PORT_D, PCH_HDMID); +} + /* Scan out the current hw modeset state, * and sanitizes it to the current state */ @@ -15674,6 +15732,9 @@ intel_modeset_setup_hw_state(struct drm_device *dev, /* HW state is read out, now we need to sanitize this mess. */ get_encoder_power_domains(dev_priv); + if (HAS_PCH_IBX(dev_priv)) + ibx_sanitize_pch_ports(dev_priv); + /* * intel_sanitize_plane_mapping() may need to do vblank * waits, so we need vblank interrupts restored beforehand.