[PATCH v3 1/5] drm/xe/hwmon: Add support to manage power limits though mailbox
Poosa, Karthik
karthik.poosa at intel.com
Mon May 5 14:02:16 UTC 2025
On 02-05-2025 20:32, Nilawar, Badal wrote:
>
>
> On 01-05-2025 02:06, Karthik Poosa wrote:
>> Add support to manage power limits using pcode mailbox commands
>> for supported platforms.
> Please add Fixes: tag here.
>> Signed-off-by: Karthik Poosa<karthik.poosa at intel.com>
>> ---
>> drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 10 +-
>> drivers/gpu/drm/xe/xe_device_types.h | 4 +
>> drivers/gpu/drm/xe/xe_hwmon.c | 348 ++++++++++++++++++-----
>> drivers/gpu/drm/xe/xe_pci.c | 5 +
>> drivers/gpu/drm/xe/xe_pcode.c | 11 +
>> drivers/gpu/drm/xe/xe_pcode.h | 3 +
>> drivers/gpu/drm/xe/xe_pcode_api.h | 7 +
>> 7 files changed, 307 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> index f5e5234857c1..5394a1373a6b 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> @@ -38,10 +38,10 @@
>> #define TEMP_MASK REG_GENMASK(7, 0)
>>
>> #define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
>> -#define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
>> -#define PKG_PWR_LIM_1_EN REG_BIT(15)
>> -#define PKG_PWR_LIM_1_TIME REG_GENMASK(23, 17)
>> -#define PKG_PWR_LIM_1_TIME_X REG_GENMASK(23, 22)
>> -#define PKG_PWR_LIM_1_TIME_Y REG_GENMASK(21, 17)
>> +#define PWR_LIM_VAL REG_GENMASK(14, 0)
>> +#define PWR_LIM_EN REG_BIT(15)
>> +#define PWR_LIM_TIME REG_GENMASK(23, 17)
>> +#define PWR_LIM_TIME_X REG_GENMASK(23, 22)
>> +#define PWR_LIM_TIME_Y REG_GENMASK(21, 17)
>>
>> #endif /* _XE_MCHBAR_REGS_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index 495bc00ebed4..2c8321eb41b9 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -326,6 +326,10 @@ struct xe_device {
>> u8 has_heci_gscfi:1;
>> /** @info.has_llc: Device has a shared CPU+GPU last level cache */
>> u8 has_llc:1;
>> + /** @info.has_mbx_power_limits: Device has support to manage power limits using
>> + * pcode mailbox commands.
>> + */
>> + u8 has_mbx_power_limits:1;
>> /** @info.has_pxp: Device has PXP support */
>> u8 has_pxp:1;
>> /** @info.has_range_tlb_invalidation: Has range based TLB invalidations */
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>> index eb293aec36a0..b906a1a058e0 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -51,6 +51,14 @@ enum xe_fan_channel {
>> FAN_MAX,
>> };
>>
>> +/*
>> + * For platforms that support mailbox commands for power limits, REG_PKG_POWER_SKU_UNIT is
>> + * not supported and below are SKU units to be used.
>> + */
>> +#define PWR_UNIT 0x3
>> +#define ENERGY_UNIT 0xe
>> +#define TIME_UNIT 0xa
>> +
>> /*
>> * SF_* - scale factors for particular quantities according to hwmon spec.
>> */
>> @@ -60,6 +68,18 @@ enum xe_fan_channel {
>> #define SF_ENERGY 1000000 /* microjoules */
>> #define SF_TIME 1000 /* milliseconds */
>>
>> +/*
>> + * PL*_HWMON_ATTR - mapping of hardware power limits to corresponding hwmon power attribute.
>> + */
>> +#define PL1_HWMON_ATTR hwmon_power_max
>> +
>> +#define PWR_ATTR_TO_STR(attr) (((attr) == hwmon_power_max) ? "PL1" : "Invalid")
>> +
>> +/*
>> + * Timeout for power limit write mailbox command.
>> + */
>> +#define PL_WRITE_MBX_TIMEOUT_MS (1)
>> +
>> /**
>> * struct xe_hwmon_energy_info - to accumulate energy
>> */
>> @@ -100,8 +120,81 @@ struct xe_hwmon {
>> struct xe_hwmon_energy_info ei[CHANNEL_MAX];
>> /** @fi: Fan info for fanN_input */
>> struct xe_hwmon_fan_info fi[FAN_MAX];
>> + /** @boot_power_limit_read: is boot power limits read */
>> + bool boot_power_limit_read;
>> + /** pl1_on_boot: power limit PL1 on boot */
>> + u32 pl1_on_boot[CHANNEL_MAX];
>> };
>>
>> +static int xe_hwmon_pcode_read_power_limit(const struct xe_hwmon *hwmon, u32 attr, int channel,
>> + u32 *uval)
>> +{
>> + struct xe_tile *root_tile = xe_device_get_root_tile(hwmon->xe);
>> + u32 val0 = 0, val1 = 0;
>> + int ret = 0;
>> +
>> + ret = xe_pcode_read(root_tile, PCODE_MBOX(PCODE_POWER_SETUP,
>> + (channel == CHANNEL_CARD) ?
>> + READ_PSYSGPU_POWER_LIMIT :
>> + READ_PACKAGE_POWER_LIMIT,
>> + hwmon->boot_power_limit_read ?
>> + READ_PL_FROM_PCODE : READ_PL_FROM_BIOS),
>> + &val0, &val1);
>> +
>> + if (ret) {
>> + drm_dbg(&hwmon->xe->drm, "read failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
>> + channel, val0, val1, ret);
>> + *uval = 0;
>> + return ret;
>> + }
>> +
>> + /* return the value only if limit is enabled */
>> + if (attr == PL1_HWMON_ATTR)
>> + *uval = (val0 & PWR_LIM_EN) ? val0 : 0;
>> + else if (attr == hwmon_power_label)
>> + *uval = (val0 & PWR_LIM_EN) ? 1 : (val1 & PWR_LIM_EN) ? 1 : 0;
>> + else
>> + *uval = 0;
>> +
>> + return ret;
>> +}
>> +
>> +static int xe_hwmon_pcode_write_power_limit(const struct xe_hwmon *hwmon, u32 attr, u8 channel,
>> + u32 uval)
>> +{
>> + struct xe_tile *root_tile = xe_device_get_root_tile(hwmon->xe);
>> + u32 val0, val1;
>> + int ret = 0;
>> +
>> + ret = xe_pcode_read(root_tile, PCODE_MBOX(PCODE_POWER_SETUP,
>> + (channel == CHANNEL_CARD) ?
>> + READ_PSYSGPU_POWER_LIMIT :
>> + READ_PACKAGE_POWER_LIMIT,
>> + hwmon->boot_power_limit_read ?
>> + READ_PL_FROM_PCODE : READ_PL_FROM_BIOS),
>> + &val0, &val1);
>> +
>> + if (ret)
>> + drm_dbg(&hwmon->xe->drm, "read failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
>> + channel, val0, val1, ret);
>> +
>> + if (attr == PL1_HWMON_ATTR)
>> + val0 = uval;
>> + else
>> + return -EIO;
>> +
>> + drm_dbg(&hwmon->xe->drm, "writing limit val %x channel %d\n", uval, channel);
>> + ret = xe_pcode_write64_timeout(root_tile, PCODE_MBOX(PCODE_POWER_SETUP,
>> + (channel == CHANNEL_CARD) ?
>> + WRITE_PSYSGPU_POWER_LIMIT :
>> + WRITE_PACKAGE_POWER_LIMIT, 1),
> Any specific reason to use param2=1? In mbx doc I don't see param2 is
> needed for power limit write.
This can be removed, it is valid only for read.
>> + val0, val1, PL_WRITE_MBX_TIMEOUT_MS);
>> + if (ret)
>> + drm_dbg(&hwmon->xe->drm, "write failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
>> + channel, val0, val1, ret);
>> + return ret;
>> +}
>> +
>> static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
>> int channel)
>> {
>> @@ -181,7 +274,7 @@ static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg
>> return XE_REG(0);
>> }
>>
>> -#define PL1_DISABLE 0
>> +#define PL_DISABLE 0
>>
>> /*
>> * HW allows arbitrary PL1 limits to be set but silently clamps these values to
>> @@ -189,67 +282,87 @@ static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg
>> * same pattern for sysfs, allow arbitrary PL1 limits to be set but display
>> * clamped values when read.
>> */
>> -static void xe_hwmon_power_max_read(struct xe_hwmon *hwmon, int channel, long *value)
>> +static void xe_hwmon_power_max_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *value)
>> {
>> u64 reg_val, min, max;
>> struct xe_device *xe = hwmon->xe;
>> struct xe_reg rapl_limit, pkg_power_sku;
>> struct xe_mmio *mmio = xe_root_tile_mmio(xe);
>>
>> - rapl_limit = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
>> - pkg_power_sku = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel);
>> + mutex_lock(&hwmon->hwmon_lock);
>>
>> - /*
>> - * Valid check of REG_PKG_RAPL_LIMIT is already done in xe_hwmon_power_is_visible.
>> - * So not checking it again here.
>> - */
>> - if (!xe_reg_is_valid(pkg_power_sku)) {
>> - drm_warn(&xe->drm, "pkg_power_sku invalid\n");
>> - *value = 0;
>> - return;
>> + if (hwmon->xe->info.has_mbx_power_limits) {
>> + xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, (u32 *)®_val);
>> + } else {
>> + rapl_limit = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
>> + pkg_power_sku = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel);
>> +
>> + /*
>> + * Valid check of REG_PKG_RAPL_LIMIT is already done in xe_hwmon_power_is_visible.
>> + * So not checking it again here.
>> + */
>> + if (!xe_reg_is_valid(pkg_power_sku)) {
>> + drm_warn(&xe->drm, "pkg_power_sku invalid\n");
>> + *value = 0;
>> + goto unlock;
>> + }
>> + reg_val = xe_mmio_read32(mmio, rapl_limit);
>> }
>>
>> - mutex_lock(&hwmon->hwmon_lock);
>> -
>> - reg_val = xe_mmio_read32(mmio, rapl_limit);
>> - /* Check if PL1 limit is disabled */
>> - if (!(reg_val & PKG_PWR_LIM_1_EN)) {
>> - *value = PL1_DISABLE;
>> + /* Check if PL limits are disabled */
>> + if (!(reg_val & PWR_LIM_EN)) {
>> + *value = PL_DISABLE;
>> + drm_warn(&hwmon->xe->drm, "%s disabled for channel %d !, val 0x%016llx\n",
>> + PWR_ATTR_TO_STR(attr), channel, reg_val);
>> goto unlock;
>> }
>>
>> - reg_val = REG_FIELD_GET(PKG_PWR_LIM_1, reg_val);
>> + reg_val = REG_FIELD_GET(PWR_LIM_VAL, reg_val);
>> *value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
>>
>> - reg_val = xe_mmio_read64_2x32(mmio, pkg_power_sku);
>> - min = REG_FIELD_GET(PKG_MIN_PWR, reg_val);
>> + if (hwmon->xe->info.has_mbx_power_limits) {
>> + /* No MIN_PWR defined, using boot PL1 as max */
>> + min = 0;
>
> In my opinion, the pcode clips the values according to the BIOS
> settings, so should we simply return value provided by the pcode?
>
I shall check on this.
>> + max = hwmon->pl1_on_boot[channel] & PWR_LIM_VAL;
>> + } else {
>> + reg_val = xe_mmio_read64_2x32(mmio, pkg_power_sku);
>> + min = REG_FIELD_GET(PKG_MIN_PWR, reg_val);
>> + max = REG_FIELD_GET(PKG_MAX_PWR, reg_val);
>> + }
>> +
>> min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
>> - max = REG_FIELD_GET(PKG_MAX_PWR, reg_val);
>> max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power);
>> -
>> if (min && max)
>> *value = clamp_t(u64, *value, min, max);
>> unlock:
>> mutex_unlock(&hwmon->hwmon_lock);
>> }
>>
>> -static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, int channel, long value)
>> +static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, u32 attr, int channel, long value)
>> {
>> struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>> int ret = 0;
>> - u64 reg_val;
>> + u32 reg_val;
>> struct xe_reg rapl_limit;
>>
>> + mutex_lock(&hwmon->hwmon_lock);
>> +
>> rapl_limit = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
>>
>> - mutex_lock(&hwmon->hwmon_lock);
>> + /* Disable Power Limit and verify, as limit cannot be disabled on all platforms */
>> + if (value == PL_DISABLE) {
>> + if (hwmon->xe->info.has_mbx_power_limits) {
>> + drm_dbg(&hwmon->xe->drm, "disabling %s on channel %d\n",
>> + PWR_ATTR_TO_STR(attr), channel);
>> + xe_hwmon_pcode_write_power_limit(hwmon, attr, channel, 0);
>> + xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, ®_val);
>> + } else {
>> + reg_val = xe_mmio_rmw32(mmio, rapl_limit, PWR_LIM_EN, 0);
>> + reg_val = xe_mmio_read32(mmio, rapl_limit);
>> + }
>>
>> - /* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
>> - if (value == PL1_DISABLE) {
>> - reg_val = xe_mmio_rmw32(mmio, rapl_limit, PKG_PWR_LIM_1_EN, 0);
>> - reg_val = xe_mmio_read32(mmio, rapl_limit);
>> - if (reg_val & PKG_PWR_LIM_1_EN) {
>> - drm_warn(&hwmon->xe->drm, "PL1 disable is not supported!\n");
>> + if (reg_val & PWR_LIM_EN) {
>> + drm_warn(&hwmon->xe->drm, "Power limit disable is not supported!\n");
>> ret = -EOPNOTSUPP;
>> }
>> goto unlock;
>> @@ -257,26 +370,38 @@ static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, int channel, long va
>>
>> /* Computation in 64-bits to avoid overflow. Round to nearest. */
>> reg_val = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
>> - reg_val = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, reg_val);
>> - reg_val = xe_mmio_rmw32(mmio, rapl_limit, PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
>> + reg_val = PWR_LIM_EN | REG_FIELD_PREP(PWR_LIM_VAL, reg_val);
>>
>> + if (hwmon->xe->info.has_mbx_power_limits)
>> + ret = xe_hwmon_pcode_write_power_limit(hwmon, attr, channel, reg_val);
>> + else
>> + reg_val = xe_mmio_rmw32(mmio, rapl_limit, PWR_LIM_EN | PWR_LIM_VAL,
>> + reg_val);
>> unlock:
>> mutex_unlock(&hwmon->hwmon_lock);
>> return ret;
>> }
>>
>> -static void xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, int channel, long *value)
>> +static void xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, u32 attr, int channel,
>> + long *value)
>> {
>> struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>> - struct xe_reg reg = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel);
>> - u64 reg_val;
>> + u32 reg_val;
>> +
>> + if (hwmon->xe->info.has_mbx_power_limits) {
>> + /* PL1 is rated max if supported */
>> + xe_hwmon_pcode_read_power_limit(hwmon, PL1_HWMON_ATTR, channel, ®_val);
>> + } else {
>> + /*
>> + * This sysfs file won't be visible if REG_PKG_POWER_SKU is invalid, so valid check
>> + * for this register can be skipped.
>> + * See xe_hwmon_power_is_visible.
>> + */
>> + struct xe_reg reg = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel);
>> +
>> + reg_val = xe_mmio_read32(mmio, reg);
>> + }
>>
>> - /*
>> - * This sysfs file won't be visible if REG_PKG_POWER_SKU is invalid, so valid check
>> - * for this register can be skipped.
>> - * See xe_hwmon_power_is_visible.
>> - */
>> - reg_val = xe_mmio_read32(mmio, reg);
>> reg_val = REG_FIELD_GET(PKG_TDP, reg_val);
>> *value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
>> }
>> @@ -330,20 +455,32 @@ xe_hwmon_power_max_interval_show(struct device *dev, struct device_attribute *at
>> struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>> u32 x, y, x_w = 2; /* 2 bits */
>> u64 r, tau4, out;
>> - int sensor_index = to_sensor_dev_attr(attr)->index;
>> + int channel = to_sensor_dev_attr(attr)->index;
>> + u32 power_attr = PL1_HWMON_ATTR;
>> + int ret = 0;
>>
>> xe_pm_runtime_get(hwmon->xe);
>>
>> mutex_lock(&hwmon->hwmon_lock);
>>
>> - r = xe_mmio_read32(mmio, xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, sensor_index));
>> + if (hwmon->xe->info.has_mbx_power_limits) {
>> + ret = xe_hwmon_pcode_read_power_limit(hwmon, power_attr, channel, (u32 *)&r);
>> + if (ret) {
>> + drm_err(&hwmon->xe->drm,
>> + "power interval read fail, ch %d, attr %d, r 0%llx, ret %d\n",
>> + channel, power_attr, r, ret);
>> + r = 0;
>> + }
>> + } else {
>> + r = xe_mmio_read32(mmio, xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel));
>> + }
>>
>> mutex_unlock(&hwmon->hwmon_lock);
>>
>> xe_pm_runtime_put(hwmon->xe);
>>
>> - x = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_X, r);
>> - y = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_Y, r);
>> + x = REG_FIELD_GET(PWR_LIM_TIME_X, r);
>> + y = REG_FIELD_GET(PWR_LIM_TIME_Y, r);
>>
>> /*
>> * tau = 1.x * power(2,y), x = bits(23:22), y = bits(21:17)
>> @@ -373,12 +510,16 @@ xe_hwmon_power_max_interval_store(struct device *dev, struct device_attribute *a
>> u64 tau4, r, max_win;
>> unsigned long val;
>> int ret;
>> - int sensor_index = to_sensor_dev_attr(attr)->index;
>> + int channel = to_sensor_dev_attr(attr)->index;
>> + u32 power_attr = PL1_HWMON_ATTR;
>>
>> ret = kstrtoul(buf, 0, &val);
>> if (ret)
>> return ret;
>>
>> + drm_dbg(&hwmon->xe->drm, "power_max_interval store ch %d, attr %s val 0x%lx\n", channel,
>> + PWR_ATTR_TO_STR(power_attr), val);
>> +
>> /*
>> * Max HW supported tau in '1.x * power(2,y)' format, x = 0, y = 0x12.
>> * The hwmon->scl_shift_time default of 0xa results in a max tau of 256 seconds.
>> @@ -400,8 +541,10 @@ xe_hwmon_power_max_interval_store(struct device *dev, struct device_attribute *a
>> tau4 = (u64)((1 << x_w) | x) << y;
>> max_win = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
>>
>> - if (val > max_win)
>> + if (val > max_win) {
>> + drm_warn(&hwmon->xe->drm, "power_interval invalid val 0x%lx\n", val);
>> return -EINVAL;
>> + }
>>
>> /* val in hw units */
>> val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME);
>> @@ -419,14 +562,22 @@ xe_hwmon_power_max_interval_store(struct device *dev, struct device_attribute *a
>> x = (val - (1ul << y)) << x_w >> y;
>> }
>>
>> - rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y);
>> + rxy = REG_FIELD_PREP(PWR_LIM_TIME_X, x) |
>> + REG_FIELD_PREP(PWR_LIM_TIME_Y, y);
>>
>> xe_pm_runtime_get(hwmon->xe);
>>
>> mutex_lock(&hwmon->hwmon_lock);
>>
>> - r = xe_mmio_rmw32(mmio, xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, sensor_index),
>> - PKG_PWR_LIM_1_TIME, rxy);
>> + if (hwmon->xe->info.has_mbx_power_limits) {
>> + ret = xe_hwmon_pcode_read_power_limit(hwmon, power_attr, channel, (u32 *)&r);
>> + r = (r & ~PWR_LIM_TIME) | rxy;
>> + drm_dbg(&hwmon->xe->drm, "power_max_interval store ch %d rxy 0x%x\n", channel, rxy);
>> + xe_hwmon_pcode_write_power_limit(hwmon, power_attr, channel, r);
>> + } else {
>> + r = xe_mmio_rmw32(mmio, xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel),
>> + PWR_LIM_TIME, rxy);
>> + }
>>
>> mutex_unlock(&hwmon->hwmon_lock);
>>
>> @@ -435,6 +586,7 @@ xe_hwmon_power_max_interval_store(struct device *dev, struct device_attribute *a
>> return count;
>> }
>>
>> +/* PSYS PL1 */
>> static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,
>> xe_hwmon_power_max_interval_show,
>> xe_hwmon_power_max_interval_store, CHANNEL_CARD);
>> @@ -455,10 +607,19 @@ static umode_t xe_hwmon_attributes_visible(struct kobject *kobj,
>> struct device *dev = kobj_to_dev(kobj);
>> struct xe_hwmon *hwmon = dev_get_drvdata(dev);
>> int ret = 0;
>> + int channel = index ? CHANNEL_PKG : CHANNEL_CARD;
>> + u32 power_attr = PL1_HWMON_ATTR;
>> + u32 uval;
>>
>> xe_pm_runtime_get(hwmon->xe);
>>
>> - ret = xe_reg_is_valid(xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, index)) ? attr->mode : 0;
>> + if (hwmon->xe->info.has_mbx_power_limits) {
>> + xe_hwmon_pcode_read_power_limit(hwmon, power_attr, channel, &uval);
>> + ret = (uval & PWR_LIM_EN) ? attr->mode : 0;
>> + } else {
>> + ret = xe_reg_is_valid(xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> + channel)) ? attr->mode : 0;
>> + }
>>
>> xe_pm_runtime_put(hwmon->xe);
>>
>> @@ -604,19 +765,27 @@ xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>>
>> switch (attr) {
>> case hwmon_power_max:
>> - return xe_reg_is_valid(xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> + if (hwmon->xe->info.has_mbx_power_limits) {
>> + xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, &uval);
>> + return (uval) ? (attr == hwmon_power_label) ? 0444 : 0664 : 0;
> In this code block this check is unnecessary.
yes, will remove it.
>> + } else {
>> + return xe_reg_is_valid(xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> channel)) ? 0664 : 0;
>> + }
>> case hwmon_power_rated_max:
>> - return xe_reg_is_valid(xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU,
>> - channel)) ? 0444 : 0;
>> + if (hwmon->xe->info.has_mbx_power_limits)
>> + return 0;
>> + else
>> + return xe_reg_is_valid(xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU,
>> + channel)) ? 0444 : 0;
>> case hwmon_power_crit:
>> - if (channel == CHANNEL_PKG)
>> - return (xe_hwmon_pcode_read_i1(hwmon, &uval) ||
>> - !(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;
>> - break;
>> case hwmon_power_label:
>> - return xe_reg_is_valid(xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU_UNIT,
>> - channel)) ? 0444 : 0;
>> + if (channel == CHANNEL_PKG) {
>> + xe_hwmon_pcode_read_i1(hwmon, &uval);
>> + return (uval & POWER_SETUP_I1_WATTS) ? (attr == hwmon_power_label) ?
>> + 0444 : 0644 : 0;
> In this code block this check is unnecessary.
Here it is needed, because case defintion is same hwmon_power_crit and
hwmon_power_label
and for case for label we dont have write permission.
>> + }
>> + break;
>> default:
>> return 0;
>> }
>> @@ -628,10 +797,10 @@ xe_hwmon_power_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *val)
>> {
>> switch (attr) {
>> case hwmon_power_max:
>> - xe_hwmon_power_max_read(hwmon, channel, val);
>> + xe_hwmon_power_max_read(hwmon, attr, channel, val);
>> return 0;
>> case hwmon_power_rated_max:
>> - xe_hwmon_power_rated_max_read(hwmon, channel, val);
>> + xe_hwmon_power_rated_max_read(hwmon, attr, channel, val);
>> return 0;
>> case hwmon_power_crit:
>> return xe_hwmon_power_curr_crit_read(hwmon, channel, val, SF_POWER);
>> @@ -645,7 +814,7 @@ xe_hwmon_power_write(struct xe_hwmon *hwmon, u32 attr, int channel, long val)
>> {
>> switch (attr) {
>> case hwmon_power_max:
>> - return xe_hwmon_power_max_write(hwmon, channel, val);
>> + return xe_hwmon_power_max_write(hwmon, attr, channel, val);
>> case hwmon_power_crit:
>> return xe_hwmon_power_curr_crit_write(hwmon, channel, val, SF_POWER);
>> default:
>> @@ -965,18 +1134,45 @@ xe_hwmon_get_preregistration_info(struct xe_hwmon *hwmon)
>> int channel;
>> struct xe_reg pkg_power_sku_unit;
>>
>> - /*
>> - * The contents of register PKG_POWER_SKU_UNIT do not change,
>> - * so read it once and store the shift values.
>> - */
>> - pkg_power_sku_unit = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU_UNIT, 0);
>> - if (xe_reg_is_valid(pkg_power_sku_unit)) {
>> - val_sku_unit = xe_mmio_read32(mmio, pkg_power_sku_unit);
>> - hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
>> - hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, val_sku_unit);
>> - hwmon->scl_shift_time = REG_FIELD_GET(PKG_TIME_UNIT, val_sku_unit);
>> + if (hwmon->xe->info.has_mbx_power_limits) {
>> + /* Check if mailbox power limits commands are supported by the firmware */
>> + if (!xe_hwmon_pcode_read_power_limit(hwmon, PL1_HWMON_ATTR, CHANNEL_CARD,
>> + &hwmon->pl1_on_boot[CHANNEL_CARD])) {
>> + /* Read all default power limits */
>> + if (xe_hwmon_pcode_read_power_limit(hwmon, PL1_HWMON_ATTR, CHANNEL_PKG,
>> + &hwmon->pl1_on_boot[CHANNEL_PKG])) {
>> + drm_warn(&hwmon->xe->drm, "Failed to read pkg power limit\n");
>> + } else {
>> + /* Write default limits to read from pcode from now on */
>> + xe_hwmon_pcode_write_power_limit(hwmon, PL1_HWMON_ATTR,
>> + CHANNEL_CARD,
>> + hwmon->pl1_on_boot[CHANNEL_CARD]);
>> + xe_hwmon_pcode_write_power_limit(hwmon, PL1_HWMON_ATTR, CHANNEL_PKG,
>> + hwmon->pl1_on_boot[CHANNEL_PKG]);
>> +
>> + hwmon->scl_shift_power = PWR_UNIT;
>> + hwmon->scl_shift_energy = ENERGY_UNIT;
>> + hwmon->scl_shift_time = TIME_UNIT;
>> + drm_info(&hwmon->xe->drm, "Using mailbox commands for power limits\n");
>> + hwmon->boot_power_limit_read = true;
>> + }
>> + } else {
>> + drm_warn(&hwmon->xe->drm, "Failed to read power limits, check firmware !\n");
> s/check firmware/check card firmware/ ?
>> + }
>
> Could you please simplify this if else block.
>
> if (xe_hwmon_pcode_read_power_limit(hwmon, PL1_HWMON_ATTR, CHANNEL_CARD,
> &hwmon->pl1_on_boot[CHANNEL_CARD])) {
> drm_warn;
> return;
> } if (xe_hwmon_pcode_read_power_limit(hwmon, PL1_HWMON_ATTR, CHANNEL_PKG,
> &hwmon->pl1_on_boot[CHANNEL_PKG])) {
> drm_warn;
> return;
> }
> update default values.
>
> Regards,
> Badal
>> + } else {
>> + drm_info(&hwmon->xe->drm, "Using register for power limits\n");
>> + /*
>> + * The contents of register PKG_POWER_SKU_UNIT do not change,
>> + * so read it once and store the shift values.
>> + */
>> + pkg_power_sku_unit = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU_UNIT, 0);
>> + if (xe_reg_is_valid(pkg_power_sku_unit)) {
>> + val_sku_unit = xe_mmio_read32(mmio, pkg_power_sku_unit);
>> + hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
>> + hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, val_sku_unit);
>> + hwmon->scl_shift_time = REG_FIELD_GET(PKG_TIME_UNIT, val_sku_unit);
>> + }
>> }
>> -
>> /*
>> * Initialize 'struct xe_hwmon_energy_info', i.e. set fields to the
>> * first value of the energy register read
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 882398e09b7e..95a2a458e8f7 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -66,6 +66,7 @@ struct xe_device_desc {
>> u8 has_heci_gscfi:1;
>> u8 has_heci_cscfi:1;
>> u8 has_llc:1;
>> + u8 has_mbx_power_limits:1;
>> u8 has_pxp:1;
>> u8 has_sriov:1;
>> u8 needs_scratch:1;
>> @@ -305,6 +306,7 @@ static const struct xe_device_desc dg2_desc = {
>> DG2_FEATURES,
>> .has_display = true,
>> .has_fan_control = true,
>> + .has_mbx_power_limits = false,
>> };
>>
>> static const __maybe_unused struct xe_device_desc pvc_desc = {
>> @@ -316,6 +318,7 @@ static const __maybe_unused struct xe_device_desc pvc_desc = {
>> .has_heci_gscfi = 1,
>> .max_remote_tiles = 1,
>> .require_force_probe = true,
>> + .has_mbx_power_limits = false,
>> };
>>
>> static const struct xe_device_desc mtl_desc = {
>> @@ -341,6 +344,7 @@ static const struct xe_device_desc bmg_desc = {
>> .dma_mask_size = 46,
>> .has_display = true,
>> .has_fan_control = true,
>> + .has_mbx_power_limits = true,
>> .has_heci_cscfi = 1,
>> .needs_scratch = true,
>> };
>> @@ -583,6 +587,7 @@ static int xe_info_init_early(struct xe_device *xe,
>> xe->info.dma_mask_size = desc->dma_mask_size;
>> xe->info.is_dgfx = desc->is_dgfx;
>> xe->info.has_fan_control = desc->has_fan_control;
>> + xe->info.has_mbx_power_limits = desc->has_mbx_power_limits;
>> xe->info.has_heci_gscfi = desc->has_heci_gscfi;
>> xe->info.has_heci_cscfi = desc->has_heci_cscfi;
>> xe->info.has_llc = desc->has_llc;
>> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
>> index cf955b3ed52c..9189117fe825 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode.c
>> +++ b/drivers/gpu/drm/xe/xe_pcode.c
>> @@ -109,6 +109,17 @@ int xe_pcode_write_timeout(struct xe_tile *tile, u32 mbox, u32 data, int timeout
>> return err;
>> }
>>
>> +int xe_pcode_write64_timeout(struct xe_tile *tile, u32 mbox, u32 data0, u32 data1, int timeout)
>> +{
>> + int err;
>> +
>> + mutex_lock(&tile->pcode.lock);
>> + err = pcode_mailbox_rw(tile, mbox, &data0, &data1, timeout, false, false);
>> + mutex_unlock(&tile->pcode.lock);
>> +
>> + return err;
>> +}
>> +
>> int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val, u32 *val1)
>> {
>> int err;
>> diff --git a/drivers/gpu/drm/xe/xe_pcode.h b/drivers/gpu/drm/xe/xe_pcode.h
>> index ba33991d72a7..de38f44f3201 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode.h
>> +++ b/drivers/gpu/drm/xe/xe_pcode.h
>> @@ -18,6 +18,9 @@ int xe_pcode_init_min_freq_table(struct xe_tile *tile, u32 min_gt_freq,
>> int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val, u32 *val1);
>> int xe_pcode_write_timeout(struct xe_tile *tile, u32 mbox, u32 val,
>> int timeout_ms);
>> +int xe_pcode_write64_timeout(struct xe_tile *tile, u32 mbox, u32 data0,
>> + u32 data1, int timeout);
>> +
>> #define xe_pcode_write(tile, mbox, val) \
>> xe_pcode_write_timeout(tile, mbox, val, 1)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
>> index e622ae17f08d..8c8be7c5f47b 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
>> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
>> @@ -42,6 +42,13 @@
>> #define POWER_SETUP_I1_SHIFT 6 /* 10.6 fixed point format */
>> #define POWER_SETUP_I1_DATA_MASK REG_GENMASK(15, 0)
>>
>> +#define READ_PSYSGPU_POWER_LIMIT 0x6
>> +#define WRITE_PSYSGPU_POWER_LIMIT 0x7
>> +#define READ_PACKAGE_POWER_LIMIT 0x8
>> +#define WRITE_PACKAGE_POWER_LIMIT 0x9
>> +#define READ_PL_FROM_BIOS 0x1
>> +#define READ_PL_FROM_PCODE 0x0
>> +
>> #define PCODE_FREQUENCY_CONFIG 0x6e
>> /* Frequency Config Sub Commands (param1) */
>> #define PCODE_MBOX_FC_SC_READ_FUSED_P0 0x0
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20250505/f5b3bd6b/attachment-0001.htm>
More information about the Intel-xe
mailing list