Re: [PATCH 2/7] drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV

[Deepak S <deepak.s@linux.intel.com>]

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@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@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@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@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx