| Message ID | 20230208190312.1611335-1-ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "drm/i915/hwmon: Enable PL1 power limit" | expand |
On Wed, Feb 08, 2023 at 11:03:12AM -0800, Ashutosh Dixit wrote: > This reverts commit 0349c41b05968befaffa5fbb7e73d0ee6004f610. > > 0349c41b0596 ("drm/i915/hwmon: Enable PL1 power limit") is incorrect and > caused a major regression on ATSM. The change enabled the PL1 power limit > but FW sets the default value of the PL1 limit to 0 which implies HW now > works at minimum power and therefore the lowest effective frequency. This > means all workloads now run slower resulting in even GuC FW load operations > timing out, rendering ATSM unusable. > > A different solution to the original issue of the PL1 limit being disabled > on ATSM is needed but till that is developed, revert 0349c41b0596. > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 pushed to drm-intel-next and removed from drm-intel-fixes. Thanks for the quick reaction. > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/i915_hwmon.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 4683a5b96eff1..1225bc432f0d5 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -687,11 +687,6 @@ hwm_get_preregistration_info(struct drm_i915_private *i915) > for_each_gt(gt, i915, i) > hwm_energy(&hwmon->ddat_gt[i], &energy); > } > - > - /* Enable PL1 power limit */ > - if (i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > - hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, > - PKG_PWR_LIM_1_EN, PKG_PWR_LIM_1_EN); > } > > void i915_hwmon_register(struct drm_i915_private *i915) > -- > 2.38.0 >
On Wed, 08 Feb 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > On Wed, Feb 08, 2023 at 11:03:12AM -0800, Ashutosh Dixit wrote: >> This reverts commit 0349c41b05968befaffa5fbb7e73d0ee6004f610. >> >> 0349c41b0596 ("drm/i915/hwmon: Enable PL1 power limit") is incorrect and >> caused a major regression on ATSM. The change enabled the PL1 power limit >> but FW sets the default value of the PL1 limit to 0 which implies HW now >> works at minimum power and therefore the lowest effective frequency. This >> means all workloads now run slower resulting in even GuC FW load operations >> timing out, rendering ATSM unusable. >> >> A different solution to the original issue of the PL1 limit being disabled >> on ATSM is needed but till that is developed, revert 0349c41b0596. >> >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > > pushed to drm-intel-next and removed from drm-intel-fixes. > > Thanks for the quick reaction. Please always add Fixes: tags also to reverts. I suppose we should fix dim to also detect reverts, but I ended up cherry-picking and pushing the original commit out to drm-intel-next-fixes before realizing it's been reverted. BR, Jani. > >> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> >> --- >> drivers/gpu/drm/i915/i915_hwmon.c | 5 ----- >> 1 file changed, 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c >> index 4683a5b96eff1..1225bc432f0d5 100644 >> --- a/drivers/gpu/drm/i915/i915_hwmon.c >> +++ b/drivers/gpu/drm/i915/i915_hwmon.c >> @@ -687,11 +687,6 @@ hwm_get_preregistration_info(struct drm_i915_private *i915) >> for_each_gt(gt, i915, i) >> hwm_energy(&hwmon->ddat_gt[i], &energy); >> } >> - >> - /* Enable PL1 power limit */ >> - if (i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) >> - hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, >> - PKG_PWR_LIM_1_EN, PKG_PWR_LIM_1_EN); >> } >> >> void i915_hwmon_register(struct drm_i915_private *i915) >> -- >> 2.38.0 >>
On Wed, 15 Feb 2023 07:37:30 -0800, Jani Nikula wrote: > > On Wed, 08 Feb 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > On Wed, Feb 08, 2023 at 11:03:12AM -0800, Ashutosh Dixit wrote: > >> This reverts commit 0349c41b05968befaffa5fbb7e73d0ee6004f610. > >> > >> 0349c41b0596 ("drm/i915/hwmon: Enable PL1 power limit") is incorrect and > >> caused a major regression on ATSM. The change enabled the PL1 power limit > >> but FW sets the default value of the PL1 limit to 0 which implies HW now > >> works at minimum power and therefore the lowest effective frequency. This > >> means all workloads now run slower resulting in even GuC FW load operations > >> timing out, rendering ATSM unusable. > >> > >> A different solution to the original issue of the PL1 limit being disabled > >> on ATSM is needed but till that is developed, revert 0349c41b0596. > >> > >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > > > > pushed to drm-intel-next and removed from drm-intel-fixes. > > > > Thanks for the quick reaction. > > Please always add Fixes: tags also to reverts. > > I suppose we should fix dim to also detect reverts, but I ended up > cherry-picking and pushing the original commit out to > drm-intel-next-fixes before realizing it's been reverted. Oops, sorry!
On Wed, Feb 15, 2023 at 08:24:51AM -0800, Dixit, Ashutosh wrote: > On Wed, 15 Feb 2023 07:37:30 -0800, Jani Nikula wrote: > > > > On Wed, 08 Feb 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > > On Wed, Feb 08, 2023 at 11:03:12AM -0800, Ashutosh Dixit wrote: > > >> This reverts commit 0349c41b05968befaffa5fbb7e73d0ee6004f610. > > >> > > >> 0349c41b0596 ("drm/i915/hwmon: Enable PL1 power limit") is incorrect and > > >> caused a major regression on ATSM. The change enabled the PL1 power limit > > >> but FW sets the default value of the PL1 limit to 0 which implies HW now > > >> works at minimum power and therefore the lowest effective frequency. This > > >> means all workloads now run slower resulting in even GuC FW load operations > > >> timing out, rendering ATSM unusable. > > >> > > >> A different solution to the original issue of the PL1 limit being disabled > > >> on ATSM is needed but till that is developed, revert 0349c41b0596. > > >> > > >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > > > > > > pushed to drm-intel-next and removed from drm-intel-fixes. > > > > > > Thanks for the quick reaction. > > > > Please always add Fixes: tags also to reverts. > > > > I suppose we should fix dim to also detect reverts, but I ended up > > cherry-picking and pushing the original commit out to > > drm-intel-next-fixes before realizing it's been reverted. > > Oops, sorry! That's my mistake. I should had thought about this when pushing and removing from the fixes. I just realized yet, when this patch showed up in my -fixes cherry-pick again, but without the revert. I'm sorry.
Jani/Rodrigo, Original Subject: Re: [Intel-gfx] [PATCH] Revert "drm/i915/hwmon: Enable PL1 power limit" On Wed, 15 Feb 2023 09:19:07 -0800, Rodrigo Vivi wrote: > > On Wed, Feb 15, 2023 at 08:24:51AM -0800, Dixit, Ashutosh wrote: > > On Wed, 15 Feb 2023 07:37:30 -0800, Jani Nikula wrote: > > > > > > On Wed, 08 Feb 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > > > On Wed, Feb 08, 2023 at 11:03:12AM -0800, Ashutosh Dixit wrote: > > > >> This reverts commit 0349c41b05968befaffa5fbb7e73d0ee6004f610. > > > >> > > > >> 0349c41b0596 ("drm/i915/hwmon: Enable PL1 power limit") is incorrect and > > > >> caused a major regression on ATSM. The change enabled the PL1 power limit > > > >> but FW sets the default value of the PL1 limit to 0 which implies HW now > > > >> works at minimum power and therefore the lowest effective frequency. This > > > >> means all workloads now run slower resulting in even GuC FW load operations > > > >> timing out, rendering ATSM unusable. > > > >> > > > >> A different solution to the original issue of the PL1 limit being disabled > > > >> on ATSM is needed but till that is developed, revert 0349c41b0596. > > > >> > > > >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > > > > > > > > pushed to drm-intel-next and removed from drm-intel-fixes. > > > > > > > > Thanks for the quick reaction. > > > > > > Please always add Fixes: tags also to reverts. > > > > > > I suppose we should fix dim to also detect reverts, but I ended up > > > cherry-picking and pushing the original commit out to > > > drm-intel-next-fixes before realizing it's been reverted. > > > > Oops, sorry! > > That's my mistake. I should had thought about this when pushing > and removing from the fixes. I just realized yet, when this patch > showed up in my -fixes cherry-pick again, but without the revert. > > I'm sorry. Not sure if it's related to this, but the reverted patch below has reappeared on drm-tip. Newest on top: ee892ea83d996 drm/i915/hwmon: Enable PL1 power limit 05d5562e401eb Revert "drm/i915/hwmon: Enable PL1 power limit" 0349c41b05968 drm/i915/hwmon: Enable PL1 power limit The new patch is: commit ee892ea83d99610fa33bea612de058e0955eec3a Author: Ashutosh Dixit <ashutosh.dixit@intel.com> AuthorDate: Fri Feb 3 07:53:09 2023 -0800 Commit: Jani Nikula <jani.nikula@intel.com> CommitDate: Mon Mar 13 11:38:05 2023 +0200 drm/i915/hwmon: Enable PL1 power limit Sorry I couldn't track which branch did this new patch come from (looks like drm-tip itself?). This is breaking ATSM again: https://intel-gfx-ci.01.org/tree/drm-tip/bat-atsm-1.html so needs to be reverted again and stay reverted. I could send a revert or any of you can also do it. Thanks. -- Ashutosh
On Fri, 17 Mar 2023 20:28:58 -0700, Dixit, Ashutosh wrote: > > Jani/Rodrigo, > > Original Subject: Re: [Intel-gfx] [PATCH] Revert "drm/i915/hwmon: Enable PL1 power limit" > > On Wed, 15 Feb 2023 09:19:07 -0800, Rodrigo Vivi wrote: > > > > On Wed, Feb 15, 2023 at 08:24:51AM -0800, Dixit, Ashutosh wrote: > > > On Wed, 15 Feb 2023 07:37:30 -0800, Jani Nikula wrote: > > > > > > > > On Wed, 08 Feb 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > > > > On Wed, Feb 08, 2023 at 11:03:12AM -0800, Ashutosh Dixit wrote: > > > > >> This reverts commit 0349c41b05968befaffa5fbb7e73d0ee6004f610. > > > > >> > > > > >> 0349c41b0596 ("drm/i915/hwmon: Enable PL1 power limit") is incorrect and > > > > >> caused a major regression on ATSM. The change enabled the PL1 power limit > > > > >> but FW sets the default value of the PL1 limit to 0 which implies HW now > > > > >> works at minimum power and therefore the lowest effective frequency. This > > > > >> means all workloads now run slower resulting in even GuC FW load operations > > > > >> timing out, rendering ATSM unusable. > > > > >> > > > > >> A different solution to the original issue of the PL1 limit being disabled > > > > >> on ATSM is needed but till that is developed, revert 0349c41b0596. > > > > >> > > > > >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > > > > > > > > > > pushed to drm-intel-next and removed from drm-intel-fixes. > > > > > > > > > > Thanks for the quick reaction. > > > > > > > > Please always add Fixes: tags also to reverts. > > > > > > > > I suppose we should fix dim to also detect reverts, but I ended up > > > > cherry-picking and pushing the original commit out to > > > > drm-intel-next-fixes before realizing it's been reverted. > > > > > > Oops, sorry! > > > > That's my mistake. I should had thought about this when pushing > > and removing from the fixes. I just realized yet, when this patch > > showed up in my -fixes cherry-pick again, but without the revert. > > > > I'm sorry. > > Not sure if it's related to this, but the reverted patch below has > reappeared on drm-tip. Newest on top: > > ee892ea83d996 drm/i915/hwmon: Enable PL1 power limit > 05d5562e401eb Revert "drm/i915/hwmon: Enable PL1 power limit" > 0349c41b05968 drm/i915/hwmon: Enable PL1 power limit > > The new patch is: > > commit ee892ea83d99610fa33bea612de058e0955eec3a > Author: Ashutosh Dixit <ashutosh.dixit@intel.com> > AuthorDate: Fri Feb 3 07:53:09 2023 -0800 > Commit: Jani Nikula <jani.nikula@intel.com> > CommitDate: Mon Mar 13 11:38:05 2023 +0200 > > drm/i915/hwmon: Enable PL1 power limit > > Sorry I couldn't track which branch did this new patch come from (looks > like drm-tip itself?). > > This is breaking ATSM again: > > https://intel-gfx-ci.01.org/tree/drm-tip/bat-atsm-1.html > > so needs to be reverted again and stay reverted. I could send a revert or > any of you can also do it. I have sent out the revert of ee892ea83d996: https://patchwork.freedesktop.org/series/113793/ ee892ea83d996 is also present in Linus' tree (in v6.3-rc2) so will need to be reverted there too. The previous two commits (the original commit and its revert) are not present in Linus' tree, at least yet. Thanks. -- Ashutosh
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 4683a5b96eff1..1225bc432f0d5 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -687,11 +687,6 @@ hwm_get_preregistration_info(struct drm_i915_private *i915) for_each_gt(gt, i915, i) hwm_energy(&hwmon->ddat_gt[i], &energy); } - - /* Enable PL1 power limit */ - if (i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) - hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, - PKG_PWR_LIM_1_EN, PKG_PWR_LIM_1_EN); } void i915_hwmon_register(struct drm_i915_private *i915)
This reverts commit 0349c41b05968befaffa5fbb7e73d0ee6004f610. 0349c41b0596 ("drm/i915/hwmon: Enable PL1 power limit") is incorrect and caused a major regression on ATSM. The change enabled the PL1 power limit but FW sets the default value of the PL1 limit to 0 which implies HW now works at minimum power and therefore the lowest effective frequency. This means all workloads now run slower resulting in even GuC FW load operations timing out, rendering ATSM unusable. A different solution to the original issue of the PL1 limit being disabled on ATSM is needed but till that is developed, revert 0349c41b0596. Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 5 ----- 1 file changed, 5 deletions(-)