[PATCH] drm/i915: prefer wide & slow to fast & narrow in DP configs

[PATCH] drm/i915: prefer wide & slow to fast & narrow in DP configs

[Jesse Barnes]

High frequency link configurations have the potential to cause trouble
with long and/or cheap cables, so prefer slow and wide configurations
instead.  This patch has the potential to cause trouble for eDP
configurations that lie about available lanes, so if we run into that we
can make it conditional on eDP.

References: https://bugs.freedesktop.org/show_bug.cgi?id=45801
Tested-by: peter@colberg.org
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6538c46..df8800e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -725,8 +725,8 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	bpp = adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 24;
 	mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
 
-	for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
-		for (clock = 0; clock <= max_clock; clock++) {
+	for (clock = 0; clock <= max_clock; clock++) {
+		for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
 			int link_avail = intel_dp_max_data_rate(intel_dp_link_clock(bws[clock]), lane_count);
 
 			if (mode_rate <= link_avail) {
-- 
1.7.9.5

Re: [PATCH] drm/i915: prefer wide & slow to fast & narrow in DP configs

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 506 bytes --]

Jesse Barnes <jbarnes@virtuousgeek.org> writes:

> High frequency link configurations have the potential to cause trouble
> with long and/or cheap cables, so prefer slow and wide configurations
> instead.  This patch has the potential to cause trouble for eDP
> configurations that lie about available lanes, so if we run into that we
> can make it conditional on eDP.

I *have* run into this on eDP machines already, which is why the code
loops this way today...

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: prefer wide & slow to fast & narrow in DP configs

[Chris Wilson]

On Thu, 21 Jun 2012 18:13:19 -0700, Keith Packard <keithp@keithp.com> wrote:
> Jesse Barnes <jbarnes@virtuousgeek.org> writes:
> 
> > High frequency link configurations have the potential to cause trouble
> > with long and/or cheap cables, so prefer slow and wide configurations
> > instead.  This patch has the potential to cause trouble for eDP
> > configurations that lie about available lanes, so if we run into that we
> > can make it conditional on eDP.
> 
> I *have* run into this on eDP machines already, which is why the code
> loops this way today...

It was structured to minimise lane count because certain chipsets did
not wire up all the lanes, right? Is that still relevant as we are using
the advertised max_lane_count from the DPCD now?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: prefer wide & slow to fast & narrow in DP configs

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 1694 bytes --]

On Fri, 2012-06-22 at 10:05 +0100, Chris Wilson wrote:
> On Thu, 21 Jun 2012 18:13:19 -0700, Keith Packard <keithp@keithp.com> wrote:
> > Jesse Barnes <jbarnes@virtuousgeek.org> writes:
> > 
> > > High frequency link configurations have the potential to cause trouble
> > > with long and/or cheap cables, so prefer slow and wide configurations
> > > instead.  This patch has the potential to cause trouble for eDP
> > > configurations that lie about available lanes, so if we run into that we
> > > can make it conditional on eDP.

Have we considered looking at the link quality bits of DPCD for this?
Section 2.5.3.5 of the DP 1.1 spec looks apropos.  It looks painfully
slow to get all the way to the actual spec error rate, but it might not
be a bad first test to run for a second before doing actual link
training.  Do you have a crappy cable that produces this problem?

There's also a comment about the sink clearing the symbol lock and lane
alignment bits on too many errors (3.5.1.3.2); we're not periodically
re-checking those bits, maybe we should.

It's a shame they didn't bother to spec anything actually good, like
"sink must report the number of ECC corrections it's done".  But I
suppose that applies to DP as a whole really.

> > I *have* run into this on eDP machines already, which is why the code
> > loops this way today...
> 
> It was structured to minimise lane count because certain chipsets did
> not wire up all the lanes, right? Is that still relevant as we are using
> the advertised max_lane_count from the DPCD now?

Pretty sure it's structured to use minimum lane count because that's the
correct thing to do for power.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: prefer wide & slow to fast & narrow in DP configs

[Jesse Barnes]

On Fri, 22 Jun 2012 10:05:19 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 21 Jun 2012 18:13:19 -0700, Keith Packard <keithp@keithp.com> wrote:
> > Jesse Barnes <jbarnes@virtuousgeek.org> writes:
> > 
> > > High frequency link configurations have the potential to cause trouble
> > > with long and/or cheap cables, so prefer slow and wide configurations
> > > instead.  This patch has the potential to cause trouble for eDP
> > > configurations that lie about available lanes, so if we run into that we
> > > can make it conditional on eDP.
> > 
> > I *have* run into this on eDP machines already, which is why the code
> > loops this way today...
> 
> It was structured to minimise lane count because certain chipsets did
> not wire up all the lanes, right? Is that still relevant as we are using
> the advertised max_lane_count from the DPCD now?

In embedded applications, some of the lanes may not exist, but the DPCD
should indicate that (though as Keith says, some lie about it).  But if
we set aside eDP it may be safe...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: prefer wide & slow to fast & narrow in DP configs

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 571 bytes --]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Thu, 21 Jun 2012 18:13:19 -0700, Keith Packard <keithp@keithp.com> wrote:

