Re: [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 22 Sep 2022 00:08:46 -0700, Gupta, Anshuman wrote:
>

Hi Anshuman,

> On 9/21/2022 8:23 PM, Nilawar, Badal wrote:
> >
> > On 21-09-2022 17:15, Gupta, Anshuman wrote:
> >>
> >>> +static int
> >>> +hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
> >>> +{
> >>> +    struct i915_hwmon *hwmon = ddat->hwmon;
> >>> +
> >>> +    switch (attr) {
> >>> +    case hwmon_power_max:
> >>> +        *val = hwm_field_read_and_scale(ddat,
> >>> +                        hwmon->rg.pkg_rapl_limit,
> >>> +                        PKG_PWR_LIM_1,
> >>> +                        hwmon->scl_shift_power,
> >>> +                        SF_POWER);
> >>> +        return 0;
> >>> +    case hwmon_power_rated_max:
> >>> +        *val = hwm_field_read_and_scale(ddat,
> >>> +                        hwmon->rg.pkg_power_sku,
> >>> +                        PKG_PKG_TDP,It seems a dead code,
> >>> pkg_power_sky register in initialized with
> >> INVALID_MMMIO_REG, why are we exposing this, unless i am missing
> >> something ?
> > Agree that for platforms considered in this series does not support
> > hwmon_power_rated_max. In fact hwm_power_is_visible will not allow to
> > create sysfs entry if pkg_power_sku is not supported. Considering future
> > dgfx platforms we didn't remove this entry. In future for supported
> > platforms we just need to assign valid register to pkg_power_sku.
>
> AFAIU PACKAGE_POWER_SKU reg is valid for both DG1 and DG2 from BSpec:51862
> So we need to define the register.
> See once more comment below,

Thanks for pointing out, I didn't know where to look for it. We will add
it. Thanks to Badal for locating the register too.

> >
> > Regards,
> > Badal
> >> Br,
> >> Anshuman.
> >>> +                        hwmon->scl_shift_power,
> >>> +                        SF_POWER);
> >>> +        return 0;
> >>> +    default:
> >>> +        return -EOPNOTSUPP;
> >>> +    }
> >>> +}
> >>> +
> >>> +static int
> /snip
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >>> b/drivers/gpu/drm/i915/i915_reg.h
> >>> index 1a9bd829fc7e..55c35903adca 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -1807,6 +1807,11 @@
> >>>   #define   POWER_LIMIT_1_MASK        REG_BIT(10)
> >>>   #define   POWER_LIMIT_2_MASK        REG_BIT(11)
> >>> +/*
> >>> + * *_PACKAGE_POWER_SKU - SKU power and timing parameters.
> >>> + */
> >>> +#define   PKG_PKG_TDP            GENMASK_ULL(14, 0)
> Define register above this definition, GENMASK should follow
> by a register.

Will do.

Thanks.
--
Ashutosh