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

[Ghimiray, Himal Prasad]

On 12-12-2023 06:11, Matt Roper wrote:
> 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?

The explanation provided by you is exactly how ccs metadata handling 
works in case of

igfx. Will update the commit message with  explanation.

>> 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?
Apart from bo type being ttm_bo_type_device there shouldn't be any such 
case.
>
> In general, this function might be easier to read if the condition was
> broken up.  E.g.,

My intention was to avoid multiple if conditions. But if the 
recommendation is to use

it this way for better readability will update in next version.

BR

Himal.

>
>          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
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20231212/26ea8f1f/attachment.htm>