Re: [Intel-gfx] [PATCH v2] drm/i915/hwmon: Fix a build error used with clang compiler
On Wed, 02 Nov 2022, "Dixit, Ashutosh" wrote:
> On Fri, 28 Oct 2022 21:42:30 -0700, Gwan-gyeong Mun wrote:
>>
>> Use REG_FIELD_PREP() and a constant value for hwm_field_scale_and_write()
>
> R-b'ing this so that this can get merged since this compile break is
> blocking drm-intel-gt-next pull request:
>
> Reviewed-by: Ashutosh Dixit
Acked-by: Jani Nikula
on this one as the stopgap measure.
>
>> If the first argument of FIELD_PREP() is not a compile-time constant value
>> or unsigned long long type, this routine of the __BF_FIELD_CHECK() macro
>> used internally by the FIELD_PREP() macro always returns false.
>>
>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>> __bf_cast_unsigned(_reg, ~0ull),\
>> _pfx "type of reg too small for mask"); \
>>
>> And it returns a build error by the option among the clang
>> compilation options. [-Werror,-Wtautological-constant-out-of-range-compare]
>>
>> Reported build error while using clang compiler:
>>
>> drivers/gpu/drm/i915/i915_hwmon.c:115:16: error: result of comparison of
>> constant 18446744073709551615 with expression of type 'typeof
>> (_Generic((field_msk), char: (unsigned char)0, unsigned char: (unsigned
>> char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0,
>> short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned
>> int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned
>> long long: (unsigned long long)0, long long: (unsigned long long)0, default:
>> (field_msk)))' (aka 'unsigned int') is always false
>> [-Werror,-Wtautological-constant-out-of-range-compare]
>> bits_to_set = FIELD_PREP(field_msk, nval);
>> ^~~
>> ./include/linux/bitfield.h:114:3: note: expanded from macro 'FIELD_PREP'
>> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");\
>> ^~~
>> ./include/linux/bitfield.h:71:53: note: expanded from macro
>> '__BF_FIELD_CHECK'
>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>> ~~^~~
>> ./include/linux/build_bug.h:39:58: note: expanded from macro
>> 'BUILD_BUG_ON_MSG'
>> ~^~~
>> ./include/linux/compiler_types.h:357:22: note: expanded from macro
>> 'compiletime_assert'
>> _compiletime_assert(condition, msg, __compiletime_assert_,
>> __COUNTER__)
>>
>> ^~~
>> ./include/linux/compiler_types.h:345:23: note: expanded from macro
>> '_compiletime_assert'
>> __compiletime_assert(condition, msg, prefix, suffix)
>> ~^~~
>> ./include/linux/compiler_types.h:337:9: note: expanded from macro
>> '__compiletime_assert'
>> if (!(condition)) \
>>
>> v2: Use REG_FIELD_PREP() macro instead of FIELD_PREP() (Jani)
>>
>> Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
>> Cc: Ashutosh Dixit
>> Cc: Anshuman Gupta
>> Cc: Andi Shyti
>> Cc: Jani Nikula
>> Signed-off-by: Gwan-gyeong Mun
>> ---
>> drivers/gpu/drm/i915/i915_hwmon.c | 12 +++-
>> 1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
>> b/drivers/gpu/drm/i915/i915_hwmon.c
>> index 9e9781493025..c588a17f97e9 100644
>> --- a/drivers/gpu/drm/i915/i915_hwmon.c
>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
>> @@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
>> i915_reg_t rgadr,
>>
>> static void
>> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
>> - u32 field_msk, int nshift,
>> - unsigned int scale_factor, long lval)
>> + int nshift, unsigned int scale_factor, long lval)
>> {
>> u32 nval;
>> -u32 bits_to_clear;
>> -u32 bits_to_set;
>>
>> /* Computation in 64-bits to avoid overflow. Round to nearest. */
>> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>>
>> -bits_to_clear = field_msk;
>> -bits_to_set = FIELD_PREP(field_msk, nval);
>> -
>> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
>> -bits_to_clear, bits_to_set);
>> +PKG_PWR_LIM_1,
>> +REG_FIELD_PREP(PKG_PWR_LIM_1,
>> nval));
>> }
>>
>> /*
>> @@ -406,7 +401,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int
>> chan, long val)
>> case hwmon_power_max:
>> hwm_field_scale_and_write(ddat,
>>hwmon->rg.pkg_rapl_limit,
>> - PKG_PWR_LIM_1,
>>
Re: [Intel-gfx] [PATCH v2] drm/i915/hwmon: Fix a build error used with clang compiler
On Fri, 28 Oct 2022 21:42:30 -0700, Gwan-gyeong Mun wrote:
>
> Use REG_FIELD_PREP() and a constant value for hwm_field_scale_and_write()
R-b'ing this so that this can get merged since this compile break is
blocking drm-intel-gt-next pull request:
Reviewed-by: Ashutosh Dixit
> If the first argument of FIELD_PREP() is not a compile-time constant value
> or unsigned long long type, this routine of the __BF_FIELD_CHECK() macro
> used internally by the FIELD_PREP() macro always returns false.
>
> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> __bf_cast_unsigned(_reg, ~0ull),\
> _pfx "type of reg too small for mask"); \
>
> And it returns a build error by the option among the clang
> compilation options. [-Werror,-Wtautological-constant-out-of-range-compare]
>
> Reported build error while using clang compiler:
>
> drivers/gpu/drm/i915/i915_hwmon.c:115:16: error: result of comparison of
> constant 18446744073709551615 with expression of type 'typeof
> (_Generic((field_msk), char: (unsigned char)0, unsigned char: (unsigned
> char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0,
> short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned
> int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long
> long: (unsigned long long)0, long long: (unsigned long long)0, default:
> (field_msk)))' (aka 'unsigned int') is always false
> [-Werror,-Wtautological-constant-out-of-range-compare]
> bits_to_set = FIELD_PREP(field_msk, nval);
> ^~~
> ./include/linux/bitfield.h:114:3: note: expanded from macro 'FIELD_PREP'
> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");\
> ^~~
> ./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> ~~^~~
> ./include/linux/build_bug.h:39:58: note: expanded from macro
> 'BUILD_BUG_ON_MSG'
> ~^~~
> ./include/linux/compiler_types.h:357:22: note: expanded from macro
> 'compiletime_assert'
> _compiletime_assert(condition, msg, __compiletime_assert_,
> __COUNTER__)
>
> ^~~
> ./include/linux/compiler_types.h:345:23: note: expanded from macro
> '_compiletime_assert'
> __compiletime_assert(condition, msg, prefix, suffix)
> ~^~~
> ./include/linux/compiler_types.h:337:9: note: expanded from macro
> '__compiletime_assert'
> if (!(condition)) \
>
> v2: Use REG_FIELD_PREP() macro instead of FIELD_PREP() (Jani)
>
> Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> Cc: Ashutosh Dixit
> Cc: Anshuman Gupta
> Cc: Andi Shyti
> Cc: Jani Nikula
> Signed-off-by: Gwan-gyeong Mun
> ---
> drivers/gpu/drm/i915/i915_hwmon.c | 12 +++-
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> b/drivers/gpu/drm/i915/i915_hwmon.c
> index 9e9781493025..c588a17f97e9 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
> i915_reg_t rgadr,
>
> static void
> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> - u32 field_msk, int nshift,
> - unsigned int scale_factor, long lval)
> + int nshift, unsigned int scale_factor, long lval)
> {
> u32 nval;
> - u32 bits_to_clear;
> - u32 bits_to_set;
>
> /* Computation in 64-bits to avoid overflow. Round to nearest. */
> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>
> - bits_to_clear = field_msk;
> - bits_to_set = FIELD_PREP(field_msk, nval);
> -
> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> - bits_to_clear, bits_to_set);
> + PKG_PWR_LIM_1,
> + REG_FIELD_PREP(PKG_PWR_LIM_1,
> nval));
> }
>
> /*
> @@ -406,7 +401,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int
> chan, long val)
> case hwmon_power_max:
> hwm_field_scale_and_write(ddat,
> hwmon->rg.pkg_rapl_limit,
> - PKG_PWR_LIM_1,
> hwmon->scl_shift_power,
> SF_POWER, val);
> return 0;
> --
> 2.37.1
>
Re: [Intel-gfx] [PATCH v2] drm/i915/hwmon: Fix a build error used with clang compiler
On Mon, 31 Oct 2022, "Dixit, Ashutosh" wrote:
> On Sun, 30 Oct 2022 23:37:59 -0700, Gwan-gyeong Mun wrote:
>>
>
> Hi GG,
>
>> On 10/31/22 7:19 AM, Dixit, Ashutosh wrote:
>> > On Fri, 28 Oct 2022 21:42:30 -0700, Gwan-gyeong Mun wrote:
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
>> >> b/drivers/gpu/drm/i915/i915_hwmon.c
>> >> index 9e9781493025..c588a17f97e9 100644
>> >> --- a/drivers/gpu/drm/i915/i915_hwmon.c
>> >> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
>> >> @@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
>> >> i915_reg_t rgadr,
>> >>
>> >> static void
>> >> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
>> >> - u32 field_msk, int nshift,
>> >> - unsigned int scale_factor, long lval)
>> >> + int nshift, unsigned int scale_factor, long lval)
>> >> {
>> >> u32 nval;
>> >> - u32 bits_to_clear;
>> >> - u32 bits_to_set;
>> >>
>> >> /* Computation in 64-bits to avoid overflow. Round to nearest. */
>> >> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>> >>
>> >> - bits_to_clear = field_msk;
>> >> - bits_to_set = FIELD_PREP(field_msk, nval);
>> >> -
>> >> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
>> >> - bits_to_clear, bits_to_set);
>> >> + PKG_PWR_LIM_1,
>> >> + REG_FIELD_PREP(PKG_PWR_LIM_1,
>> >> nval));
>> >
>> > I registered my objection to this patch already here:
>> >
>> > https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.di...@intel.com/
>> >
>> > the crux of which is "hwm_field_scale_and_write() pairs with
>> > hwm_field_read_and_scale() (they are basically a set/get pair) so it is
>> > desirable they have identical arguments. This patch breaks that symmetry".
>> >
>> > We can merge this patch now but the moment a second caller of
>> > hwm_field_scale_and_write arrives this patch will need to be reverted.
>> >
>> > I have also posted my preferred way (as I previously indiecated) of fixing
>> > this issue here (if this needs to be fixed in i915):
>> >
>> > https://patchwork.freedesktop.org/series/110301/
>> >
>> The given link (https://patchwork.freedesktop.org/series/110301/) is an
>> inline function that reduces the checks of REG_FIELD_PREP() and pursues the
>> same functionality.
>> It's not a good idea to add and use duplicate new inline functions with
>> reduced functionality under different names.
>
> See if you like v2 better :-)
>
>> +/* FIELD_PREP and REG_FIELD_PREP require a compile time constant mask */
>> +static u32 hwm_field_prep(u32 mask, u32 val)
>> +{
>> +return (val << __bf_shf(mask)) & mask;
>> +}
>> +
>> static void
>> hwm_locked_with_pm_intel_uncore_rmw(struct hwm_drvdata *ddat,
>> i915_reg_t reg, u32 clear, u32 set)
>> @@ -112,7 +118,7 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
>> i915_reg_t rgadr,
>> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>>
>> bits_to_clear = field_msk;
>> -bits_to_set = FIELD_PREP(field_msk, nval);
>> +bits_to_set = hwm_field_prep(field_msk, nval);
>>
>> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
>> bits_to_clear, bits_to_set);
>>
>>
>> The patch
>> (https://patchwork.freedesktop.org/patch/509248/?series=110094&rev=5) that
>> fixed the build error in a simplified form was added as there is only one
>> place that calls the hwm_field_scale_and_write() function at this time.
>>
>> If more places that use the hwm_field_scale_and_write() function are added
>> in the future, how about changing the interface that calls this function as
>> Jani previously suggested?
>
> Sorry, which interface change which Jani suggested are you referring to? I
> don't recall seeing anything but maybe I am mistaken.
Maybe it was in another context, but this looks like the interfaces are
built a bit too much on the registers. There are chains of single-use
functions that scale and lock and read/write register fields. And it's
all based on the assumption that any given value maps to a single
bitfield in a single register and can be handled with a single read or
rmw, and it falls part otherwise.
I think the registers should be abstracted at the lowest level,
completely, not brought in top down as register addresses and masks. For
inspiration, maybe look at intel_backlight.c.
So here, you'd have hwm_read/hwm_write levels completely agnostic about
all the details, just muxing to different items.
The hwm_*_read/hwm_*_write would operate in a domain that handles all
generic i915 stuff, but still would not know anything about registers or
scaling. And then you'd have the lowest, platform specific level that
you'd call via platform specific function pointers. They would be passed
and return values in the right scale (maybe using common helpers).
You would *not* have or initialize
Re: [Intel-gfx] [PATCH v2] drm/i915/hwmon: Fix a build error used with clang compiler
On Sun, 30 Oct 2022 23:37:59 -0700, Gwan-gyeong Mun wrote:
>
Hi GG,
> On 10/31/22 7:19 AM, Dixit, Ashutosh wrote:
> > On Fri, 28 Oct 2022 21:42:30 -0700, Gwan-gyeong Mun wrote:
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> >> b/drivers/gpu/drm/i915/i915_hwmon.c
> >> index 9e9781493025..c588a17f97e9 100644
> >> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> >> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> >> @@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
> >> i915_reg_t rgadr,
> >>
> >> static void
> >> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> >> -u32 field_msk, int nshift,
> >> -unsigned int scale_factor, long lval)
> >> +int nshift, unsigned int scale_factor, long lval)
> >> {
> >>u32 nval;
> >> - u32 bits_to_clear;
> >> - u32 bits_to_set;
> >>
> >>/* Computation in 64-bits to avoid overflow. Round to nearest. */
> >>nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
> >>
> >> - bits_to_clear = field_msk;
> >> - bits_to_set = FIELD_PREP(field_msk, nval);
> >> -
> >>hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> >> - bits_to_clear, bits_to_set);
> >> + PKG_PWR_LIM_1,
> >> + REG_FIELD_PREP(PKG_PWR_LIM_1,
> >> nval));
> >
> > I registered my objection to this patch already here:
> >
> > https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.di...@intel.com/
> >
> > the crux of which is "hwm_field_scale_and_write() pairs with
> > hwm_field_read_and_scale() (they are basically a set/get pair) so it is
> > desirable they have identical arguments. This patch breaks that symmetry".
> >
> > We can merge this patch now but the moment a second caller of
> > hwm_field_scale_and_write arrives this patch will need to be reverted.
> >
> > I have also posted my preferred way (as I previously indiecated) of fixing
> > this issue here (if this needs to be fixed in i915):
> >
> > https://patchwork.freedesktop.org/series/110301/
> >
> The given link (https://patchwork.freedesktop.org/series/110301/) is an
> inline function that reduces the checks of REG_FIELD_PREP() and pursues the
> same functionality.
> It's not a good idea to add and use duplicate new inline functions with
> reduced functionality under different names.
See if you like v2 better :-)
> +/* FIELD_PREP and REG_FIELD_PREP require a compile time constant mask */
> +static u32 hwm_field_prep(u32 mask, u32 val)
> +{
> + return (val << __bf_shf(mask)) & mask;
> +}
> +
> static void
> hwm_locked_with_pm_intel_uncore_rmw(struct hwm_drvdata *ddat,
> i915_reg_t reg, u32 clear, u32 set)
> @@ -112,7 +118,7 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
> i915_reg_t rgadr,
> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>
> bits_to_clear = field_msk;
> - bits_to_set = FIELD_PREP(field_msk, nval);
> + bits_to_set = hwm_field_prep(field_msk, nval);
>
> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> bits_to_clear, bits_to_set);
>
>
> The patch
> (https://patchwork.freedesktop.org/patch/509248/?series=110094&rev=5) that
> fixed the build error in a simplified form was added as there is only one
> place that calls the hwm_field_scale_and_write() function at this time.
>
> If more places that use the hwm_field_scale_and_write() function are added
> in the future, how about changing the interface that calls this function as
> Jani previously suggested?
Sorry, which interface change which Jani suggested are you referring to? I
don't recall seeing anything but maybe I am mistaken.
Thanks.
--
Ashutosh
> > IMO it would be a mistake to use REG_FIELD_PREP or FIELD_PREP here since
> > here the mask comes in as a function argument whereas REG_FIELD_PREP and
> > FIELD_PREP require that mask to be a compile time constant.
Re: [Intel-gfx] [PATCH v2] drm/i915/hwmon: Fix a build error used with clang compiler
On 10/31/22 7:19 AM, Dixit, Ashutosh wrote:
On Fri, 28 Oct 2022 21:42:30 -0700, Gwan-gyeong Mun wrote:
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
b/drivers/gpu/drm/i915/i915_hwmon.c
index 9e9781493025..c588a17f97e9 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
static void
hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
- u32 field_msk, int nshift,
- unsigned int scale_factor, long lval)
+ int nshift, unsigned int scale_factor, long lval)
{
u32 nval;
- u32 bits_to_clear;
- u32 bits_to_set;
/* Computation in 64-bits to avoid overflow. Round to nearest. */
nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
- bits_to_clear = field_msk;
- bits_to_set = FIELD_PREP(field_msk, nval);
-
hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
- bits_to_clear, bits_to_set);
+ PKG_PWR_LIM_1,
+ REG_FIELD_PREP(PKG_PWR_LIM_1,
nval));
I registered my objection to this patch already here:
https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.di...@intel.com/
the crux of which is "hwm_field_scale_and_write() pairs with
hwm_field_read_and_scale() (they are basically a set/get pair) so it is
desirable they have identical arguments. This patch breaks that symmetry".
We can merge this patch now but the moment a second caller of
hwm_field_scale_and_write arrives this patch will need to be reverted.
I have also posted my preferred way (as I previously indiecated) of fixing
this issue here (if this needs to be fixed in i915):
https://patchwork.freedesktop.org/series/110301/
The given link (https://patchwork.freedesktop.org/series/110301/) is an
inline function that reduces the checks of REG_FIELD_PREP() and pursues
the same functionality.
It's not a good idea to add and use duplicate new inline functions with
reduced functionality under different names.
+/* FIELD_PREP and REG_FIELD_PREP require a compile time constant mask */
+static u32 hwm_field_prep(u32 mask, u32 val)
+{
+ return (val << __bf_shf(mask)) & mask;
+}
+
static void
hwm_locked_with_pm_intel_uncore_rmw(struct hwm_drvdata *ddat,
i915_reg_t reg, u32 clear, u32 set)
@@ -112,7 +118,7 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
bits_to_clear = field_msk;
- bits_to_set = FIELD_PREP(field_msk, nval);
+ bits_to_set = hwm_field_prep(field_msk, nval);
hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
bits_to_clear, bits_to_set);
The patch
(https://patchwork.freedesktop.org/patch/509248/?series=110094&rev=5)
that fixed the build error in a simplified form was added as there is
only one place that calls the hwm_field_scale_and_write() function at
this time.
If more places that use the hwm_field_scale_and_write() function are
added in the future, how about changing the interface that calls this
function as Jani previously suggested?
Br,
G.G.
IMO it would be a mistake to use REG_FIELD_PREP or FIELD_PREP here since
here the mask comes in as a function argument whereas REG_FIELD_PREP and
FIELD_PREP require that mask to be a compile time constant.
Thanks.
--
Ashutosh
Re: [Intel-gfx] [PATCH v2] drm/i915/hwmon: Fix a build error used with clang compiler
On Fri, 28 Oct 2022 21:42:30 -0700, Gwan-gyeong Mun wrote:
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> b/drivers/gpu/drm/i915/i915_hwmon.c
> index 9e9781493025..c588a17f97e9 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
> i915_reg_t rgadr,
>
> static void
> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> - u32 field_msk, int nshift,
> - unsigned int scale_factor, long lval)
> + int nshift, unsigned int scale_factor, long lval)
> {
> u32 nval;
> - u32 bits_to_clear;
> - u32 bits_to_set;
>
> /* Computation in 64-bits to avoid overflow. Round to nearest. */
> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>
> - bits_to_clear = field_msk;
> - bits_to_set = FIELD_PREP(field_msk, nval);
> -
> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> - bits_to_clear, bits_to_set);
> + PKG_PWR_LIM_1,
> + REG_FIELD_PREP(PKG_PWR_LIM_1,
> nval));
I registered my objection to this patch already here:
https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.di...@intel.com/
the crux of which is "hwm_field_scale_and_write() pairs with
hwm_field_read_and_scale() (they are basically a set/get pair) so it is
desirable they have identical arguments. This patch breaks that symmetry".
We can merge this patch now but the moment a second caller of
hwm_field_scale_and_write arrives this patch will need to be reverted.
I have also posted my preferred way (as I previously indiecated) of fixing
this issue here (if this needs to be fixed in i915):
https://patchwork.freedesktop.org/series/110301/
IMO it would be a mistake to use REG_FIELD_PREP or FIELD_PREP here since
here the mask comes in as a function argument whereas REG_FIELD_PREP and
FIELD_PREP require that mask to be a compile time constant.
Thanks.
--
Ashutosh
[Intel-gfx] [PATCH v2] drm/i915/hwmon: Fix a build error used with clang compiler
Use REG_FIELD_PREP() and a constant value for hwm_field_scale_and_write()
If the first argument of FIELD_PREP() is not a compile-time constant value
or unsigned long long type, this routine of the __BF_FIELD_CHECK() macro
used internally by the FIELD_PREP() macro always returns false.
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull),\
_pfx "type of reg too small for mask"); \
And it returns a build error by the option among the clang
compilation options. [-Werror,-Wtautological-constant-out-of-range-compare]
Reported build error while using clang compiler:
drivers/gpu/drm/i915/i915_hwmon.c:115:16: error: result of comparison of
constant 18446744073709551615 with expression of type 'typeof
(_Generic((field_msk), char: (unsigned char)0, unsigned char: (unsigned char)0,
signed char: (unsigned char)0, unsigned short: (unsigned short)0, short:
(unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned int)0,
unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long long:
(unsigned long long)0, long long: (unsigned long long)0, default:
(field_msk)))' (aka 'unsigned int') is always false
[-Werror,-Wtautological-constant-out-of-range-compare]
bits_to_set = FIELD_PREP(field_msk, nval);
^~~
./include/linux/bitfield.h:114:3: note: expanded from macro 'FIELD_PREP'
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");\
^~~
./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
~~^~~
./include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
~^~~
./include/linux/compiler_types.h:357:22: note: expanded from macro
'compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
^~~
./include/linux/compiler_types.h:345:23: note: expanded from macro
'_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
~^~~
./include/linux/compiler_types.h:337:9: note: expanded from macro
'__compiletime_assert'
if (!(condition)) \
v2: Use REG_FIELD_PREP() macro instead of FIELD_PREP() (Jani)
Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
Cc: Ashutosh Dixit
Cc: Anshuman Gupta
Cc: Andi Shyti
Cc: Jani Nikula
Signed-off-by: Gwan-gyeong Mun
---
drivers/gpu/drm/i915/i915_hwmon.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
b/drivers/gpu/drm/i915/i915_hwmon.c
index 9e9781493025..c588a17f97e9 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
static void
hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
- u32 field_msk, int nshift,
- unsigned int scale_factor, long lval)
+ int nshift, unsigned int scale_factor, long lval)
{
u32 nval;
- u32 bits_to_clear;
- u32 bits_to_set;
/* Computation in 64-bits to avoid overflow. Round to nearest. */
nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
- bits_to_clear = field_msk;
- bits_to_set = FIELD_PREP(field_msk, nval);
-
hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
- bits_to_clear, bits_to_set);
+ PKG_PWR_LIM_1,
+ REG_FIELD_PREP(PKG_PWR_LIM_1,
nval));
}
/*
@@ -406,7 +401,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int
chan, long val)
case hwmon_power_max:
hwm_field_scale_and_write(ddat,
hwmon->rg.pkg_rapl_limit,
- PKG_PWR_LIM_1,
hwmon->scl_shift_power,
SF_POWER, val);
return 0;
--
2.37.1
