* [PATCH 0/3] PL1 power limit fixes for ATSM @ 2023-02-14 5:33 ` Ashutosh Dixit 0 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx Cc: dri-devel, Anshuman Gupta, Riana Tauro, Badal Nilawar, gwan-gyeong.mun, linux-hwmon Previous PL1 power limit implementation assumed that the PL1 limit is always enabled in HW. However we now find this not to be the case on ATSM where the PL1 limit is disabled at power up. This requires changes in the previous PL1 limit implementation. Submitting 3 patches for easier review but patches can be squashed if needed. Ashutosh Dixit (3): drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write drm/i915/hwmon: Enable PL1 limit when writing limit value to HW drm/i915/hwmon: Expose power1_max_enable .../ABI/testing/sysfs-driver-intel-i915-hwmon | 7 ++ drivers/gpu/drm/i915/i915_hwmon.c | 85 +++++++++++++------ 2 files changed, 68 insertions(+), 24 deletions(-) -- 2.38.0 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [Intel-gfx] [PATCH 0/3] PL1 power limit fixes for ATSM @ 2023-02-14 5:33 ` Ashutosh Dixit 0 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx; +Cc: linux-hwmon, dri-devel Previous PL1 power limit implementation assumed that the PL1 limit is always enabled in HW. However we now find this not to be the case on ATSM where the PL1 limit is disabled at power up. This requires changes in the previous PL1 limit implementation. Submitting 3 patches for easier review but patches can be squashed if needed. Ashutosh Dixit (3): drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write drm/i915/hwmon: Enable PL1 limit when writing limit value to HW drm/i915/hwmon: Expose power1_max_enable .../ABI/testing/sysfs-driver-intel-i915-hwmon | 7 ++ drivers/gpu/drm/i915/i915_hwmon.c | 85 +++++++++++++------ 2 files changed, 68 insertions(+), 24 deletions(-) -- 2.38.0 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 0/3] PL1 power limit fixes for ATSM @ 2023-02-14 5:33 ` Ashutosh Dixit 0 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx Cc: linux-hwmon, Anshuman Gupta, dri-devel, gwan-gyeong.mun, Badal Nilawar, Riana Tauro Previous PL1 power limit implementation assumed that the PL1 limit is always enabled in HW. However we now find this not to be the case on ATSM where the PL1 limit is disabled at power up. This requires changes in the previous PL1 limit implementation. Submitting 3 patches for easier review but patches can be squashed if needed. Ashutosh Dixit (3): drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write drm/i915/hwmon: Enable PL1 limit when writing limit value to HW drm/i915/hwmon: Expose power1_max_enable .../ABI/testing/sysfs-driver-intel-i915-hwmon | 7 ++ drivers/gpu/drm/i915/i915_hwmon.c | 85 +++++++++++++------ 2 files changed, 68 insertions(+), 24 deletions(-) -- 2.38.0 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write 2023-02-14 5:33 ` Ashutosh Dixit (?) @ 2023-02-14 5:33 ` Ashutosh Dixit -1 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx Cc: dri-devel, Anshuman Gupta, Riana Tauro, Badal Nilawar, gwan-gyeong.mun, linux-hwmon hwm_field_scale_and_write has a single caller hwm_power_write and is specific to hwm_power_write but makes it appear that it is a general function which can have multiple callers. Replace the function with hwm_power_max_write which is specific to hwm_power_write and use that in future patches where the function needs to be extended. Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 36 ++++++++++++++----------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 1225bc432f0d5..85195d61f89c7 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -99,20 +99,6 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr, return mul_u64_u32_shr(reg_value, scale_factor, nshift); } -static void -hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, - int nshift, unsigned int scale_factor, long lval) -{ - u32 nval; - - /* Computation in 64-bits to avoid overflow. Round to nearest. */ - nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); - - hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, - PKG_PWR_LIM_1, - REG_FIELD_PREP(PKG_PWR_LIM_1, nval)); -} - /* * hwm_energy - Obtain energy value * @@ -391,6 +377,21 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) return 0; } +static int +hwm_power_max_write(struct hwm_drvdata *ddat, long val) +{ + struct i915_hwmon *hwmon = ddat->hwmon; + u32 nval; + + /* Computation in 64-bits to avoid overflow. Round to nearest. */ + nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER); + + hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1, + REG_FIELD_PREP(PKG_PWR_LIM_1, nval)); + return 0; +} + static int hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val) { @@ -425,16 +426,11 @@ hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val) static int hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) { - struct i915_hwmon *hwmon = ddat->hwmon; u32 uval; switch (attr) { case hwmon_power_max: - hwm_field_scale_and_write(ddat, - hwmon->rg.pkg_rapl_limit, - hwmon->scl_shift_power, - SF_POWER, val); - return 0; + return hwm_power_max_write(ddat, val); case hwmon_power_crit: uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_POWER); return hwm_pcode_write_i1(ddat->uncore->i915, uval); -- 2.38.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write @ 2023-02-14 5:33 ` Ashutosh Dixit 0 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx; +Cc: linux-hwmon, dri-devel hwm_field_scale_and_write has a single caller hwm_power_write and is specific to hwm_power_write but makes it appear that it is a general function which can have multiple callers. Replace the function with hwm_power_max_write which is specific to hwm_power_write and use that in future patches where the function needs to be extended. Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 36 ++++++++++++++----------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 1225bc432f0d5..85195d61f89c7 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -99,20 +99,6 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr, return mul_u64_u32_shr(reg_value, scale_factor, nshift); } -static void -hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, - int nshift, unsigned int scale_factor, long lval) -{ - u32 nval; - - /* Computation in 64-bits to avoid overflow. Round to nearest. */ - nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); - - hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, - PKG_PWR_LIM_1, - REG_FIELD_PREP(PKG_PWR_LIM_1, nval)); -} - /* * hwm_energy - Obtain energy value * @@ -391,6 +377,21 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) return 0; } +static int +hwm_power_max_write(struct hwm_drvdata *ddat, long val) +{ + struct i915_hwmon *hwmon = ddat->hwmon; + u32 nval; + + /* Computation in 64-bits to avoid overflow. Round to nearest. */ + nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER); + + hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1, + REG_FIELD_PREP(PKG_PWR_LIM_1, nval)); + return 0; +} + static int hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val) { @@ -425,16 +426,11 @@ hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val) static int hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) { - struct i915_hwmon *hwmon = ddat->hwmon; u32 uval; switch (attr) { case hwmon_power_max: - hwm_field_scale_and_write(ddat, - hwmon->rg.pkg_rapl_limit, - hwmon->scl_shift_power, - SF_POWER, val); - return 0; + return hwm_power_max_write(ddat, val); case hwmon_power_crit: uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_POWER); return hwm_pcode_write_i1(ddat->uncore->i915, uval); -- 2.38.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write @ 2023-02-14 5:33 ` Ashutosh Dixit 0 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx Cc: linux-hwmon, Anshuman Gupta, dri-devel, gwan-gyeong.mun, Badal Nilawar, Riana Tauro hwm_field_scale_and_write has a single caller hwm_power_write and is specific to hwm_power_write but makes it appear that it is a general function which can have multiple callers. Replace the function with hwm_power_max_write which is specific to hwm_power_write and use that in future patches where the function needs to be extended. Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 36 ++++++++++++++----------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 1225bc432f0d5..85195d61f89c7 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -99,20 +99,6 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr, return mul_u64_u32_shr(reg_value, scale_factor, nshift); } -static void -hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, - int nshift, unsigned int scale_factor, long lval) -{ - u32 nval; - - /* Computation in 64-bits to avoid overflow. Round to nearest. */ - nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); - - hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, - PKG_PWR_LIM_1, - REG_FIELD_PREP(PKG_PWR_LIM_1, nval)); -} - /* * hwm_energy - Obtain energy value * @@ -391,6 +377,21 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) return 0; } +static int +hwm_power_max_write(struct hwm_drvdata *ddat, long val) +{ + struct i915_hwmon *hwmon = ddat->hwmon; + u32 nval; + + /* Computation in 64-bits to avoid overflow. Round to nearest. */ + nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER); + + hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1, + REG_FIELD_PREP(PKG_PWR_LIM_1, nval)); + return 0; +} + static int hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val) { @@ -425,16 +426,11 @@ hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val) static int hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) { - struct i915_hwmon *hwmon = ddat->hwmon; u32 uval; switch (attr) { case hwmon_power_max: - hwm_field_scale_and_write(ddat, - hwmon->rg.pkg_rapl_limit, - hwmon->scl_shift_power, - SF_POWER, val); - return 0; + return hwm_power_max_write(ddat, val); case hwmon_power_crit: uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_POWER); return hwm_pcode_write_i1(ddat->uncore->i915, uval); -- 2.38.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 2/3] drm/i915/hwmon: Enable PL1 limit when writing limit value to HW 2023-02-14 5:33 ` Ashutosh Dixit (?) @ 2023-02-14 5:33 ` Ashutosh Dixit -1 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx Cc: dri-devel, Anshuman Gupta, Riana Tauro, Badal Nilawar, gwan-gyeong.mun, linux-hwmon Previous documentation suggested that the PL1 power limit is always enabled in HW. However we now find this not to be the case on some platforms (such as ATSM). Therefore enable the PL1 power limit (by setting the enable bit) when writing the PL1 limit value to HW. Bspec: 51864 Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 85195d61f89c7..7c20a6f47b92e 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -385,10 +385,11 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) /* Computation in 64-bits to avoid overflow. Round to nearest. */ nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER); + nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval); hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, - PKG_PWR_LIM_1, - REG_FIELD_PREP(PKG_PWR_LIM_1, nval)); + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, + nval); return 0; } -- 2.38.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Intel-gfx] [PATCH 2/3] drm/i915/hwmon: Enable PL1 limit when writing limit value to HW @ 2023-02-14 5:33 ` Ashutosh Dixit 0 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx; +Cc: linux-hwmon, dri-devel Previous documentation suggested that the PL1 power limit is always enabled in HW. However we now find this not to be the case on some platforms (such as ATSM). Therefore enable the PL1 power limit (by setting the enable bit) when writing the PL1 limit value to HW. Bspec: 51864 Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 85195d61f89c7..7c20a6f47b92e 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -385,10 +385,11 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) /* Computation in 64-bits to avoid overflow. Round to nearest. */ nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER); + nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval); hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, - PKG_PWR_LIM_1, - REG_FIELD_PREP(PKG_PWR_LIM_1, nval)); + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, + nval); return 0; } -- 2.38.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 2/3] drm/i915/hwmon: Enable PL1 limit when writing limit value to HW @ 2023-02-14 5:33 ` Ashutosh Dixit 0 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx Cc: linux-hwmon, Anshuman Gupta, dri-devel, gwan-gyeong.mun, Badal Nilawar, Riana Tauro Previous documentation suggested that the PL1 power limit is always enabled in HW. However we now find this not to be the case on some platforms (such as ATSM). Therefore enable the PL1 power limit (by setting the enable bit) when writing the PL1 limit value to HW. Bspec: 51864 Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 85195d61f89c7..7c20a6f47b92e 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -385,10 +385,11 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) /* Computation in 64-bits to avoid overflow. Round to nearest. */ nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER); + nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval); hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, - PKG_PWR_LIM_1, - REG_FIELD_PREP(PKG_PWR_LIM_1, nval)); + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, + nval); return 0; } -- 2.38.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable 2023-02-14 5:33 ` Ashutosh Dixit (?) @ 2023-02-14 5:33 ` Ashutosh Dixit -1 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx Cc: dri-devel, Anshuman Gupta, Riana Tauro, Badal Nilawar, gwan-gyeong.mun, linux-hwmon 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@intel.com> --- .../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 +Date: May 2023 +KernelVersion: 6.3 +Contact: intel-gfx@lists.freedesktop.org +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; -- 2.38.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-14 5:33 ` Ashutosh Dixit 0 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx; +Cc: linux-hwmon, dri-devel 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@intel.com> --- .../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 +Date: May 2023 +KernelVersion: 6.3 +Contact: intel-gfx@lists.freedesktop.org +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; -- 2.38.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-14 5:33 ` Ashutosh Dixit 0 siblings, 0 replies; 29+ messages in thread From: Ashutosh Dixit @ 2023-02-14 5:33 UTC (permalink / raw) To: intel-gfx Cc: linux-hwmon, Anshuman Gupta, dri-devel, gwan-gyeong.mun, Badal Nilawar, Riana Tauro 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@intel.com> --- .../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 +Date: May 2023 +KernelVersion: 6.3 +Contact: intel-gfx@lists.freedesktop.org +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; -- 2.38.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable 2023-02-14 5:33 ` Ashutosh Dixit (?) @ 2023-02-14 6:16 ` Guenter Roeck -1 siblings, 0 replies; 29+ messages in thread From: Guenter Roeck @ 2023-02-14 6:16 UTC (permalink / raw) To: Ashutosh Dixit, intel-gfx Cc: dri-devel, Anshuman Gupta, Riana Tauro, Badal Nilawar, gwan-gyeong.mun, linux-hwmon 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@intel.com> > --- > .../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. Guenter > +Date: May 2023 > +KernelVersion: 6.3 > +Contact: intel-gfx@lists.freedesktop.org > +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; ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-14 6:16 ` Guenter Roeck 0 siblings, 0 replies; 29+ messages in thread From: Guenter Roeck @ 2023-02-14 6:16 UTC (permalink / raw) To: Ashutosh Dixit, intel-gfx; +Cc: linux-hwmon, dri-devel 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@intel.com> > --- > .../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. Guenter > +Date: May 2023 > +KernelVersion: 6.3 > +Contact: intel-gfx@lists.freedesktop.org > +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; ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-14 6:16 ` Guenter Roeck 0 siblings, 0 replies; 29+ messages in thread From: Guenter Roeck @ 2023-02-14 6:16 UTC (permalink / raw) To: Ashutosh Dixit, intel-gfx Cc: linux-hwmon, Anshuman Gupta, dri-devel, gwan-gyeong.mun, Badal Nilawar, Riana Tauro 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@intel.com> > --- > .../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. Guenter > +Date: May 2023 > +KernelVersion: 6.3 > +Contact: intel-gfx@lists.freedesktop.org > +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; ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable 2023-02-14 6:16 ` Guenter Roeck (?) @ 2023-02-15 3:11 ` Dixit, Ashutosh -1 siblings, 0 replies; 29+ messages in thread From: Dixit, Ashutosh @ 2023-02-15 3:11 UTC (permalink / raw) To: Guenter Roeck Cc: intel-gfx, dri-devel, Anshuman Gupta, Riana Tauro, Badal Nilawar, gwan-gyeong.mun, linux-hwmon, Rodrigo Vivi, Joonas Lahtinen, Vinay Belgaumkar 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@intel.com> > > --- > > .../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). 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@lists.freedesktop.org > > +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; > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-15 3:11 ` Dixit, Ashutosh 0 siblings, 0 replies; 29+ messages in thread From: Dixit, Ashutosh @ 2023-02-15 3:11 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-hwmon, intel-gfx, dri-devel, Rodrigo Vivi 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@intel.com> > > --- > > .../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). 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@lists.freedesktop.org > > +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; > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-15 3:11 ` Dixit, Ashutosh 0 siblings, 0 replies; 29+ messages in thread From: Dixit, Ashutosh @ 2023-02-15 3:11 UTC (permalink / raw) To: Guenter Roeck Cc: linux-hwmon, Anshuman Gupta, intel-gfx, dri-devel, gwan-gyeong.mun, Badal Nilawar, Rodrigo Vivi, Vinay Belgaumkar, Riana Tauro 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@intel.com> > > --- > > .../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). 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@lists.freedesktop.org > > +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; > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable 2023-02-15 3:11 ` Dixit, Ashutosh (?) @ 2023-02-16 18:57 ` Rodrigo Vivi -1 siblings, 0 replies; 29+ messages in thread From: Rodrigo Vivi @ 2023-02-16 18:57 UTC (permalink / raw) To: Dixit, Ashutosh Cc: Guenter Roeck, intel-gfx, dri-devel, Anshuman Gupta, Riana Tauro, Badal Nilawar, gwan-gyeong.mun, linux-hwmon, Joonas Lahtinen, Vinay Belgaumkar 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@intel.com> > > > --- > > > .../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@lists.freedesktop.org > > > +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; > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-16 18:57 ` Rodrigo Vivi 0 siblings, 0 replies; 29+ messages in thread From: Rodrigo Vivi @ 2023-02-16 18:57 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: linux-hwmon, intel-gfx, dri-devel, Guenter Roeck 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@intel.com> > > > --- > > > .../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@lists.freedesktop.org > > > +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; > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-16 18:57 ` Rodrigo Vivi 0 siblings, 0 replies; 29+ messages in thread From: Rodrigo Vivi @ 2023-02-16 18:57 UTC (permalink / raw) To: Dixit, Ashutosh Cc: linux-hwmon, Anshuman Gupta, intel-gfx, dri-devel, gwan-gyeong.mun, Guenter Roeck, Badal Nilawar, Vinay Belgaumkar, Riana Tauro 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@intel.com> > > > --- > > > .../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@lists.freedesktop.org > > > +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; > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable 2023-02-16 18:57 ` Rodrigo Vivi (?) @ 2023-02-16 19:25 ` Guenter Roeck -1 siblings, 0 replies; 29+ messages in thread From: Guenter Roeck @ 2023-02-16 19:25 UTC (permalink / raw) To: Rodrigo Vivi, Dixit, Ashutosh Cc: intel-gfx, dri-devel, Anshuman Gupta, Riana Tauro, Badal Nilawar, gwan-gyeong.mun, linux-hwmon, Joonas Lahtinen, Vinay Belgaumkar On 2/16/23 10:57, Rodrigo Vivi wrote: > 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@intel.com> >>>> --- >>>> .../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'? > Not really. My suggested solution would have been to use a value of '0' to indicate 'disabled' and document it accordingly. > 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. Yes, sure, freely. You conveniently ignore Do not create non-standard attributes unless really needed. If you have to use non-standard attributes, or you believe you do, discuss it on the mailing list first. Either case, provide a detailed explanation why you need the non-standard attribute(s). Standard attributes are specified in Naming and data format standards for sysfs files. from Documentation/hwmon/submitting-patches.rst. > 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. > Oh, I really don't want to keep arguing, especially after your "freely" above. Do whatever you want, just keep it out of drivers/hwmon. Guenter ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-16 19:25 ` Guenter Roeck 0 siblings, 0 replies; 29+ messages in thread From: Guenter Roeck @ 2023-02-16 19:25 UTC (permalink / raw) To: Rodrigo Vivi, Dixit, Ashutosh; +Cc: linux-hwmon, intel-gfx, dri-devel On 2/16/23 10:57, Rodrigo Vivi wrote: > 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@intel.com> >>>> --- >>>> .../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'? > Not really. My suggested solution would have been to use a value of '0' to indicate 'disabled' and document it accordingly. > 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. Yes, sure, freely. You conveniently ignore Do not create non-standard attributes unless really needed. If you have to use non-standard attributes, or you believe you do, discuss it on the mailing list first. Either case, provide a detailed explanation why you need the non-standard attribute(s). Standard attributes are specified in Naming and data format standards for sysfs files. from Documentation/hwmon/submitting-patches.rst. > 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. > Oh, I really don't want to keep arguing, especially after your "freely" above. Do whatever you want, just keep it out of drivers/hwmon. Guenter ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-16 19:25 ` Guenter Roeck 0 siblings, 0 replies; 29+ messages in thread From: Guenter Roeck @ 2023-02-16 19:25 UTC (permalink / raw) To: Rodrigo Vivi, Dixit, Ashutosh Cc: linux-hwmon, Anshuman Gupta, intel-gfx, dri-devel, gwan-gyeong.mun, Badal Nilawar, Vinay Belgaumkar, Riana Tauro On 2/16/23 10:57, Rodrigo Vivi wrote: > 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@intel.com> >>>> --- >>>> .../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'? > Not really. My suggested solution would have been to use a value of '0' to indicate 'disabled' and document it accordingly. > 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. Yes, sure, freely. You conveniently ignore Do not create non-standard attributes unless really needed. If you have to use non-standard attributes, or you believe you do, discuss it on the mailing list first. Either case, provide a detailed explanation why you need the non-standard attribute(s). Standard attributes are specified in Naming and data format standards for sysfs files. from Documentation/hwmon/submitting-patches.rst. > 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. > Oh, I really don't want to keep arguing, especially after your "freely" above. Do whatever you want, just keep it out of drivers/hwmon. Guenter ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable 2023-02-16 19:25 ` Guenter Roeck (?) @ 2023-02-16 20:13 ` Rodrigo Vivi -1 siblings, 0 replies; 29+ messages in thread From: Rodrigo Vivi @ 2023-02-16 20:13 UTC (permalink / raw) To: Guenter Roeck Cc: Dixit, Ashutosh, intel-gfx, dri-devel, Anshuman Gupta, Riana Tauro, Badal Nilawar, gwan-gyeong.mun, linux-hwmon, Joonas Lahtinen, Vinay Belgaumkar On Thu, Feb 16, 2023 at 11:25:50AM -0800, Guenter Roeck wrote: > On 2/16/23 10:57, Rodrigo Vivi wrote: > > 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@intel.com> > > > > > --- > > > > > .../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'? > > > > Not really. My suggested solution would have been to use a value of '0' > to indicate 'disabled' and document it accordingly. > > > 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. > > Yes, sure, freely. You conveniently ignore > > Do not create non-standard attributes unless really needed. If you have to use > non-standard attributes, or you believe you do, discuss it on the mailing list > first. Either case, provide a detailed explanation why you need the non-standard > attribute(s). Standard attributes are specified in Naming and data format > standards for sysfs files. > > from Documentation/hwmon/submitting-patches.rst. I'm sorry for having missed this part. It is not that I ignored it, I hadn't opened it because the title is on how to get patches "accepted into the hwmon subsystem". I was only reading the docs related to use hwmon in the drivers, not yet at the point were I thought this case was generic enough to get that *into* hwmon subsystem. > > > 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. > > > > Oh, I really don't want to keep arguing, especially after your "freely" > above. Do whatever you want, just keep it out of drivers/hwmon. For the record, I am also not arguing. I'm just trying to understand the rules. I believe hwmon is such a good infra and based on the basic docs I had the impression that the expansion of non-standards was allowed and desireable on non-standard cases and that the contribution to get standard into hwmon would just come on things that it really looks that more devices have in common. > > Guenter > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-16 20:13 ` Rodrigo Vivi 0 siblings, 0 replies; 29+ messages in thread From: Rodrigo Vivi @ 2023-02-16 20:13 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-hwmon, intel-gfx, dri-devel On Thu, Feb 16, 2023 at 11:25:50AM -0800, Guenter Roeck wrote: > On 2/16/23 10:57, Rodrigo Vivi wrote: > > 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@intel.com> > > > > > --- > > > > > .../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'? > > > > Not really. My suggested solution would have been to use a value of '0' > to indicate 'disabled' and document it accordingly. > > > 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. > > Yes, sure, freely. You conveniently ignore > > Do not create non-standard attributes unless really needed. If you have to use > non-standard attributes, or you believe you do, discuss it on the mailing list > first. Either case, provide a detailed explanation why you need the non-standard > attribute(s). Standard attributes are specified in Naming and data format > standards for sysfs files. > > from Documentation/hwmon/submitting-patches.rst. I'm sorry for having missed this part. It is not that I ignored it, I hadn't opened it because the title is on how to get patches "accepted into the hwmon subsystem". I was only reading the docs related to use hwmon in the drivers, not yet at the point were I thought this case was generic enough to get that *into* hwmon subsystem. > > > 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. > > > > Oh, I really don't want to keep arguing, especially after your "freely" > above. Do whatever you want, just keep it out of drivers/hwmon. For the record, I am also not arguing. I'm just trying to understand the rules. I believe hwmon is such a good infra and based on the basic docs I had the impression that the expansion of non-standards was allowed and desireable on non-standard cases and that the contribution to get standard into hwmon would just come on things that it really looks that more devices have in common. > > Guenter > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable @ 2023-02-16 20:13 ` Rodrigo Vivi 0 siblings, 0 replies; 29+ messages in thread From: Rodrigo Vivi @ 2023-02-16 20:13 UTC (permalink / raw) To: Guenter Roeck Cc: linux-hwmon, Anshuman Gupta, intel-gfx, dri-devel, gwan-gyeong.mun, Dixit, Ashutosh, Badal Nilawar, Vinay Belgaumkar, Riana Tauro On Thu, Feb 16, 2023 at 11:25:50AM -0800, Guenter Roeck wrote: > On 2/16/23 10:57, Rodrigo Vivi wrote: > > 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@intel.com> > > > > > --- > > > > > .../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'? > > > > Not really. My suggested solution would have been to use a value of '0' > to indicate 'disabled' and document it accordingly. > > > 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. > > Yes, sure, freely. You conveniently ignore > > Do not create non-standard attributes unless really needed. If you have to use > non-standard attributes, or you believe you do, discuss it on the mailing list > first. Either case, provide a detailed explanation why you need the non-standard > attribute(s). Standard attributes are specified in Naming and data format > standards for sysfs files. > > from Documentation/hwmon/submitting-patches.rst. I'm sorry for having missed this part. It is not that I ignored it, I hadn't opened it because the title is on how to get patches "accepted into the hwmon subsystem". I was only reading the docs related to use hwmon in the drivers, not yet at the point were I thought this case was generic enough to get that *into* hwmon subsystem. > > > 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. > > > > Oh, I really don't want to keep arguing, especially after your "freely" > above. Do whatever you want, just keep it out of drivers/hwmon. For the record, I am also not arguing. I'm just trying to understand the rules. I believe hwmon is such a good infra and based on the basic docs I had the impression that the expansion of non-standards was allowed and desireable on non-standard cases and that the contribution to get standard into hwmon would just come on things that it really looks that more devices have in common. > > Guenter > ^ permalink raw reply [flat|nested] 29+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for PL1 power limit fixes for ATSM 2023-02-14 5:33 ` Ashutosh Dixit ` (4 preceding siblings ...) (?) @ 2023-02-14 6:35 ` Patchwork -1 siblings, 0 replies; 29+ messages in thread From: Patchwork @ 2023-02-14 6:35 UTC (permalink / raw) To: Ashutosh Dixit; +Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 2978 bytes --] == Series Details == Series: PL1 power limit fixes for ATSM URL : https://patchwork.freedesktop.org/series/113984/ State : success == Summary == CI Bug Log - changes from CI_DRM_12735 -> Patchwork_113984v1 ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/index.html Participating hosts (38 -> 36) ------------------------------ Missing (2): fi-snb-2520m fi-pnv-d510 Known issues ------------ Here are the changes found in Patchwork_113984v1 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@i915_selftest@live@gt_heartbeat: - fi-apl-guc: [PASS][1] -> [DMESG-FAIL][2] ([i915#5334]) [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html #### Possible fixes #### * igt@i915_selftest@live@hangcheck: - fi-skl-guc: [DMESG-WARN][3] ([i915#8073]) -> [PASS][4] [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/fi-skl-guc/igt@i915_selftest@live@hangcheck.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/fi-skl-guc/igt@i915_selftest@live@hangcheck.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983 [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334 [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367 [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997 [i915#7609]: https://gitlab.freedesktop.org/drm/intel/issues/7609 [i915#7852]: https://gitlab.freedesktop.org/drm/intel/issues/7852 [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911 [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913 [i915#7981]: https://gitlab.freedesktop.org/drm/intel/issues/7981 [i915#8073]: https://gitlab.freedesktop.org/drm/intel/issues/8073 Build changes ------------- * Linux: CI_DRM_12735 -> Patchwork_113984v1 CI-20190529: 20190529 CI_DRM_12735: e725cf8c052d7cbf1170a5b4ad7e10667cc225b7 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_7160: 45da871dd2684227e93a2fc002b87dfc58bd5fd9 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_113984v1: e725cf8c052d7cbf1170a5b4ad7e10667cc225b7 @ git://anongit.freedesktop.org/gfx-ci/linux ### Linux commits edb533df0fd7 drm/i915/hwmon: Expose power1_max_enable 6652d79affae drm/i915/hwmon: Enable PL1 limit when writing limit value to HW a2c4bf867130 drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/index.html [-- Attachment #2: Type: text/html, Size: 3077 bytes --] ^ permalink raw reply [flat|nested] 29+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for PL1 power limit fixes for ATSM 2023-02-14 5:33 ` Ashutosh Dixit ` (5 preceding siblings ...) (?) @ 2023-02-14 8:27 ` Patchwork -1 siblings, 0 replies; 29+ messages in thread From: Patchwork @ 2023-02-14 8:27 UTC (permalink / raw) To: Ashutosh Dixit; +Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 21355 bytes --] == Series Details == Series: PL1 power limit fixes for ATSM URL : https://patchwork.freedesktop.org/series/113984/ State : success == Summary == CI Bug Log - changes from CI_DRM_12735_full -> Patchwork_113984v1_full ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/index.html Participating hosts (11 -> 10) ------------------------------ Missing (1): shard-rkl0 New tests --------- New tests have been introduced between CI_DRM_12735_full and Patchwork_113984v1_full: ### New IGT tests (1) ### * igt@kms_cursor_edge_walk@64x64-top-bottom@pipe-b-edp-1: - Statuses : 1 pass(s) - Exec time: [0.0] s Known issues ------------ Here are the changes found in Patchwork_113984v1_full that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_exec_fair@basic-pace-solo@rcs0: - shard-apl: [PASS][1] -> [FAIL][2] ([i915#2842]) [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-apl4/igt@gem_exec_fair@basic-pace-solo@rcs0.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-apl1/igt@gem_exec_fair@basic-pace-solo@rcs0.html * igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions: - shard-apl: [PASS][3] -> [FAIL][4] ([i915#2346]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-apl4/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-apl1/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions.html * igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-hdmi-a-1: - shard-snb: NOTRUN -> [SKIP][5] ([fdo#109271]) +24 similar issues [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-snb1/igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-hdmi-a-1.html * igt@kms_setmode@basic@pipe-a-hdmi-a-1: - shard-snb: NOTRUN -> [FAIL][6] ([i915#5465]) +1 similar issue [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-snb1/igt@kms_setmode@basic@pipe-a-hdmi-a-1.html #### Possible fixes #### * igt@drm_fdinfo@virtual-idle: - {shard-rkl}: [FAIL][7] ([i915#7742]) -> [PASS][8] +1 similar issue [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-6/igt@drm_fdinfo@virtual-idle.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-1/igt@drm_fdinfo@virtual-idle.html * igt@gem_ctx_exec@basic-nohangcheck: - {shard-rkl}: [FAIL][9] ([i915#6268]) -> [PASS][10] [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-3/igt@gem_ctx_exec@basic-nohangcheck.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-6/igt@gem_ctx_exec@basic-nohangcheck.html * igt@gem_exec_fair@basic-none-solo@rcs0: - shard-apl: [FAIL][11] ([i915#2842]) -> [PASS][12] +1 similar issue [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-apl3/igt@gem_exec_fair@basic-none-solo@rcs0.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-apl7/igt@gem_exec_fair@basic-none-solo@rcs0.html * igt@gem_exec_fair@basic-none@vcs0: - {shard-rkl}: [FAIL][13] ([i915#2842]) -> [PASS][14] +3 similar issues [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-4/igt@gem_exec_fair@basic-none@vcs0.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-5/igt@gem_exec_fair@basic-none@vcs0.html * igt@gem_exec_reloc@basic-cpu-gtt: - {shard-rkl}: [SKIP][15] ([i915#3281]) -> [PASS][16] +5 similar issues [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-4/igt@gem_exec_reloc@basic-cpu-gtt.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-5/igt@gem_exec_reloc@basic-cpu-gtt.html * igt@gem_lmem_swapping@random-engines@lmem0: - {shard-dg1}: [DMESG-WARN][17] -> [PASS][18] [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-dg1-15/igt@gem_lmem_swapping@random-engines@lmem0.html [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-dg1-18/igt@gem_lmem_swapping@random-engines@lmem0.html * igt@gem_ppgtt@blt-vs-render-ctxn: - shard-snb: [DMESG-FAIL][19] -> [PASS][20] [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-snb1/igt@gem_ppgtt@blt-vs-render-ctxn.html [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-snb7/igt@gem_ppgtt@blt-vs-render-ctxn.html * igt@gem_userptr_blits@forbidden-operations: - {shard-rkl}: [SKIP][21] ([i915#3282]) -> [PASS][22] +1 similar issue [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-4/igt@gem_userptr_blits@forbidden-operations.html [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-5/igt@gem_userptr_blits@forbidden-operations.html * igt@gen9_exec_parse@bb-chained: - {shard-rkl}: [SKIP][23] ([i915#2527]) -> [PASS][24] +1 similar issue [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-4/igt@gen9_exec_parse@bb-chained.html [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-5/igt@gen9_exec_parse@bb-chained.html * igt@i915_pm_dc@dc6-dpms: - {shard-rkl}: [SKIP][25] ([i915#3361]) -> [PASS][26] [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-5/igt@i915_pm_dc@dc6-dpms.html [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-1/igt@i915_pm_dc@dc6-dpms.html * igt@i915_selftest@live@gt_pm: - {shard-rkl}: [DMESG-FAIL][27] ([i915#4258]) -> [PASS][28] [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-4/igt@i915_selftest@live@gt_pm.html [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-5/igt@i915_selftest@live@gt_pm.html * igt@kms_big_fb@x-tiled-8bpp-rotate-180: - {shard-tglu}: [SKIP][29] ([i915#1845] / [i915#7651]) -> [PASS][30] +9 similar issues [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-tglu-6/igt@kms_big_fb@x-tiled-8bpp-rotate-180.html [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-tglu-5/igt@kms_big_fb@x-tiled-8bpp-rotate-180.html * igt@kms_big_fb@y-tiled-addfb-size-offset-overflow: - {shard-rkl}: [SKIP][31] ([i915#1845] / [i915#4098]) -> [PASS][32] +12 similar issues [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-2/igt@kms_big_fb@y-tiled-addfb-size-offset-overflow.html [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-6/igt@kms_big_fb@y-tiled-addfb-size-offset-overflow.html * igt@kms_fbcon_fbt@fbc-suspend: - shard-apl: [FAIL][33] ([i915#4767]) -> [PASS][34] [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-apl6/igt@kms_fbcon_fbt@fbc-suspend.html [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-apl6/igt@kms_fbcon_fbt@fbc-suspend.html * igt@kms_flip@flip-vs-expired-vblank-interruptible@c-hdmi-a1: - shard-glk: [FAIL][35] ([i915#79]) -> [PASS][36] [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-glk5/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-hdmi-a1.html [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-glk9/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-hdmi-a1.html * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt: - {shard-tglu}: [SKIP][37] ([i915#1849]) -> [PASS][38] +4 similar issues [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-tglu-6/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt.html [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-tglu-5/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt.html * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-pwrite: - {shard-rkl}: [SKIP][39] ([i915#1849] / [i915#4098]) -> [PASS][40] +9 similar issues [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-pwrite.html [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-6/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-pwrite.html * igt@kms_plane@plane-panning-bottom-right@pipe-a-planes: - {shard-rkl}: [SKIP][41] ([i915#1849]) -> [PASS][42] +2 similar issues [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-2/igt@kms_plane@plane-panning-bottom-right@pipe-a-planes.html [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-6/igt@kms_plane@plane-panning-bottom-right@pipe-a-planes.html * igt@kms_psr@sprite_mmap_gtt: - {shard-rkl}: [SKIP][43] ([i915#1072]) -> [PASS][44] +1 similar issue [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-3/igt@kms_psr@sprite_mmap_gtt.html [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-6/igt@kms_psr@sprite_mmap_gtt.html * igt@kms_universal_plane@universal-plane-pageflip-windowed-pipe-b: - {shard-rkl}: [SKIP][45] ([i915#4070] / [i915#4098]) -> [PASS][46] [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-2/igt@kms_universal_plane@universal-plane-pageflip-windowed-pipe-b.html [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-6/igt@kms_universal_plane@universal-plane-pageflip-windowed-pipe-b.html * igt@perf@gen8-unprivileged-single-ctx-counters: - {shard-rkl}: [SKIP][47] ([i915#2436]) -> [PASS][48] [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-4/igt@perf@gen8-unprivileged-single-ctx-counters.html [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-5/igt@perf@gen8-unprivileged-single-ctx-counters.html * igt@perf@polling-small-buf: - {shard-rkl}: [FAIL][49] ([i915#1722]) -> [PASS][50] [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-4/igt@perf@polling-small-buf.html [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-5/igt@perf@polling-small-buf.html * igt@prime_vgem@basic-fence-flip: - {shard-rkl}: [SKIP][51] ([fdo#109295] / [i915#3708] / [i915#4098]) -> [PASS][52] [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-rkl-3/igt@prime_vgem@basic-fence-flip.html [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-rkl-6/igt@prime_vgem@basic-fence-flip.html #### Warnings #### * igt@i915_pm_rpm@pm-tiling: - shard-snb: [SKIP][53] ([fdo#109271]) -> [INCOMPLETE][54] ([i915#2295]) [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12735/shard-snb1/igt@i915_pm_rpm@pm-tiling.html [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/shard-snb6/igt@i915_pm_rpm@pm-tiling.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274 [fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279 [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280 [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285 [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289 [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291 [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295 [fdo#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312 [fdo#109314]: https://bugs.freedesktop.org/show_bug.cgi?id=109314 [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315 [fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506 [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642 [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189 [fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723 [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068 [fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614 [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615 [fdo#111644]: https://bugs.freedesktop.org/show_bug.cgi?id=111644 [fdo#111656]: https://bugs.freedesktop.org/show_bug.cgi?id=111656 [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825 [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827 [fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283 [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072 [i915#1257]: https://gitlab.freedesktop.org/drm/intel/issues/1257 [i915#132]: https://gitlab.freedesktop.org/drm/intel/issues/132 [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397 [i915#1722]: https://gitlab.freedesktop.org/drm/intel/issues/1722 [i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825 [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839 [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845 [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849 [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190 [i915#2295]: https://gitlab.freedesktop.org/drm/intel/issues/2295 [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346 [i915#2434]: https://gitlab.freedesktop.org/drm/intel/issues/2434 [i915#2436]: https://gitlab.freedesktop.org/drm/intel/issues/2436 [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437 [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527 [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575 [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582 [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587 [i915#2658]: https://gitlab.freedesktop.org/drm/intel/issues/2658 [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672 [i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681 [i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705 [i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280 [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842 [i915#2856]: https://gitlab.freedesktop.org/drm/intel/issues/2856 [i915#2920]: https://gitlab.freedesktop.org/drm/intel/issues/2920 [i915#3116]: https://gitlab.freedesktop.org/drm/intel/issues/3116 [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281 [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282 [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297 [i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299 [i915#3323]: https://gitlab.freedesktop.org/drm/intel/issues/3323 [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359 [i915#3361]: https://gitlab.freedesktop.org/drm/intel/issues/3361 [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458 [i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469 [i915#3528]: https://gitlab.freedesktop.org/drm/intel/issues/3528 [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539 [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546 [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555 [i915#3558]: https://gitlab.freedesktop.org/drm/intel/issues/3558 [i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591 [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637 [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638 [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689 [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708 [i915#3734]: https://gitlab.freedesktop.org/drm/intel/issues/3734 [i915#3742]: https://gitlab.freedesktop.org/drm/intel/issues/3742 [i915#3804]: https://gitlab.freedesktop.org/drm/intel/issues/3804 [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840 [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886 [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955 [i915#3966]: https://gitlab.freedesktop.org/drm/intel/issues/3966 [i915#3989]: https://gitlab.freedesktop.org/drm/intel/issues/3989 [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070 [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078 [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098 [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103 [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258 [i915#426]: https://gitlab.freedesktop.org/drm/intel/issues/426 [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270 [i915#4281]: https://gitlab.freedesktop.org/drm/intel/issues/4281 [i915#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349 [i915#4387]: https://gitlab.freedesktop.org/drm/intel/issues/4387 [i915#4525]: https://gitlab.freedesktop.org/drm/intel/issues/4525 [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538 [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454 [i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565 [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613 [i915#4767]: https://gitlab.freedesktop.org/drm/intel/issues/4767 [i915#4771]: https://gitlab.freedesktop.org/drm/intel/issues/4771 [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833 [i915#5122]: https://gitlab.freedesktop.org/drm/intel/issues/5122 [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176 [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235 [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286 [i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289 [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325 [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533 [i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439 [i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461 [i915#5465]: https://gitlab.freedesktop.org/drm/intel/issues/5465 [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095 [i915#6227]: https://gitlab.freedesktop.org/drm/intel/issues/6227 [i915#6247]: https://gitlab.freedesktop.org/drm/intel/issues/6247 [i915#6248]: https://gitlab.freedesktop.org/drm/intel/issues/6248 [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268 [i915#6301]: https://gitlab.freedesktop.org/drm/intel/issues/6301 [i915#6335]: https://gitlab.freedesktop.org/drm/intel/issues/6335 [i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433 [i915#6497]: https://gitlab.freedesktop.org/drm/intel/issues/6497 [i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524 [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658 [i915#6590]: https://gitlab.freedesktop.org/drm/intel/issues/6590 [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621 [i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768 [i915#6944]: https://gitlab.freedesktop.org/drm/intel/issues/6944 [i915#6953]: https://gitlab.freedesktop.org/drm/intel/issues/6953 [i915#7037]: https://gitlab.freedesktop.org/drm/intel/issues/7037 [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116 [i915#7118]: https://gitlab.freedesktop.org/drm/intel/issues/7118 [i915#7128]: https://gitlab.freedesktop.org/drm/intel/issues/7128 [i915#7294]: https://gitlab.freedesktop.org/drm/intel/issues/7294 [i915#7330]: https://gitlab.freedesktop.org/drm/intel/issues/7330 [i915#7443]: https://gitlab.freedesktop.org/drm/intel/issues/7443 [i915#7651]: https://gitlab.freedesktop.org/drm/intel/issues/7651 [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697 [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701 [i915#7707]: https://gitlab.freedesktop.org/drm/intel/issues/7707 [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711 [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742 [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828 [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79 [i915#7949]: https://gitlab.freedesktop.org/drm/intel/issues/7949 [i915#7957]: https://gitlab.freedesktop.org/drm/intel/issues/7957 [i915#8152]: https://gitlab.freedesktop.org/drm/intel/issues/8152 Build changes ------------- * Linux: CI_DRM_12735 -> Patchwork_113984v1 CI-20190529: 20190529 CI_DRM_12735: e725cf8c052d7cbf1170a5b4ad7e10667cc225b7 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_7160: 45da871dd2684227e93a2fc002b87dfc58bd5fd9 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_113984v1: e725cf8c052d7cbf1170a5b4ad7e10667cc225b7 @ git://anongit.freedesktop.org/gfx-ci/linux == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/index.html [-- Attachment #2: Type: text/html, Size: 15404 bytes --] ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2023-02-16 20:13 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-14 5:33 [PATCH 0/3] PL1 power limit fixes for ATSM Ashutosh Dixit 2023-02-14 5:33 ` [Intel-gfx] " Ashutosh Dixit 2023-02-14 5:33 ` Ashutosh Dixit 2023-02-14 5:33 ` [PATCH 1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write Ashutosh Dixit 2023-02-14 5:33 ` [Intel-gfx] " Ashutosh Dixit 2023-02-14 5:33 ` Ashutosh Dixit 2023-02-14 5:33 ` [PATCH 2/3] drm/i915/hwmon: Enable PL1 limit when writing limit value to HW Ashutosh Dixit 2023-02-14 5:33 ` [Intel-gfx] " Ashutosh Dixit 2023-02-14 5:33 ` Ashutosh Dixit 2023-02-14 5:33 ` [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable Ashutosh Dixit 2023-02-14 5:33 ` [Intel-gfx] " Ashutosh Dixit 2023-02-14 5:33 ` Ashutosh Dixit 2023-02-14 6:16 ` Guenter Roeck 2023-02-14 6:16 ` [Intel-gfx] " Guenter Roeck 2023-02-14 6:16 ` Guenter Roeck 2023-02-15 3:11 ` Dixit, Ashutosh 2023-02-15 3:11 ` [Intel-gfx] " Dixit, Ashutosh 2023-02-15 3:11 ` Dixit, Ashutosh 2023-02-16 18:57 ` Rodrigo Vivi 2023-02-16 18:57 ` [Intel-gfx] " Rodrigo Vivi 2023-02-16 18:57 ` Rodrigo Vivi 2023-02-16 19:25 ` Guenter Roeck 2023-02-16 19:25 ` [Intel-gfx] " Guenter Roeck 2023-02-16 19:25 ` Guenter Roeck 2023-02-16 20:13 ` Rodrigo Vivi 2023-02-16 20:13 ` [Intel-gfx] " Rodrigo Vivi 2023-02-16 20:13 ` Rodrigo Vivi 2023-02-14 6:35 ` [Intel-gfx] ✓ Fi.CI.BAT: success for PL1 power limit fixes for ATSM Patchwork 2023-02-14 8:27 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.