Re: [Intel-gfx] [PATCH] drm/i915: Sanitize PCH port transcoder select on IBX

2018-11-09 Thread Ville Syrjälä
On Thu, Nov 08, 2018 at 06:36:15PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 08, 2018 at 04:23:48PM +, Chris Wilson wrote:
> > Quoting Ville Syrjala (2018-11-08 14:36:35)
> > > From: Ville Syrjälä 
> > > 
> > > 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 
> > > Signed-off-by: Ville Syrjälä 
> > > ---
> > >  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 
> > 
> > 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.

-- 
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Sanitize PCH port transcoder select on IBX

2018-11-08 Thread Ville Syrjälä
On Thu, Nov 08, 2018 at 04:23:48PM +, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-11-08 14:36:35)
> > From: Ville Syrjälä 
> > 
> > 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 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  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 
> 
> 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.

-- 
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Sanitize PCH port transcoder select on IBX

2018-11-08 Thread Chris Wilson
Quoting Ville Syrjala (2018-11-08 14:36:35)
> From: Ville Syrjälä 
> 
> 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 
> Signed-off-by: Ville Syrjälä 
> ---
>  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 

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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH] drm/i915: Sanitize PCH port transcoder select on IBX

2018-11-08 Thread Ville Syrjala
From: Ville Syrjälä 

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 
Signed-off-by: Ville Syrjälä 
---
 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;
+
+   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.
-- 
2.18.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx