Re: [PATCH] drm/i915/pps: Disable DPLS_GATING around pps sequence

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, May 09, 2024 at 08:59:23AM +0530, Suraj Kandpal wrote:
> Disable bit 29 of SCLKGATE_DIS register around pps sequence
> when we turn panel power on.
> 
> --v2
> -Squash two commit together [Jani]
> -Use IS_DISPLAY_VER [Jani]
> -Fix multiline comment [Jani]
> 
> --v3
> -Define register in a more appropriate place [Mitul]
> 
> Bspec: 49304
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_pps.c | 12 ++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h          |  4 ++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index 0ccbf9a85914..d774aeb1673e 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -948,6 +948,14 @@ void intel_pps_on_unlocked(struct intel_dp *intel_dp)
>  		intel_de_posting_read(dev_priv, pp_ctrl_reg);
>  	}
>  
> +	/*
> +	 * WA: 16023567976
> +	 * Disable DPLS gating around power sequence.
> +	 */
> +	if (IS_DISPLAY_VER(dev_priv, 12, 14))

The issue has supposedly existed since at least BXT.
It was documented as w/a #1124 there:
https://patchwork.freedesktop.org/series/70655/

The original w/a called for keeping the clock gating
disabled between the PP_ON_DELAYS and PP_CONTROL
writes, which would have been annoying to implement
so I went with the extra delay instead. But if the new
approach of just toggle the clock gating around the
PP_CONTROL write works then that is definitely better.

Sadly I wasn't able to reproduce this issue locally. Gave it
a decent try on GLK, TGL, and ADL, but no joy. So can't be
sure this actually works.

I suppose technically it doesn't matter for us since we always use
the VDD override anyway, but no harm in having the w/a implemented
anyway in case we ever change that.

> +		intel_de_rmw(dev_priv, SCLKGATE_DIS,
> +			     0, DPLS_GATING_DISABLE);

IIRC on BXT/GLK we need to poke at some north clock gating register.

And on BXT/GLK, and ICP+ we can have two power sequencers so we
probably want to poke the bits for both of them.

The other issue is that we are poking at these register from multiple
places, so we probably need a lock to protect it. I'm think we could
have just a single chicken_lock or something for these kinds of use
cases.

> +
>  	pp |= PANEL_POWER_ON;
>  	if (!IS_IRONLAKE(dev_priv))
>  		pp |= PANEL_POWER_RESET;
> @@ -958,6 +966,10 @@ void intel_pps_on_unlocked(struct intel_dp *intel_dp)
>  	wait_panel_on(intel_dp);
>  	intel_dp->pps.last_power_on = jiffies;
>  
> +	if (IS_DISPLAY_VER(dev_priv, 12, 14))
> +		intel_de_rmw(dev_priv, SCLKGATE_DIS,
> +			     DPLS_GATING_DISABLE, 0);
> +
>  	if (IS_IRONLAKE(dev_priv)) {
>  		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
>  		intel_de_write(dev_priv, pp_ctrl_reg, pp);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5670eee4a498..4cc82360298b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5151,6 +5151,10 @@ enum skl_power_gate {
>  
>  #define _MMIO_PLANE_GAMC(plane, i, a, b)  _MMIO(_PIPE(plane, a, b) + (i) * 4)
>  
> +/* SCLKGATE_DIS */
> +#define SCLKGATE_DIS			_MMIO(0xc2020)

We already have that register.

> +#define  DPLS_GATING_DISABLE		REG_BIT(29)
> +
>  /* Plane CSC Registers */
>  #define _PLANE_CSC_RY_GY_1_A	0x70210
>  #define _PLANE_CSC_RY_GY_2_A	0x70310
> -- 
> 2.43.2

-- 
Ville Syrjälä
Intel