On Mon, 31 Oct 2022, "Dixit, Ashutosh" <ashutosh.dixit@xxxxxxxxx> 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@xxxxxxxxx/
>> >
>> > 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.
--
Jani Nikula, Intel Open Source Graphics Center
