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@xxxxxxxxx> > --- > 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
