On Sun, 30 Oct 2022 23:37:59 -0700, Gwan-gyeong Mun wrote: > Hi GG,
> On 10/31/22 7:19 AM, Dixit, Ashutosh wrote: > > On Fri, 28 Oct 2022 21:42:30 -0700, Gwan-gyeong Mun wrote: > >> > >> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c > >> b/drivers/gpu/drm/i915/i915_hwmon.c > >> index 9e9781493025..c588a17f97e9 100644 > >> --- a/drivers/gpu/drm/i915/i915_hwmon.c > >> +++ b/drivers/gpu/drm/i915/i915_hwmon.c > >> @@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, > >> i915_reg_t rgadr, > >> > >> static void > >> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, > >> - u32 field_msk, int nshift, > >> - unsigned int scale_factor, long lval) > >> + int nshift, unsigned int scale_factor, long lval) > >> { > >> u32 nval; > >> - u32 bits_to_clear; > >> - u32 bits_to_set; > >> > >> /* Computation in 64-bits to avoid overflow. Round to nearest. */ > >> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); > >> > >> - bits_to_clear = field_msk; > >> - bits_to_set = FIELD_PREP(field_msk, nval); > >> - > >> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, > >> - bits_to_clear, bits_to_set); > >> + PKG_PWR_LIM_1, > >> + REG_FIELD_PREP(PKG_PWR_LIM_1, > >> nval)); > > > > I registered my objection to this patch already here: > > > > https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.di...@intel.com/ > > > > the crux of which is "hwm_field_scale_and_write() pairs with > > hwm_field_read_and_scale() (they are basically a set/get pair) so it is > > desirable they have identical arguments. This patch breaks that symmetry". > > > > We can merge this patch now but the moment a second caller of > > hwm_field_scale_and_write arrives this patch will need to be reverted. > > > > I have also posted my preferred way (as I previously indiecated) of fixing > > this issue here (if this needs to be fixed in i915): > > > > https://patchwork.freedesktop.org/series/110301/ > > > The given link (https://patchwork.freedesktop.org/series/110301/) is an > inline function that reduces the checks of REG_FIELD_PREP() and pursues the > same functionality. > It's not a good idea to add and use duplicate new inline functions with > reduced functionality under different names. See if you like v2 better :-) > +/* FIELD_PREP and REG_FIELD_PREP require a compile time constant mask */ > +static u32 hwm_field_prep(u32 mask, u32 val) > +{ > + return (val << __bf_shf(mask)) & mask; > +} > + > static void > hwm_locked_with_pm_intel_uncore_rmw(struct hwm_drvdata *ddat, > i915_reg_t reg, u32 clear, u32 set) > @@ -112,7 +118,7 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat, > i915_reg_t rgadr, > nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); > > bits_to_clear = field_msk; > - bits_to_set = FIELD_PREP(field_msk, nval); > + bits_to_set = hwm_field_prep(field_msk, nval); > > hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, > bits_to_clear, bits_to_set); > > > The patch > (https://patchwork.freedesktop.org/patch/509248/?series=110094&rev=5) that > fixed the build error in a simplified form was added as there is only one > place that calls the hwm_field_scale_and_write() function at this time. > > If more places that use the hwm_field_scale_and_write() function are added > in the future, how about changing the interface that calls this function as > Jani previously suggested? Sorry, which interface change which Jani suggested are you referring to? I don't recall seeing anything but maybe I am mistaken. Thanks. -- Ashutosh > > IMO it would be a mistake to use REG_FIELD_PREP or FIELD_PREP here since > > here the mask comes in as a function argument whereas REG_FIELD_PREP and > > FIELD_PREP require that mask to be a compile time constant.