On Tue, Feb 14, 2023 at 07:11:16PM -0800, Dixit, Ashutosh wrote: > On Mon, 13 Feb 2023 22:16:44 -0800, Guenter Roeck wrote: > > > > Hi Guenter, > > > On 2/13/23 21:33, Ashutosh Dixit wrote: > > > On ATSM the PL1 power limit is disabled at power up. The previous uapi > > > assumed that the PL1 limit is always enabled and therefore did not have a > > > notion of a disabled PL1 limit. This results in erroneous PL1 limit values > > > when PL1 limit is disabled. For example at power up, the disabled ATSM PL1 > > > limit is shown as 0 which means a low PL1 limit whereas the limit being > > > disabled actually implies a high effective PL1 limit value. > > > > > > To get round this problem, expose power1_max_enable as a custom hwmon > > > attribute. power1_max_enable can be used in conjunction with power1_max to > > > interpret power1_max (PL1 limit) values correctly. It can also be used to > > > enable/disable the PL1 power limit. > > > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx> > > > --- > > > .../ABI/testing/sysfs-driver-intel-i915-hwmon | 7 +++ > > > drivers/gpu/drm/i915/i915_hwmon.c | 48 +++++++++++++++++-- > > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > > > index 2d6a472eef885..edd94a44b4570 100644 > > > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > > > @@ -18,6 +18,13 @@ Description: RW. Card reactive sustained (PL1/Tau) power limit in microwatts. > > > Only supported for particular Intel i915 graphics > > > platforms. > > > +What: /sys/devices/.../hwmon/hwmon<i>/power1_max_enable > > > > This is not a standard hwmon attribute. The standard attribute would be > > power1_enable. > > > > So from hwmon perspective this is a NACK. > > Thanks for the feedback. I did consider power1_enable but decided to go > with the power1_max_enable custom attribute. Documentation for > power1_enable says it is to "Enable or disable the sensors" but in our case > we are not enabling/disabling sensors (which we don't have any ability to, > neither do we expose any power measurements, only energy from which power > can be derived) but enabling/disabling a "power limit" (a limit beyond > which HW takes steps to limit power). Hi Guenter, are you okay with this explanation to release the previous 'nack'? For me it looks like this case really doesn't fit in the standard ones. But also this made me wonder what are the rules for non-standard cases? I couldn't find any clear guidelines in here: https://docs.kernel.org/hwmon/hwmon-kernel-api.html#driver-provided-sysfs-attributes We are seeing drivers around to freely use non-standard hwmon. Are we free to add non standard ones as long if doesn't fit in the defined standards, or should we really limit the use and do our own thing on our own? I mean, for the new Xe driver I was considering to standardize everything related to freq and power on top of the hwmon instead of separated sysfs files. But this would mean a lot of non-standard stuff on top of a few standard hwmon stuff. But I will hold this plan if you tell me that we should avoid and limit the non-standard cases. > > As mentioned in the commit message, power1_max_enable is exposed to avoid > possible misinterpretations in measured energy in response to the set power > limit (something specific to our HW). We may have multiple such limits in > the future with similar issues and multiplexing enabling/disabling these > power limits via a single power1_enable file will not provide sufficient > granularity for our purposes. > > Also, I had previously posted this patch: > > https://patchwork.freedesktop.org/patch/522612/?series=113972&rev=1 > > which avoids the power1_max_enable file and instead uses a power1_max value > of -1 to indicate that the power1_max limit is disabled. > > So in summary we have the following options: > > 1. Go with power1_max_enable (preferred, works well for us) > 2. Go with -1 to indicate that the power1_max limit is disabled > (non-intuitive and also a little ugly) > 3. Go with power1_enable (possible but confusing because multiple power > limits/entities are multiplexed via a single file) > > If you still think we should not use power1_max_enable I think I might drop > this patch for now (I am trying to preempt future issues but in this case > it's better to wait till people actually complain rather than expose a > non-ideal uapi). > > Even if drop we this patch now, it would be good to know your preference in > case we need to revisit this issue later. > > Thanks and also sorry for the rather long winded email. > > Ashutosh > > > Guenter > > > > > +Date: May 2023 > > > +KernelVersion: 6.3 > > > +Contact: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > +Description: RW. Enable/disable the PL1 power limit (power1_max). > > > + > > > + Only supported for particular Intel i915 graphics platforms. > > > What: /sys/devices/.../hwmon/hwmon<i>/power1_rated_max > > > Date: February 2023 > > > KernelVersion: 6.2 > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > > index 7c20a6f47b92e..5665869d8602b 100644 > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > > @@ -230,13 +230,52 @@ hwm_power1_max_interval_store(struct device *dev, > > > PKG_PWR_LIM_1_TIME, rxy); > > > return count; > > > } > > > +static SENSOR_DEVICE_ATTR_RW(power1_max_interval, hwm_power1_max_interval, 0); > > > -static SENSOR_DEVICE_ATTR(power1_max_interval, 0664, > > > - hwm_power1_max_interval_show, > > > - hwm_power1_max_interval_store, 0); > > > +static ssize_t > > > +hwm_power1_max_enable_show(struct device *dev, struct device_attribute *attr, char *buf) > > > +{ > > > + struct hwm_drvdata *ddat = dev_get_drvdata(dev); > > > + intel_wakeref_t wakeref; > > > + u32 r; > > > + > > > + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) > > > + r = intel_uncore_read(ddat->uncore, ddat->hwmon->rg.pkg_rapl_limit); > > > + > > > + return sysfs_emit(buf, "%u\n", !!(r & PKG_PWR_LIM_1_EN)); > > > +} > > > + > > > +static ssize_t > > > +hwm_power1_max_enable_store(struct device *dev, struct device_attribute *attr, > > > + const char *buf, size_t count) > > > +{ > > > + struct hwm_drvdata *ddat = dev_get_drvdata(dev); > > > + intel_wakeref_t wakeref; > > > + u32 en, r; > > > + bool _en; > > > + int ret; > > > + > > > + ret = kstrtobool(buf, &_en); > > > + if (ret) > > > + return ret; > > > + > > > + en = REG_FIELD_PREP(PKG_PWR_LIM_1_EN, _en); > > > + hwm_locked_with_pm_intel_uncore_rmw(ddat, ddat->hwmon->rg.pkg_rapl_limit, > > > + PKG_PWR_LIM_1_EN, en); > > > + > > > + /* Verify, because PL1 limit cannot be disabled on all platforms */ > > > + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) > > > + r = intel_uncore_read(ddat->uncore, ddat->hwmon->rg.pkg_rapl_limit); > > > + if ((r & PKG_PWR_LIM_1_EN) != en) > > > + return -EPERM; > > > + > > > + return count; > > > +} > > > +static SENSOR_DEVICE_ATTR_RW(power1_max_enable, hwm_power1_max_enable, 0); > > > static struct attribute *hwm_attributes[] = { > > > &sensor_dev_attr_power1_max_interval.dev_attr.attr, > > > + &sensor_dev_attr_power1_max_enable.dev_attr.attr, > > > NULL > > > }; > > > @@ -247,7 +286,8 @@ static umode_t hwm_attributes_visible(struct > > > kobject *kobj, > > > struct hwm_drvdata *ddat = dev_get_drvdata(dev); > > > struct i915_hwmon *hwmon = ddat->hwmon; > > > - if (attr == &sensor_dev_attr_power1_max_interval.dev_attr.attr) > > > + if (attr == &sensor_dev_attr_power1_max_interval.dev_attr.attr || > > > + attr == &sensor_dev_attr_power1_max_enable.dev_attr.attr) > > > return i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit) ? attr->mode : 0; > > > return 0; > >
