| Message ID | 1396979263-3245-2-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2014-04-08 14:47 GMT-03:00 Todd Previte <tprevite@gmail.com>: > Adds a function to set the training pattern for Displayport. This is > functionality required to establish more fine-grained control over > the Displayport interface, both for operational reliability and > compliance testing. Just a suggestion: it's quite hard to review a patch that adds a new function that is not used anywhere. Is the new function a complete equivalent of some code we already have? Is it possible to just erase some code on this file and replace it for a call to the new function? If yes, then I suggest you do it on the same patch, since IMHO it makes things much easier to review since we won't need to guess how the function will be called later. When extracting+changing functions from current code, we usually do the following approach: 1 - Write a patch that creates a new function that is completely equivalent to the current code, and replace the old code with a call to the new function. This patch can't change how the code behaves. 2 - Write another patch doing whatever modification is necessary to the new function. This is the patch that should introduce new cases/features, and adjust any possible callers to do the correct thing. A little more about coding style below: > > Signed-off-by: Todd Previte <tprevite@gmail.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > drivers/gpu/drm/i915/intel_dp.c | 70 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 72 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index adcb9c7..0f0d549 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -5303,6 +5303,8 @@ enum punit_power_well { > #define DP_TP_CTL_LINK_TRAIN_NORMAL (3<<8) > #define DP_TP_CTL_SCRAMBLE_DISABLE (1<<7) > > +#define DP_TRAINING_PATTERN_MASK_1P2 0x7 Shouldn't this be defined at include/drm/drm_dp_helper.h? If yes, then I guess it needs to be in a separate patch since it will touch a generic drm header. Also, is "1P2" supposed to mean "version one point two"? If yes, I think I'd suggest adding a "v" somewhere to indicate it's a version :) > + > /* DisplayPort Transport Status */ > #define DP_TP_STATUS_A 0x64044 > #define DP_TP_STATUS_B 0x64144 > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 6f767e5..64c9803 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2356,6 +2356,76 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP) > *DP = (*DP & ~mask) | signal_levels; > } > > +uint32_t > +intel_dp_set_training_pattern(uint8_t training_pattern, If this function is going to be called only from intel_dp.c, then it needs to be static. But since it has no callers, we will start getting a WARN saying we don't have any callers. That's one of the reasons for my suggestion above. Also, you have trailing white spaces on the two lines above. I configure my editor to show any white-space-or-tab-at-the-end-of-the-line as a big red square. > + struct intel_dp *intel_dp) Lots of tabs here. Please set your editor to display tabs as 8 spaces. I also configure my editor to paint the background of column 81 as red, so I can easily see if I go past it. > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + enum port port = intel_dig_port->port; > + uint8_t buf[sizeof(intel_dp->train_set) + 1]; > + int ret, len; > + > + uint32_t reg_value, ctrl_reg, tp_select = 0; > + uint32_t tp_mask = DP_TRAINING_PATTERN_MASK; > + > + if (HAS_DDI(dev)) > + ctrl_reg = DP_TP_CTL(port); > + else > + ctrl_reg = intel_dp->output_reg; > + > + reg_value = I915_READ(ctrl_reg); > + > + // Check DPCD revision to enable TP3 Please use C-style /* comments */ here and below. See Documentation/CodingStyle Chapter 8. > + if (intel_dp->dpcd[0] >= 12) On patch 4 you replace this zero with "DP_DPCD_REV". I think you should use the macro right now. > + tp_mask = DP_TRAINING_PATTERN_MASK_1P2; > + > + // Mask selection above ensures TP3 does not get enabled for < DP 1.2 > + switch (training_pattern & tp_mask) { > + case DP_TRAINING_PATTERN_DISABLE: > + tp_select = DP_TP_CTL_LINK_TRAIN_NORMAL; > + break; > + case DP_TRAINING_PATTERN_1: > + tp_select = DP_TP_CTL_LINK_TRAIN_PAT1; > + break; > + case DP_TRAINING_PATTERN_2: > + tp_select = DP_TP_CTL_LINK_TRAIN_PAT2; > + break; > + case DP_TRAINING_PATTERN_3: > + tp_select = DP_TP_CTL_LINK_TRAIN_PAT3; > + break; > + } Per Documentation/CodingStyle Chapter 1, please put the "case" in the same column as the "switch". > + > + if (HAS_DDI(dev) || (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || port != PORT_A))) { > + reg_value &= ~(tp_mask << 8); > + reg_value |= tp_select; > + } > + else { > + reg_value &= ~(tp_mask << 28); > + reg_value |= tp_select << 20; > + } > + > + I915_WRITE(ctrl_reg, reg_value); > + POSTING_READ(ctrl_reg); > + > + buf[0] = training_pattern; > + if ((training_pattern & tp_mask) == > + DP_TRAINING_PATTERN_DISABLE) { > + /* don't write DP_TRAINING_LANEx_SET on disable */ > + len = 1; > + } else { > + /* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */ > + memcpy(buf + 1, intel_dp->train_set, intel_dp->lane_count); > + len = intel_dp->lane_count + 1; > + } > + > + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET, > + buf, len); > + > + return ret == len; I can't see the callers, but do we ever expect to have "ret != len"? If this is something that should never happen and we have to real way to recover from, I would suggest just using WARN(). > +} > + > static bool > intel_dp_set_link_train(struct intel_dp *intel_dp, > uint32_t *DP, > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index adcb9c7..0f0d549 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5303,6 +5303,8 @@ enum punit_power_well { #define DP_TP_CTL_LINK_TRAIN_NORMAL (3<<8) #define DP_TP_CTL_SCRAMBLE_DISABLE (1<<7) +#define DP_TRAINING_PATTERN_MASK_1P2 0x7 + /* DisplayPort Transport Status */ #define DP_TP_STATUS_A 0x64044 #define DP_TP_STATUS_B 0x64144 diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 6f767e5..64c9803 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2356,6 +2356,76 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP) *DP = (*DP & ~mask) | signal_levels; } +uint32_t +intel_dp_set_training_pattern(uint8_t training_pattern, + struct intel_dp *intel_dp) +{ + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + enum port port = intel_dig_port->port; + uint8_t buf[sizeof(intel_dp->train_set) + 1]; + int ret, len; + + uint32_t reg_value, ctrl_reg, tp_select = 0; + uint32_t tp_mask = DP_TRAINING_PATTERN_MASK; + + if (HAS_DDI(dev)) + ctrl_reg = DP_TP_CTL(port); + else + ctrl_reg = intel_dp->output_reg; + + reg_value = I915_READ(ctrl_reg); + + // Check DPCD revision to enable TP3 + if (intel_dp->dpcd[0] >= 12) + tp_mask = DP_TRAINING_PATTERN_MASK_1P2; + + // Mask selection above ensures TP3 does not get enabled for < DP 1.2 + switch (training_pattern & tp_mask) { + case DP_TRAINING_PATTERN_DISABLE: + tp_select = DP_TP_CTL_LINK_TRAIN_NORMAL; + break; + case DP_TRAINING_PATTERN_1: + tp_select = DP_TP_CTL_LINK_TRAIN_PAT1; + break; + case DP_TRAINING_PATTERN_2: + tp_select = DP_TP_CTL_LINK_TRAIN_PAT2; + break; + case DP_TRAINING_PATTERN_3: + tp_select = DP_TP_CTL_LINK_TRAIN_PAT3; + break; + } + + if (HAS_DDI(dev) || (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || port != PORT_A))) { + reg_value &= ~(tp_mask << 8); + reg_value |= tp_select; + } + else { + reg_value &= ~(tp_mask << 28); + reg_value |= tp_select << 20; + } + + I915_WRITE(ctrl_reg, reg_value); + POSTING_READ(ctrl_reg); + + buf[0] = training_pattern; + if ((training_pattern & tp_mask) == + DP_TRAINING_PATTERN_DISABLE) { + /* don't write DP_TRAINING_LANEx_SET on disable */ + len = 1; + } else { + /* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */ + memcpy(buf + 1, intel_dp->train_set, intel_dp->lane_count); + len = intel_dp->lane_count + 1; + } + + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET, + buf, len); + + return ret == len; +} + static bool intel_dp_set_link_train(struct intel_dp *intel_dp, uint32_t *DP,
Adds a function to set the training pattern for Displayport. This is functionality required to establish more fine-grained control over the Displayport interface, both for operational reliability and compliance testing. Signed-off-by: Todd Previte <tprevite@gmail.com> --- drivers/gpu/drm/i915/i915_reg.h | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 70 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+)