> It was structured to minimise lane count because certain chipsets did
> not wire up all the lanes, right? Is that still relevant as we are using
> the advertised max_lane_count from the DPCD now?

We've always used the max_lane_count from dpcd; has there been some
recent change that fixed usage of that? What I recall is one acer laptop
that advertised 4 lanes but had only wired up two of them.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: prefer wide & slow to fast & narrow in DP configs

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 392 bytes --]

Jesse Barnes <jbarnes@virtuousgeek.org> writes:

> In embedded applications, some of the lanes may not exist, but the DPCD
> should indicate that (though as Keith says, some lie about it).  But if
> we set aside eDP it may be safe...

Yeah, that's my thinking. We should probably include eDP hooked up to
the PCH DP lanes (for all-in-one systems) too.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: prefer wide & slow to fast & narrow in DP configs

[Chris Wilson]

On Fri, 22 Jun 2012 10:40:22 -0700, Keith Packard <keithp@keithp.com> wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Thu, 21 Jun 2012 18:13:19 -0700, Keith Packard <keithp@keithp.com> wrote:
> 
> > It was structured to minimise lane count because certain chipsets did
> > not wire up all the lanes, right? Is that still relevant as we are using
> > the advertised max_lane_count from the DPCD now?
> 
> We've always used the max_lane_count from dpcd; has there been some
> recent change that fixed usage of that? What I recall is one acer laptop
> that advertised 4 lanes but had only wired up two of them.

The only recentish change was your

commit 9a10f401a401ca69c6537641c8fc0d6b57b5aee8
Author: Keith Packard <keithp@keithp.com>
Date:   Wed Nov 2 13:03:47 2011 -0700

    drm/i915: Use DPCD value for max DP lanes.

    The BIOS VBT value for an eDP panel has been shown to be incorrect on
    one machine, and we haven't found any machines where the DPCD value
    was wrong, so we'll use the DPCD value everywhere.

We can but hope that no manufacturer lies in the DPCD.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: prefer wide & slow to fast & narrow in DP configs

[Daniel Vetter]

On Fri, Jun 22, 2012 at 06:53:01PM +0100, Chris Wilson wrote:
> On Fri, 22 Jun 2012 10:40:22 -0700, Keith Packard <keithp@keithp.com> wrote:
> > Chris Wilson <chris@chris-wilson.co.uk> writes:
> > 
> > > On Thu, 21 Jun 2012 18:13:19 -0700, Keith Packard <keithp@keithp.com> wrote:
> > 
> > > It was structured to minimise lane count because certain chipsets did
> > > not wire up all the lanes, right? Is that still relevant as we are using
> > > the advertised max_lane_count from the DPCD now?
> > 
> > We've always used the max_lane_count from dpcd; has there been some
> > recent change that fixed usage of that? What I recall is one acer laptop
> > that advertised 4 lanes but had only wired up two of them.
> 
> The only recentish change was your
> 
> commit 9a10f401a401ca69c6537641c8fc0d6b57b5aee8
> Author: Keith Packard <keithp@keithp.com>
> Date:   Wed Nov 2 13:03:47 2011 -0700
> 
>     drm/i915: Use DPCD value for max DP lanes.
> 
>     The BIOS VBT value for an eDP panel has been shown to be incorrect on
>     one machine, and we haven't found any machines where the DPCD value
>     was wrong, so we'll use the DPCD value everywhere.
> 
> We can but hope that no manufacturer lies in the DPCD.

Ok, I've merged this patch because it fixes a regression and this commit
Chris has dug out seems to indicate that we won't hit any known issues on
eDP panels. I guess if it blows up again, we'll have to take another look.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[3.2.y, 3.4.y, 3.5.y] Re: [PATCH] drm/i915: prefer wide & slow to fast & narrow in DP configs

[Jonathan Nieder]

Hi Ben, Greg, et al,

Please consider

  2514bc510d0c drm/i915: prefer wide & slow to fast & narrow in DP
               configs, 2012-06-21

for application to the 3.2.y, 3.4.y, and 3.5.y trees.

It addresses a regression which bisects to v3.2-rc1~135^2~2^2~7
(drm/i915/dp: Fix the math in intel_dp_link_required, 2011-10-14).
An external display connected through DisplayPort would give "DP no
signal" unless the resolution was overridden to be artificially low.

In discussion of the patch, two worries came up:

 - In older kernels, some machines lied about the maximum number of
   DP lanes.  Luckily v3.2-rc3~8^2~5^2~2 (drm/i915: Use DPCD value for
   max DP lanes, 2011-11-02) solved that, so the patch should be safe.

 - Using the minimum lane count is the right thing to do to minimize
   power consumption.  But a working display is more important.

Peter who discovered the bug tested the patch in June and found it to
work.  He tried it against a 3.2.y tree in July and it still worked.
The patch has been in mainline for about a week and a half and in
Debian's 3.2.y-based kernel since around the same time.  No complaints
yet.

Thoughts of all kinds welcome, as usual.

Hope that helps,
Jonathan