| Message ID | 20210127100830.162292-1-tejaskumarx.surendrakumar.upadhyay@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gen9bc: Handle TGP PCH during suspend/resume | expand |
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@intel.com> > Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com> > --- > 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. > } > > static void gen11_display_irq_reset(struct drm_i915_private *dev_priv) > @@ -3442,6 +3454,9 @@ static void spt_hpd_irq_setup(struct drm_i915_private *dev_priv) > ibx_display_interrupt_update(dev_priv, hotplug_irqs, enabled_irqs); > > spt_hpd_detection_setup(dev_priv); > + > + if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP) > + icp_hpd_irq_setup(dev_priv); > } > > static u32 ilk_hotplug_enables(struct drm_i915_private *i915, > @@ -3729,9 +3744,19 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv) > } > } > > +static void icp_irq_postinstall(struct drm_i915_private *dev_priv) > +{ > + struct intel_uncore *uncore = &dev_priv->uncore; > + u32 mask = SDE_GMBUS_ICP; > + > + GEN3_IRQ_INIT(uncore, SDE, ~mask, 0xffffffff); > +} > + > static void gen8_irq_postinstall(struct drm_i915_private *dev_priv) > { > - if (HAS_PCH_SPLIT(dev_priv)) > + if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP) > + icp_irq_postinstall(dev_priv); > + else if (HAS_PCH_SPLIT(dev_priv)) > ibx_irq_postinstall(dev_priv); > > gen8_gt_irq_postinstall(&dev_priv->gt); > @@ -3740,13 +3765,6 @@ static void gen8_irq_postinstall(struct drm_i915_private *dev_priv) > gen8_master_intr_enable(dev_priv->uncore.regs); > } > > -static void icp_irq_postinstall(struct drm_i915_private *dev_priv) > -{ > - struct intel_uncore *uncore = &dev_priv->uncore; > - u32 mask = SDE_GMBUS_ICP; > - > - GEN3_IRQ_INIT(uncore, SDE, ~mask, 0xffffffff); > -} > > static void gen11_irq_postinstall(struct drm_i915_private *dev_priv) > { > -- > 2.30.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: 28 January 2021 04:46 > To: Surendrakumar Upadhyay, TejaskumarX > <tejaskumarx.surendrakumar.upadhyay@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Pandey, Hariom > <hariom.pandey@intel.com>; Roper, Matthew D > <matthew.d.roper@intel.com> > Subject: Re: [Intel-gfx] [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@intel.com> > > Signed-off-by: Tejas Upadhyay > > <tejaskumarx.surendrakumar.upadhyay@intel.com> > > --- > > 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); } > > > } > > > > static void gen11_display_irq_reset(struct drm_i915_private > > *dev_priv) @@ -3442,6 +3454,9 @@ static void spt_hpd_irq_setup(struct > drm_i915_private *dev_priv) > > ibx_display_interrupt_update(dev_priv, hotplug_irqs, enabled_irqs); > > > > spt_hpd_detection_setup(dev_priv); > > + > > + if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP) > > + icp_hpd_irq_setup(dev_priv); > > } > > > > static u32 ilk_hotplug_enables(struct drm_i915_private *i915, @@ > > -3729,9 +3744,19 @@ static void gen8_de_irq_postinstall(struct > drm_i915_private *dev_priv) > > } > > } > > > > +static void icp_irq_postinstall(struct drm_i915_private *dev_priv) { > > + struct intel_uncore *uncore = &dev_priv->uncore; > > + u32 mask = SDE_GMBUS_ICP; > > + > > + GEN3_IRQ_INIT(uncore, SDE, ~mask, 0xffffffff); } > > + > > static void gen8_irq_postinstall(struct drm_i915_private *dev_priv) > > { > > - if (HAS_PCH_SPLIT(dev_priv)) > > + if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP) > > + icp_irq_postinstall(dev_priv); > > + else if (HAS_PCH_SPLIT(dev_priv)) > > ibx_irq_postinstall(dev_priv); > > > > gen8_gt_irq_postinstall(&dev_priv->gt); > > @@ -3740,13 +3765,6 @@ static void gen8_irq_postinstall(struct > drm_i915_private *dev_priv) > > gen8_master_intr_enable(dev_priv->uncore.regs); > > } > > > > -static void icp_irq_postinstall(struct drm_i915_private *dev_priv) -{ > > - struct intel_uncore *uncore = &dev_priv->uncore; > > - u32 mask = SDE_GMBUS_ICP; > > - > > - GEN3_IRQ_INIT(uncore, SDE, ~mask, 0xffffffff); > > -} > > > > static void gen11_irq_postinstall(struct drm_i915_private *dev_priv) > > { > > -- > > 2.30.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
On Tue, Feb 02, 2021 at 05:52:28AM +0000, Surendrakumar Upadhyay, TejaskumarX wrote: > > > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: 28 January 2021 04:46 > > To: Surendrakumar Upadhyay, TejaskumarX > > <tejaskumarx.surendrakumar.upadhyay@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; Pandey, Hariom > > <hariom.pandey@intel.com>; Roper, Matthew D > > <matthew.d.roper@intel.com> > > Subject: Re: [Intel-gfx] [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@intel.com> > > > Signed-off-by: Tejas Upadhyay > > > <tejaskumarx.surendrakumar.upadhyay@intel.com> > > > --- > > > 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+?
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@linux.intel.com> > > > Sent: 28 January 2021 04:46 > > > To: Surendrakumar Upadhyay, TejaskumarX > > > <tejaskumarx.surendrakumar.upadhyay@intel.com> > > > Cc: intel-gfx@lists.freedesktop.org; Pandey, Hariom > > > <hariom.pandey@intel.com>; Roper, Matthew D > > > <matthew.d.roper@intel.com> > > > Subject: Re: [Intel-gfx] [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@intel.com> > > > > Signed-off-by: Tejas Upadhyay > > > > <tejaskumarx.surendrakumar.upadhyay@intel.com> > > > > --- > > > > 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.
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: 02 February 2021 12:42 > To: Surendrakumar Upadhyay, TejaskumarX > <tejaskumarx.surendrakumar.upadhyay@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Pandey, Hariom > <hariom.pandey@intel.com> > Subject: Re: [Intel-gfx] [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@linux.intel.com> > > > > Sent: 28 January 2021 04:46 > > > > To: Surendrakumar Upadhyay, TejaskumarX > > > > <tejaskumarx.surendrakumar.upadhyay@intel.com> > > > > Cc: intel-gfx@lists.freedesktop.org; Pandey, Hariom > > > > <hariom.pandey@intel.com>; Roper, Matthew D > > > > <matthew.d.roper@intel.com> > > > > Subject: Re: [Intel-gfx] [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@intel.com> > > > > > Signed-off-by: Tejas Upadhyay > > > > > <tejaskumarx.surendrakumar.upadhyay@intel.com> > > > > > --- > > > > > 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! > -- > Ville Syrjälä > Intel
On Tue, Feb 02, 2021 at 08:59:20AM +0000, Surendrakumar Upadhyay, TejaskumarX wrote: > > > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: 02 February 2021 12:42 > > To: Surendrakumar Upadhyay, TejaskumarX > > <tejaskumarx.surendrakumar.upadhyay@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; Pandey, Hariom > > <hariom.pandey@intel.com> > > Subject: Re: [Intel-gfx] [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@linux.intel.com> > > > > > Sent: 28 January 2021 04:46 > > > > > To: Surendrakumar Upadhyay, TejaskumarX > > > > > <tejaskumarx.surendrakumar.upadhyay@intel.com> > > > > > Cc: intel-gfx@lists.freedesktop.org; Pandey, Hariom > > > > > <hariom.pandey@intel.com>; Roper, Matthew D > > > > > <matthew.d.roper@intel.com> > > > > > Subject: Re: [Intel-gfx] [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@intel.com> > > > > > > Signed-off-by: Tejas Upadhyay > > > > > > <tejaskumarx.surendrakumar.upadhyay@intel.com> > > > > > > --- > > > > > > 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
..snip.. (comments down below) On Tue, 2021-02-02 at 18:14 +0200, Imre Deak wrote: > > 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. > Poking Anshuman here, is there any update on this? I'm looking to push these patches forward as some of Red Hat's hardware partners are very eager for this to get upstream asap as we're running out of time from our end. If you can answer this, I can handle respinning this patch as needed. > --Imre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
> -----Original Message----- > From: Lyude Paul <lyude@redhat.com> > Sent: Wednesday, February 10, 2021 1:34 AM > To: Deak, Imre <imre.deak@intel.com>; Surendrakumar Upadhyay, TejaskumarX > <tejaskumarx.surendrakumar.upadhyay@intel.com>; Gupta, Anshuman > <anshuman.gupta@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Pandey, Hariom > <hariom.pandey@intel.com> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gen9bc: Handle TGP PCH during > suspend/resume > > ..snip.. (comments down below) > > On Tue, 2021-02-02 at 18:14 +0200, Imre Deak wrote: > > > > 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. > > > > Poking Anshuman here, is there any update on this? I'm looking to push these > patches forward as some of Red Hat's hardware partners are very eager for this > to get upstream asap as we're running out of time from our end. If you can > answer this, I can handle respinning this patch as needed. My sincere apology, I had missed this thread. We have decided to keep the alternative WA i.e setting/clearing 0xC2000 bit #7 before entering after exiting s0ix to fix the deeper s0ix power consumption issues on ICL_PCH families platforms. This alternative WA was added to B.Spec on our request. But on TGL_PCH first alternative WA logic i.e in irq_reset() was working to attain deeper s0ix residencies so we haven't changed that. Thanks, Anshuman Gupta > > > --Imre > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Sincerely, > Lyude Paul (she/her) > Software Engineer at Red Hat > > Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've > asked me a question, are waiting for a review/merge on a patch, etc. and I > haven't responded in a while, please feel free to send me another email to check > on my status. I don't bite!
On Wed, Feb 10, 2021 at 06:33:20AM +0200, Gupta, Anshuman wrote: > > On Tue, 2021-02-02 at 18:14 +0200, Imre Deak wrote: > > > > > > 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. > > > > > > > Poking Anshuman here, is there any update on this? I'm looking to push these > > patches forward as some of Red Hat's hardware partners are very eager for this > > to get upstream asap as we're running out of time from our end. If you can > > answer this, I can handle respinning this patch as needed. > > My sincere apology, I had missed this thread. We have decided to keep > the alternative WA i.e setting/clearing 0xC2000 bit #7 before > entering after exiting s0ix to fix the deeper s0ix power consumption > issues on ICL_PCH families platforms. This alternative WA was added > to B.Spec on our request. But on TGL_PCH first alternative WA logic > i.e in irq_reset() was working to attain deeper s0ix residencies so > we haven't changed that. Ok, thanks for the explanation. For now I'd just ask to add a TODO: in this patch to check if the WA can be applied in the s0ix suspend/resume handlers as on earlier platforms and whether the WA is also needed for runtime s/r. --Imre
On Wed, 2021-02-10 at 04:33 +0000, Gupta, Anshuman wrote: > My sincere apology, I had missed this thread. No problem! Happens all the time to me :) > We have decided to keep the alternative WA i.e setting/clearing 0xC2000 bit > #7 > before entering after exiting s0ix to fix the deeper s0ix power consumption > issues on ICL_PCH > families platforms. This alternative WA was added to B.Spec on our request. > But on TGL_PCH first alternative WA logic i.e in irq_reset() was working to > attain deeper s0ix residencies so > we haven't changed that. Awesome, thanks for the info!
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); + } } static void gen11_display_irq_reset(struct drm_i915_private *dev_priv) @@ -3442,6 +3454,9 @@ static void spt_hpd_irq_setup(struct drm_i915_private *dev_priv) ibx_display_interrupt_update(dev_priv, hotplug_irqs, enabled_irqs); spt_hpd_detection_setup(dev_priv); + + if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP) + icp_hpd_irq_setup(dev_priv); } static u32 ilk_hotplug_enables(struct drm_i915_private *i915, @@ -3729,9 +3744,19 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv) } } +static void icp_irq_postinstall(struct drm_i915_private *dev_priv) +{ + struct intel_uncore *uncore = &dev_priv->uncore; + u32 mask = SDE_GMBUS_ICP; + + GEN3_IRQ_INIT(uncore, SDE, ~mask, 0xffffffff); +} + static void gen8_irq_postinstall(struct drm_i915_private *dev_priv) { - if (HAS_PCH_SPLIT(dev_priv)) + if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP) + icp_irq_postinstall(dev_priv); + else if (HAS_PCH_SPLIT(dev_priv)) ibx_irq_postinstall(dev_priv); gen8_gt_irq_postinstall(&dev_priv->gt); @@ -3740,13 +3765,6 @@ static void gen8_irq_postinstall(struct drm_i915_private *dev_priv) gen8_master_intr_enable(dev_priv->uncore.regs); } -static void icp_irq_postinstall(struct drm_i915_private *dev_priv) -{ - struct intel_uncore *uncore = &dev_priv->uncore; - u32 mask = SDE_GMBUS_ICP; - - GEN3_IRQ_INIT(uncore, SDE, ~mask, 0xffffffff); -} static void gen11_irq_postinstall(struct drm_i915_private *dev_priv) {
For Legacy S3 suspend/resume GEN9 BC needs to enable and setup TGP PCH. Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 36 ++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-)