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

[Dixit, Ashutosh]

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 = >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 

Thanks.
--
Ashutosh

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

[Andi Shyti]

Hi Badal,

[...]

>  hwm_get_preregistration_info(struct drm_i915_private *i915)
>  {
>   struct i915_hwmon *hwmon = i915->hwmon;
> + struct intel_uncore *uncore = >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.

Other than that:

Reviewed-by: Andi Shyti 

Thanks,
Andi

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

[Tvrtko Ursulin]

On 21/09/2022 01:02, Dixit, Ashutosh wrote:

On Fri, 16 Sep 2022 08:00:50 -0700, Badal Nilawar wrote:


diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon 
b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
index e2974f928e58..bc061238e35c 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -5,3 +5,23 @@ Contact:   dri-devel@lists.freedesktop.org
  Description:  RO. Current Voltage in millivolt.

Only supported for particular Intel i915 graphics platforms.
+
+What:  /sys/devices/.../hwmon/hwmon/power1_max
+Date:  September 2022
+KernelVersion: 6


Maybe we should ask someone but even if we merge this today to drm-tip this
will appear in kernel.org Linus' version only in 6.2. So I think we should
set this as 6.2 on all patches.


Correct, if merged today it will appear in 6.2 so please change to that 
before merging.


As for the date that's harder to predict and I am not really sure how 
best to handle it. Crystal ball predicts February 2023 fwiw so maybe go 
with that for now. Seems less important than the release for me anyway.


Regards,

Tvrtko


Except for this, thanks for making the changes, this is:

Reviewed-by: Ashutosh Dixit