[Intel-gfx] [PATCH 7/8] drm/i915: Move max voltage and pre emphasis to intel_dp_link_training.c
Thulasimani, Sivakumar
sivakumar.thulasimani at intel.com
Sun Oct 18 23:13:16 PDT 2015
On 10/19/2015 11:29 AM, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-10-19 at 10:44 +0530, Thulasimani, Sivakumar wrote:
>> As an FYI, both of these functions need to be rewritten when we want the
>> code to be
>> compliant to DP spec.
>> We should read the pre-emphasis given by the panel and if vwing exeeds
>> max value
>> we should use the max vswing supported for that pre-emphasis but current
>> code
>> is opposite of that. you can read more detailed logic in section
>> "3.5.1.2 Link Training"
>> in DP Spec.
> Yeah, the spec is pretty clear on that.
>
>
>> by the way same is true for much of the link training logic and related
>> functions
> Do you have a list of other spec violations? I'm aware that we don't fallback to
> lower bandwidth when link training fails, but that problem can't be solved with
> the driver alone. Since we already pick the lowest bandwidth that can support
> the mode chosen by userspace, the fallback needs to allow it to choose a new
> mode.
>
> There's been some discussion on how to solve this, but I don't know if there was
> any conclusions. One of the possibilities was to send an uevent to userspace to
> tell it to to choose a smaller mode.
here is a list of changes i had to do for local branch, that i should be
upstreaming in the future
drm/i915: Fix swing & emphasis usage (this is the patch for issue i
talked about above)
drm/i915: EQ retry must be 5 times only
drm/i915: Start link training from max link rate
drm/i915: Fix Clock recovery sequence ( fixes the whole clock recovery
to be per spec)
drm/i915: Fix EQ sequence in Link training (fixes the whole EQ to be per
spec)
drm/i915: Check EQ only if CR passed (This seems to be in place in your
changes)
All these are just related to link training, i have been asked for list
of issues recently
as well, so i will probably file each of them in bugzilla/freedesktop so
someone
will track it.
regards,
Sivakumar
>
>> i don't know if such rewriting should be done after fixing those or
>> before :(.
> I have posted a new version of the refactoring patches which I believed are
> ready to go. So I'd go for getting those changes in and fix the max voltage vs.
> pre-emphasis problem on top of that.
>
> Thanks,
> Ander
>
>> regards,
>> Sivakumar.
>>
>> On 9/8/2015 6:31 PM, Ville Syrjälä wrote:
>>> On Tue, Sep 08, 2015 at 03:27:58PM +0300, Ander Conselvan de Oliveira wrote:
>>>> So link training tests can use real hardware limits.
>>> These need to be kept in sync with the _signal_levels() functions, so
>>> moving them to a separate file is a bit questionable.
>>>
>>> I suggest that we should attempt to restructure this information as
>>> some kind of tables, from which we can look up the max values as
>>> well as the hardware specific values for each setting.
>>>
>>> That of course won't solve your problem. So far I don't know what your
>>> test even does and why does it need this information, so I guess I'll
>>> need to read on...
>>>
>>>> ---
>>>> drivers/gpu/drm/i915/intel_dp.c | 99 ---------------------
>>>> ------
>>>> drivers/gpu/drm/i915/intel_dp_link_training.c | 92
>>>> +++++++++++++++++++++++++
>>>> drivers/gpu/drm/i915/intel_drv.h | 11 +--
>>>> 3 files changed, 99 insertions(+), 103 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 5778059..da87aef 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -111,13 +111,6 @@ static bool is_edp(struct intel_dp *intel_dp)
>>>> return intel_dig_port->base.type == INTEL_OUTPUT_EDP;
>>>> }
>>>>
>>>> -static struct drm_device *intel_dp_to_dev(struct intel_dp *intel_dp)
>>>> -{
>>>> - struct intel_digital_port *intel_dig_port =
>>>> dp_to_dig_port(intel_dp);
>>>> -
>>>> - return intel_dig_port->base.base.dev;
>>>> -}
>>>> -
>>>> static struct intel_dp *intel_attached_dp(struct drm_connector
>>>> *connector)
>>>> {
>>>> return enc_to_intel_dp(&intel_attached_encoder(connector)
>>>> ->base);
>>>> @@ -3054,98 +3047,6 @@ intel_dp_get_link_status(struct intel_dp *intel_dp,
>>>> uint8_t link_status[DP_LINK_
>>>> DP_LINK_STATUS_SIZE) ==
>>>> DP_LINK_STATUS_SIZE;
>>>> }
>>>>
>>>> -/* These are source-specific values. */
>>>> -uint8_t
>>>> -intel_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;
>>>> - enum port port = dp_to_dig_port(intel_dp)->port;
>>>> -
>>>> - if (IS_BROXTON(dev))
>>>> - return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>>>> - else if (INTEL_INFO(dev)->gen >= 9) {
>>>> - if (dev_priv->edp_low_vswing && port == PORT_A)
>>>> - return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>>>> - return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
>>>> - } else if (IS_VALLEYVIEW(dev))
>>>> - return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>>>> - else if (IS_GEN7(dev) && port == PORT_A)
>>>> - return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
>>>> - else if (HAS_PCH_CPT(dev) && port != PORT_A)
>>>> - return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>>>> - else
>>>> - return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
>>>> -}
>>>> -
>>>> -uint8_t
>>>> -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 (INTEL_INFO(dev)->gen >= 9) {
>>>> - switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_3;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> - default:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> - }
>>>> - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>>>> - switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_3;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
>>>> - default:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> - }
>>>> - } else if (IS_VALLEYVIEW(dev)) {
>>>> - switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_3;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
>>>> - default:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> - }
>>>> - } else if (IS_GEN7(dev) && port == PORT_A) {
>>>> - switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>> - default:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> - }
>>>> - } else {
>>>> - switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>> - case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
>>>> - default:
>>>> - return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> - }
>>>> - }
>>>> -}
>>>> -
>>>> static uint32_t vlv_signal_levels(struct intel_dp *intel_dp)
>>>> {
>>>> struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
>>>> b/drivers/gpu/drm/i915/intel_dp_link_training.c
>>>> index f33cbbb..8d27dce 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
>>>> @@ -23,6 +23,98 @@
>>>>
>>>> #include "intel_drv.h"
>>>>
>>>> +/* These are source-specific values. */
>>>> +static uint8_t
>>>> +intel_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;
>>>> + enum port port = dp_to_dig_port(intel_dp)->port;
>>>> +
>>>> + if (IS_BROXTON(dev))
>>>> + return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>>>> + else if (INTEL_INFO(dev)->gen >= 9) {
>>>> + if (dev_priv->edp_low_vswing && port == PORT_A)
>>>> + return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>>>> + return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
>>>> + } else if (IS_VALLEYVIEW(dev))
>>>> + return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>>>> + else if (IS_GEN7(dev) && port == PORT_A)
>>>> + return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
>>>> + else if (HAS_PCH_CPT(dev) && port != PORT_A)
>>>> + return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>>>> + else
>>>> + return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
>>>> +}
>>>> +
>>>> +static uint8_t
>>>> +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 (INTEL_INFO(dev)->gen >= 9) {
>>>> + switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_3;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> + default:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> + }
>>>> + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>>>> + switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_3;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
>>>> + default:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> + }
>>>> + } else if (IS_VALLEYVIEW(dev)) {
>>>> + switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_3;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
>>>> + default:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> + }
>>>> + } else if (IS_GEN7(dev) && port == PORT_A) {
>>>> + switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>> + default:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> + }
>>>> + } else {
>>>> + switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
>>>> + default:
>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> static void
>>>> intel_get_adjust_train(struct intel_dp *intel_dp,
>>>> const uint8_t link_status[DP_LINK_STATUS_SIZE])
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>>> b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 29ae4bb..671a20f 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -1216,15 +1216,18 @@ intel_dp_program_link_training_pattern(struct
>>>> intel_dp *intel_dp,
>>>> void
>>>> intel_dp_update_signal_levels(struct intel_dp *intel_dp);
>>>> void intel_dp_set_idle_link_train(struct intel_dp *intel_dp);
>>>> -uint8_t
>>>> -intel_dp_voltage_max(struct intel_dp *intel_dp);
>>>> -uint8_t
>>>> -intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t
>>>> voltage_swing);
>>>> void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
>>>> uint8_t *link_bw, uint8_t *rate_select);
>>>> bool intel_dp_source_supports_hbr2(struct drm_device *dev);
>>>> bool
>>>> intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t
>>>> link_status[DP_LINK_STATUS_SIZE]);
>>>> +static inline struct drm_device *intel_dp_to_dev(struct intel_dp
>>>> *intel_dp)
>>>> +{
>>>> + struct intel_digital_port *intel_dig_port =
>>>> dp_to_dig_port(intel_dp);
>>>> +
>>>> + return intel_dig_port->base.base.dev;
>>>> +}
>>>> +
>>>>
>>>> /* intel_dp_mst.c */
>>>> int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port,
>>>> int conn_id);
More information about the Intel-gfx
mailing list