Re: [PATCH] drm/i915/hwmon: Enable PL1 power limit
On Tue, 7 Feb 2023 at 17:19, Dixit, Ashutosh wrote:
>
> On Tue, 07 Feb 2023 08:12:25 -0800, Dixit, Ashutosh wrote:
> >
> > On Tue, 07 Feb 2023 01:32:44 -0800, Matthew Auld wrote:
> > >
> > > On Fri, 3 Feb 2023 at 15:54, Ashutosh Dixit
> > > wrote:
> > > >
> > > > Previous documentation suggested that PL1 power limit is always
> > > > enabled. However we now find this not to be the case on some
> > > > platforms (such as ATSM). Therefore enable PL1 power limit during hwmon
> > > > initialization.
> > >
> > > For some reason it looks like this change is impacting the atsm in CI:
> > > https://intel-gfx-ci.01.org/tree/drm-tip/bat-atsm-1.html
> >
> > Hmm, the change was meant for ATSM. Anyway let me try to get hold of an
> > ATSM and see if I can figure out what might be going on with these
> > seemingly unrelated failures and if I can repro them locally. Thanks!
>
> Rodrigo/Matt,
>
> I am proposing we revert this now and remerge again after investigating,
> even getting ATSM systems to investigate is not easy so it might take a few
> days to investigate. What do you guys think?
Yeah, maybe just revert for now.
>
> Thanks.
> --
> Ashutosh
>
>
> >
> > >
> > > >
> > > > Bspec: 51864
> > > >
> > > > v2: Add Bspec reference (Gwan-gyeong)
> > > > v3: Add Fixes tag
> > > >
> > > > Fixes: 99f55efb79114 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> > > > Signed-off-by: Ashutosh Dixit
> > > > Reviewed-by: Gwan-gyeong Mun
> > > > ---
> > > > drivers/gpu/drm/i915/i915_hwmon.c | 5 +
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> > > > b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > index 1225bc432f0d5..4683a5b96eff1 100644
> > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > @@ -687,6 +687,11 @@ hwm_get_preregistration_info(struct
> > > > drm_i915_private *i915)
> > > > for_each_gt(gt, i915, i)
> > > > hwm_energy(>ddat_gt[i], );
> > > > }
> > > > +
> > > > + /* 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
> > > >
Re: [PATCH] drm/i915/hwmon: Enable PL1 power limit
On Tue, 07 Feb 2023 08:12:25 -0800, Dixit, Ashutosh wrote:
>
> On Tue, 07 Feb 2023 01:32:44 -0800, Matthew Auld wrote:
> >
> > On Fri, 3 Feb 2023 at 15:54, Ashutosh Dixit
> > wrote:
> > >
> > > Previous documentation suggested that PL1 power limit is always
> > > enabled. However we now find this not to be the case on some
> > > platforms (such as ATSM). Therefore enable PL1 power limit during hwmon
> > > initialization.
> >
> > For some reason it looks like this change is impacting the atsm in CI:
> > https://intel-gfx-ci.01.org/tree/drm-tip/bat-atsm-1.html
>
> Hmm, the change was meant for ATSM. Anyway let me try to get hold of an
> ATSM and see if I can figure out what might be going on with these
> seemingly unrelated failures and if I can repro them locally. Thanks!
Rodrigo/Matt,
I am proposing we revert this now and remerge again after investigating,
even getting ATSM systems to investigate is not easy so it might take a few
days to investigate. What do you guys think?
Thanks.
--
Ashutosh
>
> >
> > >
> > > Bspec: 51864
> > >
> > > v2: Add Bspec reference (Gwan-gyeong)
> > > v3: Add Fixes tag
> > >
> > > Fixes: 99f55efb79114 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> > > Signed-off-by: Ashutosh Dixit
> > > Reviewed-by: Gwan-gyeong Mun
> > > ---
> > > drivers/gpu/drm/i915/i915_hwmon.c | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> > > b/drivers/gpu/drm/i915/i915_hwmon.c
> > > index 1225bc432f0d5..4683a5b96eff1 100644
> > > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > > @@ -687,6 +687,11 @@ hwm_get_preregistration_info(struct drm_i915_private
> > > *i915)
> > > for_each_gt(gt, i915, i)
> > > hwm_energy(>ddat_gt[i], );
> > > }
> > > +
> > > + /* 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
> > >
Re: [PATCH] drm/i915/hwmon: Enable PL1 power limit
On Tue, 07 Feb 2023 01:32:44 -0800, Matthew Auld wrote:
>
> On Fri, 3 Feb 2023 at 15:54, Ashutosh Dixit wrote:
> >
> > Previous documentation suggested that PL1 power limit is always
> > enabled. However we now find this not to be the case on some
> > platforms (such as ATSM). Therefore enable PL1 power limit during hwmon
> > initialization.
>
> For some reason it looks like this change is impacting the atsm in CI:
> https://intel-gfx-ci.01.org/tree/drm-tip/bat-atsm-1.html
Hmm, the change was meant for ATSM. Anyway let me try to get hold of an
ATSM and see if I can figure out what might be going on with these
seemingly unrelated failures and if I can repro them locally. Thanks!
>
> >
> > Bspec: 51864
> >
> > v2: Add Bspec reference (Gwan-gyeong)
> > v3: Add Fixes tag
> >
> > Fixes: 99f55efb79114 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> > Signed-off-by: Ashutosh Dixit
> > Reviewed-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/i915_hwmon.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> > b/drivers/gpu/drm/i915/i915_hwmon.c
> > index 1225bc432f0d5..4683a5b96eff1 100644
> > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > @@ -687,6 +687,11 @@ hwm_get_preregistration_info(struct drm_i915_private
> > *i915)
> > for_each_gt(gt, i915, i)
> > hwm_energy(>ddat_gt[i], );
> > }
> > +
> > + /* 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
> >
Re: [PATCH] drm/i915/hwmon: Enable PL1 power limit
On Fri, 3 Feb 2023 at 15:54, Ashutosh Dixit wrote:
>
> Previous documentation suggested that PL1 power limit is always
> enabled. However we now find this not to be the case on some
> platforms (such as ATSM). Therefore enable PL1 power limit during hwmon
> initialization.
For some reason it looks like this change is impacting the atsm in CI:
https://intel-gfx-ci.01.org/tree/drm-tip/bat-atsm-1.html
>
> Bspec: 51864
>
> v2: Add Bspec reference (Gwan-gyeong)
> v3: Add Fixes tag
>
> Fixes: 99f55efb79114 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> Signed-off-by: Ashutosh Dixit
> Reviewed-by: Gwan-gyeong Mun
> ---
> drivers/gpu/drm/i915/i915_hwmon.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> b/drivers/gpu/drm/i915/i915_hwmon.c
> index 1225bc432f0d5..4683a5b96eff1 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -687,6 +687,11 @@ hwm_get_preregistration_info(struct drm_i915_private
> *i915)
> for_each_gt(gt, i915, i)
> hwm_energy(>ddat_gt[i], );
> }
> +
> + /* 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
>
Re: [PATCH] drm/i915/hwmon: Enable PL1 power limit
looks good to me, but could you please add bpsec# to commit log? Reviewed-by: Gwan-gyeong Mun On 2/2/23 4:52 AM, Ashutosh Dixit wrote: Previous documentation suggested that PL1 power limit is always enabled. However we now find this not to be the case on some platforms (such as ATSM). Therefore enable PL1 power limit during hwmon initialization. Signed-off-by: Ashutosh Dixit --- drivers/gpu/drm/i915/i915_hwmon.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 1225bc432f0d5..4683a5b96eff1 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -687,6 +687,11 @@ hwm_get_preregistration_info(struct drm_i915_private *i915) for_each_gt(gt, i915, i) hwm_energy(>ddat_gt[i], ); } + + /* 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)
