Re: [PATCH v1 2/4] drm/xe/ttm: Add PF-mem VRAM placement types for TTM

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Incorrect physical address calculation for PFMEM resources during SG table allocation.
- [High] Unpinned BOs in PF-mem regions will not be evicted during system suspend.
--

--- Patch [2]: [PATCH v1 2/4] drm/xe/ttm: Add PF-mem VRAM placement types for TTM ---
commit f6760ba28a684206d85498b2ae22ad0e9bc13d79
Author: Piotr Piórkowski <[email protected]>

drm/xe/ttm: Add PF-mem VRAM placement types for TTM

This commit adds dedicated TTM placement types for kernel-only PF-mem VRAM 
regions. It updates the BO memory iterators and eviction paths to recognize 
and route these new VRAM placements.

> diff --git a/drivers/gpu/drm/xe/xe_bo_evict.c b/drivers/gpu/drm/xe/xe_bo_evict.c
> --- a/drivers/gpu/drm/xe/xe_bo_evict.c
> +++ b/drivers/gpu/drm/xe/xe_bo_evict.c
> @@ -303,7 +303,7 @@ void xe_bo_pci_dev_remove_all(struct xe_device *xe)
>  	 * Move pagemap bos and exported dma-buf to system, and
>  	 * purge everything else.
>  	 */
> -	for (mem_type = XE_PL_VRAM1; mem_type >= XE_PL_TT; --mem_type) {
> +	for (mem_type = XE_PL_VRAM1_PFMEM; mem_type >= XE_PL_TT; --mem_type) {
>  		struct ttm_resource_manager *man =

[Severity: High]
This updates the eviction loop in xe_bo_pci_dev_remove_all() to include the new
PF-mem regions, but does xe_bo_evict_all_user() in this same file also need to 
be updated?

Looking at xe_bo_evict_all_user(), it still uses a hardcoded limit:

	for (mem_type = XE_PL_TT; mem_type <= XE_PL_VRAM1; ++mem_type) {
		struct ttm_resource_manager *man = ...

If this loop skips the newly added XE_PL_VRAM0_PFMEM and XE_PL_VRAM1_PFMEM 
regions, will unpinned BOs in these regions fail to evict during system 
suspend or D3cold transitions, potentially causing data loss?

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> @@ -405,7 +403,7 @@ int xe_ttm_vram_mgr_alloc_sgt(struct xe_device *xe,
>  			      enum dma_data_direction dir,
>  			      struct sg_table **sgt)
>  {
> -	struct xe_tile *tile = &xe->tiles[res->mem_type - XE_PL_VRAM0];
> +	struct xe_tile *tile = &xe->tiles[xe_vram_pl_to_tile_id(xe, res->mem_type)];
>  	struct xe_ttm_vram_mgr_resource *vres = to_xe_ttm_vram_mgr_resource(res);

[Severity: Critical]
Can this lead to incorrect physical address mapping for PFMEM resources?

While the tile ID is correctly resolved here, later in this same function the
physical address is calculated by unconditionally using the standard VRAM 
region:

	phys_addr_t phys = cursor.start + xe_vram_region_io_start(tile->mem.vram);

Since PFMEM regions represent distinct memory allocations, will hardcoding
tile->mem.vram (instead of resolving the correct region for the resource) use
the wrong IO start address for PFMEM resources?

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