[PATCH v7 03/10] drm/xe/xe2: Allocate extra pages for ccs during bo create

[Matt Roper]

On Mon, Dec 11, 2023 at 07:13:49PM +0530, Himal Prasad Ghimiray wrote:
> Allocate extra pages to handle ccs region for igfx too.

I still think we need more explanation on this patch since this commit
message makes it sound like we're allocating the CCS region itself,
which isn't the case.  The CCS region is already pre-allocated at a
fixed location reserved by the BIOS.  My understanding is that every
location in physical system memory has a corresponding location in the
reserved region to hold its CCS data, regardless of whether that memory
is being used for "graphics" purposes or not, right?  So if a buffer was
staying in RAM forever, you'd never need any extra storage.  However if
the buffer can get swapped out to disk, that's when you need extra space
to hold an extra copy of the CCS data for swapout.

I'm still not super familiar with TTM details, but I _think_ that means
that a buffer in PL_SYSTEM (which isn't mapped into any GTT or being
touched by hardware) potentially needs the extra storage space because
the OS might wind up swapping it out to disk, whereas a buffer in PL_TT
wouldn't actually need any extra CCS storage since it can't be swapped
out [yet].  And I'm guessing it's simpler / more efficient to just
allocate the extra storage once up front rather than trying to just
allocate it when a buffer moves from PL_TT -> PL_SYSTEM?

> 
> Bspec:58796
> 
> v2:
>  - For dgfx ensure system bit is not set.
>  - Modify comments.(Thomas)
> 
> v3:
>  - Seperate out patch to modify main memory to ccs memory ratio.(Matt)
> 
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Reviewed-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_bo.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 7e25c8b7a01a..0df8f6a77040 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -2183,8 +2183,8 @@ int xe_bo_evict(struct xe_bo *bo, bool force_alloc)
>   * placed in system memory.
>   * @bo: The xe_bo
>   *
> - * If a bo has an allowable placement in XE_PL_TT memory, it can't use
> - * flat CCS compression, because the GPU then has no way to access the
> + * For dgfx if a bo has an allowable placement in XE_PL_TT memory, it can't
> + * use flat CCS compression, because the GPU then has no way to access the
>   * CCS metadata using relevant commands. For the opposite case, we need to
>   * allocate storage for the CCS metadata when the BO is not resident in
>   * VRAM memory.
> @@ -2193,9 +2193,13 @@ int xe_bo_evict(struct xe_bo *bo, bool force_alloc)
>   */
>  bool xe_bo_needs_ccs_pages(struct xe_bo *bo)
>  {
> -	return bo->ttm.type == ttm_bo_type_device &&
> -		!(bo->flags & XE_BO_CREATE_SYSTEM_BIT) &&
> -		(bo->flags & XE_BO_CREATE_VRAM_MASK);
> +	struct xe_device *xe = xe_bo_device(bo);
> +
> +	return (xe_device_has_flat_ccs(xe) &&
> +		bo->ttm.type == ttm_bo_type_device &&
> +		((IS_DGFX(xe) && (bo->flags & XE_BO_CREATE_VRAM_MASK) &&
> +		  !(bo->flags & XE_BO_CREATE_SYSTEM_BIT)) ||
> +		(!IS_DGFX(xe) && (bo->flags & XE_BO_CREATE_SYSTEM_BIT))));

On an IGPU platform, is there ever a case where XE_BO_CREATE_SYSTEM_BIT
isn't set?  For that matter, is there ever a case on igpu where
compression is supported by the device and we _don't_ need the extra
storage to hold the CCS data during swapout?

In general, this function might be easier to read if the condition was
broken up.  E.g.,

        if (!xe_device_has_flat_ccs(xe))
                return false;

        /*
         * On discrete GPUs, if the GPU can access this buffer from
         * system memory (i.e., it allows XE_PL_TT placement), FlatCCS
         * can't be used since there's no CCS storage associated with
         * non-VRAM addresses.
         */
        if ((IS_DGFX(xe) && bo->flags & XE_BO_CREATE_SYSTEM_BIT))
                return false;

        /*
         * ...any cases where extra pages aren't needed on igpu?...
         */
        if (!IS_DGFX && ...)
                return false;

        return true;


Matt

>  }
>  
>  /**
> -- 
> 2.25.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation