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

On 20-Aug-26 12:59 AM, [email protected] wrote:
> 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.

Yes, I should rethink on it. I should pass the extra flag from caller to 
determine that whether it is coming for gt or device check. I should 
revert these changes and modify the existing function definition.

Dnyaneshwar,>
>>   			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);
>>   }
>