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

[Gustavo Sousa]

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().

>
>>  {
>>          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