[Intel-xe] [PATCH] drm/xe: Replace DRM_ERROR() with drm_err()

[Matt Roper]

On Tue, May 30, 2023 at 03:30:17PM -0300, Gustavo Sousa wrote:
> Quoting Matt Roper (2023-05-30 14:04:32-03:00)
> >On Mon, May 29, 2023 at 12:08:31PM -0300, Gustavo Sousa wrote:
> >> DRM_ERROR() has been deprecated in favor of pr_err(). However, we should
> >> prefer to use drm_err() whenever possible so we get device-specific
> >> output with the error message.
> >> 
> >> Adding an "xe" member to struct xe_reg_sr does not seem very
> >> justifiable, since the only as of now is to register a clean up function
> >> and, with this change, print error messages. So let's just add a
> >> parameter to xe_reg_sr_add().
> >> 
> >> v2:
> >>   - Prefer drm_err() over pr_err(). (Matt, Jani)
> >> 
> >> Cc: Matt Roper <matthew.d.roper at intel.com>
> >> Cc: Jani Nikula <jani.nikula at intel.com>
> >> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> >> Cc: Haridhar Kalvala <haridhar.kalvala at intel.com>
> >> Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/xe_reg_sr.c | 14 ++++++++------
> >>  drivers/gpu/drm/xe/xe_reg_sr.h |  3 ++-
> >>  drivers/gpu/drm/xe/xe_rtp.c    |  2 +-
> >>  drivers/gpu/drm/xe/xe_vm.c     |  3 ++-
> >>  4 files changed, 13 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/xe/xe_reg_sr.c b/drivers/gpu/drm/xe/xe_reg_sr.c
> >> index 24d9c73ef279..02ba95223d7b 100644
> >> --- a/drivers/gpu/drm/xe/xe_reg_sr.c
> >> +++ b/drivers/gpu/drm/xe/xe_reg_sr.c
> >> @@ -90,7 +90,8 @@ static void reg_sr_inc_error(struct xe_reg_sr *sr)
> >>  }
> >>  
> >>  int xe_reg_sr_add(struct xe_reg_sr *sr,
> >> -                  const struct xe_reg_sr_entry *e)
> >> +                  const struct xe_reg_sr_entry *e,
> >> +                  struct xe_device *xe)
> >
> >I think we traditionally try to keep the device pointer as the first
> >parameter in the list.  It looks a bit strange to have it here at the
> >end.
> 
> Hm... But xe is only used as an auxiliary thing so we get richer error message,
> right?
> 
> By the way, I tried to follow what is already being done in this file:
> xe_reg_sr_init() kinda does the same.
> 
> Let me know if xe as the first argument is still the preferred style here. Then
> I can fix this one and send a patch for xe_reg_sr_init().

Okay, that's fair.  Having xe at the end looks a bit odd to me, but if
we're doing the same elsewhere then it's fine to keep this as-is.


Matt

> 
> >
> >>  {
> >>          unsigned long idx = e->reg.addr;
> >>          struct xe_reg_sr_entry *pentry = xa_load(&sr->xa, idx);
> >> @@ -123,11 +124,12 @@ int xe_reg_sr_add(struct xe_reg_sr *sr,
> >>          return 0;
> >>  
> >>  fail:
> >> -        DRM_ERROR("Discarding save-restore reg %04lx (clear: %08x, set: %08x, masked: %s, mcr: %s): ret=%d\n",
> >> -                  idx, e->clr_bits, e->set_bits,
> >> -                  str_yes_no(e->reg.masked),
> >> -                  str_yes_no(e->reg.mcr),
> >> -                  ret);
> >> +        drm_err(&xe->drm,
> >> +                "Discarding save-restore reg %04lx (clear: %08x, set: %08x, masked: %s, mcr: %s): ret=%d\n",
> >> +                idx, e->clr_bits, e->set_bits,
> >> +                str_yes_no(e->reg.masked),
> >> +                str_yes_no(e->reg.mcr),
> >> +                ret);
> >>          reg_sr_inc_error(sr);
> >>  
> >>          return ret;
> >> diff --git a/drivers/gpu/drm/xe/xe_reg_sr.h b/drivers/gpu/drm/xe/xe_reg_sr.h
> >> index 0bfe66ea29bf..3e337f174ae3 100644
> >> --- a/drivers/gpu/drm/xe/xe_reg_sr.h
> >> +++ b/drivers/gpu/drm/xe/xe_reg_sr.h
> >> @@ -19,7 +19,8 @@ struct drm_printer;
> >>  int xe_reg_sr_init(struct xe_reg_sr *sr, const char *name, struct xe_device *xe);
> >>  void xe_reg_sr_dump(struct xe_reg_sr *sr, struct drm_printer *p);
> >>  
> >> -int xe_reg_sr_add(struct xe_reg_sr *sr, const struct xe_reg_sr_entry *e);
> >> +int xe_reg_sr_add(struct xe_reg_sr *sr, const struct xe_reg_sr_entry *e,
> >> +                  struct xe_device *xe);
> >>  void xe_reg_sr_apply_mmio(struct xe_reg_sr *sr, struct xe_gt *gt);
> >>  void xe_reg_sr_apply_whitelist(struct xe_reg_sr *sr, u32 mmio_base,
> >>                                 struct xe_gt *gt);
> >> diff --git a/drivers/gpu/drm/xe/xe_rtp.c b/drivers/gpu/drm/xe/xe_rtp.c
> >> index 43a86358efb6..93067f4fd480 100644
> >> --- a/drivers/gpu/drm/xe/xe_rtp.c
> >> +++ b/drivers/gpu/drm/xe/xe_rtp.c
> >> @@ -122,7 +122,7 @@ static void rtp_add_sr_entry(const struct xe_rtp_action *action,
> >>          };
> >>  
> >>          sr_entry.reg.addr += mmio_base;
> >> -        xe_reg_sr_add(sr, &sr_entry);
> >> +        xe_reg_sr_add(sr, &sr_entry, gt_to_xe(gt));
> >
> >If we have a GT here, then it may be best to pass that down and use
> >xe_gt_err() above instead.  That will include extra GT identification
> >output that makes debugging easier when running on a platform that has
> >more than one GT.
> 
> Yeah... I realized that a little too late. I'll send a new version once I get an
> actionable reply for my comment above.
> 
> Thanks!
> 
> --
> Gustavo Sousa

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation