2013/11/4 Ben Widawsky <ben@xxxxxxxxxxxx>:
> On Sat, Nov 02, 2013 at 09:07:43PM -0700, Ben Widawsky wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> They're not the same as the Haswell ones.
>>
>> Reviewed-by: Art Runyan <arthur.j.runyan@xxxxxxxxx>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 11 +++++++++
>> drivers/gpu/drm/i915/intel_dp.c | 55 ++++++++++++++++++++++++++++++++++++++---
>> 2 files changed, 63 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 4131223..6f834b3 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -5176,6 +5176,7 @@
>> #define DDI_BUF_CTL_B 0x64100
>> #define DDI_BUF_CTL(port) _PORT(port, DDI_BUF_CTL_A, DDI_BUF_CTL_B)
>> #define DDI_BUF_CTL_ENABLE (1<<31)
>> +/* Haswell */
>> #define DDI_BUF_EMP_400MV_0DB_HSW (0<<24) /* Sel0 */
>> #define DDI_BUF_EMP_400MV_3_5DB_HSW (1<<24) /* Sel1 */
>> #define DDI_BUF_EMP_400MV_6DB_HSW (2<<24) /* Sel2 */
>> @@ -5185,6 +5186,16 @@
>> #define DDI_BUF_EMP_600MV_6DB_HSW (6<<24) /* Sel6 */
>> #define DDI_BUF_EMP_800MV_0DB_HSW (7<<24) /* Sel7 */
>> #define DDI_BUF_EMP_800MV_3_5DB_HSW (8<<24) /* Sel8 */
>> +/* Broadwell */
>> +#define DDI_BUF_EMP_400MV_0DB_BDW (0<<24) /* Sel0 */
>> +#define DDI_BUF_EMP_400MV_3_5DB_BDW (1<<24) /* Sel1 */
>> +#define DDI_BUF_EMP_400MV_6DB_BDW (2<<24) /* Sel2 */
>> +#define DDI_BUF_EMP_600MV_0DB_BDW (3<<24) /* Sel3 */
>> +#define DDI_BUF_EMP_600MV_3_5DB_BDW (4<<24) /* Sel4 */
>> +#define DDI_BUF_EMP_600MV_6DB_BDW (5<<24) /* Sel5 */
>
> Maybe I am misreading this, isn't this:
> 600mV, 4.5dB?
>
The DP spec defines voltage swing and pre-emphasis "levels" (0 to 3),
and each level has a "Min", "Nom" and "Max" value. The definitions on
the DRM layer (include/drm/drm_dp_helper.h) don't use the level
numbers, they use the "Nom" values to identify the levels (e.g, level
1 is 3.5dB). Our HW doesn't use the exact "Nom" values for each of the
levels, so there is some inconsistency. The correct thing to do is to
patch the DRM macros and then all the drivers that use them, but for
BDW I didn't want a huge patch, so the strategy was to just name our
own macros in a way that they would match the DRM macros, so the code
wouldn't look confusing. Ideally, after we fix the DRM layer, we
should use just LEVEL{0,1,2,3} for everything on our driver too and
completely ignore the real mV and dB used by the HW.
>> +#define DDI_BUF_EMP_800MV_0DB_BDW (6<<24) /* Sel6 */
>
> 850, .5
>
>> +#define DDI_BUF_EMP_800MV_3_5DB_BDW (7<<24) /* Sel7 */
>
> 750, 2.5
>
>> +#define DDI_BUF_EMP_1200MV_0DB_BDW (8<<24) /* Sel8 */
>
> 1000, 0
>
> Sorry if I just misunderstood how you're getting these values.
This is confusing, I agree.
>
>> #define DDI_BUF_EMP_MASK (0xf<<24)
>> #define DDI_BUF_PORT_REVERSAL (1<<16)
>> #define DDI_BUF_IS_IDLE (1<<7)
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index b3cc333..7725f81 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1950,7 +1950,7 @@ intel_dp_voltage_max(struct intel_dp *intel_dp)
>> struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> enum port port = dp_to_dig_port(intel_dp)->port;
>>
>> - if (IS_VALLEYVIEW(dev))
>> + if (IS_VALLEYVIEW(dev) || IS_BROADWELL(dev))
>> return DP_TRAIN_VOLTAGE_SWING_1200;
>> else if (IS_GEN7(dev) && port == PORT_A)
>> return DP_TRAIN_VOLTAGE_SWING_800;
>> @@ -1966,7 +1966,18 @@ intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing)
>> struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> enum port port = dp_to_dig_port(intel_dp)->port;
>>
>> - if (HAS_DDI(dev)) {
>> + if (IS_BROADWELL(dev)) {
>> + switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>> + case DP_TRAIN_VOLTAGE_SWING_400:
>> + case DP_TRAIN_VOLTAGE_SWING_600:
>> + return DP_TRAIN_PRE_EMPHASIS_6;
>> + case DP_TRAIN_VOLTAGE_SWING_800:
>> + return DP_TRAIN_PRE_EMPHASIS_3_5;
>> + case DP_TRAIN_VOLTAGE_SWING_1200:
>> + default:
>> + return DP_TRAIN_PRE_EMPHASIS_0;
>> + }
>> + } else if (IS_HASWELL(dev)) {
>> switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>> case DP_TRAIN_VOLTAGE_SWING_400:
>> return DP_TRAIN_PRE_EMPHASIS_9_5;
>> @@ -2278,6 +2289,41 @@ intel_hsw_signal_levels(uint8_t train_set)
>> }
>> }
>>
>> +static uint32_t
>> +intel_bdw_signal_levels(uint8_t train_set)
>> +{
>> + int signal_levels = train_set & (DP_TRAIN_VOLTAGE_SWING_MASK |
>> + DP_TRAIN_PRE_EMPHASIS_MASK);
>> + switch (signal_levels) {
>> + case DP_TRAIN_VOLTAGE_SWING_400 | DP_TRAIN_PRE_EMPHASIS_0:
>> + return DDI_BUF_EMP_400MV_0DB_BDW; /* Sel0 */
>> + case DP_TRAIN_VOLTAGE_SWING_400 | DP_TRAIN_PRE_EMPHASIS_3_5:
>> + return DDI_BUF_EMP_400MV_3_5DB_BDW; /* Sel1 */
>> + case DP_TRAIN_VOLTAGE_SWING_400 | DP_TRAIN_PRE_EMPHASIS_6:
>> + return DDI_BUF_EMP_400MV_6DB_BDW; /* Sel2 */
>> +
>> + case DP_TRAIN_VOLTAGE_SWING_600 | DP_TRAIN_PRE_EMPHASIS_0:
>> + return DDI_BUF_EMP_600MV_0DB_BDW; /* Sel3 */
>> + case DP_TRAIN_VOLTAGE_SWING_600 | DP_TRAIN_PRE_EMPHASIS_3_5:
>> + return DDI_BUF_EMP_600MV_3_5DB_BDW; /* Sel4 */
>> + case DP_TRAIN_VOLTAGE_SWING_600 | DP_TRAIN_PRE_EMPHASIS_6:
>> + return DDI_BUF_EMP_600MV_6DB_BDW; /* Sel5 */
>> +
>> + case DP_TRAIN_VOLTAGE_SWING_800 | DP_TRAIN_PRE_EMPHASIS_0:
>> + return DDI_BUF_EMP_800MV_0DB_BDW; /* Sel6 */
>> + case DP_TRAIN_VOLTAGE_SWING_800 | DP_TRAIN_PRE_EMPHASIS_3_5:
>> + return DDI_BUF_EMP_800MV_3_5DB_BDW; /* Sel7 */
>> +
>> + case DP_TRAIN_VOLTAGE_SWING_1200 | DP_TRAIN_PRE_EMPHASIS_0:
>> + return DDI_BUF_EMP_1200MV_0DB_BDW; /* Sel8 */
>> +
>> + default:
>> + DRM_DEBUG_KMS("Unsupported voltage swing/pre-emphasis level:"
>> + "0x%x\n", signal_levels);
>> + return DDI_BUF_EMP_400MV_0DB_BDW; /* Sel0 */
>> + }
>> +}
>> +
>> /* Properly updates "DP" with the correct signal levels. */
>> static void
>> intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
>> @@ -2288,7 +2334,10 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
>> uint32_t signal_levels, mask;
>> uint8_t train_set = intel_dp->train_set[0];
>>
>> - if (HAS_DDI(dev)) {
>> + if (IS_BROADWELL(dev)) {
>> + signal_levels = intel_bdw_signal_levels(train_set);
>> + mask = DDI_BUF_EMP_MASK;
>> + } else if (IS_HASWELL(dev)) {
>> signal_levels = intel_hsw_signal_levels(train_set);
>> mask = DDI_BUF_EMP_MASK;
>> } else if (IS_VALLEYVIEW(dev)) {
>
> Forgive my ignorance, but I really have no idea how to review this
> patch. Daniel, I think we have to settle for Art's r-b on this one, or
> finds someone who understands why this programming is the way it is. (Or
> wait)
>
> --
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
