[Intel-gfx] [PATCH] drm/i915/hwmon: Don't use FIELD_PREP

[Dixit, Ashutosh]

On Tue, 01 Nov 2022 03:58:13 -0700, Jani Nikula wrote:
>
> On Mon, 31 Oct 2022, Ashutosh Dixit <ashutosh.dixit at intel.com> wrote:
> > FIELD_PREP and REG_FIELD_PREP have checks requiring a compile time constant
> > mask. When the mask comes in as the argument of a function these checks can
> > can fail depending on the compiler (gcc vs clang), optimization level,
> > etc. Use a simpler version of FIELD_PREP which skips these checks. The
> > checks are not needed because the mask is formed using REG_GENMASK (so is
> > actually a compile time constant).
> >
> > v2: Split REG_FIELD_PREP into a macro with checks and one without and use
> >     the one without checks in i915_hwmon.c (Gwan-gyeong Mun)
>
> I frankly think you're solving the wrong problem here. See [1].

We can consider the sort of refactoring suggested in [1] in the future,
right now I thought I'll offer what in my opinion is the correct way to fix
the clang compile break incrementally with the current code. But otherwise
feel free to go with whatever you think is the correct course of action for
this issue. Even if we don't fix the issue the clang guys will (as they
have in the past).

Thanks.
--
Ashutosh

> [1] https://lore.kernel.org/r/87leov7yix.fsf@intel.com
>
> >
> > Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7354
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_hwmon.c    |  2 +-
> >  drivers/gpu/drm/i915/i915_reg_defs.h | 17 +++++++++++------
> >  2 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > index 9e97814930254..ae435b035229a 100644
> > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > @@ -112,7 +112,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 = __REG_FIELD_PREP(field_msk, nval);
> >
> >	hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> >					    bits_to_clear, bits_to_set);
> > diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
> > index f1859046a9c48..dddacc8d48928 100644
> > --- a/drivers/gpu/drm/i915/i915_reg_defs.h
> > +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
> > @@ -67,12 +67,17 @@
> >   *
> >   * @return: @__val masked and shifted into the field defined by @__mask.
> >   */
> > -#define REG_FIELD_PREP(__mask, __val)						\
> > -	((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) +	\
> > -	       BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) +		\
> > -	       BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) +		\
> > -	       BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \
> > -	       BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0))))
> > +#define __REG_FIELD_PREP_CHK(__mask, __val) \
> > +	(BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) + \
> > +	 BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) + \
> > +	 BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \
> > +	 BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0)))
> > +
> > +#define __REG_FIELD_PREP(__mask, __val) \
> > +	((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask))))
> > +
> > +#define REG_FIELD_PREP(__mask, __val) \
> > +	(__REG_FIELD_PREP(__mask, __val) + __REG_FIELD_PREP_CHK(__mask, __val))
> >
> >  /**
> >   * REG_FIELD_GET() - Extract a u32 bitfield value