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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox