Re: [Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write
On Tue, Feb 14, 2023 at 12:20:36PM -0800, Dixit, Ashutosh wrote:
> On Tue, 14 Feb 2023 06:50:44 -0800, Rodrigo Vivi wrote:
> >
> > > +static int
> > > +hwm_power_max_write(struct hwm_drvdata *ddat, long val)
> > +{
> > > + struct i915_hwmon *hwmon = ddat->hwmon;
> > > + u32 nval;
> > > +
> > > + /* Computation in 64-bits to avoid overflow. Round to nearest. */
> > > + nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power,
> > > SF_POWER);
> > > +
> > > + hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
> > > + PKG_PWR_LIM_1,
> > > + REG_FIELD_PREP(PKG_PWR_LIM_1,
> > > nval));
> > > + return 0;
> >
> > Let's keep this function as void and the return 0 in the previous spot.
>
> Hmm, see your point. Though there is an identical situation for
> hwm_power_max_read read too (in hwm_power_read). Maybe I'll change it there
> too in the same patch to keep things symmetrical and retain your R-b?
okay then. let's move this one as is and fix both functions in a follow up.
Thanks,
Rodrigo.
>
> > With that change:
> >
> > Reviewed-by: Rodrigo Vivi
>
> Thanks.
Re: [Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write
On Tue, 14 Feb 2023 06:50:44 -0800, Rodrigo Vivi wrote:
>
> > +static int
> > +hwm_power_max_write(struct hwm_drvdata *ddat, long val)
> +{
> > + struct i915_hwmon *hwmon = ddat->hwmon;
> > + u32 nval;
> > +
> > + /* Computation in 64-bits to avoid overflow. Round to nearest. */
> > + nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power,
> > SF_POWER);
> > +
> > + hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
> > + PKG_PWR_LIM_1,
> > + REG_FIELD_PREP(PKG_PWR_LIM_1,
> > nval));
> > + return 0;
>
> Let's keep this function as void and the return 0 in the previous spot.
Hmm, see your point. Though there is an identical situation for
hwm_power_max_read read too (in hwm_power_read). Maybe I'll change it there
too in the same patch to keep things symmetrical and retain your R-b?
> With that change:
>
> Reviewed-by: Rodrigo Vivi
Thanks.
Re: [Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write
On Mon, Feb 13, 2023 at 01:00:47PM -0800, Ashutosh Dixit wrote:
> hwm_field_scale_and_write has a single caller hwm_power_write and is
> specific to hwm_power_write but makes it appear that it is a general
> function which can have multiple callers. Replace the function with
> hwm_power_max_write which is specific to hwm_power_write and use that in
> future patches where the function needs to be extended.
>
> Signed-off-by: Ashutosh Dixit
> ---
> drivers/gpu/drm/i915/i915_hwmon.c | 36 ++-
> 1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> b/drivers/gpu/drm/i915/i915_hwmon.c
> index 1225bc432f0d5..85195d61f89c7 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -99,20 +99,6 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
> i915_reg_t rgadr,
> return mul_u64_u32_shr(reg_value, scale_factor, nshift);
> }
>
> -static void
> -hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> - int nshift, unsigned int scale_factor, long lval)
> -{
> - u32 nval;
> -
> - /* Computation in 64-bits to avoid overflow. Round to nearest. */
> - nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
> -
> - hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> - PKG_PWR_LIM_1,
> - REG_FIELD_PREP(PKG_PWR_LIM_1,
> nval));
> -}
> -
> /*
> * hwm_energy - Obtain energy value
> *
> @@ -391,6 +377,21 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val)
> return 0;
> }
>
> +static int
> +hwm_power_max_write(struct hwm_drvdata *ddat, long val)
+{
> + struct i915_hwmon *hwmon = ddat->hwmon;
> + u32 nval;
> +
> + /* Computation in 64-bits to avoid overflow. Round to nearest. */
> + nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power,
> SF_POWER);
> +
> + hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
> + PKG_PWR_LIM_1,
> + REG_FIELD_PREP(PKG_PWR_LIM_1,
> nval));
> + return 0;
Let's keep this function as void and the return 0 in the previous spot.
With that change:
Reviewed-by: Rodrigo Vivi
> +}
> +
> static int
> hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
> {
> @@ -425,16 +426,11 @@ hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int
> chan, long *val)
> static int
> hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
> {
> - struct i915_hwmon *hwmon = ddat->hwmon;
> u32 uval;
>
> switch (attr) {
> case hwmon_power_max:
> - hwm_field_scale_and_write(ddat,
> - hwmon->rg.pkg_rapl_limit,
> - hwmon->scl_shift_power,
> - SF_POWER, val);
> - return 0;
> + return hwm_power_max_write(ddat, val);
> case hwmon_power_crit:
> uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT,
> SF_POWER);
> return hwm_pcode_write_i1(ddat->uncore->i915, uval);
> --
> 2.38.0
>
[Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write
hwm_field_scale_and_write has a single caller hwm_power_write and is
specific to hwm_power_write but makes it appear that it is a general
function which can have multiple callers. Replace the function with
hwm_power_max_write which is specific to hwm_power_write and use that in
future patches where the function needs to be extended.
Signed-off-by: Ashutosh Dixit
---
drivers/gpu/drm/i915/i915_hwmon.c | 36 ++-
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
b/drivers/gpu/drm/i915/i915_hwmon.c
index 1225bc432f0d5..85195d61f89c7 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -99,20 +99,6 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
return mul_u64_u32_shr(reg_value, scale_factor, nshift);
}
-static void
-hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
- int nshift, unsigned int scale_factor, long lval)
-{
- u32 nval;
-
- /* Computation in 64-bits to avoid overflow. Round to nearest. */
- nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
-
- hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
- PKG_PWR_LIM_1,
- REG_FIELD_PREP(PKG_PWR_LIM_1,
nval));
-}
-
/*
* hwm_energy - Obtain energy value
*
@@ -391,6 +377,21 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val)
return 0;
}
+static int
+hwm_power_max_write(struct hwm_drvdata *ddat, long val)
+{
+ struct i915_hwmon *hwmon = ddat->hwmon;
+ u32 nval;
+
+ /* Computation in 64-bits to avoid overflow. Round to nearest. */
+ nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power,
SF_POWER);
+
+ hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
+ PKG_PWR_LIM_1,
+ REG_FIELD_PREP(PKG_PWR_LIM_1,
nval));
+ return 0;
+}
+
static int
hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
{
@@ -425,16 +426,11 @@ hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int
chan, long *val)
static int
hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
{
- struct i915_hwmon *hwmon = ddat->hwmon;
u32 uval;
switch (attr) {
case hwmon_power_max:
- hwm_field_scale_and_write(ddat,
- hwmon->rg.pkg_rapl_limit,
- hwmon->scl_shift_power,
- SF_POWER, val);
- return 0;
+ return hwm_power_max_write(ddat, val);
case hwmon_power_crit:
uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT,
SF_POWER);
return hwm_pcode_write_i1(ddat->uncore->i915, uval);
--
2.38.0
[Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write
hwm_field_scale_and_write has a single caller hwm_power_write and is
specific to hwm_power_write but makes it appear that it is a general
function which can have multiple callers. Replace the function with
hwm_power_max_write which is specific to hwm_power_write and use that in
future patches where the function needs to be extended.
Signed-off-by: Ashutosh Dixit
---
drivers/gpu/drm/i915/i915_hwmon.c | 36 ++-
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
b/drivers/gpu/drm/i915/i915_hwmon.c
index 1225bc432f0d5..85195d61f89c7 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -99,20 +99,6 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
return mul_u64_u32_shr(reg_value, scale_factor, nshift);
}
-static void
-hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
- int nshift, unsigned int scale_factor, long lval)
-{
- u32 nval;
-
- /* Computation in 64-bits to avoid overflow. Round to nearest. */
- nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
-
- hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
- PKG_PWR_LIM_1,
- REG_FIELD_PREP(PKG_PWR_LIM_1,
nval));
-}
-
/*
* hwm_energy - Obtain energy value
*
@@ -391,6 +377,21 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val)
return 0;
}
+static int
+hwm_power_max_write(struct hwm_drvdata *ddat, long val)
+{
+ struct i915_hwmon *hwmon = ddat->hwmon;
+ u32 nval;
+
+ /* Computation in 64-bits to avoid overflow. Round to nearest. */
+ nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power,
SF_POWER);
+
+ hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
+ PKG_PWR_LIM_1,
+ REG_FIELD_PREP(PKG_PWR_LIM_1,
nval));
+ return 0;
+}
+
static int
hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
{
@@ -425,16 +426,11 @@ hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int
chan, long *val)
static int
hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
{
- struct i915_hwmon *hwmon = ddat->hwmon;
u32 uval;
switch (attr) {
case hwmon_power_max:
- hwm_field_scale_and_write(ddat,
- hwmon->rg.pkg_rapl_limit,
- hwmon->scl_shift_power,
- SF_POWER, val);
- return 0;
+ return hwm_power_max_write(ddat, val);
case hwmon_power_crit:
uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT,
SF_POWER);
return hwm_pcode_write_i1(ddat->uncore->i915, uval);
--
2.38.0
