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

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

On Mon, 03 Oct 2022 14:05:14 -0700, Andi Shyti wrote:
>
> Hi Badal,
>
> [...]
>
> >  hwm_get_preregistration_info(struct drm_i915_private *i915)
> >  {
> >	struct i915_hwmon *hwmon = i915->hwmon;
> > +	struct intel_uncore *uncore = &i915->uncore;
> > +	intel_wakeref_t wakeref;
> > +	u32 val_sku_unit;
> >
> > -	if (IS_DG1(i915) || IS_DG2(i915))
> > +	if (IS_DG1(i915) || IS_DG2(i915)) {
> >		hwmon->rg.gt_perf_status = GEN12_RPSTAT1;
> > -	else
> > +		hwmon->rg.pkg_power_sku_unit = PCU_PACKAGE_POWER_SKU_UNIT;
> > +		hwmon->rg.pkg_power_sku = PCU_PACKAGE_POWER_SKU;
> > +		hwmon->rg.pkg_rapl_limit = PCU_PACKAGE_RAPL_LIMIT;
> > +	} else {
> >		hwmon->rg.gt_perf_status = INVALID_MMIO_REG;
> > +		hwmon->rg.pkg_power_sku_unit = INVALID_MMIO_REG;
> > +		hwmon->rg.pkg_power_sku = INVALID_MMIO_REG;
> > +		hwmon->rg.pkg_rapl_limit = INVALID_MMIO_REG;
> > +	}
> > +
> > +	with_intel_runtime_pm(uncore->rpm, wakeref) {
> > +		/*
> > +		 * The contents of register hwmon->rg.pkg_power_sku_unit do not change,
> > +		 * so read it once and store the shift values.
> > +		 */
> > +		if (i915_mmio_reg_valid(hwmon->rg.pkg_power_sku_unit)) {
> > +			val_sku_unit = intel_uncore_read(uncore,
> > +							 hwmon->rg.pkg_power_sku_unit);
> > +		} else {
> > +			val_sku_unit = 0;
> > +		}
>
> please remove the brackets here and, just a small nitpick:
>
> move val_sky_unit inside the "with_intel_runtime_pm()" and
> initialize it to '0', you will save the else statement.

Hi Andi, fixed in v9 of the series.

>
> Other than that:
>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks.
--
Ashutosh