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

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

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?
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?

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