[Intel-xe] [PATCH v4 2/6] drm/xe: add xe_ttm_stolen_cpu_access_needs_ggtt()

[Thomas Hellström]

On 3/14/23 09:58, Matthew Auld wrote:
> xe_ttm_stolen_cpu_inaccessible() was originally meant to just cover the
> case where stolen is not directly CPU accessible on some older
> integrated platforms, and as such a GGTT mapping was also required for
> CPU access (as per the check in xe_bo_create_pin_map_at()).
>
> However with small-bar systems on dgfx we have one more case where
> stolen is also inaccessible, however here we don't have any fallback
> GGTT mode for CPU access. Fix the check in xe_bo_create_pin_map_at() to
> make this distinction clear. In such a case the later vmap() will fail
> anyway.
>
> v2: fix kernel-doc warning
> v3: Simplify further and remove cpu_inaccessible()
>
> Suggested-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
> ---
>   drivers/gpu/drm/xe/xe_bo.c             |  2 +-
>   drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c | 39 ++++++++------------------
>   drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h |  2 +-
>   3 files changed, 14 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 3e90ea42e075..73a7f2cd4ad8 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -1151,7 +1151,7 @@ struct xe_bo *xe_bo_create_pin_map_at(struct xe_device *xe, struct xe_gt *gt,
>   	u64 end = offset == ~0ull ? offset : start + size;
>   
>   	if (flags & XE_BO_CREATE_STOLEN_BIT &&
> -	    xe_ttm_stolen_cpu_inaccessible(xe))
> +	    xe_ttm_stolen_cpu_access_needs_ggtt(xe))
>   		flags |= XE_BO_CREATE_GGTT_BIT;
>   
>   	bo = xe_bo_create_locked_range(xe, gt, vm, size, start, end, type, flags);
> diff --git a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
> index 27cc31f022a5..8759c13acd9b 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
> @@ -38,32 +38,17 @@ to_stolen_mgr(struct ttm_resource_manager *man)
>   }
>   
>   /**
> - * xe_ttm_stolen_cpu_inaccessible - Can we directly CPU access stolen memory for
> - * this device.
> + * xe_ttm_stolen_cpu_access_needs_ggtt - If we can't directly CPU access stolen,

Nit: Should be xe_ttm_stolen_cpu_access_needs_ggtt() ?

Otherwise LGTM.

/Thomas


> + * can we then fallback to mapping through the GGTT.
>    * @xe: xe device
>    *
> - * On some integrated platforms we can't directly access stolen via the CPU
> - * (like some normal system memory).  Also on small-bar systems for discrete,
> - * since stolen is always as the end of normal VRAM, and the BAR likely doesn't
> - * stretch that far. However CPU access of stolen is generally rare, and at
> - * least on discrete should not be needed.
> - *
> - * If this is indeed inaccessible then we fallback to using the GGTT mappable
> - * aperture for CPU access. On discrete platforms we have no such thing, so when
> - * later attempting to CPU map the memory an error is instead thrown.
> + * Some older integrated platforms don't support reliable CPU access for stolen,
> + * however on such hardware we can always use the mappable part of the GGTT for
> + * CPU access. Check if that's the case for this device.
>    */
> -bool xe_ttm_stolen_cpu_inaccessible(struct xe_device *xe)
> +bool xe_ttm_stolen_cpu_access_needs_ggtt(struct xe_device *xe)
>   {
> -	struct ttm_resource_manager *ttm_mgr =
> -		ttm_manager_type(&xe->ttm, XE_PL_STOLEN);
> -	struct xe_ttm_stolen_mgr *mgr;
> -
> -	if (!ttm_mgr)
> -		return true;
> -
> -	mgr = to_stolen_mgr(ttm_mgr);
> -
> -	return !mgr->io_base || GRAPHICS_VERx100(xe) < 1270;
> +	return GRAPHICS_VERx100(xe) < 1270 && !IS_DGFX(xe);
>   }
>   
>   static s64 detect_bar2_dgfx(struct xe_device *xe, struct xe_ttm_stolen_mgr *mgr)
> @@ -178,7 +163,7 @@ void xe_ttm_stolen_mgr_init(struct xe_device *xe)
>   	drm_dbg_kms(&xe->drm, "Initialized stolen memory support with %llu bytes\n",
>   		    stolen_size);
>   
> -	if (!xe_ttm_stolen_cpu_inaccessible(xe))
> +	if (mgr->io_base && !xe_ttm_stolen_cpu_access_needs_ggtt(xe))
>   		mgr->mapping = devm_ioremap_wc(&pdev->dev, mgr->io_base, stolen_size);
>   }
>   
> @@ -191,7 +176,7 @@ u64 xe_ttm_stolen_io_offset(struct xe_bo *bo, u32 offset)
>   
>   	XE_BUG_ON(!mgr->io_base);
>   
> -	if (!IS_DGFX(xe) && xe_ttm_stolen_cpu_inaccessible(xe))
> +	if (xe_ttm_stolen_cpu_access_needs_ggtt(xe))
>   		return mgr->io_base + xe_bo_ggtt_addr(bo) + offset;
>   
>   	xe_res_first(bo->ttm.resource, offset, 4096, &cur);
> @@ -257,10 +242,10 @@ int xe_ttm_stolen_io_mem_reserve(struct xe_device *xe, struct ttm_resource *mem)
>   	if (!mgr || !mgr->io_base)
>   		return -EIO;
>   
> -	if (!xe_ttm_stolen_cpu_inaccessible(xe))
> -		return __xe_ttm_stolen_io_mem_reserve_bar2(xe, mgr, mem);
> -	else
> +	if (xe_ttm_stolen_cpu_access_needs_ggtt(xe))
>   		return __xe_ttm_stolen_io_mem_reserve_stolen(xe, mgr, mem);
> +	else
> +		return __xe_ttm_stolen_io_mem_reserve_bar2(xe, mgr, mem);
>   }
>   
>   u64 xe_ttm_stolen_gpu_offset(struct xe_device *xe)
> diff --git a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h
> index 2fda97b97a05..1777245ff810 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h
> +++ b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h
> @@ -14,7 +14,7 @@ struct xe_device;
>   
>   void xe_ttm_stolen_mgr_init(struct xe_device *xe);
>   int xe_ttm_stolen_io_mem_reserve(struct xe_device *xe, struct ttm_resource *mem);
> -bool xe_ttm_stolen_cpu_inaccessible(struct xe_device *xe);
> +bool xe_ttm_stolen_cpu_access_needs_ggtt(struct xe_device *xe);
>   u64 xe_ttm_stolen_io_offset(struct xe_bo *bo, u32 offset);
>   u64 xe_ttm_stolen_gpu_offset(struct xe_device *xe);
>