Re: [PATCH] drm/i915: Enable 5.4Ghz (HBR2) link rate for Displayport 1.2-capable devices

[Todd Previte <tprevite@gmail.com>]

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
>
>