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

[Matt Roper]

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.

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


Matt

>  }
>  
>  static bool rtp_process_one_sr(const struct xe_rtp_entry_sr *entry,
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index f73e08b60670..bf4a2732447b 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/dma-fence-array.h>
>  
> +#include <drm/drm_print.h>
>  #include <drm/ttm/ttm_execbuf_util.h>
>  #include <drm/ttm/ttm_tt.h>
>  #include <drm/xe_drm.h>
> @@ -3035,7 +3036,7 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  	}
>  
>  	if (XE_IOCTL_ERR(xe, xe_vm_is_closed(vm))) {
> -		DRM_ERROR("VM closed while we began looking up?\n");
> +		drm_err(&xe->drm, "VM closed while we began looking up?\n");
>  		err = -ENOENT;
>  		goto put_vm;
>  	}
> -- 
> 2.40.1
> 

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