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