<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 12-12-2023 06:11, Matt Roper wrote: </div> <blockquote type="cite" cite="mid:20231212004132.GD1327160@mdroper-desk1.amr.corp.intel.com"> <pre class="moz-quote-pre" wrap="">On Mon, Dec 11, 2023 at 07:13:49PM +0530, Himal Prasad Ghimiray wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">Allocate extra pages to handle ccs region for igfx too. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> 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? </pre> </blockquote> The explanation provided by you is exactly how ccs metadata handling works in case of igfx. Will update the commit message with explanation. <blockquote type="cite" cite="mid:20231212004132.GD1327160@mdroper-desk1.amr.corp.intel.com"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> 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 <a class="moz-txt-link-rfc2396E" href="mailto:matthew.d.roper@intel.com"><matthew.d.roper@intel.com></a> Cc: Thomas Hellström <a class="moz-txt-link-rfc2396E" href="mailto:thomas.hellstrom@linux.intel.com"><thomas.hellstrom@linux.intel.com></a> Reviewed-by: Thomas Hellström <a class="moz-txt-link-rfc2396E" href="mailto:thomas.hellstrom@linux.intel.com"><thomas.hellstrom@linux.intel.com></a> Signed-off-by: Himal Prasad Ghimiray <a class="moz-txt-link-rfc2396E" href="mailto:himal.prasad.ghimiray@intel.com"><himal.prasad.ghimiray@intel.com></a> --- 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)))); </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> 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?</pre> </blockquote> Apart from bo type being ttm_bo_type_device there shouldn't be any such case. <blockquote type="cite" cite="mid:20231212004132.GD1327160@mdroper-desk1.amr.corp.intel.com"> <pre class="moz-quote-pre" wrap=""> In general, this function might be easier to read if the condition was broken up. E.g.,</pre> </blockquote> 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. <blockquote type="cite" cite="mid:20231212004132.GD1327160@mdroper-desk1.amr.corp.intel.com"> <pre class="moz-quote-pre" wrap=""> 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 </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> } /** -- 2.25.1 </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> </pre> </blockquote> </body> </html>