From: "Poosa, Karthik" <karthik.poosa@intel.com>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: <intel-xe@lists.freedesktop.org>, <anshuman.gupta@intel.com>,
<badal.nilawar@intel.com>, <raag.jadav@intel.com>,
<riana.tauro@intel.com>, <sk.anirban@intel.com>,
<mallesh.koujalagi@intel.com>, <soham.purkait@intel.com>
Subject: Re: [PATCH v2 2/5] drm/xe/hwmon: Use VRAM temperature sensor count from thermal config on CRI
Date: Thu, 10 Sep 2026 17:03:20 +0530 [thread overview]
Message-ID: <da5dfa60-5f0e-4dfe-ba40-eb02b1da803b@intel.com> (raw)
In-Reply-To: <aphuND-AkxP4_XQw@intel.com>
On 03-09-2026 00:13, Rodrigo Vivi wrote:
> On Wed, Sep 02, 2026 at 11:25:04PM +0530, Karthik Poosa wrote:
>> Read the number of VRAM temperature sensor channels from the second byte
>> of READ_THERMAL_CONFIG on CRI platforms. Use the reported count to avoid
>> exposing hwmon attributes for unavailable VRAM temperature sensors, while
>> retaining the maximum supported channel count on non-CRI platforms.
>>
>> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
>> ---
>> v2:
>> - Address review comments from sashiko-bot@kernel.org.
>> - Address review comments from Badal and Raag.
>> - Update HWMON_CHANNEL_INFO() to supports maximum VRAM channels of CRI.
>>
>> drivers/gpu/drm/xe/xe_hwmon.c | 102 ++++++++++++++++++++++++++++--
>> drivers/gpu/drm/xe/xe_pcode_api.h | 1 +
>> 2 files changed, 96 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>> index 9433b47a19c8..a7c04c25e1c2 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -39,7 +39,8 @@ enum xe_hwmon_reg_operation {
>> REG_READ64,
>> };
>>
>> -#define MAX_VRAM_CHANNELS (16)
>> +#define XE_MAX_VRAM_CHANNELS (80)
>> +#define BMG_MAX_VRAM_CHANNELS (16)
ok
> Please remove the parenthesis...
> Also invert the prefix logic...
>
> BMG_ would be read like started on BMG and continued after that
> while XE_ was the original one...
>
> clearly not your intent, but that is how we should read, so
> fix it please....
>
>>
>> enum xe_hwmon_channel {
>> CHANNEL_CARD,
>> @@ -48,7 +49,8 @@ enum xe_hwmon_channel {
>> CHANNEL_MCTRL,
>> CHANNEL_PCIE,
>> CHANNEL_VRAM_N,
>> - CHANNEL_VRAM_N_MAX = CHANNEL_VRAM_N + MAX_VRAM_CHANNELS - 1,
>> + /* Compile-time upper bound; actual channel count is hwmon->temp.vram_count */
>> + CHANNEL_VRAM_N_MAX = CHANNEL_VRAM_N + XE_MAX_VRAM_CHANNELS - 1,
>> CHANNEL_MAX,
>> };
>>
>> @@ -142,12 +144,14 @@ struct xe_hwmon_thermal_info {
>> /** @data: temperature limits in dwords */
>> u32 data[DIV_ROUND_UP(TEMP_LIMIT_MAX, sizeof(u32))];
>> };
>> - /** @count: no of temperature sensors available for the platform */
> + /** @count: temperature sensors count from READ_THERMAL_CONFIG */
>> u8 count;
>> + /** @vram_count: number of VRAM temperature sensors available for the platform */
>> + u8 vram_count;
>> /** @value: signed value from each sensor */
>> s8 value[U8_MAX];
>> - /** @vram_label: vram label names */
>> - char vram_label[MAX_VRAM_CHANNELS][MAX_LABEL_SIZE];
>> + /** @vram_label: vram label names, dynamically allocated based on vram_count */
>> + char (*vram_label)[MAX_LABEL_SIZE];
>> };
>>
>> /**
>> @@ -271,7 +275,7 @@ static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg
>> return BMG_PACKAGE_TEMPERATURE;
>> else if (channel == CHANNEL_VRAM)
>> return BMG_VRAM_TEMPERATURE;
>> - else if (in_range(channel, CHANNEL_VRAM_N, MAX_VRAM_CHANNELS))
>> + else if (in_range(channel, CHANNEL_VRAM_N, hwmon->temp.vram_count))
>> return BMG_VRAM_TEMPERATURE_N(channel - CHANNEL_VRAM_N);
>> } else if (xe->info.platform == XE_DG2) {
>> if (channel == CHANNEL_PKG)
>> @@ -777,6 +781,70 @@ static const struct hwmon_channel_info * const hwmon_info[] = {
>> HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> + HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL,
>> HWMON_T_CRIT | HWMON_T_EMERGENCY | HWMON_T_INPUT | HWMON_T_LABEL),
>> HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_LABEL | HWMON_P_CRIT |
>> HWMON_P_CAP,
>> @@ -810,6 +878,17 @@ static int xe_hwmon_pcode_read_thermal_info(struct xe_hwmon *hwmon)
>> drm_dbg(&hwmon->xe->drm, "thermal config count 0x%x\n", config);
>> hwmon->temp.count = REG_FIELD_GET(TEMP_MASK, config);
>>
>> + if (hwmon->xe->info.platform >= XE_CRESCENTISLAND) {
>> + hwmon->temp.vram_count = REG_FIELD_GET(VRAM_COUNT_MASK, config);
>> + if (hwmon->temp.vram_count > XE_MAX_VRAM_CHANNELS) {
>> + drm_warn(&hwmon->xe->drm, "VRAM channel count %d exceeds max %d, clamping\n",
>> + hwmon->temp.vram_count, XE_MAX_VRAM_CHANNELS);
>> + hwmon->temp.vram_count = XE_MAX_VRAM_CHANNELS;
>> + }
>> + } else {
>> + hwmon->temp.vram_count = BMG_MAX_VRAM_CHANNELS;
> looking this code here and thinking about the defines, inverting the defines
> will also end up with an ugly code anyway...
>
> So let's do this:
>
> define a platform info:
>
> u8 has_fixed_vram_channels:1
>
> set this true only on older platforms...
>
> then
>
> #define MAX_VRAM_CHANNELS 80
> #define FIXED_VRAM_CHANNELS 16
>
> if (hwmon->xe->info.has_fixed_vram_channels)
> hwmon->temp.vram_count = FIXED_VRAM_CHANNELS;
> else
> hwmon->temp.vram_count = REG_FIELD_GET(VRAM_COUNT_MASK, config);
>
> if (hwmon->temp.vram_count > MAX_VRAM_CHANNELS) {
> drm_warn(&hwmon->xe->drm, "VRAM channel count %d exceeds max %d, clamping\n",
> hwmon->temp.vram_count, MAX_VRAM_CHANNELS);
> hwmon->temp.vram_count = MAX_VRAM_CHANNELS;
> }
ok
>> return ret;
>> }
>>
>> @@ -1507,7 +1586,7 @@ static int xe_hwmon_read_label(struct device *dev,
>> *str = "mctrl";
>> else if (channel == CHANNEL_PCIE)
>> *str = "pcie";
>> - else if (in_range(channel, CHANNEL_VRAM_N, MAX_VRAM_CHANNELS))
>> + else if (in_range(channel, CHANNEL_VRAM_N, hwmon->temp.vram_count))
>> *str = hwmon->temp.vram_label[channel - CHANNEL_VRAM_N];
>> return 0;
>> case hwmon_power:
>> @@ -1636,6 +1715,15 @@ int xe_hwmon_register(struct xe_device *xe)
>>
>> xe_hwmon_get_preregistration_info(hwmon);
>>
>> + if (hwmon->temp.vram_count) {
>> + hwmon->temp.vram_label = devm_kcalloc(dev, hwmon->temp.vram_count,
>> + MAX_LABEL_SIZE, GFP_KERNEL);
>> + if (!hwmon->temp.vram_label) {
>> + xe->hwmon = NULL;
> ouch, please, while at it refactor this function to the most common xe style:
>
> if (!hwmon->temp.vram_label) {
> ret = -ENOMEM;
> goto err_null_hwmon;
> }
>
> - xe->hwmon = NULL;
> - return PTR_ERR(hwmon->hwmon_dev);
> + ret = PTR_ERR(hwmon->hwmon_dev);
> + goto err_null_hwmon;
> }
>
> return 0;
>
> err_null_hwmon:
> xe->hwmon = NULL;
> return ret;
ok
>
>> + return -ENOMEM;
>> + }
>> + }
>> +
>> drm_dbg(&xe->drm, "Register xe hwmon interface\n");
>>
>> /* hwmon_dev points to device hwmon<i> */
>> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
>> index 94575c476e3d..e1079eff72c6 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
>> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
>> @@ -57,6 +57,7 @@
>> #define PCODE_THERMAL_INFO 0x25
>> #define READ_THERMAL_LIMITS 0x0
>> #define READ_THERMAL_CONFIG 0x1
>> +#define VRAM_COUNT_MASK REG_GENMASK(15, 8)
>> #define READ_THERMAL_DATA 0x2
>> #define PCIE_SENSOR_GROUP_ID 0x2
>> #define PCIE_SENSOR_MASK REG_GENMASK(31, 16)
>> --
>> 2.25.1
>>
next prev parent reply other threads:[~2026-09-10 11:33 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-02 17:55 [PATCH v2 0/5] drm/xe/hwmon: Update hwmon thermal mailbox handling Karthik Poosa
2026-09-02 17:55 ` [PATCH v2 1/5] drm/xe/hwmon: Detect unavailable temperature sensors Karthik Poosa
2026-09-02 17:55 ` [PATCH v2 2/5] drm/xe/hwmon: Use VRAM temperature sensor count from thermal config on CRI Karthik Poosa
2026-09-02 18:10 ` sashiko-bot
2026-09-10 10:42 ` Poosa, Karthik
2026-09-02 18:43 ` Rodrigo Vivi
2026-09-10 11:33 ` Poosa, Karthik [this message]
2026-09-02 17:55 ` [PATCH v2 3/5] drm/xe/hwmon: Correct group selection for memory controller temperature Karthik Poosa
2026-09-02 18:17 ` sashiko-bot
2026-09-10 17:00 ` Poosa, Karthik
2026-09-02 17:55 ` [PATCH v2 4/5] drm/xe/hwmon: Decode CRI temperature registers as IEEE-754 Karthik Poosa
2026-09-02 18:08 ` sashiko-bot
2026-09-10 17:05 ` Poosa, Karthik
2026-09-02 17:55 ` [PATCH v2 5/5] drm/xe/hwmon: Disable memory controller and PCIe temperatures on CRI Karthik Poosa
2026-09-02 18:07 ` sashiko-bot
2026-09-03 6:11 ` Poosa, Karthik
2026-09-02 18:28 ` ✓ CI.KUnit: success for drm/xe/hwmon: Update hwmon thermal mailbox handling (rev2) Patchwork
2026-09-02 19:22 ` ✗ Xe.CI.BAT: failure " Patchwork
2026-09-03 9:37 ` ✗ Xe.CI.FULL: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=da5dfa60-5f0e-4dfe-ba40-eb02b1da803b@intel.com \
--to=karthik.poosa@intel.com \
--cc=anshuman.gupta@intel.com \
--cc=badal.nilawar@intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=mallesh.koujalagi@intel.com \
--cc=raag.jadav@intel.com \
--cc=riana.tauro@intel.com \
--cc=rodrigo.vivi@intel.com \
--cc=sk.anirban@intel.com \
--cc=soham.purkait@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.