Re: [PATCH v2 1/4] drm/xe/vram: Add binding information to VRAM regions

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The device-level aggregate VRAM region (`xe->mem.vram`) is left with an implicitly zeroed `binding` field, causing it to incorrectly identify as a tile-bound region for tile 0 and violating uniqueness constraints.
--

--- Patch [1]: [PATCH v2 1/4] drm/xe/vram: Add binding information to VRAM regions ---
commit c73dc6ee289feb3d1e22f53db84480b8a5bc094f
Author: Piotr Piórkowski <[email protected]>
Subject: drm/xe/vram: Add binding information to VRAM regions

Add a binding field to VRAM regions and pass it at allocation time.
This allows distinguishing VRAM regions serving different purposes. It is
needed for future patches where different VRAM regions serve different
purposes, such as dedicated VRAM for kernel allocations.

> diff --git a/drivers/gpu/drm/xe/xe_vram_types.h b/drivers/gpu/drm/xe/xe_vram_types.h
> index 646e3c12ae9f7..51884e7a679d1 100644
> --- a/drivers/gpu/drm/xe/xe_vram_types.h
> +++ b/drivers/gpu/drm/xe/xe_vram_types.h
[ ... ]
> @@ -26,9 +34,11 @@ struct xe_vram_region {
>  	/**
>  	 * @id: VRAM region instance id
>  	 *
> -	 * The value should be unique for VRAM region.
> +	 * The value should be unique within a given binding.
>  	 */
>  	u8 id;
> +	/** @binding: VRAM region instance binding */
> +	enum xe_vram_binding binding;

[Severity: Medium]
Will this default initialization cause duplicate IDs for the aggregate device
VRAM region?

The aggregate VRAM region is allocated in
drivers/gpu/drm/xe/xe_device.c:xe_device_vram_alloc() using drmm_kzalloc(),
which zeroes the memory but doesn't explicitly initialize the binding or id
fields. Because XE_VRAM_BINDING_TILE is 0, xe->mem.vram implicitly defaults
to XE_VRAM_BINDING_TILE with an id of 0.

This appears to violate the new struct documentation stating that id should be
unique within a given binding, since both the aggregate region and the actual
tile 0 region will have id 0 and XE_VRAM_BINDING_TILE.

> diff --git a/drivers/gpu/drm/xe/xe_vram.c b/drivers/gpu/drm/xe/xe_vram.c
> index 23eb7edbdd572..e5f83df988263 100644
> --- a/drivers/gpu/drm/xe/xe_vram.c
> +++ b/drivers/gpu/drm/xe/xe_vram.c
[ ... ]
> @@ -217,11 +241,13 @@ static void print_vram_region_info(struct xe_device *xe, struct xe_vram_region *
>  		drm_info(drm, "Small BAR device\n");
>  
>  	drm_info(drm,
> -		 "VRAM[%u]: Actual physical size %pa, usable size exclude stolen %pa, CPU accessible size %pa\n",
> -		 vram->id, &vram->actual_physical_size, &vram->usable_size, &vram->io_size);
> -	drm_info(drm, "VRAM[%u]: DPA range: [%pa-%llx], io range: [%pa-%llx]\n",
> -		 vram->id, &vram->dpa_base, vram->dpa_base + (u64)vram->actual_physical_size,
> -		 &vram->io_start, vram->io_start + (u64)vram->io_size);
> +		 "%s[%u] VRAM region: Actual physical size %pa, usable size exclude stolen %pa, CPU accessible size %pa\n",
> +		 stringify_vram_binding(vram->binding), vram->id, &vram->actual_physical_size,
> +		 &vram->usable_size, &vram->io_size);

[Severity: Medium]
Will this cause print_vram_region_info() to print two identical
"Tile[0] VRAM region:" messages?

Since xe->mem.vram defaults to XE_VRAM_BINDING_TILE and id 0, it seems like we
would get one log for the actual tile and one for the device-level aggregated
sizes, both labeled as "Tile[0]".

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