On Tue, Feb 02, 2021 at 08:59:20AM +0000, Surendrakumar Upadhyay, TejaskumarX wrote:
>
>
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Sent: 02 February 2021 12:42
> > To: Surendrakumar Upadhyay, TejaskumarX
> > <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx>
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Pandey, Hariom
> > <hariom.pandey@xxxxxxxxx>
> > Subject: Re: [PATCH] drm/i915/gen9bc: Handle TGP PCH during
> > suspend/resume
> >
> > On Tue, Feb 02, 2021 at 08:31:48AM +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 02, 2021 at 05:52:28AM +0000, Surendrakumar Upadhyay,
> > TejaskumarX wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > > Sent: 28 January 2021 04:46
> > > > > To: Surendrakumar Upadhyay, TejaskumarX
> > > > > <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx>
> > > > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Pandey, Hariom
> > > > > <hariom.pandey@xxxxxxxxx>; Roper, Matthew D
> > > > > <matthew.d.roper@xxxxxxxxx>
> > > > > Subject: Re: [PATCH] drm/i915/gen9bc: Handle TGP PCH
> > > > > during suspend/resume
> > > > >
> > > > > On Wed, Jan 27, 2021 at 03:38:30PM +0530, Tejas Upadhyay wrote:
> > > > > > For Legacy S3 suspend/resume GEN9 BC needs to enable and setup
> > > > > > TGP PCH.
> > > > > >
> > > > > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > > > > Signed-off-by: Tejas Upadhyay
> > > > > > <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_irq.c | 36
> > > > > > ++++++++++++++++++++++++---------
> > > > > > 1 file changed, 27 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > b/drivers/gpu/drm/i915/i915_irq.c index
> > > > > > a31980f69120..6dcefc3e24ac
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > @@ -3026,8 +3026,20 @@ static void gen8_irq_reset(struct drm_i915_private *dev_priv)
> > > > > > GEN3_IRQ_RESET(uncore, GEN8_DE_MISC_);
> > > > > > GEN3_IRQ_RESET(uncore, GEN8_PCU_);
> > > > > >
> > > > > > - if (HAS_PCH_SPLIT(dev_priv))
> > > > > > + if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> > > > > > + GEN3_IRQ_RESET(uncore, SDE);
> > > > > > + else if (HAS_PCH_SPLIT(dev_priv))
> > > > > > ibx_irq_reset(dev_priv);
> > > > > > +
> > > > > > + /* Wa_14010685332:cnp/cmp,tgp,adp */
> > > > > > + if (INTEL_PCH_TYPE(dev_priv) == PCH_CNP ||
> > > > > > + (INTEL_PCH_TYPE(dev_priv) >= PCH_TGP &&
> > > > > > + INTEL_PCH_TYPE(dev_priv) < PCH_DG1)) {
> > > > > > + intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > > > > > + SBCLK_RUN_REFCLK_DIS, SBCLK_RUN_REFCLK_DIS);
> > > > > > + intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > > > > > + SBCLK_RUN_REFCLK_DIS, 0);
> > > > > > + }
> > > > >
> > > > > Time to refactor instead of copypasta.
> > > > Do you expect below? :
> > > >
> > > > If ((INTEL_PCH_TYPE(dev_priv) == PCH_TGP) {
> > > > intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > > > SBCLK_RUN_REFCLK_DIS,
> > > > SBCLK_RUN_REFCLK_DIS);
> > > > intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > > > SBCLK_RUN_REFCLK_DIS, 0);
> > > > }
> > >
> > > I expect a new function instead of copy pasting this whole thing into
> > > multiple places.
> > >
> > > That said even the current code doesn't make any sense to me.
> > > Take for instance this part:
> > > if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> > > GEN3_IRQ_RESET(uncore, SDE); What is that PCH type
> > > check doing there? What weird PCH type are we supposed to have that
> > > doesn't need this?
> > >
> > > Also the Wa_14010685332 part looks a bit odd. Is it correct that icp
> > > doesn't need that, but cnp and tgp both do somehow? Can we even have
> > > cnp on icl+?
> >
> > Hmm. Looking at it a bit more, that w/a seems to have something to do with
> > suspend/resume, so seems rather misplaced in irq_reset(). Should probably
> > just move the whole thing into a more appropriate place.
> >
> GEN11+ needs these checks in irq_reset(). Please check irq_reset for
> GEN11. Now that customer like dell are expecting TGP PCH with gen9bc
> platforms, I have done similar PCH checking in irq_reset() for gen9bc.
> You mean these checks are at wrong place for GEN11 irq_reset() as
> well? Or you want one function doing these checks and calling it
> everywhere!
BSpec says about this WA for both ICL and TGL:
"""
Display driver should set and clear register offset 0xC2000 bit #7 as
last step in programming south display registers in preparation for
entering S0ix state, or set 0xC2000 bit #7 on S0ix entry and clear it on
S0ix exit.
"""
This means to me the WA is only relevant for S0ix and we can implement
it by setting/clearing 0xC2000 bit #7 right before entering/right after
exiting S0ix. This is done atm on PCH_ICP..PCH_MCC in
intel_display_power_suspend_late()/intel_display_power_resume_early(),
so I'd move the WA for all platforms there.
I assume the current code in irq_reset() was the first alternative to
implement the WA, but it wasn't enough. Not sure why, maybe there is a
south display register access after irq_reset() during suspend. Adding
Anshuman for an idea on that.
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
