On Sun, 2015-10-25 at 10:54 +0530, Thulasimani, Sivakumar wrote:
> This is a good simplification but for a wrong implementation :). we know
> that our current
> implementation is wrong and needs to be done the opposite way to make
> our code compliant with spec. i assume that it will then be a question
> of when
> this will be removed rather than if. In such a scenario is this and
> following
> patches/optimizations needed "at present" ?
How much of this needs to be removed in order to make the implementation spec
compliant? I was under the impression it would be sufficient to change the
signature of intel_dp_voltage_max() and intel_dp_pre_emphasis_max() and change
the way we store the maximums so we can query the max voltage for a given pre
-emphasis level. But perhaps this is more complicated than I'm anticipating. It
would be good if you could share your patch that fixes this issue to clarify
what's necessary.
Thanks,
Ander
> regards,
> Sivakumar
> On 10/23/2015 3:31 PM, Ander Conselvan de Oliveira wrote:
> > In order to clarify which platforms support which combination of voltage
> > swing and pre emphasis level, introduce struct intel_dp_signal_levels.
> > With the new struct, the if ladder to determine the values used is put
> > in one place, intel_dp_init_signal_levels().
> >
> > This also wires gens 4, 5 and non-eDP ports on gen 6 and 7 to use the
> > new struct.
> >
> > v2: Rebase
> > Signed-off-by: Ander Conselvan de Oliveira <
> > ander.conselvan.de.oliveira@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 2 +
> > drivers/gpu/drm/i915/intel_dp_signal_levels.c | 103 ++++++++++++++++++++++
> > ----
> > drivers/gpu/drm/i915/intel_drv.h | 11 +++
> > 3 files changed, 101 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 1236791..e640b59 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5220,6 +5220,8 @@ intel_dp_init_connector(struct intel_digital_port
> > *intel_dig_port,
> > if (HAS_DDI(dev))
> > intel_dp->prepare_link_retrain =
> > intel_ddi_prepare_link_retrain;
> >
> > + intel_dp_init_signal_levels(intel_dp);
> > +
> > /* Preserve the current hw state. */
> > intel_dp->DP = I915_READ(intel_dp->output_reg);
> > intel_dp->attached_connector = intel_connector;
> > diff --git a/drivers/gpu/drm/i915/intel_dp_signal_levels.c
> > b/drivers/gpu/drm/i915/intel_dp_signal_levels.c
> > index e516dd2..365bdc4 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_signal_levels.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_signal_levels.c
> > @@ -24,8 +24,8 @@
> > #include "intel_drv.h"
> >
> > /* These are source-specific values. */
> > -uint8_t
> > -intel_dp_voltage_max(struct intel_dp *intel_dp)
> > +static uint8_t
> > +_dp_voltage_max(struct intel_dp *intel_dp)
> > {
> > struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -47,8 +47,8 @@ intel_dp_voltage_max(struct intel_dp *intel_dp)
> > return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
> > }
> >
> > -uint8_t
> > -intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing)
> > +static uint8_t
> > +_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;
> > @@ -389,10 +389,10 @@ static uint32_t chv_signal_levels(struct intel_dp
> > *intel_dp)
> > return 0;
> > }
> >
> > -static uint32_t
> > -gen4_signal_levels(uint8_t train_set)
> > +static void
> > +gen4_set_signal_levels(struct intel_dp *intel_dp, uint8_t train_set)
> > {
> > - uint32_t signal_levels = 0;
> > + uint32_t signal_levels = 0;
> >
> > switch (train_set & DP_TRAIN_VOLTAGE_SWING_MASK) {
> > case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
> > @@ -424,9 +424,38 @@ gen4_signal_levels(uint8_t train_set)
> > signal_levels |= DP_PRE_EMPHASIS_9_5;
> > break;
> > }
> > - return signal_levels;
> > +
> > + DRM_DEBUG_KMS("Using signal levels %08x\n", signal_levels);
> > +
> > + intel_dp->DP &= ~(DP_VOLTAGE_MASK | DP_PRE_EMPHASIS_MASK);
> > + intel_dp->DP |= signal_levels;
> > }
> >
> > +static const struct signal_levels gen4_signal_levels = {
> > + .max_voltage = DP_TRAIN_VOLTAGE_SWING_LEVEL_2,
> > + .max_pre_emph = {
> > + DP_TRAIN_PRE_EMPH_LEVEL_2,
> > + DP_TRAIN_PRE_EMPH_LEVEL_2,
> > + DP_TRAIN_PRE_EMPH_LEVEL_1,
> > + DP_TRAIN_PRE_EMPH_LEVEL_0,
> > + },
> > +
> > + .set = gen4_set_signal_levels,
> > +};
> > +
> > +/* Not for eDP */
> > +static const struct signal_levels snb_signal_levels = {
> > + .max_voltage = DP_TRAIN_VOLTAGE_SWING_LEVEL_3,
> > + .max_pre_emph = {
> > + DP_TRAIN_PRE_EMPH_LEVEL_2,
> > + DP_TRAIN_PRE_EMPH_LEVEL_2,
> > + DP_TRAIN_PRE_EMPH_LEVEL_1,
> > + DP_TRAIN_PRE_EMPH_LEVEL_0,
> > + },
> > +
> > + .set = gen4_set_signal_levels,
> > +};
> > +
> > /* Gen6's DP voltage swing and pre-emphasis control */
> > static uint32_t
> > gen6_edp_signal_levels(uint8_t train_set)
> > @@ -486,13 +515,12 @@ gen7_edp_signal_levels(uint8_t train_set)
> > }
> > }
> >
> > -void
> > -intel_dp_set_signal_levels(struct intel_dp *intel_dp)
> > +static void
> > +_update_signal_levels(struct intel_dp *intel_dp)
> > {
> > struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > enum port port = intel_dig_port->port;
> > struct drm_device *dev = intel_dig_port->base.base.dev;
> > - struct drm_i915_private *dev_priv = to_i915(dev);
> > uint32_t signal_levels, mask = 0;
> > uint8_t train_set = intel_dp->train_set[0];
> >
> > @@ -514,21 +542,66 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp)
> > signal_levels = gen6_edp_signal_levels(train_set);
> > mask = EDP_LINK_TRAIN_VOL_EMP_MASK_SNB;
> > } else {
> > - signal_levels = gen4_signal_levels(train_set);
> > - mask = DP_VOLTAGE_MASK | DP_PRE_EMPHASIS_MASK;
> > + WARN(1, "Should be calling intel_dp->signal_levels->set
> > instead.");
> > + return;
> > }
> >
> > if (mask)
> > DRM_DEBUG_KMS("Using signal levels %08x\n",
> > signal_levels);
> >
> > + intel_dp->DP = (intel_dp->DP & ~mask) | signal_levels;
> > +}
> > +
> > +uint8_t
> > +intel_dp_voltage_max(struct intel_dp *intel_dp)
> > +{
> > + if (intel_dp->signal_levels)
> > + return intel_dp->signal_levels->max_voltage;
> > + else
> > + return _dp_voltage_max(intel_dp);
> > +}
> > +
> > +uint8_t
> > +intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing)
> > +{
> > + if (intel_dp->signal_levels)
> > + return intel_dp->signal_levels
> > ->max_pre_emph[voltage_swing];
> > + else
> > + return _dp_pre_emphasis_max(intel_dp, voltage_swing);
> > +}
> > +
> > +void
> > +intel_dp_set_signal_levels(struct intel_dp *intel_dp)
> > +{
> > + struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > + uint8_t train_set = intel_dp->train_set[0];
> > +
> > + if (intel_dp->signal_levels)
> > + intel_dp->signal_levels->set(intel_dp, train_set);
> > + else
> > + _update_signal_levels(intel_dp);
> > +
> > DRM_DEBUG_KMS("Using vswing level %d\n",
> > train_set & DP_TRAIN_VOLTAGE_SWING_MASK);
> > DRM_DEBUG_KMS("Using pre-emphasis level %d\n",
> > (train_set & DP_TRAIN_PRE_EMPHASIS_MASK) >>
> > DP_TRAIN_PRE_EMPHASIS_SHIFT);
> >
> > - intel_dp->DP = (intel_dp->DP & ~mask) | signal_levels;
> > -
> > I915_WRITE(intel_dp->output_reg, intel_dp->DP);
> > POSTING_READ(intel_dp->output_reg);
> > }
> > +
> > +void
> > +intel_dp_init_signal_levels(struct intel_dp *intel_dp)
> > +{
> > + struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > + struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > + enum port port = intel_dig_port->port;
> > +
> > + if (port != PORT_A && (IS_IVYBRIDGE(dev) || IS_GEN6(dev)))
> > + intel_dp->signal_levels = &snb_signal_levels;
> > +
> > + if (INTEL_INFO(dev)->gen <= 5)
> > + intel_dp->signal_levels = &gen4_signal_levels;
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index d758e94..e00ce6c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -726,6 +726,14 @@ struct sink_crc {
> > int last_count;
> > };
> >
> > +struct intel_dp;
> > +struct signal_levels {
> > + uint8_t max_voltage;
> > + uint8_t max_pre_emph[4];
> > +
> > + void (*set)(struct intel_dp *intel_dp, uint8_t train_set);
> > +};
> > +
> > struct intel_dp {
> > uint32_t output_reg;
> > uint32_t aux_ch_ctl_reg;
> > @@ -744,6 +752,7 @@ struct intel_dp {
> > int sink_rates[DP_MAX_SUPPORTED_RATES];
> > struct sink_crc sink_crc;
> > struct drm_dp_aux aux;
> > + const struct signal_levels *signal_levels;
> > uint8_t train_set[4];
> > int panel_power_up_delay;
> > int panel_power_down_delay;
> > @@ -1263,6 +1272,8 @@ bool intel_dp_source_supports_hbr2(struct intel_dp
> > *intel_dp);
> > bool
> > intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t
> > link_status[DP_LINK_STATUS_SIZE]);
> >
> > +void intel_dp_init_signal_levels(struct intel_dp *intel_dp);
> > +
> > /* intel_dp_mst.c */
> > int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port,
> > int conn_id);
> > void intel_dp_mst_encoder_cleanup(struct intel_digital_port
> > *intel_dig_port);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
