Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
On 11/2/22 8:32 AM, Joonas Lahtinen wrote:
Quoting Jani Nikula (2022-10-28 11:46:21)
On Fri, 28 Oct 2022, Gwan-gyeong Mun wrote:
Resend, because some content was accidentally omitted from the previous
reply.
Please ignore the previous email.
Hi all,
I should have written the original commit message more accurately, but
it seems that it was written inaccurately.
If the FIELD_PREP macro is expanded, the following macros are used.
#define FIELD_PREP(_mask, _val) \
({ \
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");\
((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
})
#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
({ \
BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
_pfx "mask is not constant"); \
BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero");\
BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
_pfx "value too large for the field"); \
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
(1ULL << __bf_shf(_mask))); \
})
Among them, a build error is generated by the lower part of the
__BF_FIELD_CHECK() macro.
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
Here, if you apply an argument to this macro, it will look like the
following.
__bf_cast_unsigned(field_msk, field_msk) > __bf_cast_unsigned(0ULL, ~0ull)
The result is always false because an unsigned int value of type
field_msk is not always greater than the maximum value of unsigned long
long .
So, a build error occurs due to the following part of the clang compiler
option.
[-Werror,-Wtautological-constant-out-of-range-compare]
You can simply override this warning in Clang by adding the build option
below, but this seems like a bad attempt
i915/Makefile
CFLAGS_i915_hwmon.o += -Wno-tautological-constant-out-of-range-compare
The easiest way to solve this is to use a constant value, not a
variable, as an argument to FIELD_PREP.
And since the REG_FIELD_PREP() macro suggested by Jani requires a const
expression as the first argument, it cannot be changed with this macro
alone in the existing code, it must be changed to input a constant value
as shown below.
We've added REG_FIELD_PREP() precisely to avoid the problems with the
types and ranges, as we want it to operate on u32. It also uses
__is_constexpr() to avoid dependencies on compiler implementation and
optimizations.
Please use REG_FIELD_PREP() and a constant value. Maybe rethink the
interface if needed.
Ashutosh and GG, can we get a fix for this merged ASAP. It's currently
blocking the drm-intel-gt-next pull request.
Regards, Joonas
Hi Joonas,
As a workaround patch, this patch[1] was reviewed by Ashutoshr and acked
by Jani.
[1] https://patchwork.freedesktop.org/patch/509248/?series=110094&rev=5
Br,
G.G.
BR,
Jani.
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
b/drivers/gpu/drm/i915/i915_hwmon.c
index 08c921421a5f..abb3a194c548 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -101,7 +101,7 @@ 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,
- const u32 field_msk, int nshift,
+ int nshift,
unsigned int scale_factor, long lval)
{
u32 nval;
@@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
/* 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 = REG_FIELD_PREP(field_msk, nval);
+ bits_to_clear = PKG_PWR_LIM_1;
+ bits_to_set = REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
bits_to_clear, bits_to_set);
@@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr,
int chan, long val)
case hwmon_power_max:
hwm_field_scale_and_write(ddat,
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
Quoting Jani Nikula (2022-10-28 11:46:21)
> On Fri, 28 Oct 2022, Gwan-gyeong Mun wrote:
> > Resend, because some content was accidentally omitted from the previous
> > reply.
> > Please ignore the previous email.
> >
> > Hi all,
> >
> > I should have written the original commit message more accurately, but
> > it seems that it was written inaccurately.
> >
> > If the FIELD_PREP macro is expanded, the following macros are used.
> >
> > #define FIELD_PREP(_mask, _val)
> > \
> > ({ \
> > __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");\
> > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
> > })
> >
> >
> > #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
> > ({ \
> > BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
> >_pfx "mask is not constant"); \
> > BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero");\
> > BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
> >~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
> >_pfx "value too large for the field"); \
> > BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> >__bf_cast_unsigned(_reg, ~0ull), \
> >_pfx "type of reg too small for mask"); \
> > __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
> > (1ULL << __bf_shf(_mask))); \
> > })
> >
> > Among them, a build error is generated by the lower part of the
> > __BF_FIELD_CHECK() macro.
> >
> > BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> >__bf_cast_unsigned(_reg, ~0ull), \
> >_pfx "type of reg too small for mask"); \
> >
> >
> > Here, if you apply an argument to this macro, it will look like the
> > following.
> >
> > __bf_cast_unsigned(field_msk, field_msk) > __bf_cast_unsigned(0ULL, ~0ull)
> >
> > The result is always false because an unsigned int value of type
> > field_msk is not always greater than the maximum value of unsigned long
> > long .
> > So, a build error occurs due to the following part of the clang compiler
> > option.
> >
> > [-Werror,-Wtautological-constant-out-of-range-compare]
> >
> > You can simply override this warning in Clang by adding the build option
> > below, but this seems like a bad attempt
> >
> > i915/Makefile
> > CFLAGS_i915_hwmon.o += -Wno-tautological-constant-out-of-range-compare
> >
> > The easiest way to solve this is to use a constant value, not a
> > variable, as an argument to FIELD_PREP.
> >
> > And since the REG_FIELD_PREP() macro suggested by Jani requires a const
> > expression as the first argument, it cannot be changed with this macro
> > alone in the existing code, it must be changed to input a constant value
> > as shown below.
>
> We've added REG_FIELD_PREP() precisely to avoid the problems with the
> types and ranges, as we want it to operate on u32. It also uses
> __is_constexpr() to avoid dependencies on compiler implementation and
> optimizations.
>
> Please use REG_FIELD_PREP() and a constant value. Maybe rethink the
> interface if needed.
Ashutosh and GG, can we get a fix for this merged ASAP. It's currently
blocking the drm-intel-gt-next pull request.
Regards, Joonas
>
> BR,
> Jani.
>
>
>
>
> >
> > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> > b/drivers/gpu/drm/i915/i915_hwmon.c
> > index 08c921421a5f..abb3a194c548 100644
> > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > @@ -101,7 +101,7 @@ 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,
> > - const u32 field_msk, int nshift,
> > + int nshift,
> >unsigned int scale_factor, long lval)
> > {
> > u32 nval;
> > @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
> > i915_reg_t rgadr,
> > /* 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 = REG_FIELD_PREP(field_msk, nval);
> > + bits_to_clear = PKG_PWR_LIM_1;
> > + bits_to_set = REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
> >
> > hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> > bits_to_clear, bits_to_set);
> > @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *d
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
On Fri, 28 Oct 2022, Gwan-gyeong Mun wrote:
> Resend, because some content was accidentally omitted from the previous
> reply.
> Please ignore the previous email.
>
> Hi all,
>
> I should have written the original commit message more accurately, but
> it seems that it was written inaccurately.
>
> If the FIELD_PREP macro is expanded, the following macros are used.
>
> #define FIELD_PREP(_mask, _val)
> \
> ({ \
> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");\
> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
> })
>
>
> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
> ({ \
> BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
>_pfx "mask is not constant"); \
> BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero");\
> BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
>~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
>_pfx "value too large for the field"); \
> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>__bf_cast_unsigned(_reg, ~0ull), \
>_pfx "type of reg too small for mask"); \
> __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
> (1ULL << __bf_shf(_mask))); \
> })
>
> Among them, a build error is generated by the lower part of the
> __BF_FIELD_CHECK() macro.
>
> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>__bf_cast_unsigned(_reg, ~0ull), \
>_pfx "type of reg too small for mask"); \
>
>
> Here, if you apply an argument to this macro, it will look like the
> following.
>
> __bf_cast_unsigned(field_msk, field_msk) > __bf_cast_unsigned(0ULL, ~0ull)
>
> The result is always false because an unsigned int value of type
> field_msk is not always greater than the maximum value of unsigned long
> long .
> So, a build error occurs due to the following part of the clang compiler
> option.
>
> [-Werror,-Wtautological-constant-out-of-range-compare]
>
> You can simply override this warning in Clang by adding the build option
> below, but this seems like a bad attempt
>
> i915/Makefile
> CFLAGS_i915_hwmon.o += -Wno-tautological-constant-out-of-range-compare
>
> The easiest way to solve this is to use a constant value, not a
> variable, as an argument to FIELD_PREP.
>
> And since the REG_FIELD_PREP() macro suggested by Jani requires a const
> expression as the first argument, it cannot be changed with this macro
> alone in the existing code, it must be changed to input a constant value
> as shown below.
We've added REG_FIELD_PREP() precisely to avoid the problems with the
types and ranges, as we want it to operate on u32. It also uses
__is_constexpr() to avoid dependencies on compiler implementation and
optimizations.
Please use REG_FIELD_PREP() and a constant value. Maybe rethink the
interface if needed.
BR,
Jani.
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> b/drivers/gpu/drm/i915/i915_hwmon.c
> index 08c921421a5f..abb3a194c548 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -101,7 +101,7 @@ 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,
> - const u32 field_msk, int nshift,
> + int nshift,
>unsigned int scale_factor, long lval)
> {
> u32 nval;
> @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
> i915_reg_t rgadr,
> /* 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 = REG_FIELD_PREP(field_msk, nval);
> + bits_to_clear = PKG_PWR_LIM_1;
> + bits_to_set = REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
>
> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> bits_to_clear, bits_to_set);
> @@ -406,7 +406,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);
> re
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
Resend, because some content was accidentally omitted from the previous
reply.
Please ignore the previous email.
Hi all,
I should have written the original commit message more accurately, but
it seems that it was written inaccurately.
If the FIELD_PREP macro is expanded, the following macros are used.
#define FIELD_PREP(_mask, _val) \
({ \
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
})
#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
({ \
BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
_pfx "mask is not constant");\
BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
_pfx "value too large for the field"); \
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
(1ULL << __bf_shf(_mask))); \
})
Among them, a build error is generated by the lower part of the
__BF_FIELD_CHECK() macro.
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
Here, if you apply an argument to this macro, it will look like the
following.
__bf_cast_unsigned(field_msk, field_msk) > __bf_cast_unsigned(0ULL, ~0ull)
The result is always false because an unsigned int value of type
field_msk is not always greater than the maximum value of unsigned long
long .
So, a build error occurs due to the following part of the clang compiler
option.
[-Werror,-Wtautological-constant-out-of-range-compare]
You can simply override this warning in Clang by adding the build option
below, but this seems like a bad attempt
i915/Makefile
CFLAGS_i915_hwmon.o += -Wno-tautological-constant-out-of-range-compare
The easiest way to solve this is to use a constant value, not a
variable, as an argument to FIELD_PREP.
And since the REG_FIELD_PREP() macro suggested by Jani requires a const
expression as the first argument, it cannot be changed with this macro
alone in the existing code, it must be changed to input a constant value
as shown below.
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
b/drivers/gpu/drm/i915/i915_hwmon.c
index 08c921421a5f..abb3a194c548 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -101,7 +101,7 @@ 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,
- const u32 field_msk, int nshift,
+ int nshift,
unsigned int scale_factor, long lval)
{
u32 nval;
@@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
/* 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 = REG_FIELD_PREP(field_msk, nval);
+ bits_to_clear = PKG_PWR_LIM_1;
+ bits_to_set = REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
bits_to_clear, bits_to_set);
@@ -406,7 +406,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;
In addition, if there is no build problem regardless of the size of the
type as the first argument in FIELD_PREP, it is possible through the
following modification.
(Since this modification modifies include/linux/bitfield.h , I will send
it as a separate patch.
)
However, it seems that we need to have Jani's confirm whether it is okay
to use FIELD_PREP() instead of REG_FIELD_PREP() which is forced to u32
return type in i915.
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index c9be1657f03
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
Hi all,
I should have written the commit message more accurately, but it seems
that it was written inaccurately.
If the FIELD_PREP macro is expanded, the following macros are used.
#define FIELD_PREP(_mask, _val) \
({ \
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
})
#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
({ \
BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
_pfx "mask is not constant");\
BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
_pfx "value too large for the field"); \
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
(1ULL << __bf_shf(_mask))); \
})
Among them, a build error is generated by the lower part of the
__BF_FIELD_CHECK() macro.
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
Here, if you apply an argument to this macro, it will look like the
following.
__bf_cast_unsigned(field_msk, field_msk) > __bf_cast_unsigned(0ULL, ~0ull)
The result is always false because an unsigned int value of type
field_msk is not always greater than the maximum value of unsigned long
long .
So, a build error occurs due to the following part of the clang compiler
option.
[-Werror,-Wtautological-constant-out-of-range-compare]
You can simply override this warning in Clang by adding the build option
below, but this seems like a bad attempt
i915/Makefile
CFLAGS_i915_hwmon.o += -Wno-tautological-constant-out-of-range-compare
The easiest way to solve this is to use a constant value, not a
variable, as an argument to FIELD_PREP.
And since the REG_FIELD_PREP() macro suggested by Jani requires a const
expression as the first argument, it cannot be changed with this macro
alone in the existing code, it must be changed to input a constant value
as shown below.
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
b/drivers/gpu/drm/i915/i915_hwmon.c
index 08c921421a5f..abb3a194c548 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -101,7 +101,7 @@ 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,
- const u32 field_msk, int nshift,
+ int nshift,
unsigned int scale_factor, long lval)
{
u32 nval;
@@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
/* 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 = REG_FIELD_PREP(field_msk, nval);
+ bits_to_clear = PKG_PWR_LIM_1;
+ bits_to_set = REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
bits_to_clear, bits_to_set);
@@ -406,7 +406,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;
In addition, if there is no build problem regardless of the size of the
type as the first argument in FIELD_PREP, it is possible through the
following modification.
(Since this modification modifies include/linux/bitfield.h , I will send
it as a separate patch.
)
However, it seems that we need to have Jani's confirm whether it is okay
to use FIELD_PREP() instead of REG_FIELD_PREP() which is forced to u32
return type.
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index c9be1657f03d..6e96799b6f38 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -9,7 +9,7 @@
#include
#include
-
+#incl
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
On Thu, 27 Oct 2022 10:16:47 -0700, Nick Desaulniers wrote:
>
Hi Nick,
> Thanks, I can repro now.
>
> I haven't detangled the macro soup, but I noticed:
>
> 1. FIELD_PREP is defined in include/linux/bitfield.h which has the
> following comment:
> 18 * Mask must be a compilation time constant.
I had comments about this here:
https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.di...@intel.com/
The relevant part being:
{quote}
> > > ./include/linux/bitfield.h:71:53: note: expanded from macro
> > > '__BF_FIELD_CHECK'
> > > BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull
also occurs here):
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
So it goes through previous checks including the "mask is not constant"
check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated
after most optimizations have run" so by that time both compilers (gcc and
clang) have figured out that even though _mask is coming in as function
argument it is really the constant below:
#define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
But it is not clear why clang chokes on this "type of reg too small for
mask" check (and gcc doesn't) since everything is u32.
{end quote}
>
> 2. hwm_field_scale_and_write only has one callsite.
>
> The following patch works:
If we need to fix it at our end yes we can come up with one of these
patches. But we were hoping someone from clang/llvm can comment about the
"type of reg too small for mask" stuff. If this is something which needs to
be fixed in clang/llvm we probably don't want to hide the issue.
>
> ```
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> b/drivers/gpu/drm/i915/i915_hwmon.c
> index 9e9781493025..6ac29d90b92a 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -101,7 +101,7 @@ 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,
> + int nshift,
> unsigned int scale_factor, long lval)
> {
> u32 nval;
> @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata
> *ddat, i915_reg_t rgadr,
> /* 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);
> + bits_to_clear = PKG_PWR_LIM_1;
> + bits_to_set = FIELD_PREP(PKG_PWR_LIM_1, nval);
>
> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> bits_to_clear, bits_to_set);
> @@ -406,7 +406,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;
> ```
> Though I'm not sure if you're planning to add further callsites of
> hwm_field_scale_and_write with different field_masks?
I have reasons for keeping it this way, it's there in the link above if you
are interested.
>
> Alternatively, (without the above diff),
>
> ```
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index c9be1657f03d..6f40f12bcf89 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -8,6 +8,7 @@
> #define _LINUX_BITFIELD_H
>
> #include
> +#include
> #include
>
> /*
> @@ -62,7 +63,7 @@
>
> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
> ({ \
> - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
> + BUILD_BUG_ON_MSG(!__is_constexpr(_mask),\
> _pfx "mask is not constant"); \
> BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero");\
> BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
> ```
> will produce:
> error: call to __compiletime_assert_407 declared with 'error'
> attribute: FIELD_PREP: mask is not constant
>
> I haven't tested if that change is also feasible (on top of fixing
> this specific instance), but I think it might help avoid more of these
> subtleties wrt. __builtin_constant_p that depende heavily on compiler,
> compiler version, optimization level.
Not disagreeing, can do somethin
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
On Thu, Oct 27, 2022 at 9:53 AM Dixit, Ashutosh
wrote:
>
> On Thu, 27 Oct 2022 09:35:24 -0700, Nick Desaulniers wrote:
> >
>
> Hi Nick,
>
> > On Tue, Oct 25, 2022 at 5:18 PM Andi Shyti
> > wrote:
> > >
> > > Hi Ashutosh,
> > >
> > > > But I'd wait to hear from clang/llvm folks first.
> > >
> > > Yeah! Looking forward to getting some ideas :)
> >
> > Gwan-gyeong, which tree and set of configs are necessary to reproduce
> > the observed warning?
> >
> > Warnings are treated as errors, so I don't want this breaking our CI.
>
> The following or equivalent should do it:
>
> git clone https://anongit.freedesktop.org/git/drm/drm-tip
> git checkout drm-tip
>
> Kernel config:
> CONFIG_DRM_I915=m
> CONFIG_HWMON=y
>
> Files:
> drivers/gpu/drm/i915/i915_hwmon.c/.h
>
> Thanks for taking a look.
Thanks, I can repro now.
I haven't detangled the macro soup, but I noticed:
1. FIELD_PREP is defined in include/linux/bitfield.h which has the
following comment:
18 * Mask must be a compilation time constant.
2. hwm_field_scale_and_write only has one callsite.
The following patch works:
```
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
b/drivers/gpu/drm/i915/i915_hwmon.c
index 9e9781493025..6ac29d90b92a 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -101,7 +101,7 @@ 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,
+ int nshift,
unsigned int scale_factor, long lval)
{
u32 nval;
@@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata
*ddat, i915_reg_t rgadr,
/* 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);
+ bits_to_clear = PKG_PWR_LIM_1;
+ bits_to_set = FIELD_PREP(PKG_PWR_LIM_1, nval);
hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
bits_to_clear, bits_to_set);
@@ -406,7 +406,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;
```
Though I'm not sure if you're planning to add further callsites of
hwm_field_scale_and_write with different field_masks?
Alternatively, (without the above diff),
```
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index c9be1657f03d..6f40f12bcf89 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -8,6 +8,7 @@
#define _LINUX_BITFIELD_H
#include
+#include
#include
/*
@@ -62,7 +63,7 @@
#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
({ \
- BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
+ BUILD_BUG_ON_MSG(!__is_constexpr(_mask),\
_pfx "mask is not constant"); \
BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero");\
BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
```
will produce:
error: call to __compiletime_assert_407 declared with 'error'
attribute: FIELD_PREP: mask is not constant
I haven't tested if that change is also feasible (on top of fixing
this specific instance), but I think it might help avoid more of these
subtleties wrt. __builtin_constant_p that depende heavily on compiler,
compiler version, optimization level.
--
Thanks,
~Nick Desaulniers
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
On Thu, 27 Oct 2022 09:35:24 -0700, Nick Desaulniers wrote: > Hi Nick, > On Tue, Oct 25, 2022 at 5:18 PM Andi Shyti wrote: > > > > Hi Ashutosh, > > > > > But I'd wait to hear from clang/llvm folks first. > > > > Yeah! Looking forward to getting some ideas :) > > Gwan-gyeong, which tree and set of configs are necessary to reproduce > the observed warning? > > Warnings are treated as errors, so I don't want this breaking our CI. The following or equivalent should do it: git clone https://anongit.freedesktop.org/git/drm/drm-tip git checkout drm-tip Kernel config: CONFIG_DRM_I915=m CONFIG_HWMON=y Files: drivers/gpu/drm/i915/i915_hwmon.c/.h Thanks for taking a look. -- Ashutosh
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
On Tue, Oct 25, 2022 at 5:18 PM Andi Shyti wrote: > > Hi Ashutosh, > > > But I'd wait to hear from clang/llvm folks first. > > Yeah! Looking forward to getting some ideas :) Gwan-gyeong, which tree and set of configs are necessary to reproduce the observed warning? Warnings are treated as errors, so I don't want this breaking our CI. -- Thanks, ~Nick Desaulniers
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
Hi Ashutosh,
> On Tue, 25 Oct 2022 02:25:06 -0700, Andi Shyti wrote:
> >
> > Hi Ashutosh,
>
> Hi Andi :)
>
> > > > If a non-constant variable is used as the first argument of the
> > > > FIELD_PREP
> > > > macro, a build error occurs when using the clang compiler.
>
> A "non-constant variable" does not seem to be the cause of the compile
> error with clang, see below.
>
> >
> > > > 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]
> > >
> > > What is 18446744073709551615? You may want to limit the length of this
> > > line
> > > or checkpatch doesn't complain?
> >
> > yeah! I am not a clang user, and this must be some ugly error
> > output. I don't think it makes sense to break it, though.
>
> 18446744073709551615 == ~0ull (see use in __BF_FIELD_CHECK).
I just wonder, then, where this number comes from, looks to me
like an ill formatted constant coming from the compiler
(definitely bigger than a ull).
> >
> > > > 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) >
> > > > \
>
> So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull
> also occurs here):
>
> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>__bf_cast_unsigned(_reg, ~0ull), \
>_pfx "type of reg too small for mask"); \
>
> So it goes through previous checks including the "mask is not constant"
> check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated
> after most optimizations have run" so by that time both compilers (gcc and
> clang) have figured out that even though _mask is coming in as function
> argument it is really the constant below:
>
> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
I also thought that the compiler should have figured it out, but
then why we got that error, and I don't see how
"bf_cast_unsigned(_reg, ~0ull)" could fail.
> But it is not clear why clang chokes on this "type of reg too small for
> mask" check (and gcc doesn't) since everything is u32.
>
> It is for this reason I want someone from llvm to chime in.
>
> > > >
> > > > ~~^~~
> > > > ./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))
> > > > \
> > > >
> > > > Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> > > > Cc: Ashutosh Dixit
> > > > Cc: Anshuman Gupta
> > > > Cc: Andi Shyti
> > > > 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..782a621b1928 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, i9
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
On Tue, 25 Oct 2022 07:30:49 -0700, Jani Nikula wrote:
>
> On Tue, 25 Oct 2022, Jani Nikula wrote:
> > On Tue, 25 Oct 2022, Gwan-gyeong Mun wrote:
> >> If a non-constant variable is used as the first argument of the FIELD_PREP
> >> macro, a build error occurs when using the clang compiler.
> >>
> >> Fix the following build error used with 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)) \
> >>
> >> Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> >> Cc: Ashutosh Dixit
> >> Cc: Anshuman Gupta
> >> Cc: Andi Shyti
> >> 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..782a621b1928 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);
> >
> > Please just switch to REG_FIELD_PREP() and it should be fine.
>
> Actually, probably not, but please switch to it anyway. ;)
This is what happens with REG_FIELD_PREP(), that is why we went ahead with
FIELD_PREP(). So REG_FIELD_PREP is not an option.
CC [M] drivers/gpu/drm/i915/i915_hwmon.o
In file included from ./include/linux/bits.h:22,
from ./include/linux/bitops.h:6,
from ./include/linux/hwmon.h:15,
from drivers/gpu/drm/i915/i915_hwmon.c:6:
drivers/gpu/drm/i915/i915_hwmon.c: In function ‘hwm_field_scale_and_write’:
./include/linux/build_bug.h:16:51: error: negative width in bit-field
‘’
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./drivers/gpu/drm/i915/i915_reg_defs.h:72:16: note: in expansion of macro
‘BUILD_BUG_ON_ZERO’
72 |BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) +
\
|^
drivers/gpu/drm/i915/i915_hwmon.c:115:23: note: in expansion of macro
‘REG_FIELD_PREP’
115 | bits_to_set = REG_FIELD_PREP(field_msk, nval);
| ^~
./include/linux/build_bug.h:16:51: error: bit-fiel
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
On Tue, 25 Oct 2022 02:25:06 -0700, Andi Shyti wrote:
>
> Hi Ashutosh,
Hi Andi :)
> > > If a non-constant variable is used as the first argument of the FIELD_PREP
> > > macro, a build error occurs when using the clang compiler.
A "non-constant variable" does not seem to be the cause of the compile
error with clang, see below.
>
> > > 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]
> >
> > What is 18446744073709551615? You may want to limit the length of this line
> > or checkpatch doesn't complain?
>
> yeah! I am not a clang user, and this must be some ugly error
> output. I don't think it makes sense to break it, though.
18446744073709551615 == ~0ull (see use in __BF_FIELD_CHECK).
>
> > > 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) > \
So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull
also occurs here):
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
So it goes through previous checks including the "mask is not constant"
check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated
after most optimizations have run" so by that time both compilers (gcc and
clang) have figured out that even though _mask is coming in as function
argument it is really the constant below:
#define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
But it is not clear why clang chokes on this "type of reg too small for
mask" check (and gcc doesn't) since everything is u32.
It is for this reason I want someone from llvm to chime in.
> > > ~~^~~
> > > ./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)) \
> > >
> > > Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> > > Cc: Ashutosh Dixit
> > > Cc: Anshuman Gupta
> > > Cc: Andi Shyti
> > > 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..782a621b1928 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_
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
Start of lore thread for context:
https://lore.kernel.org/intel-gfx/20221024210953.1572998-1-gwan-gyeong@intel.com/
On Tue, Oct 25, 2022 at 2:25 AM Andi Shyti wrote:
>
> Hi Ashutosh,
>
> > > 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]
> >
> > What is 18446744073709551615? You may want to limit the length of this line
> > or checkpatch doesn't complain?
>
> yeah! I am not a clang user, and this must be some ugly error
> output. I don't think it makes sense to break it, though.
>
> > > 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)) \
> > >
> > > Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> > > Cc: Ashutosh Dixit
> > > Cc: Anshuman Gupta
> > > Cc: Andi Shyti
> > > 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..782a621b1928 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,
> > > + FIELD_PREP(PKG_PWR_LIM_1, nval));
> >
> > I don't want to give up so easily. We might have future uses for the
> > function where we want field_msk to be passed into the function (rather
> > than set inside the function as in this patch).
> >
> > Do we understand what clang is complaining about? And why this compiles
> > with gcc?
>
> Because we are not compiling the builtin functions with gcc but
> gcc has support for them. The FIELD_PREP checks if the first
> parameter is a constant:
>
> BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),
>
> where _mask was our field_mask, but we ignore it. Apparently
> clang doesn't.
So we've been in this code before. I'm having vague memories of
commit 444da3f52407 ("bitfield.h: don't compile-time validate _val in
FIELD_FIT")
But looking at the first __builtin_constant_p check in
__BF_FIELD_CHECK, I'm curious
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
On Tue, 25 Oct 2022, Jani Nikula wrote:
> On Tue, 25 Oct 2022, Gwan-gyeong Mun wrote:
>> If a non-constant variable is used as the first argument of the FIELD_PREP
>> macro, a build error occurs when using the clang compiler.
>>
>> Fix the following build error used with 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)) \
>>
>> Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
>> Cc: Ashutosh Dixit
>> Cc: Anshuman Gupta
>> Cc: Andi Shyti
>> 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..782a621b1928 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);
>
> Please just switch to REG_FIELD_PREP() and it should be fine.
Actually, probably not, but please switch to it anyway. ;)
>
> BR,
> Jani.
>
>
>> -
>> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
>> -bits_to_clear, bits_to_set);
>> +PKG_PWR_LIM_1,
>> +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;
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
On Tue, 25 Oct 2022, Gwan-gyeong Mun wrote:
> If a non-constant variable is used as the first argument of the FIELD_PREP
> macro, a build error occurs when using the clang compiler.
>
> Fix the following build error used with 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)) \
>
> Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> Cc: Ashutosh Dixit
> Cc: Anshuman Gupta
> Cc: Andi Shyti
> 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..782a621b1928 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);
Please just switch to REG_FIELD_PREP() and it should be fine.
BR,
Jani.
> -
> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> - bits_to_clear, bits_to_set);
> + PKG_PWR_LIM_1,
> + 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;
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
Hi Ashutosh,
> > 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]
>
> What is 18446744073709551615? You may want to limit the length of this line
> or checkpatch doesn't complain?
yeah! I am not a clang user, and this must be some ugly error
output. I don't think it makes sense to break it, though.
> > 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)) \
> >
> > Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> > Cc: Ashutosh Dixit
> > Cc: Anshuman Gupta
> > Cc: Andi Shyti
> > 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..782a621b1928 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,
> > + FIELD_PREP(PKG_PWR_LIM_1, nval));
>
> I don't want to give up so easily. We might have future uses for the
> function where we want field_msk to be passed into the function (rather
> than set inside the function as in this patch).
>
> Do we understand what clang is complaining about? And why this compiles
> with gcc?
Because we are not compiling the builtin functions with gcc but
gcc has support for them. The FIELD_PREP checks if the first
parameter is a constant:
BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),
where _mask was our field_mask, but we ignore it. Apparently
clang doesn't.
If we want to stick to gcc only, then I still think the patch is
correct for two reasons:
1. it's cleaner
2. we would get on with the job and if one day we will decide
to suppport builtin functions in gcc as well, we will sleep
peacefully :)
> Copying l...@lists.linux.dev too.
maybe llvm folks have a better opinion.
Thanks,
Andi
> Thanks.
> --
> Ashutosh
>
>
> > }
> >
> > /*
> > @@ -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,
> >
Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
On Tue, Oct 25, 2022 at 12:09:53AM +0300, Gwan-gyeong Mun wrote:
> If a non-constant variable is used as the first argument of the FIELD_PREP
> macro, a build error occurs when using the clang compiler.
good catch! FIELD_PREP wants indeed a constant as a first
paramenter, also for gcc.
Reviewed-by: Andi Shyti
Thanks,
Andi
> Fix the following build error used with 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)) \
>
> Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> Cc: Ashutosh Dixit
> Cc: Anshuman Gupta
> Cc: Andi Shyti
> 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..782a621b1928 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,
> + 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] drm/i915/hwmon: Fix a build error used with clang compiler
On Mon, 24 Oct 2022 14:09:53 -0700, Gwan-gyeong Mun wrote:
>
Hi GG,
> If a non-constant variable is used as the first argument of the FIELD_PREP
> macro, a build error occurs when using the clang compiler.
>
> Fix the following build error used with 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]
What is 18446744073709551615? You may want to limit the length of this line
or checkpatch doesn't complain?
> 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)) \
>
> Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> Cc: Ashutosh Dixit
> Cc: Anshuman Gupta
> Cc: Andi Shyti
> 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..782a621b1928 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,
> + FIELD_PREP(PKG_PWR_LIM_1, nval));
I don't want to give up so easily. We might have future uses for the
function where we want field_msk to be passed into the function (rather
than set inside the function as in this patch).
Do we understand what clang is complaining about? And why this compiles
with gcc?
Copying l...@lists.linux.dev too.
Thanks.
--
Ashutosh
> }
>
> /*
> @@ -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
>
[Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
If a non-constant variable is used as the first argument of the FIELD_PREP
macro, a build error occurs when using the clang compiler.
Fix the following build error used with 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)) \
Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
Cc: Ashutosh Dixit
Cc: Anshuman Gupta
Cc: Andi Shyti
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..782a621b1928 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,
+ 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
