[Intel-gfx] [PATCH 2/7] drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV
Deepak S
deepak.s at linux.intel.com
Fri May 8 06:33:42 PDT 2015
On Friday 08 May 2015 06:49 PM, Ville Syrjälä wrote:
> On Fri, May 08, 2015 at 06:24:42PM +0530, Deepak S wrote:
>>
>> On Friday 10 April 2015 08:51 PM, ville.syrjala at linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>
>>> Sometimes (exactly when is a bit unclear) DISPLAY_PHY_CONTROL appears to
>>> get corrupted. The values I've managed to read from it seem to have some
>>> pattern but vary quite a lot. The corruption doesn't seem to just happen
>>> when the register is accessed, but can also happen spontaneosly during
>>> modeset. When this happens during a modeset things go south and the
>>> display doesn't light up.
>>>
>>> I've managed to hit the problemn when toggling HDMI on port D on and
>>> off. When things get corrupted the display doesn't light up, but as soon
>>> as I manually write the correct value to the register the display comes
>>> up.
>>>
>>> First I was suspicious that we ourselves accidentally overwrite it with
>>> garbage, but didn't catch anything with the reg_rw tracepoint. Also I
>>> sprinkled check all over the modeset path to see exactly when the
>>> corruption happens, and eg. the read back value was fine just before
>>> intel_dp_set_m(), and corrupted immediately after it. I also made my
>>> check function repair the register value whenever it was wrong, and with
>>> this approach the corruption repeated several times during the modeset
>>> operation, always seeming to trigger in the same exact calls to the
>>> check function, while other calls to the function never caught anything.
>>>
>>> So far I've not seen this problem occurring when carefully avoiding all
>>> read accesses to DISPLAY_PHY_CONTROL. Not sure if that's just pure luck
>>> or an actual workaround, but we can hope it works. So let's avoid reading
>>> the register and instead track the desired value of the register in dev_priv.
>>>
>>> v2: Read out the power well state to determine initial register value
>>> v3: Use DPIO_CHx names instead of raw numbers
>> Even reading once DISPLAY_PHY_CONTROL layer is getting corrupted?
> Not always. I think it somehow depends on what other register accesses
> happen around it. So there is perhaps some magic sequence that might allow
> reading it, but I decided that it's better to be safe and never read it.
>
>> I saw similar issues on my setup. On some platform access phy is causing system behave inconsistently :(
>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_drv.h | 2 ++
>>> drivers/gpu/drm/i915/i915_reg.h | 5 ++++-
>>> drivers/gpu/drm/i915/intel_runtime_pm.c | 36 ++++++++++++++++++++++++++++-----
>>> 3 files changed, 37 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 822f259..288c3fc 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1754,6 +1754,8 @@ struct drm_i915_private {
>>>
>>> u32 fdi_rx_config;
>>>
>>> + u32 chv_phy_control;
>>> +
>>> u32 suspend_count;
>>> struct i915_suspend_saved_registers regfile;
>>> struct vlv_s0ix_state vlv_s0ix_state;
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index cfbd5a6..98588d5 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -1887,7 +1887,10 @@ enum skl_disp_power_wells {
>>> #define DPIO_PHY_STATUS (VLV_DISPLAY_BASE + 0x6240)
>>> #define DPLL_PORTD_READY_MASK (0xf)
>>> #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
>>> -#define PHY_COM_LANE_RESET_DEASSERT(phy) (1 << (phy))
>>> +#define PHY_CH_SU_PSR 0x1
>>> +#define PHY_CH_DEEP_PSR 0x7
>> PHY_CH_DEEP_PSR defined but not used in this patch?
> Just wanted to define it since it's the only other valid value, and the
> doc situation is crap. I've not played around with PSR so I'm not
> entirely sure how these would be used in practise. My gut is telling me
> SU_PSR might be used with link standby and DEEP_PSR with link off, but
> that's just a hunch at this point.
right ville, DEEP_PSR is related to link off. I will try to find the docs related to this.
> You'll see in later patches that we'll start using the override bits to
> force the power state, so I think at that point these don't even matter.
> But I suppose when we enter PSR we should drop the override bits to
> allow the hardware to manage the power states based on the PSR mode
> selected.
>
>> other than this, patch does what it says.
>> Reviewed-by: Deepak S<deepak.s at linux.intel.com>
>>
>>> +#define PHY_CH_POWER_MODE(mode, phy, ch) ((mode) << (6*(phy)+3*(ch)+2))
>>> +#define PHY_COM_LANE_RESET_DEASSERT(phy) (1 << (phy))
>>> #define DISPLAY_PHY_STATUS (VLV_DISPLAY_BASE + 0x60104)
>>> #define PHY_POWERGOOD(phy) (((phy) == DPIO_PHY0) ? (1<<31) : (1<<30))
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> index ce00e69..b73671f 100644
>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> @@ -666,8 +666,8 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
>>> if (wait_for(I915_READ(DISPLAY_PHY_STATUS) & PHY_POWERGOOD(phy), 1))
>>> DRM_ERROR("Display PHY %d is not power up\n", phy);
>>>
>>> - I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL) |
>>> - PHY_COM_LANE_RESET_DEASSERT(phy));
>>> + dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
>>> + I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>> }
>>>
>>> static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>> @@ -687,8 +687,8 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>> assert_pll_disabled(dev_priv, PIPE_C);
>>> }
>>>
>>> - I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL) &
>>> - ~PHY_COM_LANE_RESET_DEASSERT(phy));
>>> + dev_priv->chv_phy_control &= ~PHY_COM_LANE_RESET_DEASSERT(phy);
>>> + I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>
>>> vlv_set_power_well(dev_priv, power_well, false);
>>> }
>>> @@ -1401,6 +1401,30 @@ static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
>>> mutex_unlock(&power_domains->lock);
>>> }
>>>
>>> +static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>>> +{
>>> + struct i915_power_well *cmn_bc =
>>> + lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_BC);
>>> + struct i915_power_well *cmn_d =
>>> + lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_D);
>>> +
>>> + /*
>>> + * DISPLAY_PHY_CONTROL can get corrupted if read. As a
>>> + * workaround never ever read DISPLAY_PHY_CONTROL, and
>>> + * instead maintain a shadow copy ourselves. Use the actual
>>> + * power well state to reconstruct the expected initial
>>> + * value.
>>> + */
>>> + dev_priv->chv_phy_control =
>>> + PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
>>> + PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
>>> + PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
>>> + if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
>>> + dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
>>> + if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
>>> + dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
>>> +}
>>> +
>>> static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
>>> {
>>> struct i915_power_well *cmn =
>>> @@ -1443,7 +1467,9 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv)
>>>
>>> power_domains->initializing = true;
>>>
>>> - if (IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
>>> + if (IS_CHERRYVIEW(dev)) {
>>> + chv_phy_control_init(dev_priv);
>>> + } else if (IS_VALLEYVIEW(dev)) {
>>> mutex_lock(&power_domains->lock);
>>> vlv_cmnlane_wa(dev_priv);
>>> mutex_unlock(&power_domains->lock);
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list