[PATCH] drm/i915: Sanitize the PPT fdi lane bifurcate state on ivb

[PATCH] drm/i915: Sanitize the PPT fdi lane bifurcate state on ivb

[Daniel Vetter]

We expect this bit to be always set when possible, but some BIOSes are
lazy and don't do this. The result is a pile of WARNs and unhappy fdi
link training code ...

v2: It's actually the inverse: The BIOS sets this bit when it's not
strictly needed. This should be cleaned up in the
global_modeset_resources callback, but we've failed to look at the
active bit. Which means this won't fire (and so clean up BIOS state)
when enabling pipe B or C for the first time.

v3: Wrap lines.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70507
Tested-by: Jan-Michael Brummer <jan.brummer@tabos.org>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cfe9e709..3569db6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2421,9 +2421,10 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
 			   FDI_FE_ERRC_ENABLE);
 }
 
-static bool pipe_has_enabled_pch(struct intel_crtc *intel_crtc)
+static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
 {
-	return intel_crtc->base.enabled && intel_crtc->config.has_pch_encoder;
+	return crtc->base.enabled && crtc->active &&
+		crtc->config.has_pch_encoder;
 }
 
 static void ivb_modeset_global_resources(struct drm_device *dev)
-- 
1.8.4.rc3

Re: [Intel-gfx] [PATCH] drm/i915: Sanitize the PPT fdi lane bifurcate state on ivb

[Ville Syrjälä]

On Tue, Oct 22, 2013 at 02:37:53PM +0200, Daniel Vetter wrote:
> We expect this bit to be always set when possible, but some BIOSes are
> lazy and don't do this. The result is a pile of WARNs and unhappy fdi
> link training code ...
> 
> v2: It's actually the inverse: The BIOS sets this bit when it's not
> strictly needed. This should be cleaned up in the
> global_modeset_resources callback, but we've failed to look at the
> active bit. Which means this won't fire (and so clean up BIOS state)
> when enabling pipe B or C for the first time.
> 
> v3: Wrap lines.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70507
> Tested-by: Jan-Michael Brummer <jan.brummer@tabos.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cfe9e709..3569db6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2421,9 +2421,10 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>  			   FDI_FE_ERRC_ENABLE);
>  }
>  
> -static bool pipe_has_enabled_pch(struct intel_crtc *intel_crtc)
> +static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  {
> -	return intel_crtc->base.enabled && intel_crtc->config.has_pch_encoder;
> +	return crtc->base.enabled && crtc->active &&
> +		crtc->config.has_pch_encoder;

I'm thinking the crtc->base.enabled check is actually pointless.
AFAICS we should never get here with crtc->base.enabled==false and
crtc->active==true.

We anyway re-enable the bifurcation bit when we do the mode_set.
Actually that in itself could be a maybe a bug. We'd turn off the
bifurcation bit when both pipes B and C are disabled based on
prepare_pipes, but we only do the mode_set based on modeset_pipes.
But currently we are saved by the fact that those two bitmasks are
the same.

Another potential bug I found is that we always set the bifurcation
bit in mode_set, even if we're not using FDI. So if we have eg.
pipe B enabled w/ FDI using 4 lanes, and then we enable pipe C w/ EDP,
we'd still flip the bifurcation bit in pipe C's mode_set and destroy pipe
B's output. The fix would be to call ivybridge_update_fdi_bc_bifurcation()
only when has_pch_encoder==true.

>  }

>  
>  static void ivb_modeset_global_resources(struct drm_device *dev)
> -- 
> 1.8.4.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH] drm/i915: Sanitize the PPT fdi lane bifurcate state on ivb

[Daniel Vetter]

On Wed, Oct 23, 2013 at 12:58 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> I'm thinking the crtc->base.enabled check is actually pointless.
> AFAICS we should never get here with crtc->base.enabled==false and
> crtc->active==true

Hm yeah. I was kinda shooting for the minimal thing here.

> We anyway re-enable the bifurcation bit when we do the mode_set.
> Actually that in itself could be a maybe a bug. We'd turn off the
> bifurcation bit when both pipes B and C are disabled based on
> prepare_pipes, but we only do the mode_set based on modeset_pipes.
> But currently we are saved by the fact that those two bitmasks are
> the same.

The current logic (after this patch) is to
- always clear the bit when both pipes are off.
- always set if if we either enable pipe B or C and we'd need it to
allow the other pipe to be lit up. If pipe B uses 4 lanes then we
can't light pipe C up anymore, so we don't set the bit.

There is a potential issue though with all pipes disabled with dpms
off now. I think conceptually I need to precompute the desired state
of the bifurcate bit in the compute config stage and then only set it
here.

> Another potential bug I found is that we always set the bifurcation
> bit in mode_set, even if we're not using FDI. So if we have eg.
> pipe B enabled w/ FDI using 4 lanes, and then we enable pipe C w/ EDP,
> we'd still flip the bifurcation bit in pipe C's mode_set and destroy pipe
> B's output. The fix would be to call ivybridge_update_fdi_bc_bifurcation()
> only when has_pch_encoder==true.

has_pch_encoder should only be true if we have fdi_lanes > 0. So we
shouldn't actually enable this stuff (and iirc I've tested the edp on
pipe C with pipe B using 4 lanes). The actual configuration checks all
happen in the pipe configuration computation stage. I'll rework the
patch a bit (but that'll take a while).

Thanks for the review,
Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch