Re: [PATCH v6 04/13] drm/xe/hwmon: Add helpers to validate thermal sensor readings

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Sep 29, 2026 at 10:18:14PM +0530, Poosa, Karthik wrote:
> 
> On 29-09-2026 01:08, Rodrigo Vivi wrote:
> > On Fri, Sep 25, 2026 at 02:24:20AM +0530, Karthik Poosa wrote:
> > > Add mmio_temp_valid() and mbx_temp_valid() to check whether a temperature
> > > sensor reading is valid. An MMIO value of 0xFFFFFFFF or a mailbox value of
> > > 0xFF indicates the sensor is unavailable.
> > > 
> > > Gate these checks behind has_mbx_temp_sentinel, enabled on CRI currently.
> > > 
> > > Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_device_types.h |  2 ++
> > >   drivers/gpu/drm/xe/xe_hwmon.c        | 16 ++++++++++++++++
> > >   drivers/gpu/drm/xe/xe_pci.c          |  2 ++
> > >   drivers/gpu/drm/xe/xe_pci_types.h    |  1 +
> > >   4 files changed, 21 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > > index 4661bfce2f4e..87554605fa41 100644
> > > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > > @@ -202,6 +202,8 @@ struct xe_device {
> > >   		 * pcode mailbox commands.
> > >   		 */
> > >   		u8 has_mbx_power_limits:1;
> > > +		/** @info.has_mbx_temp_sentinel: Device has temperature-unavailable sentinels */
> > > +		u8 has_mbx_temp_sentinel:1;
> > >   		/** @info.has_mbx_thermal_info: Device supports thermal mailbox commands */
> > >   		u8 has_mbx_thermal_info:1;
> > >   		/** @info.has_mem_copy_instr: Device supports MEM_COPY instruction */
> > > diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> > > index c743867a2ebf..8a1dbe708828 100644
> > > --- a/drivers/gpu/drm/xe/xe_hwmon.c
> > > +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> > > @@ -104,6 +104,9 @@ enum sensor_attr_power {
> > >   /* Index of memory controller in READ_THERMAL_DATA output */
> > >   #define TEMP_INDEX_MCTRL	2
> > > +/* Mailbox sentinel reported for an unavailable temperature sensor */
> > > +#define TEMP_MBX_INVALID	0xFF
> > > +
> > >   /* Mailbox temperature is sign-magnitude: bit 7 sign, bits 6:0 magnitude. */
> > >   #define TEMP_MBX_SIGN		BIT(7)
> > >   #define TEMP_MBX_MAG		GENMASK(6, 0)
> > > @@ -822,6 +825,19 @@ static int xe_hwmon_pcode_read_thermal_info(struct xe_hwmon *hwmon)
> > >   	return ret;
> > >   }
> > > +static inline bool mmio_temp_valid(const struct xe_hwmon *hwmon, u32 value)
> > > +{
> > > +	return hwmon->xe->info.has_mbx_temp_sentinel ? value != U32_MAX
> > > +						     : value != 0;
> > > +}
> > > +
> > > +/* Mailbox temperature is sign-magnitude (bit 7 sign, bits 6:0 magnitude). */
> > > +static inline bool mbx_temp_valid(const struct xe_hwmon *hwmon, u8 value)
> > > +{
> > > +	return hwmon->xe->info.has_mbx_temp_sentinel ? value != TEMP_MBX_INVALID
> > > +						     : value != 0;
> > > +}
> > When reviewing the patch 7, I noticed a small change that took me back to the
> > temp valid...
> > 
> > What if temperature is 0 ?!
> > Why isn't it valid?
> This can happen on pre-CRI platforms (.has_mbx_temp_sentinel!=true), where
> temperature sentinel is not supported.
> In that case, we shall return true here on those platforms.

thanks for the clarification.
rv-b remains

> > 
> > > +
> > >   /* Decode an 8-bit sign-magnitude mailbox temperature to degrees Celsius. */
> > >   static int mbx_temp_decode(u8 value)
> > >   {
> > > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > > index d66d5a0339ae..e656fc012902 100644
> > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > @@ -476,6 +476,7 @@ static const struct xe_device_desc cri_desc = {
> > >   	.has_gsc_nvm = 1,
> > >   	.has_i2c = true,
> > >   	.has_mbx_power_limits = true,
> > > +	.has_mbx_temp_sentinel = true,
> > >   	.has_mbx_thermal_info = true,
> > >   	.has_mert = true,
> > >   	.has_pre_prod_wa = 1,
> > > @@ -796,6 +797,7 @@ static int xe_info_init_early(struct xe_device *xe,
> > >   	/* runtime fusing may force flat_ccs to disabled later */
> > >   	xe->info.has_flat_ccs = desc->has_flat_ccs;
> > >   	xe->info.has_mbx_power_limits = desc->has_mbx_power_limits;
> > > +	xe->info.has_mbx_temp_sentinel = desc->has_mbx_temp_sentinel;
> > >   	xe->info.has_mbx_thermal_info = desc->has_mbx_thermal_info;
> > >   	xe->info.has_gsc_nvm = desc->has_gsc_nvm;
> > >   	xe->info.has_heci_gscfi = desc->has_heci_gscfi;
> > > diff --git a/drivers/gpu/drm/xe/xe_pci_types.h b/drivers/gpu/drm/xe/xe_pci_types.h
> > > index fed509ff601e..0041ec5676d3 100644
> > > --- a/drivers/gpu/drm/xe/xe_pci_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_pci_types.h
> > > @@ -50,6 +50,7 @@ struct xe_device_desc {
> > >   	u8 has_late_bind:1;
> > >   	u8 has_llc:1;
> > >   	u8 has_mbx_power_limits:1;
> > > +	u8 has_mbx_temp_sentinel:1;
> > >   	u8 has_mbx_thermal_info:1;
> > >   	u8 has_mert:1;
> > >   	u8 has_pre_prod_wa:1;
> > > -- 
> > > 2.25.1
> > >