| Message ID | 1389927968-2473-1-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 17, 2014 at 4:06 AM, Todd Previte <tprevite@gmail.com> wrote: > For HSW+ platforms, enable the 5.4Ghz (HBR2) link rate for devices that support it. The > sink device must report that is supports Displayport 1.2 and the HBR2 bit rate in the > DPCD in order to use HBR2. sob line missing. > --- > drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 7df5085..f92d1c0 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -102,7 +102,10 @@ intel_dp_max_link_bw(struct intel_dp *intel_dp) > case DP_LINK_BW_2_7: > break; > case DP_LINK_BW_5_4: /* 1.2 capable displays may advertise higher bw */ > - max_link_bw = DP_LINK_BW_2_7; > + if (intel_dp->dpcd[DP_DPCD_REV] == 0x12) > + max_link_bw = DP_LINK_BW_5_4; > + else > + max_link_bw = DP_LINK_BW_2_7; Is this really required, i.e. do we have dp 1.1 machines in the wild which advertise 5.4 but can't? In any case you also need to have a IS_HSW || IS_BDW check here, since only those two platforms support 5.4 GHz. > break; > default: > WARN(1, "invalid max DP link bw val %x, using 1.62Gbps\n", > @@ -805,9 +808,10 @@ intel_dp_compute_config(struct intel_encoder *encoder, > struct intel_connector *intel_connector = intel_dp->attached_connector; > int lane_count, clock; > int max_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); > - int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0; > + /* Conveniently, the link BW constants become indices with a shift...*/ > + int max_clock = intel_dp_max_link_bw(intel_dp) >> 3; > int bpp, mode_rate; > - static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 }; > + static int bws[] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7, DP_LINK_BW_5_4 }; > int link_avail, link_clock; > > if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && port != PORT_A) > @@ -2621,10 +2625,15 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > bool channel_eq = false; > int tries, cr_tries; > uint32_t DP = intel_dp->DP; > + uint32_t training_pattern = DP_TRAINING_PATTERN_2; > + > + /* Training Pattern 3 for HBR2 */ > + if (intel_dp->link_bw == DP_LINK_BW_5_4) > + training_pattern = DP_TRAINING_PATTERN_3; Hm, I've thought that with 5.4 we're supposed to do both pattern 2&3. But tbh I didn't bother to dig out the spec ;-) Does it hurt not to? Also, is there any harm in using pattern 3 for all dp 1.2 capable sinks? I've thought that it should give us more reliable link training, so might be beneficial not just for 5.4 mode. -Daniel > /* channel equalization */ > if (!intel_dp_set_link_train(intel_dp, &DP, > - DP_TRAINING_PATTERN_2 | > + training_pattern | > DP_LINK_SCRAMBLING_DISABLE)) { > DRM_ERROR("failed to start channel equalization\n"); > return; > @@ -2652,7 +2661,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > if (!drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) { > intel_dp_start_link_train(intel_dp); > intel_dp_set_link_train(intel_dp, &DP, > - DP_TRAINING_PATTERN_2 | > + training_pattern | > DP_LINK_SCRAMBLING_DISABLE); > cr_tries++; > continue; > @@ -2668,7 +2677,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > intel_dp_link_down(intel_dp); > intel_dp_start_link_train(intel_dp); > intel_dp_set_link_train(intel_dp, &DP, > - DP_TRAINING_PATTERN_2 | > + training_pattern | > DP_LINK_SCRAMBLING_DISABLE); > tries = 0; > cr_tries++; > -- > 1.8.1.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jan 16, 2014 at 08:06:08PM -0700, Todd Previte wrote: > For HSW+ platforms, enable the 5.4Ghz (HBR2) link rate for devices that support it. The > sink device must report that is supports Displayport 1.2 and the HBR2 bit rate in the > DPCD in order to use HBR2. > --- > drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 7df5085..f92d1c0 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -102,7 +102,10 @@ intel_dp_max_link_bw(struct intel_dp *intel_dp) > case DP_LINK_BW_2_7: > break; > case DP_LINK_BW_5_4: /* 1.2 capable displays may advertise higher bw */ > - max_link_bw = DP_LINK_BW_2_7; > + if (intel_dp->dpcd[DP_DPCD_REV] == 0x12) > + max_link_bw = DP_LINK_BW_5_4; > + else > + max_link_bw = DP_LINK_BW_2_7; > break; I see spaces instead of tabs. You can use the useful checkpatch.pl script on patches to catch those pesky style issues (from within a linux tree): $ ./scripts/checkpatch.pl 0001-drm-i915-Enable-5.4Ghz-HBR2-link-rate-for-Displaypor.patch [...] total: 6 errors, 9 warnings, 55 lines checked
On Fri, 17 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote: > I see spaces instead of tabs. You can use the useful checkpatch.pl > script on patches to catch those pesky style issues (from within a linux > tree): > > $ ./scripts/checkpatch.pl 0001-drm-i915-Enable-5.4Ghz-HBR2-link-rate-for-Displaypor.patch > > [...] > > total: 6 errors, 9 warnings, 55 lines checked Should anyone find this useful, I have these to check branches in my local repos directly: alias checkpatch='/path/to/checkpatch.pl -q --emacs --strict' checkbranch() { local commit local range if [ -z "$1" ]; then range="origin..HEAD" elif [ -n "`echo $1 | grep '\.\.'`" ]; then range="$1" else range="$1..HEAD" fi for commit in `git rev-list --reverse $range`; do git --no-pager log --oneline -1 $commit git format-patch --stdout -1 $commit | checkpatch - done } Then I can do: Check local patches against origin: $ checkbranch Check local patches against drm-intel/drm-intel-nightly: $ checkbranch drm-intel/drm-intel-nightly Check a revision range: $ checkbranch commit1..commit2 Do note that checkpatch is not the law. But it helps you get some of the little things straight. HTH, Jani.
On 1/16/2014 11:30 PM, Daniel Vetter wrote: > On Fri, Jan 17, 2014 at 4:06 AM, Todd Previte <tprevite@gmail.com> wrote: >> For HSW+ platforms, enable the 5.4Ghz (HBR2) link rate for devices that support it. The >> sink device must report that is supports Displayport 1.2 and the HBR2 bit rate in the >> DPCD in order to use HBR2. > sob line missing. Oops. I'll add Signed-off-by: Todd Previte <tprevite@gmail.com> to the next revision. > >> --- >> drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 7df5085..f92d1c0 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -102,7 +102,10 @@ intel_dp_max_link_bw(struct intel_dp *intel_dp) >> case DP_LINK_BW_2_7: >> break; >> case DP_LINK_BW_5_4: /* 1.2 capable displays may advertise higher bw */ >> - max_link_bw = DP_LINK_BW_2_7; >> + if (intel_dp->dpcd[DP_DPCD_REV] == 0x12) >> + max_link_bw = DP_LINK_BW_5_4; >> + else >> + max_link_bw = DP_LINK_BW_2_7; > Is this really required, i.e. do we have dp 1.1 machines in the wild > which advertise 5.4 but can't? In any case you also need to have a > IS_HSW || IS_BDW check here, since only those two platforms support > 5.4 GHz. I've not seen a case where a 1.1a capable device advertises HBR2, no. I *have* seen the case where the sink reports that it only supports RBR (1.62Ghz) but is in fact capable of 2.7Ghz. This is more of a safety measure to eliminate potential training problems, but is not strictly necessary to support HBR2. It does need the IS_HSW || IS_BDW though, so I'll fix that and resend. >> break; >> default: >> WARN(1, "invalid max DP link bw val %x, using 1.62Gbps\n", >> @@ -805,9 +808,10 @@ intel_dp_compute_config(struct intel_encoder *encoder, >> struct intel_connector *intel_connector = intel_dp->attached_connector; >> int lane_count, clock; >> int max_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); >> - int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0; >> + /* Conveniently, the link BW constants become indices with a shift...*/ >> + int max_clock = intel_dp_max_link_bw(intel_dp) >> 3; >> int bpp, mode_rate; >> - static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 }; >> + static int bws[] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7, DP_LINK_BW_5_4 }; >> int link_avail, link_clock; >> >> if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && port != PORT_A) >> @@ -2621,10 +2625,15 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) >> bool channel_eq = false; >> int tries, cr_tries; >> uint32_t DP = intel_dp->DP; >> + uint32_t training_pattern = DP_TRAINING_PATTERN_2; >> + >> + /* Training Pattern 3 for HBR2 */ >> + if (intel_dp->link_bw == DP_LINK_BW_5_4) >> + training_pattern = DP_TRAINING_PATTERN_3; > Hm, I've thought that with 5.4 we're supposed to do both pattern 2&3. > But tbh I didn't bother to dig out the spec ;-) Does it hurt not to? > Also, is there any harm in using pattern 3 for all dp 1.2 capable > sinks? I've thought that it should give us more reliable link > training, so might be beneficial not just for 5.4 mode. > -Daniel Nope. For 5.4Ghz the source is supposed to use TPS3 in place of TPS2 in the channel equalization phase. I think it would cause problems if we try to use both TPS2 and TPS3. I suspect the sink device would simple not lock onto the pattern from TPS2 since it's expecting pattern 3 when set for HBR2. So all that would do is effectively delay link training until such time as TPS3 was being transmitted. There is a bit in the DPCD (bit 6, DPCD 0x002) that indicates whether or not a sink device supports TPS3. For these devices, we should be using TPS3 instead of TPS2 for the channel equalization phase, yes. The code doesn't currently check this bit but it should. I'll add that into the configuration computation to enable TPS3 for 1.2 devices that support it. >> /* channel equalization */ >> if (!intel_dp_set_link_train(intel_dp, &DP, >> - DP_TRAINING_PATTERN_2 | >> + training_pattern | >> DP_LINK_SCRAMBLING_DISABLE)) { >> DRM_ERROR("failed to start channel equalization\n"); >> return; >> @@ -2652,7 +2661,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) >> if (!drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) { >> intel_dp_start_link_train(intel_dp); >> intel_dp_set_link_train(intel_dp, &DP, >> - DP_TRAINING_PATTERN_2 | >> + training_pattern | >> DP_LINK_SCRAMBLING_DISABLE); >> cr_tries++; >> continue; >> @@ -2668,7 +2677,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) >> intel_dp_link_down(intel_dp); >> intel_dp_start_link_train(intel_dp); >> intel_dp_set_link_train(intel_dp, &DP, >> - DP_TRAINING_PATTERN_2 | >> + training_pattern | >> DP_LINK_SCRAMBLING_DISABLE); >> tries = 0; >> cr_tries++; >> -- >> 1.8.1.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On 1/17/2014 4:55 AM, Damien Lespiau wrote: > On Thu, Jan 16, 2014 at 08:06:08PM -0700, Todd Previte wrote: >> For HSW+ platforms, enable the 5.4Ghz (HBR2) link rate for devices that support it. The >> sink device must report that is supports Displayport 1.2 and the HBR2 bit rate in the >> DPCD in order to use HBR2. >> --- >> drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 7df5085..f92d1c0 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -102,7 +102,10 @@ intel_dp_max_link_bw(struct intel_dp *intel_dp) >> case DP_LINK_BW_2_7: >> break; >> case DP_LINK_BW_5_4: /* 1.2 capable displays may advertise higher bw */ >> - max_link_bw = DP_LINK_BW_2_7; >> + if (intel_dp->dpcd[DP_DPCD_REV] == 0x12) >> + max_link_bw = DP_LINK_BW_5_4; >> + else >> + max_link_bw = DP_LINK_BW_2_7; >> break; > I see spaces instead of tabs. You can use the useful checkpatch.pl > script on patches to catch those pesky style issues (from within a linux > tree): > > $ ./scripts/checkpatch.pl 0001-drm-i915-Enable-5.4Ghz-HBR2-link-rate-for-Displaypor.patch > > [...] > > total: 6 errors, 9 warnings, 55 lines checked > Gah. That's what happens when I make changes with different editors. I'll fix those in the next revision. -T
On Fri, Jan 17, 2014 at 07:58:58AM -0700, Todd Previte wrote: > >>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >>index 7df5085..f92d1c0 100644 > >>--- a/drivers/gpu/drm/i915/intel_dp.c > >>+++ b/drivers/gpu/drm/i915/intel_dp.c > >>@@ -102,7 +102,10 @@ intel_dp_max_link_bw(struct intel_dp *intel_dp) > >> case DP_LINK_BW_2_7: > >> break; > >> case DP_LINK_BW_5_4: /* 1.2 capable displays may advertise higher bw */ > >>- max_link_bw = DP_LINK_BW_2_7; > >>+ if (intel_dp->dpcd[DP_DPCD_REV] == 0x12) > >>+ max_link_bw = DP_LINK_BW_5_4; > >>+ else > >>+ max_link_bw = DP_LINK_BW_2_7; > >Is this really required, i.e. do we have dp 1.1 machines in the wild > >which advertise 5.4 but can't? In any case you also need to have a > >IS_HSW || IS_BDW check here, since only those two platforms support > >5.4 GHz. > I've not seen a case where a 1.1a capable device advertises HBR2, > no. I *have* seen the case where the sink reports that it only > supports RBR (1.62Ghz) but is in fact capable of 2.7Ghz. This is > more of a safety measure to eliminate potential training problems, > but is not strictly necessary to support HBR2. It does need the > IS_HSW || IS_BDW though, so I'll fix that and resend. Can we make it IS_HSW || INTEL_INFO(dev)->gen >= 8, we can't quite predict the future but new platforms supporting what old platforms do support is a bet we take elsewere.
On 1/17/2014 6:32 AM, Jani Nikula wrote: > On Fri, 17 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote: >> I see spaces instead of tabs. You can use the useful checkpatch.pl >> script on patches to catch those pesky style issues (from within a linux >> tree): >> >> $ ./scripts/checkpatch.pl 0001-drm-i915-Enable-5.4Ghz-HBR2-link-rate-for-Displaypor.patch >> >> [...] >> >> total: 6 errors, 9 warnings, 55 lines checked > Should anyone find this useful, I have these to check branches in my > local repos directly: > > alias checkpatch='/path/to/checkpatch.pl -q --emacs --strict' > > checkbranch() > { > local commit > local range > > if [ -z "$1" ]; then > range="origin..HEAD" > elif [ -n "`echo $1 | grep '\.\.'`" ]; then > range="$1" > else > range="$1..HEAD" > fi > > for commit in `git rev-list --reverse $range`; do > git --no-pager log --oneline -1 $commit > git format-patch --stdout -1 $commit | checkpatch - > done > } > > Then I can do: > > Check local patches against origin: > $ checkbranch > > Check local patches against drm-intel/drm-intel-nightly: > $ checkbranch drm-intel/drm-intel-nightly > > Check a revision range: > $ checkbranch commit1..commit2 > > Do note that checkpatch is not the law. But it helps you get some of the > little things straight. > > > HTH, > Jani. > > > Cool thanks Jani. I'll give that a try. -T
On 1/17/2014 8:08 AM, Damien Lespiau wrote: > On Fri, Jan 17, 2014 at 07:58:58AM -0700, Todd Previte wrote: >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>>> index 7df5085..f92d1c0 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>> @@ -102,7 +102,10 @@ intel_dp_max_link_bw(struct intel_dp *intel_dp) >>>> case DP_LINK_BW_2_7: >>>> break; >>>> case DP_LINK_BW_5_4: /* 1.2 capable displays may advertise higher bw */ >>>> - max_link_bw = DP_LINK_BW_2_7; >>>> + if (intel_dp->dpcd[DP_DPCD_REV] == 0x12) >>>> + max_link_bw = DP_LINK_BW_5_4; >>>> + else >>>> + max_link_bw = DP_LINK_BW_2_7; >>> Is this really required, i.e. do we have dp 1.1 machines in the wild >>> which advertise 5.4 but can't? In any case you also need to have a >>> IS_HSW || IS_BDW check here, since only those two platforms support >>> 5.4 GHz. >> I've not seen a case where a 1.1a capable device advertises HBR2, >> no. I *have* seen the case where the sink reports that it only >> supports RBR (1.62Ghz) but is in fact capable of 2.7Ghz. This is >> more of a safety measure to eliminate potential training problems, >> but is not strictly necessary to support HBR2. It does need the >> IS_HSW || IS_BDW though, so I'll fix that and resend. > Can we make it IS_HSW || INTEL_INFO(dev)->gen >= 8, we can't quite > predict the future but new platforms supporting what old platforms do > support is a bet we take elsewere. > Sounds like a better solution. I should have that integrated shortly. -T
Clean up and adjustments per the feedback above.
More whitepsace cleanup. One caveat with this patch: current link policy dictates that the driver will train the "wide and slow", i.e. max lanes at low speed. It will increase lanes and speed when the specified resolution demands greater bandwidth. Consequently, the resolution will need to be set such that the data rate exceeds the bandwidth provided by 4 lanes @ 2.4Ghz before the driver will use the new 5.4Ghz rate.
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 7df5085..f92d1c0 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -102,7 +102,10 @@ intel_dp_max_link_bw(struct intel_dp *intel_dp) case DP_LINK_BW_2_7: break; case DP_LINK_BW_5_4: /* 1.2 capable displays may advertise higher bw */ - max_link_bw = DP_LINK_BW_2_7; + if (intel_dp->dpcd[DP_DPCD_REV] == 0x12) + max_link_bw = DP_LINK_BW_5_4; + else + max_link_bw = DP_LINK_BW_2_7; break; default: WARN(1, "invalid max DP link bw val %x, using 1.62Gbps\n", @@ -805,9 +808,10 @@ intel_dp_compute_config(struct intel_encoder *encoder, struct intel_connector *intel_connector = intel_dp->attached_connector; int lane_count, clock; int max_lane_count = drm_dp_max_lane_count(intel_dp->dpcd); - int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0; + /* Conveniently, the link BW constants become indices with a shift...*/ + int max_clock = intel_dp_max_link_bw(intel_dp) >> 3; int bpp, mode_rate; - static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 }; + static int bws[] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7, DP_LINK_BW_5_4 }; int link_avail, link_clock; if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && port != PORT_A) @@ -2621,10 +2625,15 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) bool channel_eq = false; int tries, cr_tries; uint32_t DP = intel_dp->DP; + uint32_t training_pattern = DP_TRAINING_PATTERN_2; + + /* Training Pattern 3 for HBR2 */ + if (intel_dp->link_bw == DP_LINK_BW_5_4) + training_pattern = DP_TRAINING_PATTERN_3; /* channel equalization */ if (!intel_dp_set_link_train(intel_dp, &DP, - DP_TRAINING_PATTERN_2 | + training_pattern | DP_LINK_SCRAMBLING_DISABLE)) { DRM_ERROR("failed to start channel equalization\n"); return; @@ -2652,7 +2661,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) if (!drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) { intel_dp_start_link_train(intel_dp); intel_dp_set_link_train(intel_dp, &DP, - DP_TRAINING_PATTERN_2 | + training_pattern | DP_LINK_SCRAMBLING_DISABLE); cr_tries++; continue; @@ -2668,7 +2677,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) intel_dp_link_down(intel_dp); intel_dp_start_link_train(intel_dp); intel_dp_set_link_train(intel_dp, &DP, - DP_TRAINING_PATTERN_2 | + training_pattern | DP_LINK_SCRAMBLING_DISABLE); tries = 0; cr_tries++;