Re: [PATCH 12/12] drm/xe/vram: Report VRAM errors using structured xe_log API

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Incomplete replacement of drm_info() with xe_log_info() for VRAM reporting.
- [Low] Brittle error handling by hardcoding -ENODEV instead of using the returned error variable.
--

--- Patch [12]: [PATCH 12/12] drm/xe/vram: Report VRAM errors using structured xe_log API ---
commit 0430f39e6a6ab1de29f7a59722a6a22038b4d848
Author: Dnyaneshwar Bhadane <[email protected]>

drm/xe/vram: Report VRAM errors using structured xe_log API

Replace drm_err()/drm_info() with xe_log_err_fatal()/xe_log_info()
using the VRAM component tag.

Move the "no CPU visible VRAM" error out of vram_region_init() into its
callers so that tile-based usage logs with the tile as location while
device-level usage logs with the device. This gives better diagnostics
by identifying which tile has no visible VRAM.

> Replace drm_err()/drm_info() with xe_log_err_fatal()/xe_log_info()
> using the VRAM component tag.

[Severity: Low]
Were the drm_info() calls in print_vram_region_info() intentionally skipped?

The commit message mentions replacing drm_info(), but print_vram_region_info()
in drivers/gpu/drm/xe/xe_vram.c still uses drm_info(). This leaves the VRAM
logging inconsistent.

> diff --git a/drivers/gpu/drm/xe/xe_vram.c b/drivers/gpu/drm/xe/xe_vram.c
> index 23eb7edbdd572..9fae593b07413 100644
> --- a/drivers/gpu/drm/xe/xe_vram.c
> +++ b/drivers/gpu/drm/xe/xe_vram.c

[ ... ]

> @@ -311,14 +311,18 @@ int xe_vram_probe(struct xe_device *xe)
>  		total_size += region_size;
>  		available_size += usable_size;
>  
> -		err = vram_region_init(xe, tile->mem.vram, &lmem_bar, tile_offset, usable_size,
> +		err = vram_region_init(xe, tile->mem.vram, &lmem_bar,
> +				       tile_offset, usable_size,
>  				       region_size, remain_io_size);
> -		if (err)
> +		if (err) {
> +			xe_log_err_fatal(tile, VRAM, -ENODEV,
> +					 "without any CPU visible VRAM. Aborting.\n");

[Severity: Low]
Would it be better to pass the 'err' variable here instead of hardcoding
-ENODEV?

If vram_region_init() is modified in the future to return other error
codes (like -ENOMEM), this log message will incorrectly report -ENODEV instead
of the actual error code.

>  			return err;
> +		}
>  
>  		if (total_size > lmem_bar.io_size) {
> -			drm_info(&xe->drm, "VRAM: %pa is larger than resource %pa\n",
> -				 &total_size, &lmem_bar.io_size);
> +			xe_log_info(xe, VRAM, "%pa is larger than resource %pa\n",
> +				    &total_size, &lmem_bar.io_size);
>  		}
>  
>  		remain_io_size -= min_t(u64, tile->mem.vram->actual_physical_size, remain_io_size);
> @@ -326,8 +330,11 @@ int xe_vram_probe(struct xe_device *xe)
>  
>  	err = vram_region_init(xe, xe->mem.vram, &lmem_bar, 0, available_size, total_size,
>  			       lmem_bar.io_size);
> -	if (err)
> +	if (err) {
> +		xe_log_err_fatal(xe, VRAM, -ENODEV,
> +				 "without any CPU visible VRAM. Aborting.\n");

[Severity: Low]
Should this also use the 'err' variable instead of hardcoding -ENODEV?

>  		return err;
> +	}
>  
>  	return devm_add_action_or_reset(xe->drm.dev, vram_fini, xe);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12