Hi
2014-04-08 14:47 GMT-03:00 Todd Previte <tprevite@xxxxxxxxx>:
> 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@xxxxxxxxx>
> ---
> 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@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
