| Message ID | 20221029044230.32128-1-gwan-gyeong.mun@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/hwmon: Fix a build error used with clang compiler | expand |
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.dixit@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
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.dixit@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
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.dixit@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.
On Mon, 31 Oct 2022, "Dixit, Ashutosh" <ashutosh.dixit@intel.com> 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.dixit@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 hwmon->rg register file. At all. The platform specific functions would handle the correct registers inline, directly. To recap, the platform specific abstraction should be the platform specific function pointers, like literally everywhere in the driver, *not* platform specific register initializations. BR, Jani. > > 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.
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 <ashutosh.dixit@intel.com> > 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 <ashutosh.dixit@intel.com> > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > --- > 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 >
On Wed, 02 Nov 2022, "Dixit, Ashutosh" <ashutosh.dixit@intel.com> 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 <ashutosh.dixit@intel.com> Acked-by: Jani Nikula <jani.nikula@intel.com> 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 <ashutosh.dixit@intel.com> >> Cc: Anshuman Gupta <anshuman.gupta@intel.com> >> Cc: Andi Shyti <andi.shyti@linux.intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> >> --- >> 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 >>
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;
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 <ashutosh.dixit@intel.com> Cc: Anshuman Gupta <anshuman.gupta@intel.com> Cc: Andi Shyti <andi.shyti@linux.intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)