-----Original Message-----
From: Wajdeczko, Michal <[email protected]>
Sent: Friday, June 5, 2026 11:42 AM
To: Cavitt, Jonathan <[email protected]>;
[email protected]; [email protected]
Cc: Gupta, Saurabhg <[email protected]>; Zuo, Alex <[email protected]>;
Jadav, Raag <[email protected]>
Subject: Re: [PATCH 2/5] drm/xe/heci: Use xe print functions in xe_heci_gsc.c
>
> On 6/5/2026 6:04 PM, Jonathan Cavitt wrote:
> > Update xe_heci_gsc.c to use the xe error reporting helper functions in
> > xe_printk.h instead of directly calling the associated drm print
> > functions from drm_print.h
> >
> > Signed-off-by: Jonathan Cavitt <[email protected]>
> > ---
> > drivers/gpu/drm/xe/xe_heci_gsc.c | 21 ++++++++++-----------
> > 1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_heci_gsc.c
> > b/drivers/gpu/drm/xe/xe_heci_gsc.c
> > index 5af8903e10af..d716371fbbe9 100644
> > --- a/drivers/gpu/drm/xe/xe_heci_gsc.c
> > +++ b/drivers/gpu/drm/xe/xe_heci_gsc.c
> > @@ -8,10 +8,9 @@
> > #include <linux/pci.h>
> > #include <linux/sizes.h>
> >
> > -#include <drm/drm_print.h>
> > -
> > #include "xe_device_types.h"
> > #include "xe_heci_gsc.h"
> > +#include "xe_printk.h"
> > #include "regs/xe_gsc_regs.h"
>
> nit: this one is at wrong place
> > #include "xe_platform_types.h"
>
> and xe_printk.h should be here
>
> > #include "xe_survivability_mode.h"
> > @@ -112,13 +111,13 @@ static int heci_gsc_irq_setup(struct xe_device *xe)
> >
> > heci_gsc->irq = irq_alloc_desc(0);
>
> nit: what about moving to devm_irq_alloc_desc() first?
I think fixing this should be addressed in a different patch series.
>
> > if (heci_gsc->irq < 0) {
> > - drm_err(&xe->drm, "gsc irq error %d\n", heci_gsc->irq);
> > + xe_err(xe, "gsc irq error %d\n", heci_gsc->irq);
>
> can we print error code in more friendly way using %pe
>
> and I guess we should use "GSC" name, not "gsc", so maybe:
>
> xe_err(xe, "GSC: irq allocation failed (%pe)\n", ERR_PTR(..
>
> > return heci_gsc->irq;
> > }
> >
> > ret = heci_gsc_irq_init(heci_gsc->irq);
> > if (ret < 0)
> > - drm_err(&xe->drm, "gsc irq init failed %d\n", ret);
> > + xe_err(xe, "gsc irq init failed %d\n", ret);
>
> xe_err(xe, "GSC: irq initialization failed (%pe)\n", ERR_PTR(..
>
> >
> > return ret;
> > }
> > @@ -151,7 +150,7 @@ static int heci_gsc_add_device(struct xe_device *xe,
> > const struct heci_gsc_def *
> >
> > ret = auxiliary_device_init(aux_dev);
> > if (ret < 0) {
> > - drm_err(&xe->drm, "gsc aux init failed %d\n", ret);
> > + xe_err(xe, "gsc aux init failed %d\n", ret);
> > kfree(adev);
> > return ret;
> > }
> > @@ -159,7 +158,7 @@ static int heci_gsc_add_device(struct xe_device *xe,
> > const struct heci_gsc_def *
> > heci_gsc->adev = adev; /* needed by the notifier */
> > ret = auxiliary_device_add(aux_dev);
> > if (ret < 0) {
> > - drm_err(&xe->drm, "gsc aux add failed %d\n", ret);
> > + xe_err(xe, "gsc aux add failed %d\n", ret);
> > heci_gsc->adev = NULL;
> >
> > /* adev will be freed with the put_device() and .release
> > sequence */
> > @@ -190,7 +189,7 @@ int xe_heci_gsc_init(struct xe_device *xe)
> > }
> >
> > if (!def || !def->name) {
>
> missing def looks like our coding error, shouldn't we just use xe_assert()?
>
> missing def->name is also our coding error,
> we should have no runtime checks for it (except xe_assert)
I think this also should be addressed in a different patch series.
I can apply the rest of the revision notes, though.
-Jonathan Cavitt
>
> > - drm_warn(&xe->drm, "HECI is not implemented!\n");
> > + xe_warn(xe, "HECI is not implemented!\n");
> > return 0;
> > }
> >
> > @@ -215,7 +214,7 @@ void xe_heci_gsc_irq_handler(struct xe_device *xe, u32
> > iir)
> > return;
> >
> > if (!xe->info.has_heci_gscfi) {
> > - drm_warn_once(&xe->drm, "GSC irq: not supported");
> > + xe_warn_once(xe, "GSC irq: not supported");
>
> xe_warn_once(xe, "GSC: unexpected irq %#x\n", iir);
>
> > return;
> > }
> >
> > @@ -224,7 +223,7 @@ void xe_heci_gsc_irq_handler(struct xe_device *xe, u32
> > iir)
> >
> > ret = generic_handle_irq_safe(xe->heci_gsc.irq);
> > if (ret)
> > - drm_err_ratelimited(&xe->drm, "error handling GSC irq: %d\n",
> > ret);
> > + xe_err_ratelimited(xe, "error handling GSC irq: %d\n", ret);
>
> xe_err_ratelimited(xe, "GSC: irq handling failed (%pe)\n",
>
> > }
> >
> > void xe_heci_csc_irq_handler(struct xe_device *xe, u32 iir)
> > @@ -235,7 +234,7 @@ void xe_heci_csc_irq_handler(struct xe_device *xe, u32
> > iir)
> > return;
> >
> > if (!xe->info.has_heci_cscfi) {
> > - drm_warn_once(&xe->drm, "CSC irq: not supported");
> > + xe_warn_once(xe, "CSC irq: not supported");
>
> xe_warn_once(xe, "CSC: unexpected irq %#x\n", iir);
>
> > return;
> > }
> >
> > @@ -244,5 +243,5 @@ void xe_heci_csc_irq_handler(struct xe_device *xe, u32
> > iir)
> >
> > ret = generic_handle_irq_safe(xe->heci_gsc.irq);
> > if (ret)
> > - drm_err_ratelimited(&xe->drm, "error handling GSC irq: %d\n",
> > ret);
> > + xe_err_ratelimited(xe, "error handling GSC irq: %d\n", ret);
>
> GSC or CSC ? function is 'heci_csc'
>
> > }
>
>
