| Message ID | 20240809061525.1368153-1-raag.jadav@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v4] drm/i915/hwmon: expose fan speed | expand |
On 09-08-2024 11:45, Raag Jadav wrote: > Add hwmon support for fan1_input attribute, which will expose fan speed > in RPM. With this in place we can monitor fan speed using lm-sensors tool. > > $ sensors > i915-pci-0300 > Adapter: PCI adapter > in0: 653.00 mV > fan1: 3833 RPM > power1: N/A (max = 43.00 W) > energy1: 32.02 kJ > > v2: > - Add mutex protection > - Handle overflow > - Add ABI documentation > - Aesthetic adjustments (Riana) > > v3: > - Declare rotations as "long" and drop redundant casting > - Change date and version in ABI documentation > - Add commenter name in changelog (Riana) > > v4: > - Fix wakeref leak > - Drop switch case and simplify hwm_fan_xx() (Andi) > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > Reviewed-by: Riana Tauro <riana.tauro@intel.com> > --- > .../ABI/testing/sysfs-driver-intel-i915-hwmon | 8 ++ > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 2 + > drivers/gpu/drm/i915/i915_hwmon.c | 81 +++++++++++++++++++ > 3 files changed, 91 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > index 92fe7c5c5ac1..be4141a7522f 100644 > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > @@ -75,3 +75,11 @@ Description: RO. Energy input of device or gt in microjoules. > for the gt. > > Only supported for particular Intel i915 graphics platforms. > + > +What: /sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/fan1_input > +Date: November 2024 Why November? > +KernelVersion: 6.12 > +Contact: intel-gfx@lists.freedesktop.org > +Description: RO. Fan speed of device in RPM. > + > + Only supported for particular Intel i915 graphics platforms. > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > index e42b3a5d4e63..57a3c83d3655 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > @@ -1553,6 +1553,8 @@ > #define VLV_RENDER_C0_COUNT _MMIO(0x138118) > #define VLV_MEDIA_C0_COUNT _MMIO(0x13811c) > > +#define PCU_PWM_FAN_SPEED _MMIO(0x138140) > + > #define GEN12_RPSTAT1 _MMIO(0x1381b4) > #define GEN12_VOLTAGE_MASK REG_GENMASK(10, 0) > #define GEN12_CAGF_MASK REG_GENMASK(19, 11) > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 49db3e09826c..bafa5a11ed0f 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -36,6 +36,7 @@ struct hwm_reg { > i915_reg_t pkg_rapl_limit; > i915_reg_t energy_status_all; > i915_reg_t energy_status_tile; > + i915_reg_t fan_speed; > }; > > struct hwm_energy_info { > @@ -43,11 +44,17 @@ struct hwm_energy_info { > long accum_energy; /* Accumulated energy for energy1_input */ > }; > > +struct hwm_fan_info { > + u32 reg_val_prev; > + u32 time_prev; > +}; > + > struct hwm_drvdata { > struct i915_hwmon *hwmon; > struct intel_uncore *uncore; > struct device *hwmon_dev; > struct hwm_energy_info ei; /* Energy info for energy1_input */ > + struct hwm_fan_info fi; /* Fan info for fan1_input */ > char name[12]; > int gt_n; > bool reset_in_progress; > @@ -276,6 +283,7 @@ static const struct hwmon_channel_info * const hwm_info[] = { > HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT), > HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT), > HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT), > + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT), > NULL > }; > > @@ -613,6 +621,63 @@ hwm_curr_write(struct hwm_drvdata *ddat, u32 attr, long val) > } > } > > +static umode_t > +hwm_fan_is_visible(const struct hwm_drvdata *ddat, u32 attr) > +{ > + struct i915_hwmon *hwmon = ddat->hwmon; > + > + return attr == hwmon_fan_input && > + i915_mmio_reg_valid(hwmon->rg.fan_speed) ? 0444 : 0; > +} > + > +static int > +hwm_fan_read(struct hwm_drvdata *ddat, u32 attr, long *val) > +{ > + struct i915_hwmon *hwmon = ddat->hwmon; > + struct hwm_fan_info *fi = &ddat->fi; > + u32 reg_val, pulses, time, time_now; > + intel_wakeref_t wakeref; > + long rotations; > + int ret = 0; > + > + if (attr != hwmon_fan_input) > + return -EOPNOTSUPP; Using a switch case in rev3 is more logical here. It will also simplify adding more fan attributes in the future. This is why switch cases are used in other parts of the file. Regards, Badal > + > + wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > + mutex_lock(&hwmon->hwmon_lock); > + > + reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.fan_speed); > + time_now = jiffies_to_msecs(jiffies); > + > + /* Handle overflow */ > + if (reg_val >= fi->reg_val_prev) > + pulses = reg_val - fi->reg_val_prev; > + else > + pulses = UINT_MAX - fi->reg_val_prev + reg_val; > + > + /* > + * HW register value is accumulated count of pulses from > + * PWM fan with the scale of 2 pulses per rotation. > + */ > + rotations = pulses >> 1; > + time = time_now - fi->time_prev; > + > + if (unlikely(!time)) { > + ret = -EAGAIN; > + goto exit; > + } > + > + /* Convert to minutes for calculating RPM */ > + *val = DIV_ROUND_UP(rotations * (60 * MSEC_PER_SEC), time); > + > + fi->reg_val_prev = reg_val; > + fi->time_prev = time_now; > +exit: > + mutex_unlock(&hwmon->hwmon_lock); > + intel_runtime_pm_put(ddat->uncore->rpm, wakeref); > + return ret; > +} > + > static umode_t > hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type, > u32 attr, int channel) > @@ -628,6 +693,8 @@ hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type, > return hwm_energy_is_visible(ddat, attr); > case hwmon_curr: > return hwm_curr_is_visible(ddat, attr); > + case hwmon_fan: > + return hwm_fan_is_visible(ddat, attr); > default: > return 0; > } > @@ -648,6 +715,8 @@ hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, > return hwm_energy_read(ddat, attr, val); > case hwmon_curr: > return hwm_curr_read(ddat, attr, val); > + case hwmon_fan: > + return hwm_fan_read(ddat, attr, val); > default: > return -EOPNOTSUPP; > } > @@ -739,12 +808,14 @@ hwm_get_preregistration_info(struct drm_i915_private *i915) > hwmon->rg.pkg_rapl_limit = PCU_PACKAGE_RAPL_LIMIT; > hwmon->rg.energy_status_all = PCU_PACKAGE_ENERGY_STATUS; > hwmon->rg.energy_status_tile = INVALID_MMIO_REG; > + hwmon->rg.fan_speed = PCU_PWM_FAN_SPEED; > } else { > 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; > hwmon->rg.energy_status_all = INVALID_MMIO_REG; > hwmon->rg.energy_status_tile = INVALID_MMIO_REG; > + hwmon->rg.fan_speed = INVALID_MMIO_REG; > } > > with_intel_runtime_pm(uncore->rpm, wakeref) { > @@ -755,6 +826,16 @@ hwm_get_preregistration_info(struct drm_i915_private *i915) > if (i915_mmio_reg_valid(hwmon->rg.pkg_power_sku_unit)) > val_sku_unit = intel_uncore_read(uncore, > hwmon->rg.pkg_power_sku_unit); > + > + /* > + * Store the initial fan register value, so that we can use it for > + * initial fan speed calculation. > + */ > + if (i915_mmio_reg_valid(hwmon->rg.fan_speed)) { > + ddat->fi.reg_val_prev = intel_uncore_read(uncore, > + hwmon->rg.fan_speed); > + ddat->fi.time_prev = jiffies_to_msecs(jiffies); > + } > } > > hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
On Fri, Aug 09, 2024 at 03:03:20PM +0530, Nilawar, Badal wrote: > > > On 09-08-2024 11:45, Raag Jadav wrote: > > Add hwmon support for fan1_input attribute, which will expose fan speed > > in RPM. With this in place we can monitor fan speed using lm-sensors tool. > > > > $ sensors > > i915-pci-0300 > > Adapter: PCI adapter > > in0: 653.00 mV > > fan1: 3833 RPM > > power1: N/A (max = 43.00 W) > > energy1: 32.02 kJ > > > > v2: > > - Add mutex protection > > - Handle overflow > > - Add ABI documentation > > - Aesthetic adjustments (Riana) > > > > v3: > > - Declare rotations as "long" and drop redundant casting > > - Change date and version in ABI documentation > > - Add commenter name in changelog (Riana) > > > > v4: > > - Fix wakeref leak > > - Drop switch case and simplify hwm_fan_xx() (Andi) > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > > Reviewed-by: Riana Tauro <riana.tauro@intel.com> > > --- > > .../ABI/testing/sysfs-driver-intel-i915-hwmon | 8 ++ > > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 2 + > > drivers/gpu/drm/i915/i915_hwmon.c | 81 +++++++++++++++++++ > > 3 files changed, 91 insertions(+) > > > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > > index 92fe7c5c5ac1..be4141a7522f 100644 > > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > > @@ -75,3 +75,11 @@ Description: RO. Energy input of device or gt in microjoules. > > for the gt. > > Only supported for particular Intel i915 graphics platforms. > > + > > +What: /sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/fan1_input > > +Date: November 2024 > Why November? This is expected to land upstream in next cycle right? > > +KernelVersion: 6.12 > > +Contact: intel-gfx@lists.freedesktop.org > > +Description: RO. Fan speed of device in RPM. > > + > > + Only supported for particular Intel i915 graphics platforms. > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > > index e42b3a5d4e63..57a3c83d3655 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > > @@ -1553,6 +1553,8 @@ > > #define VLV_RENDER_C0_COUNT _MMIO(0x138118) > > #define VLV_MEDIA_C0_COUNT _MMIO(0x13811c) > > +#define PCU_PWM_FAN_SPEED _MMIO(0x138140) > > + > > #define GEN12_RPSTAT1 _MMIO(0x1381b4) > > #define GEN12_VOLTAGE_MASK REG_GENMASK(10, 0) > > #define GEN12_CAGF_MASK REG_GENMASK(19, 11) > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > index 49db3e09826c..bafa5a11ed0f 100644 > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > @@ -36,6 +36,7 @@ struct hwm_reg { > > i915_reg_t pkg_rapl_limit; > > i915_reg_t energy_status_all; > > i915_reg_t energy_status_tile; > > + i915_reg_t fan_speed; > > }; > > struct hwm_energy_info { > > @@ -43,11 +44,17 @@ struct hwm_energy_info { > > long accum_energy; /* Accumulated energy for energy1_input */ > > }; > > +struct hwm_fan_info { > > + u32 reg_val_prev; > > + u32 time_prev; > > +}; > > + > > struct hwm_drvdata { > > struct i915_hwmon *hwmon; > > struct intel_uncore *uncore; > > struct device *hwmon_dev; > > struct hwm_energy_info ei; /* Energy info for energy1_input */ > > + struct hwm_fan_info fi; /* Fan info for fan1_input */ > > char name[12]; > > int gt_n; > > bool reset_in_progress; > > @@ -276,6 +283,7 @@ static const struct hwmon_channel_info * const hwm_info[] = { > > HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT), > > HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT), > > HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT), > > + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT), > > NULL > > }; > > @@ -613,6 +621,63 @@ hwm_curr_write(struct hwm_drvdata *ddat, u32 attr, long val) > > } > > } > > +static umode_t > > +hwm_fan_is_visible(const struct hwm_drvdata *ddat, u32 attr) > > +{ > > + struct i915_hwmon *hwmon = ddat->hwmon; > > + > > + return attr == hwmon_fan_input && > > + i915_mmio_reg_valid(hwmon->rg.fan_speed) ? 0444 : 0; > > +} > > + > > +static int > > +hwm_fan_read(struct hwm_drvdata *ddat, u32 attr, long *val) > > +{ > > + struct i915_hwmon *hwmon = ddat->hwmon; > > + struct hwm_fan_info *fi = &ddat->fi; > > + u32 reg_val, pulses, time, time_now; > > + intel_wakeref_t wakeref; > > + long rotations; > > + int ret = 0; > > + > > + if (attr != hwmon_fan_input) > > + return -EOPNOTSUPP; > Using a switch case in rev3 is more logical here. It will also simplify > adding more fan attributes in the future. This is why switch cases are used > in other parts of the file. Current support is for DG1 and DG2, which do not include any other features that can be supported by fan attributes. Raag
Hi Badal, > > +static int > > +hwm_fan_read(struct hwm_drvdata *ddat, u32 attr, long *val) > > +{ > > + struct i915_hwmon *hwmon = ddat->hwmon; > > + struct hwm_fan_info *fi = &ddat->fi; > > + u32 reg_val, pulses, time, time_now; > > + intel_wakeref_t wakeref; > > + long rotations; > > + int ret = 0; > > + > > + if (attr != hwmon_fan_input) > > + return -EOPNOTSUPP; > Using a switch case in rev3 is more logical here. It will also simplify > adding more fan attributes in the future. This is why switch cases are used > in other parts of the file. it was my suggestion and to be honest I would rather prefer it this way. I can understand it if we were expecting more cases in the immediate, like it was in your case. But I wouldn't have an ugly and unreadable one-case-switch in the eventuality that something comes in the future. In that case, we can always convert it. Thanks, Andi
On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote: > Add hwmon support for fan1_input attribute, which will expose fan speed > in RPM. With this in place we can monitor fan speed using lm-sensors tool. > > $ sensors > i915-pci-0300 > Adapter: PCI adapter > in0: 653.00 mV > fan1: 3833 RPM > power1: N/A (max = 43.00 W) > energy1: 32.02 kJ > v2: > - Add mutex protection > - Handle overflow > - Add ABI documentation > - Aesthetic adjustments (Riana) > > v3: > - Declare rotations as "long" and drop redundant casting > - Change date and version in ABI documentation > - Add commenter name in changelog (Riana) > > v4: > - Fix wakeref leak > - Drop switch case and simplify hwm_fan_xx() (Andi) I do not understand why we pollute Git history with changelogs, but it's probably the ugly atavism in DRM workflow. ... > +hwm_fan_is_visible(const struct hwm_drvdata *ddat, u32 attr) > +{ > + struct i915_hwmon *hwmon = ddat->hwmon; > + > + return attr == hwmon_fan_input && > + i915_mmio_reg_valid(hwmon->rg.fan_speed) ? 0444 : 0; Not sure why ternary here, it's not well readable in my opinion. if (attr == hwmon_fan_input && i915_mmio_reg_valid(hwmon->rg.fan_speed)) return 0444; return 0; looks better, no? > +} ... > + /* > + * HW register value is accumulated count of pulses from > + * PWM fan with the scale of 2 pulses per rotation. > + */ > + rotations = pulses >> 1; In accordance with the comment the rotations = pulses / 2; looks better. ... (1) > + time = time_now - fi->time_prev; > + I think location of this blank line is better at (1) above. > + if (unlikely(!time)) { > + ret = -EAGAIN; > + goto exit; > + } ... > + /* Convert to minutes for calculating RPM */ > + *val = DIV_ROUND_UP(rotations * (60 * MSEC_PER_SEC), time); Have you considered to keep jiffies in the fi and use something from jiffies.h here? To me it feels like we multiply and divide when it can be avoided. Please, think about it (I haven't checked myself, just an idea to share). Also comment probably needs to be expanded to explain the formulas behind all this.
On Fri, Aug 09, 2024 at 02:48:08PM +0300, Andy Shevchenko wrote: > On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote: > > Add hwmon support for fan1_input attribute, which will expose fan speed > > in RPM. With this in place we can monitor fan speed using lm-sensors tool. > > > > $ sensors > > i915-pci-0300 > > Adapter: PCI adapter > > in0: 653.00 mV > > fan1: 3833 RPM > > power1: N/A (max = 43.00 W) > > energy1: 32.02 kJ > > > v2: > > - Add mutex protection > > - Handle overflow > > - Add ABI documentation > > - Aesthetic adjustments (Riana) > > > > v3: > > - Declare rotations as "long" and drop redundant casting > > - Change date and version in ABI documentation > > - Add commenter name in changelog (Riana) > > > > v4: > > - Fix wakeref leak > > - Drop switch case and simplify hwm_fan_xx() (Andi) > > I do not understand why we pollute Git history with changelogs, but it's > probably the ugly atavism in DRM workflow. I never liked it! Besides it should even be against the submitting patches recommendation. I don't understand what interest might have someone in a couple of years, reading this commit, knowing an unintellegible list of differences between v2 and v3. I consider it a random pollution of the commit log. Andi
On Fri, Aug 09, 2024 at 02:48:08PM +0300, Andy Shevchenko wrote: > On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote: > > Add hwmon support for fan1_input attribute, which will expose fan speed > > in RPM. With this in place we can monitor fan speed using lm-sensors tool. > > > > $ sensors > > i915-pci-0300 > > Adapter: PCI adapter > > in0: 653.00 mV > > fan1: 3833 RPM > > power1: N/A (max = 43.00 W) > > energy1: 32.02 kJ > > > v2: > > - Add mutex protection > > - Handle overflow > > - Add ABI documentation > > - Aesthetic adjustments (Riana) > > > > v3: > > - Declare rotations as "long" and drop redundant casting > > - Change date and version in ABI documentation > > - Add commenter name in changelog (Riana) > > > > v4: > > - Fix wakeref leak > > - Drop switch case and simplify hwm_fan_xx() (Andi) > > I do not understand why we pollute Git history with changelogs, but it's > probably the ugly atavism in DRM workflow. Yeah I'm still getting used to it. Also welcome back, hope it's not a bad start ;) > ... > > > +hwm_fan_is_visible(const struct hwm_drvdata *ddat, u32 attr) > > +{ > > + struct i915_hwmon *hwmon = ddat->hwmon; > > + > > + return attr == hwmon_fan_input && > > + i915_mmio_reg_valid(hwmon->rg.fan_speed) ? 0444 : 0; > > Not sure why ternary here, it's not well readable in my opinion. > > if (attr == hwmon_fan_input && i915_mmio_reg_valid(hwmon->rg.fan_speed)) > return 0444; > > return 0; > > looks better, no? Andi had a preference for single return statement. I'm personally fine with both. > ... > > > + /* > > + * HW register value is accumulated count of pulses from > > + * PWM fan with the scale of 2 pulses per rotation. > > + */ > > + rotations = pulses >> 1; > > In accordance with the comment the > > rotations = pulses / 2; > > looks better. > > ... > > (1) > > > + time = time_now - fi->time_prev; > > + > > I think location of this blank line is better at (1) above. > > > + if (unlikely(!time)) { > > + ret = -EAGAIN; > > + goto exit; > > + } Sure. > ... > > > + /* Convert to minutes for calculating RPM */ > > + *val = DIV_ROUND_UP(rotations * (60 * MSEC_PER_SEC), time); > > Have you considered to keep jiffies in the fi and use something from jiffies.h > here? To me it feels like we multiply and divide when it can be avoided. > Please, think about it (I haven't checked myself, just an idea to share). Will explore. Raag
On Fri, Aug 09, 2024 at 02:48:08PM +0300, Andy Shevchenko wrote: > On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote: > > > + /* > > + * HW register value is accumulated count of pulses from > > + * PWM fan with the scale of 2 pulses per rotation. > > + */ > > + rotations = pulses >> 1; > > In accordance with the comment the > > rotations = pulses / 2; > > looks better. This change seems to cause a build error in v5. Something to do with __udivdi3 on i386. Should we revert back to shift? Found something similar here, drivers/target/target_core_xcopy.c +697 Raag
On Tue, Aug 13, 2024 at 02:23:13PM +0300, Raag Jadav wrote: > On Fri, Aug 09, 2024 at 02:48:08PM +0300, Andy Shevchenko wrote: > > On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote: ... > > > + /* > > > + * HW register value is accumulated count of pulses from > > > + * PWM fan with the scale of 2 pulses per rotation. > > > + */ > > > + rotations = pulses >> 1; > > > > In accordance with the comment the > > > > rotations = pulses / 2; > > > > looks better. > > This change seems to cause a build error in v5. > Something to do with __udivdi3 on i386. No, it's not this change. Please, read report carefully.
On Tue, Aug 13, 2024 at 02:47:27PM +0300, Andy Shevchenko wrote: > On Tue, Aug 13, 2024 at 02:23:13PM +0300, Raag Jadav wrote: > > On Fri, Aug 09, 2024 at 02:48:08PM +0300, Andy Shevchenko wrote: > > > On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote: > > ... > > > > > + /* > > > > + * HW register value is accumulated count of pulses from > > > > + * PWM fan with the scale of 2 pulses per rotation. > > > > + */ > > > > + rotations = pulses >> 1; > > > > > > In accordance with the comment the > > > > > > rotations = pulses / 2; > > > > > > looks better. > > > > This change seems to cause a build error in v5. > > Something to do with __udivdi3 on i386. > > No, it's not this change. > Please, read report carefully. CI seems to point to DIV_ROUND_UP(), but it's been there since v1. So not sure if I entirely understand. Raag
On Tue, Aug 13, 2024 at 03:53:25PM +0300, Raag Jadav wrote: > On Tue, Aug 13, 2024 at 02:47:27PM +0300, Andy Shevchenko wrote: > > On Tue, Aug 13, 2024 at 02:23:13PM +0300, Raag Jadav wrote: > > > On Fri, Aug 09, 2024 at 02:48:08PM +0300, Andy Shevchenko wrote: > > > > On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote: ... > > > > > + /* > > > > > + * HW register value is accumulated count of pulses from > > > > > + * PWM fan with the scale of 2 pulses per rotation. > > > > > + */ > > > > > + rotations = pulses >> 1; > > > > > > > > In accordance with the comment the > > > > > > > > rotations = pulses / 2; > > > > > > > > looks better. > > > > > > This change seems to cause a build error in v5. > > > Something to do with __udivdi3 on i386. > > > > No, it's not this change. > > Please, read report carefully. > > CI seems to point to DIV_ROUND_UP(), but it's been there since v1. > So not sure if I entirely understand. Yes, that's the issue. You always can reproduce on your side. LKP sent you comprehensive information about their setup.
Hi Andi, On 09-08-2024 15:46, Andi Shyti wrote: > Hi Badal, > >>> +static int >>> +hwm_fan_read(struct hwm_drvdata *ddat, u32 attr, long *val) >>> +{ >>> + struct i915_hwmon *hwmon = ddat->hwmon; >>> + struct hwm_fan_info *fi = &ddat->fi; >>> + u32 reg_val, pulses, time, time_now; >>> + intel_wakeref_t wakeref; >>> + long rotations; >>> + int ret = 0; >>> + >>> + if (attr != hwmon_fan_input) >>> + return -EOPNOTSUPP; >> Using a switch case in rev3 is more logical here. It will also simplify >> adding more fan attributes in the future. This is why switch cases are used >> in other parts of the file. > > it was my suggestion and to be honest I would rather prefer it > this way. I can understand it if we were expecting more cases in > the immediate, like it was in your case. > > But I wouldn't have an ugly and unreadable one-case-switch in the > eventuality that something comes in the future. In that case, we > can always convert it. My rationale for suggesting a switch case is that in the current alignment hwm_XX_read() function is designed to handle all possible/supported attributes of the XX sensor type. With the proposed change, hwm_fan_read() would only manage the hwmon_fan_input attribute. If a single switch case isn’t preferred, I would recommend creating a helper function dedicated to hwmon_fan_input. hwm_fan_read() { if (attr == hwmon_fan_input) return helper(); //hwmon_fan_input_read() return -EOPNOTSUPP; } Regards, Badal > > Thanks, > Andi
On Wed, Aug 14, 2024 at 02:07:44PM +0530, Nilawar, Badal wrote: > > Hi Andi, > > On 09-08-2024 15:46, Andi Shyti wrote: > > Hi Badal, > > > > > > +static int > > > > +hwm_fan_read(struct hwm_drvdata *ddat, u32 attr, long *val) > > > > +{ > > > > + struct i915_hwmon *hwmon = ddat->hwmon; > > > > + struct hwm_fan_info *fi = &ddat->fi; > > > > + u32 reg_val, pulses, time, time_now; > > > > + intel_wakeref_t wakeref; > > > > + long rotations; > > > > + int ret = 0; > > > > + > > > > + if (attr != hwmon_fan_input) > > > > + return -EOPNOTSUPP; > > > Using a switch case in rev3 is more logical here. It will also simplify > > > adding more fan attributes in the future. This is why switch cases are used > > > in other parts of the file. > > > > it was my suggestion and to be honest I would rather prefer it > > this way. I can understand it if we were expecting more cases in > > the immediate, like it was in your case. > > > > But I wouldn't have an ugly and unreadable one-case-switch in the > > eventuality that something comes in the future. In that case, we > > can always convert it. > > My rationale for suggesting a switch case is that in the current alignment > hwm_XX_read() function is designed to handle all possible/supported > attributes of the XX sensor type. > With the proposed change, hwm_fan_read() would only manage the > hwmon_fan_input attribute. > If a single switch case isn’t preferred, I would recommend creating a helper > function dedicated to hwmon_fan_input. > > hwm_fan_read() > { > if (attr == hwmon_fan_input) > return helper(); //hwmon_fan_input_read() > return -EOPNOTSUPP; > } Hi Andi, If you agree with this, please let me know. Will send out a v6 accordingly. Raag
Hi Raag, I'm sorry, I missed this mail. On Mon, Aug 19, 2024 at 09:50:13AM +0300, Raag Jadav wrote: > On Wed, Aug 14, 2024 at 02:07:44PM +0530, Nilawar, Badal wrote: > > On 09-08-2024 15:46, Andi Shyti wrote: > > > > > +static int > > > > > +hwm_fan_read(struct hwm_drvdata *ddat, u32 attr, long *val) > > > > > +{ > > > > > + struct i915_hwmon *hwmon = ddat->hwmon; > > > > > + struct hwm_fan_info *fi = &ddat->fi; > > > > > + u32 reg_val, pulses, time, time_now; > > > > > + intel_wakeref_t wakeref; > > > > > + long rotations; > > > > > + int ret = 0; > > > > > + > > > > > + if (attr != hwmon_fan_input) > > > > > + return -EOPNOTSUPP; > > > > Using a switch case in rev3 is more logical here. It will also simplify > > > > adding more fan attributes in the future. This is why switch cases are used > > > > in other parts of the file. > > > > > > it was my suggestion and to be honest I would rather prefer it > > > this way. I can understand it if we were expecting more cases in > > > the immediate, like it was in your case. > > > > > > But I wouldn't have an ugly and unreadable one-case-switch in the > > > eventuality that something comes in the future. In that case, we > > > can always convert it. > > > > My rationale for suggesting a switch case is that in the current alignment > > hwm_XX_read() function is designed to handle all possible/supported > > attributes of the XX sensor type. > > With the proposed change, hwm_fan_read() would only manage the > > hwmon_fan_input attribute. > > If a single switch case isn’t preferred, I would recommend creating a helper > > function dedicated to hwmon_fan_input. > > > > hwm_fan_read() > > { > > if (attr == hwmon_fan_input) > > return helper(); //hwmon_fan_input_read() I'm not really understanding what is the point of the helper, but if it looks cleaner, I have no objection. Thanks, Andi
On Fri, Aug 09, 2024 at 12:57:54PM +0100, Andi Shyti wrote: > On Fri, Aug 09, 2024 at 02:48:08PM +0300, Andy Shevchenko wrote: > > On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote: > > > Add hwmon support for fan1_input attribute, which will expose fan speed > > > in RPM. With this in place we can monitor fan speed using lm-sensors tool. > > > > > > $ sensors > > > i915-pci-0300 > > > Adapter: PCI adapter > > > in0: 653.00 mV > > > fan1: 3833 RPM > > > power1: N/A (max = 43.00 W) > > > energy1: 32.02 kJ > > > > > v2: > > > - Add mutex protection > > > - Handle overflow > > > - Add ABI documentation > > > - Aesthetic adjustments (Riana) > > > > > > v3: > > > - Declare rotations as "long" and drop redundant casting > > > - Change date and version in ABI documentation > > > - Add commenter name in changelog (Riana) > > > > > > v4: > > > - Fix wakeref leak > > > - Drop switch case and simplify hwm_fan_xx() (Andi) > > > > I do not understand why we pollute Git history with changelogs, but it's > > probably the ugly atavism in DRM workflow. > > I never liked it! Besides it should even be against the > submitting patches recommendation. > > I don't understand what interest might have someone in a couple > of years, reading this commit, knowing an unintellegible list of > differences between v2 and v3. > > I consider it a random pollution of the commit log. Isn't it already documented? Documentation/process/submitting-patches.rst Please put this information **after** the ``---`` line which separates the changelog from the rest of the patch. The version information is not part of the changelog which gets committed to the git tree. It is additional information for the reviewers. If it's placed above the commit tags, it needs manual interaction to remove it. If it is below the separator line, it gets automatically stripped off when applying the patch:: <commit message> ... Signed-off-by: Author <author@mail> --- V2 -> V3: Removed redundant helper function V1 -> V2: Cleaned up coding style and addressed review comments path/to/file | 5+++-- ... Raag > > Andi
On Tue, Aug 20, 2024 at 12:00:27PM +0300, Raag Jadav wrote: > On Fri, Aug 09, 2024 at 12:57:54PM +0100, Andi Shyti wrote: > > On Fri, Aug 09, 2024 at 02:48:08PM +0300, Andy Shevchenko wrote: > > > On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote: > > > > Add hwmon support for fan1_input attribute, which will expose fan speed > > > > in RPM. With this in place we can monitor fan speed using lm-sensors tool. > > > > > > > > $ sensors > > > > i915-pci-0300 > > > > Adapter: PCI adapter > > > > in0: 653.00 mV > > > > fan1: 3833 RPM > > > > power1: N/A (max = 43.00 W) > > > > energy1: 32.02 kJ > > > > > > > v2: > > > > - Add mutex protection > > > > - Handle overflow > > > > - Add ABI documentation > > > > - Aesthetic adjustments (Riana) > > > > > > > > v3: > > > > - Declare rotations as "long" and drop redundant casting > > > > - Change date and version in ABI documentation > > > > - Add commenter name in changelog (Riana) > > > > > > > > v4: > > > > - Fix wakeref leak > > > > - Drop switch case and simplify hwm_fan_xx() (Andi) > > > > > > I do not understand why we pollute Git history with changelogs, but it's > > > probably the ugly atavism in DRM workflow. > > > > I never liked it! Besides it should even be against the > > submitting patches recommendation. > > > > I don't understand what interest might have someone in a couple > > of years, reading this commit, knowing an unintellegible list of > > differences between v2 and v3. > > > > I consider it a random pollution of the commit log. I agree it is ugly. But I don't agree it is just a 'random polution'. I consider a valid and very useful information of the patch history. Very useful for a later cross check to know what exactly version of that patch got merged. Useful for distros on backports as well. > > Isn't it already documented? > Documentation/process/submitting-patches.rst I think it is: "Be sure to tell the reviewers what changes you are making and to thank them for their time. Code review is a tiring and time-consuming process, and reviewers sometimes get grumpy. Even in that case, though, respond politely and address the problems they have pointed out. When sending a next version, add a ``patch changelog`` to the cover letter or to individual patches explaining difference against previous submission " Then: ''' Example of a patch submitted by the From: author:: ''' defines 'changelog' as the block above the signatures. And 'The canonical patch format' also tells that anything after '---' marker line is for "Any additional comments not suitable for the changelog." But well, the important part is to have the version information available for reviewers. Let's try to focus on that instead on visual preferences. > > Please put this information **after** the ``---`` line which separates > the changelog from the rest of the patch. The version information is > not part of the changelog which gets committed to the git tree. It is > additional information for the reviewers. If it's placed above the > commit tags, it needs manual interaction to remove it. If it is below > the separator line, it gets automatically stripped off when applying the > patch:: > > <commit message> > ... > Signed-off-by: Author <author@mail> > --- > V2 -> V3: Removed redundant helper function > V1 -> V2: Cleaned up coding style and addressed review comments > > path/to/file | 5+++-- > ... > > Raag > > > > Andi
On Tue, Aug 20, 2024 at 05:49:23PM -0400, Rodrigo Vivi wrote: > On Tue, Aug 20, 2024 at 12:00:27PM +0300, Raag Jadav wrote: > > On Fri, Aug 09, 2024 at 12:57:54PM +0100, Andi Shyti wrote: > > > On Fri, Aug 09, 2024 at 02:48:08PM +0300, Andy Shevchenko wrote: > > > > On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote: > > > > > Add hwmon support for fan1_input attribute, which will expose fan speed > > > > > in RPM. With this in place we can monitor fan speed using lm-sensors tool. > > > > > > > > > > $ sensors > > > > > i915-pci-0300 > > > > > Adapter: PCI adapter > > > > > in0: 653.00 mV > > > > > fan1: 3833 RPM > > > > > power1: N/A (max = 43.00 W) > > > > > energy1: 32.02 kJ > > > > > > > > > v2: > > > > > - Add mutex protection > > > > > - Handle overflow > > > > > - Add ABI documentation > > > > > - Aesthetic adjustments (Riana) > > > > > > > > > > v3: > > > > > - Declare rotations as "long" and drop redundant casting > > > > > - Change date and version in ABI documentation > > > > > - Add commenter name in changelog (Riana) > > > > > > > > > > v4: > > > > > - Fix wakeref leak > > > > > - Drop switch case and simplify hwm_fan_xx() (Andi) > > > > > > > > I do not understand why we pollute Git history with changelogs, but it's > > > > probably the ugly atavism in DRM workflow. > > > > > > I never liked it! Besides it should even be against the > > > submitting patches recommendation. > > > > > > I don't understand what interest might have someone in a couple > > > of years, reading this commit, knowing an unintellegible list of > > > differences between v2 and v3. > > > > > > I consider it a random pollution of the commit log. > > I agree it is ugly. But I don't agree it is just a 'random polution'. > > I consider a valid and very useful information of the patch history. > Very useful for a later cross check to know what exactly version > of that patch got merged. > Useful for distros on backports as well. Isn't this why we have 'Link' as part of commit which points to actual ML submission? > > > > Isn't it already documented? > > Documentation/process/submitting-patches.rst > > I think it is: > > "Be sure to tell the reviewers what changes you are making and to thank them > for their time. Code review is a tiring and time-consuming process, and > reviewers sometimes get grumpy. Even in that case, though, respond > politely and address the problems they have pointed out. When sending a next > version, add a ``patch changelog`` to the cover letter or to individual patches > explaining difference against previous submission > " > > Then: > > ''' > Example of a patch submitted by the From: author:: > ''' > > defines 'changelog' as the block above the signatures. > > And > > 'The canonical patch format' > > also tells that anything after '---' marker line is for > "Any additional comments not suitable for the changelog." > > But well, the important part is to have the version information > available for reviewers. Can still be available below '---' marker. Raag
On Tue, Aug 20, 2024 at 05:49:23PM -0400, Rodrigo Vivi wrote: > On Tue, Aug 20, 2024 at 12:00:27PM +0300, Raag Jadav wrote: > > On Fri, Aug 09, 2024 at 12:57:54PM +0100, Andi Shyti wrote: > > > On Fri, Aug 09, 2024 at 02:48:08PM +0300, Andy Shevchenko wrote: ... > > > I don't understand what interest might have someone in a couple > > > of years, reading this commit, knowing an unintellegible list of > > > differences between v2 and v3. > > > > > > I consider it a random pollution of the commit log. > > I agree it is ugly. But I don't agree it is just a 'random polution'. Since lore is in place and under the control of LF, this _becomes_ a random pollution. Somebody needs to change some scripts and too conservative workflows :-) > I consider a valid and very useful information of the patch history. So, we have a Link: tag. As long as the changelog was published there it's being archived. > Very useful for a later cross check to know what exactly version > of that patch got merged. > Useful for distros on backports as well.
On Wed, Aug 21, 2024 at 06:52:10AM +0300, Raag Jadav wrote: > On Tue, Aug 20, 2024 at 05:49:23PM -0400, Rodrigo Vivi wrote: > > On Tue, Aug 20, 2024 at 12:00:27PM +0300, Raag Jadav wrote: > > > On Fri, Aug 09, 2024 at 12:57:54PM +0100, Andi Shyti wrote: > > > > On Fri, Aug 09, 2024 at 02:48:08PM +0300, Andy Shevchenko wrote: > > > > > On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote: ... > > > > > I do not understand why we pollute Git history with changelogs, but it's > > > > > probably the ugly atavism in DRM workflow. > > > > > > > > I never liked it! Besides it should even be against the > > > > submitting patches recommendation. > > > > > > > > I don't understand what interest might have someone in a couple > > > > of years, reading this commit, knowing an unintellegible list of > > > > differences between v2 and v3. > > > > > > > > I consider it a random pollution of the commit log. > > > > I agree it is ugly. But I don't agree it is just a 'random polution'. > > > > I consider a valid and very useful information of the patch history. > > Very useful for a later cross check to know what exactly version > > of that patch got merged. > > Useful for distros on backports as well. > > Isn't this why we have 'Link' as part of commit which points to > actual ML submission? > > > > Isn't it already documented? > > > Documentation/process/submitting-patches.rst > > > > I think it is: > > > > "Be sure to tell the reviewers what changes you are making and to thank them > > for their time. Code review is a tiring and time-consuming process, and > > reviewers sometimes get grumpy. Even in that case, though, respond > > politely and address the problems they have pointed out. When sending a next > > version, add a ``patch changelog`` to the cover letter or to individual patches > > explaining difference against previous submission > > " > > > > Then: > > > > ''' > > Example of a patch submitted by the From: author:: > > ''' > > > > defines 'changelog' as the block above the signatures. > > > > And > > > > 'The canonical patch format' > > > > also tells that anything after '---' marker line is for > > "Any additional comments not suitable for the changelog." > > > > But well, the important part is to have the version information > > available for reviewers. > > Can still be available below '---' marker. +1 to what Raag said.
diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon index 92fe7c5c5ac1..be4141a7522f 100644 --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon @@ -75,3 +75,11 @@ Description: RO. Energy input of device or gt in microjoules. for the gt. Only supported for particular Intel i915 graphics platforms. + +What: /sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/fan1_input +Date: November 2024 +KernelVersion: 6.12 +Contact: intel-gfx@lists.freedesktop.org +Description: RO. Fan speed of device in RPM. + + Only supported for particular Intel i915 graphics platforms. diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h index e42b3a5d4e63..57a3c83d3655 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h @@ -1553,6 +1553,8 @@ #define VLV_RENDER_C0_COUNT _MMIO(0x138118) #define VLV_MEDIA_C0_COUNT _MMIO(0x13811c) +#define PCU_PWM_FAN_SPEED _MMIO(0x138140) + #define GEN12_RPSTAT1 _MMIO(0x1381b4) #define GEN12_VOLTAGE_MASK REG_GENMASK(10, 0) #define GEN12_CAGF_MASK REG_GENMASK(19, 11) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 49db3e09826c..bafa5a11ed0f 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -36,6 +36,7 @@ struct hwm_reg { i915_reg_t pkg_rapl_limit; i915_reg_t energy_status_all; i915_reg_t energy_status_tile; + i915_reg_t fan_speed; }; struct hwm_energy_info { @@ -43,11 +44,17 @@ struct hwm_energy_info { long accum_energy; /* Accumulated energy for energy1_input */ }; +struct hwm_fan_info { + u32 reg_val_prev; + u32 time_prev; +}; + struct hwm_drvdata { struct i915_hwmon *hwmon; struct intel_uncore *uncore; struct device *hwmon_dev; struct hwm_energy_info ei; /* Energy info for energy1_input */ + struct hwm_fan_info fi; /* Fan info for fan1_input */ char name[12]; int gt_n; bool reset_in_progress; @@ -276,6 +283,7 @@ static const struct hwmon_channel_info * const hwm_info[] = { HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT), HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT), HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT), + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT), NULL }; @@ -613,6 +621,63 @@ hwm_curr_write(struct hwm_drvdata *ddat, u32 attr, long val) } } +static umode_t +hwm_fan_is_visible(const struct hwm_drvdata *ddat, u32 attr) +{ + struct i915_hwmon *hwmon = ddat->hwmon; + + return attr == hwmon_fan_input && + i915_mmio_reg_valid(hwmon->rg.fan_speed) ? 0444 : 0; +} + +static int +hwm_fan_read(struct hwm_drvdata *ddat, u32 attr, long *val) +{ + struct i915_hwmon *hwmon = ddat->hwmon; + struct hwm_fan_info *fi = &ddat->fi; + u32 reg_val, pulses, time, time_now; + intel_wakeref_t wakeref; + long rotations; + int ret = 0; + + if (attr != hwmon_fan_input) + return -EOPNOTSUPP; + + wakeref = intel_runtime_pm_get(ddat->uncore->rpm); + mutex_lock(&hwmon->hwmon_lock); + + reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.fan_speed); + time_now = jiffies_to_msecs(jiffies); + + /* Handle overflow */ + if (reg_val >= fi->reg_val_prev) + pulses = reg_val - fi->reg_val_prev; + else + pulses = UINT_MAX - fi->reg_val_prev + reg_val; + + /* + * HW register value is accumulated count of pulses from + * PWM fan with the scale of 2 pulses per rotation. + */ + rotations = pulses >> 1; + time = time_now - fi->time_prev; + + if (unlikely(!time)) { + ret = -EAGAIN; + goto exit; + } + + /* Convert to minutes for calculating RPM */ + *val = DIV_ROUND_UP(rotations * (60 * MSEC_PER_SEC), time); + + fi->reg_val_prev = reg_val; + fi->time_prev = time_now; +exit: + mutex_unlock(&hwmon->hwmon_lock); + intel_runtime_pm_put(ddat->uncore->rpm, wakeref); + return ret; +} + static umode_t hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr, int channel) @@ -628,6 +693,8 @@ hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type, return hwm_energy_is_visible(ddat, attr); case hwmon_curr: return hwm_curr_is_visible(ddat, attr); + case hwmon_fan: + return hwm_fan_is_visible(ddat, attr); default: return 0; } @@ -648,6 +715,8 @@ hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, return hwm_energy_read(ddat, attr, val); case hwmon_curr: return hwm_curr_read(ddat, attr, val); + case hwmon_fan: + return hwm_fan_read(ddat, attr, val); default: return -EOPNOTSUPP; } @@ -739,12 +808,14 @@ hwm_get_preregistration_info(struct drm_i915_private *i915) hwmon->rg.pkg_rapl_limit = PCU_PACKAGE_RAPL_LIMIT; hwmon->rg.energy_status_all = PCU_PACKAGE_ENERGY_STATUS; hwmon->rg.energy_status_tile = INVALID_MMIO_REG; + hwmon->rg.fan_speed = PCU_PWM_FAN_SPEED; } else { 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; hwmon->rg.energy_status_all = INVALID_MMIO_REG; hwmon->rg.energy_status_tile = INVALID_MMIO_REG; + hwmon->rg.fan_speed = INVALID_MMIO_REG; } with_intel_runtime_pm(uncore->rpm, wakeref) { @@ -755,6 +826,16 @@ hwm_get_preregistration_info(struct drm_i915_private *i915) if (i915_mmio_reg_valid(hwmon->rg.pkg_power_sku_unit)) val_sku_unit = intel_uncore_read(uncore, hwmon->rg.pkg_power_sku_unit); + + /* + * Store the initial fan register value, so that we can use it for + * initial fan speed calculation. + */ + if (i915_mmio_reg_valid(hwmon->rg.fan_speed)) { + ddat->fi.reg_val_prev = intel_uncore_read(uncore, + hwmon->rg.fan_speed); + ddat->fi.time_prev = jiffies_to_msecs(jiffies); + } } hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);