RE: [PATCH 2/4] drm/xe: Use xe_log SIGID API for probe-path error reporting

> -----Original Message-----
> From: Wajdeczko, Michal <[email protected]>
> Sent: Thursday, August 6, 2026 11:23 PM
> To: Koujalagi, Mallesh <[email protected]>; intel-
> [email protected]; Vivi, Rodrigo <[email protected]>; Brost,
> Matthew <[email protected]>
> Cc: Gupta, Anshuman <[email protected]>; Nilawar, Badal
> <[email protected]>; Belgaumkar, Vinay
> <[email protected]>; Tauro, Riana <[email protected]>; Poosa,
> Karthik <[email protected]>; Anirban, Sk <[email protected]>;
> Jadav, Raag <[email protected]>; [email protected];
> Bhadane, Dnyaneshwar <[email protected]>
> Subject: Re: [PATCH 2/4] drm/xe: Use xe_log SIGID API for probe-path error
> reporting
> 
> 
> 
> On 8/6/2026 1:00 PM, Mallesh Koujalagi wrote:
> > From: Dnyaneshwar Bhadane <[email protected]>
> >
> > Replace ad-hoc drm_err()/xe_err() calls in the probe path with
> > structured SIGID logging helpers. This gives fleet tooling a stable,
> > machine-parseable identifier (SIGID) for each recognised fault
> > situation rather than relying on fragile string matching.
> >
> > Signed-off-by: Dnyaneshwar Bhadane <[email protected]>
> > Signed-off-by: Mallesh Koujalagi <[email protected]>
> > ---
> >  drivers/gpu/drm/xe/xe_device.c |  6 ++++--
> > drivers/gpu/drm/xe/xe_hwmon.c  |  4 +++-
> >  drivers/gpu/drm/xe/xe_irq.c    |  9 +++++----
> >  drivers/gpu/drm/xe/xe_mmio.c   |  5 +++--
> >  drivers/gpu/drm/xe/xe_pat.c    |  6 ++++--
> >  drivers/gpu/drm/xe/xe_pci.c    | 14 +++++++++-----
> >  drivers/gpu/drm/xe/xe_pcode.c  |  5 +++--
> >  drivers/gpu/drm/xe/xe_vram.c   |  5 +++--
> >  8 files changed, 34 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_device.c
> > b/drivers/gpu/drm/xe/xe_device.c index d25d02b24898..52bafe708d74
> > 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -48,6 +48,7 @@
> >  #include "xe_i2c.h"
> >  #include "xe_irq.h"
> >  #include "xe_late_bind_fw.h"
> > +#include "xe_log.h"
> >  #include "xe_mmio.h"
> >  #include "xe_module.h"
> >  #include "xe_nvm.h"
> > @@ -586,7 +587,8 @@ int xe_device_init_early(struct xe_device *xe)
> >  		 * Cleanup done in xe_device_destroy via
> >  		 * drmm_add_action_or_reset register above
> >  		 */
> > -		drm_err(&xe->drm, "Failed to allocate xe workqueues\n");
> > +		xe_log_err_fatal(xe, PROBE, -ENOMEM, "Failed to allocate xe
> > +workqueues\n");
> 
Hello, 

> OOM errors will be already printed by the MM, we likely shouldn't have this
> drm_err here...
Yes Agreed, lets dropping this for now.
> 
> can't we just leave it as-is and add xe_log_err somewhere in xe_pci_probe()?
> also, maybe we should wait with submitting more patches that use xe_log()
> until we actually get some agreement and merge that first?
> 
I am looking for more options with PROBE.
In xe_pci_probe(), there is no central point for reporting errors. Each method directly returns an error code to the kernel.
Should we have a goto label inside xe_pci_probe() so that all error returns log xe_log_err_fatal and declare PROBE failures?

Even though the same errors coming from the called components may or may not be reported by their own components 
with their respective category and location. 

As of now, most of them just return the error without reporting or logging the error location, as seen in dmesg

> > +
> >  		return -ENOMEM;
> >  	}
> >
> > @@ -717,7 +719,7 @@ static int xe_set_dma_info(struct xe_device *xe)
> >  	return 0;
> >
> >  mask_err:
> > -	drm_err(&xe->drm, "Can't set DMA mask/consistent mask (%d)\n",
> err);
> > +	xe_log_err(xe, PROBE, err, "Can't set DMA mask/consistent mask\n");
> >  	return err;
> >  }
> >
> > diff --git a/drivers/gpu/drm/xe/xe_hwmon.c
> > b/drivers/gpu/drm/xe/xe_hwmon.c index de3f2aeffc3f..e4c319598dd0
> > 100644
> > --- a/drivers/gpu/drm/xe/xe_hwmon.c
> > +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> > @@ -15,6 +15,7 @@
> >  #include "regs/xe_pcode_regs.h"
> >  #include "xe_device.h"
> >  #include "xe_hwmon.h"
> > +#include "xe_log.h"
> >  #include "xe_mmio.h"
> >  #include "xe_pcode.h"
> >  #include "xe_pcode_api.h"
> > @@ -1575,7 +1576,8 @@ int xe_hwmon_register(struct xe_device *xe)
> >
> 	&hwmon_chip_info,
> >
> 	hwmon_groups);
> >  	if (IS_ERR(hwmon->hwmon_dev)) {
> > -		drm_err(&xe->drm, "Failed to register xe hwmon (%pe)\n",
> hwmon->hwmon_dev);
> > +		xe_log_err(xe, PROBE, PTR_ERR(hwmon->hwmon_dev),
> > +			   "Failed to register xe hwmon\n");
> 
> while this is during the probe, shouldn't we use HWMON as a component?
> we allow chained SIGID reports from different layers, and this error will
> eventually lead to final xe_log_err(PROBE) anyway
> 
> also, there are at least 2 other cases where hwmon initialization can fail why
> only this one is so special that requires use of xe_log ?
> 
> and IMO it would be good to add each new SIGID usage (across different files)
> in separate patches
Yes Agreed, 
I am creating another one component as HWMON and manage it in the separate patch.

> 
> >  		xe->hwmon = NULL;
> >  		return PTR_ERR(hwmon->hwmon_dev);
> >  	}
> > diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> > index 9e49e2241da4..74d2b63cd505 100644
> > --- a/drivers/gpu/drm/xe/xe_irq.c
> > +++ b/drivers/gpu/drm/xe/xe_irq.c
> > @@ -19,6 +19,7 @@
> >  #include "xe_hw_engine.h"
> >  #include "xe_hw_error.h"
> >  #include "xe_i2c.h"
> > +#include "xe_log.h"
> >  #include "xe_memirq.h"
> >  #include "xe_mert.h"
> >  #include "xe_mmio.h"
> > @@ -759,14 +760,14 @@ static int xe_irq_msi_request_irqs(struct
> > xe_device *xe)
> >
> >  	irq_handler = xe_irq_handler(xe);
> >  	if (!irq_handler) {
> > -		drm_err(&xe->drm, "No supported interrupt handler");
> > +		xe_log_err(xe, PROBE, -EINVAL, "No supported interrupt
> handler\n");
> 
> this is already a dead code, as xe_irq_handler() never returns NULL we can use
> xe_assert instead
Agreed let me skip this for now, i will handle removal for this dead code in separate series/patch.

> 
> >  		return -EINVAL;
> >  	}
> >
> >  	irq = pci_irq_vector(pdev, 0);
> >  	err = request_irq(irq, irq_handler, IRQF_SHARED, DRIVER_NAME, xe);
> >  	if (err < 0) {
> > -		drm_err(&xe->drm, "Failed to request MSI IRQ %d\n", err);
> > +		xe_log_err(xe, PROBE, err, "Failed to request MSI IRQ\n");
> >  		return err;
> >  	}
> >
> > @@ -822,7 +823,7 @@ int xe_irq_install(struct xe_device *xe)
> >
> >  	err = pci_alloc_irq_vectors(pdev, nvec, nvec, irq_flags);
> >  	if (err < 0) {
> > -		drm_err(&xe->drm, "Failed to allocate IRQ vectors: %d\n",
> err);
> > +		xe_log_err(xe, PROBE, err, "Failed to allocate IRQ vectors\n");
> >  		return err;
> >  	}
> >
> > @@ -891,7 +892,7 @@ static int xe_irq_msix_init(struct xe_device *xe)
> >  		return 0;  /* MSI */
> >
> >  	if (nvec < 0) {
> > -		drm_err(&xe->drm, "Failed getting MSI-X vectors count:
> %d\n", nvec);
> > +		xe_log_err(xe, PROBE, nvec, "Failed getting MSI-X vectors
> > +count\n");
> 
> maybe we should have IRQ component with SIGID_SW and use it here?

Yes, it should have the IRQ. in the separate patch.
> 
> >  		return nvec;
> >  	}
> >
> > diff --git a/drivers/gpu/drm/xe/xe_mmio.c
> > b/drivers/gpu/drm/xe/xe_mmio.c index 7fa18dfcb5a2..bcf36a381424
> 100644
> > --- a/drivers/gpu/drm/xe/xe_mmio.c
> > +++ b/drivers/gpu/drm/xe/xe_mmio.c
> > @@ -15,6 +15,7 @@
> >  #include "regs/xe_bars.h"
> >  #include "xe_device.h"
> >  #include "xe_gt_sriov_vf.h"
> > +#include "xe_log.h"
> >  #include "xe_printk.h"
> >  #include "xe_sriov.h"
> >  #include "xe_tile_printk.h"
> > @@ -105,13 +106,13 @@ int xe_mmio_probe_early(struct xe_device *xe)
> >
> >  	xe->mmio.regs = pcim_iomap(pdev, GTTMMADR_BAR, 0);
> >  	if (!xe->mmio.regs) {
> > -		xe_err(xe, "Failed to map GTTMMADR_BAR\n");
> > +		xe_log_err(xe, PROBE, -EIO, "Failed to map
> GTTMMADR_BAR\n");
> 
> shouldn't we use REGS component (SIGID_IO_BUS) here?
> 
> and this is a FATAL error, no?

Yes, should have the REGS. in the separate patch.

> 
> >  		return -EIO;
> >  	}
> >
> >  	xe->mmio.size = pci_resource_len(pdev, GTTMMADR_BAR);
> >  	if (xe->mmio.size < SZ_16M) {
> > -		xe_err(xe, "GTTMMADR_BAR is too small: %zu\n", xe-
> >mmio.size);
> > +		xe_log_err(xe, PROBE, -EIO, "GTTMMADR_BAR is too small:
> %zu\n",
> > +xe->mmio.size);
> 
> ditto
Noted.
> 
> >  		return -EIO;
> >  	}
> >
> > diff --git a/drivers/gpu/drm/xe/xe_pat.c b/drivers/gpu/drm/xe/xe_pat.c
> > index a5fe1beec652..aad15a4b0ac3 100644
> > --- a/drivers/gpu/drm/xe/xe_pat.c
> > +++ b/drivers/gpu/drm/xe/xe_pat.c
> > @@ -16,6 +16,7 @@
> >  #include "xe_force_wake.h"
> >  #include "xe_gt.h"
> >  #include "xe_gt_mcr.h"
> > +#include "xe_log.h"
> >  #include "xe_mmio.h"
> >  #include "xe_sriov.h"
> >  #include "xe_wa.h"
> > @@ -686,8 +687,9 @@ void xe_pat_init_early(struct xe_device *xe)
> >  		 * raise an error rather than trying to silently inherit the
> >  		 * most recent platform's behavior.
> >  		 */
> > -		drm_err(&xe->drm, "Missing PAT table for platform with
> graphics version %d.%02d!\n",
> > -			GRAPHICS_VER(xe), GRAPHICS_VERx100(xe) % 100);
> > +		xe_log_err(xe, PROBE, -ENODEV,
> > +			   "Missing PAT table for platform with graphics version
> %d.%02d!\n",
> > +			   GRAPHICS_VER(xe), GRAPHICS_VERx100(xe) %
> 100);
> 
> this here is pure SW gap and it is broken as right after logging this error we
> might crash in the xe_assert() below or later in xe_pat_init() as xe->pat.ops is
> NULL


Yes agreed, NULL check is required before going forward from this point. 

> 
> maybe instead of blindly adding xe_log_err we can just fix that first and return
> some error to abort the probe?

Yes, please provide the input for the above xe_pci_probe() query.

> 
> also maybe we should add PAT component (with SIGID_IO_BUS?)

Yes, PAT would be good for the IO_BUS. I will create separate patch.
> 
> >  	}
> >
> >  	xe_assert(xe, xe->pat.ops->dump);
> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > index 36d62927b969..3c452f399e6d 100644
> > --- a/drivers/gpu/drm/xe/xe_pci.c
> > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > @@ -25,6 +25,7 @@
> >  #include "xe_gt_printk.h"
> >  #include "xe_gt_sriov_vf.h"
> >  #include "xe_guc.h"
> > +#include "xe_log.h"
> >  #include "xe_mmio.h"
> >  #include "xe_module.h"
> >  #include "xe_pci_error.h"
> > @@ -718,8 +719,9 @@ static int handle_gmdid(struct xe_device *xe,
> >
> >  	*graphics_ip = find_graphics_ip(ver);
> >  	if (!*graphics_ip) {
> > -		drm_err(&xe->drm, "Hardware reports unknown graphics
> version %u.%02u\n",
> > -			ver / 100, ver % 100);
> > +		xe_log_err(xe, PROBE, -ENODEV,
> > +			   "Hardware reports unknown graphics version
> %u.%02u\n",
> > +			   ver / 100, ver % 100);
> 
> maybe use REGS component here (SIGID_IO_BUS)
> 
> also, isn't that actually broken?
> with graphics_ip = NULL set here we will crash later in xe_info_init()

No, the null check already presents in the xe_probe_ips().

> >  	}
> >
> >  	ret = read_gmdid(xe, GMDID_MEDIA, &ver, media_revid); @@ -732,8
> > +734,9 @@ static int handle_gmdid(struct xe_device *xe,
> >
> >  	*media_ip = find_media_ip(ver);
> >  	if (!*media_ip) {
> > -		drm_err(&xe->drm, "Hardware reports unknown media
> version %u.%02u\n",
> > -			ver / 100, ver % 100);
> > +		xe_log_err(xe, PROBE, -ENODEV,
> > +			   "Hardware reports unknown media version
> %u.%02u\n",
> > +			   ver / 100, ver % 100);
> >  	}
> >
> >  	return 0;
> > @@ -1088,7 +1091,8 @@ static int xe_info_init(struct xe_device *xe,
> >  		 * required for VRAM management).
> >  		 */
> >  		if (!tile->primary_gt) {
> > -			drm_err(&xe->drm, "Cannot probe device with
> without a primary GT\n");
> > +			xe_log_err_fatal(xe, PROBE, -ENODEV,
> > +					 "Cannot probe device without a
> primary GT\n");
> >  			return -ENODEV;
> >  		}
> >
> > diff --git a/drivers/gpu/drm/xe/xe_pcode.c
> > b/drivers/gpu/drm/xe/xe_pcode.c index ccc3bdeed6bb..c492737242d3
> > 100644
> > --- a/drivers/gpu/drm/xe/xe_pcode.c
> > +++ b/drivers/gpu/drm/xe/xe_pcode.c
> > @@ -14,6 +14,7 @@
> >  #include "regs/xe_pmt.h"
> >  #include "xe_assert.h"
> >  #include "xe_device.h"
> > +#include "xe_log.h"
> >  #include "xe_mmio.h"
> >  #include "xe_pcode_api.h"
> >  #include "xe_pm.h"
> > @@ -320,8 +321,8 @@ int xe_pcode_ready(struct xe_device *xe, bool
> locked)
> >  		mutex_unlock(&tile->pcode.lock);
> >
> >  	if (ret)
> > -		drm_err(&xe->drm,
> > -			"PCODE initialization timedout after: 3 min\n");
> > +		xe_log_err(xe, PCODE, -ret,
> > +			   "PCODE initialization timedout after: 3 min\n");
> 
> drop "PCODE" prefix
> and move to separate patch
Noted. I will do it.
> 
> >
> >  	return ret;
> >  }
> > diff --git a/drivers/gpu/drm/xe/xe_vram.c
> > b/drivers/gpu/drm/xe/xe_vram.c index 23eb7edbdd57..dada430d7ed5
> 100644
> > --- a/drivers/gpu/drm/xe/xe_vram.c
> > +++ b/drivers/gpu/drm/xe/xe_vram.c
> > @@ -17,6 +17,7 @@
> >  #include "xe_device.h"
> >  #include "xe_force_wake.h"
> >  #include "xe_gt_mcr.h"
> > +#include "xe_log.h"
> >  #include "xe_mmio.h"
> >  #include "xe_sriov.h"
> >  #include "xe_tile_sriov_vf.h"
> > @@ -43,7 +44,7 @@ static int determine_lmem_bar_size(struct xe_device
> *xe, struct xe_vram_region *
> >  	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> >
> >  	if (!resource_is_valid(pdev, LMEM_BAR)) {
> > -		drm_err(&xe->drm, "pci resource is not valid\n");
> > +		xe_log_err(xe, PROBE, -ENXIO, "pci resource is not valid\n");
> 
> shouldn't we use REGS here ? (SIGID_IO_BUS) or add VRAM component (also
> SIGID_IO_BUS?)
I see adding vram would be good.
> 
> >  		return -ENXIO;
> >  	}
> >
> > @@ -237,7 +238,7 @@ static int vram_region_init(struct xe_device *xe,
> struct xe_vram_region *vram,
> >  	vram->io_size = min_t(u64, usable_size, remain_io_size);
> >
> >  	if (!vram->io_size) {
> > -		drm_err(&xe->drm, "Tile without any CPU visible VRAM.
> Aborting.\n");
> > +		xe_log_err(xe, PROBE, -ENODEV, "Tile without any CPU visible
> VRAM.
> > +Aborting.\n");
> 
> we should first fix the vram_region_init() as now it is used in two scenarios: to
> init each tile region, and to init device level region info
> 
> and this error is meaningful for tile-based usage, where we should use tile as a
> location
Yes agreed,  I will create VRAM specific patch for tile and device init region.

Thank you, Michal for the review. 

- Dnyaneshwar
> 
> >  		return -ENODEV;
> >  	}
> >