[PATCH v6 1/2] drm/ttm: Add a flag to allow drivers to skip clear-on-free

[PATCH v6 1/2] drm/ttm: Add a flag to allow drivers to skip clear-on-free

[Nirmoy Das]

Add TTM_TT_FLAG_CLEARED_ON_FREE, which DRM drivers can set before
releasing backing stores if they want to skip clear-on-free.

Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_pool.c | 18 +++++++++++-------
 include/drm/ttm/ttm_tt.h       |  6 +++++-
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 8504dbe19c1a..935ab3cfd046 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -222,15 +222,18 @@ static void ttm_pool_unmap(struct ttm_pool *pool, dma_addr_t dma_addr,
 }
 
 /* Give pages into a specific pool_type */
-static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p)
+static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p,
+			       bool cleared)
 {
 	unsigned int i, num_pages = 1 << pt->order;
 
-	for (i = 0; i < num_pages; ++i) {
-		if (PageHighMem(p))
-			clear_highpage(p + i);
-		else
-			clear_page(page_address(p + i));
+	if (!cleared) {
+		for (i = 0; i < num_pages; ++i) {
+			if (PageHighMem(p))
+				clear_highpage(p + i);
+			else
+				clear_page(page_address(p + i));
+		}
 	}
 
 	spin_lock(&pt->lock);
@@ -394,6 +397,7 @@ static void ttm_pool_free_range(struct ttm_pool *pool, struct ttm_tt *tt,
 				pgoff_t start_page, pgoff_t end_page)
 {
 	struct page **pages = &tt->pages[start_page];
+	bool cleared = tt->page_flags & TTM_TT_FLAG_CLEARED_ON_FREE;
 	unsigned int order;
 	pgoff_t i, nr;
 
@@ -407,7 +411,7 @@ static void ttm_pool_free_range(struct ttm_pool *pool, struct ttm_tt *tt,
 
 		pt = ttm_pool_select_type(pool, caching, order);
 		if (pt)
-			ttm_pool_type_give(pt, *pages);
+			ttm_pool_type_give(pt, *pages, cleared);
 		else
 			ttm_pool_free_page(pool, caching, order, *pages);
 	}
diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
index 2b9d856ff388..cfaf49de2419 100644
--- a/include/drm/ttm/ttm_tt.h
+++ b/include/drm/ttm/ttm_tt.h
@@ -85,6 +85,9 @@ struct ttm_tt {
 	 * fault handling abuses the DMA api a bit and dma_map_attrs can't be
 	 * used to assure pgprot always matches.
 	 *
+	 * TTM_TT_FLAG_CLEARED_ON_FREE: Set this if a drm driver handles
+	 * clearing backing store
+	 *
 	 * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only. DO NOT USE. This is
 	 * set by TTM after ttm_tt_populate() has successfully returned, and is
 	 * then unset when TTM calls ttm_tt_unpopulate().
@@ -94,8 +97,9 @@ struct ttm_tt {
 #define TTM_TT_FLAG_EXTERNAL		BIT(2)
 #define TTM_TT_FLAG_EXTERNAL_MAPPABLE	BIT(3)
 #define TTM_TT_FLAG_DECRYPTED		BIT(4)
+#define TTM_TT_FLAG_CLEARED_ON_FREE	BIT(5)
 
-#define TTM_TT_FLAG_PRIV_POPULATED	BIT(5)
+#define TTM_TT_FLAG_PRIV_POPULATED	BIT(6)
 	uint32_t page_flags;
 	/** @num_pages: Number of pages in the page array. */
 	uint32_t num_pages;
-- 
2.42.0

[PATCH v6 2/2] drm/xe/lnl: Offload system clear page activity to GPU

[Nirmoy Das]

On LNL because of flat CCS, driver creates migrates job to clear
CCS meta data. Extend that to also clear system pages using GPU.
Inform TTM to allocate pages without __GFP_ZERO to avoid double page
clearing by clearing out TTM_TT_FLAG_ZERO_ALLOC flag and set
TTM_TT_FLAG_CLEARED_ON_FREE while freeing to skip ttm pool's clear
on free as XE now takes care of clearing pages. If a bo is in system
placement such as BO created with  DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING
and there is a cpu map then for such BO gpu clear will be avoided as
there is no dma mapping for such BO at that moment to create migration
jobs.

Tested this patch api_overhead_benchmark_l0 from
https://github.com/intel/compute-benchmarks

Without the patch:
api_overhead_benchmark_l0 --testFilter=UsmMemoryAllocation:
UsmMemoryAllocation(api=l0 type=Host size=4KB) 84.206 us
UsmMemoryAllocation(api=l0 type=Host size=1GB) 105775.56 us
erf tool top 5 entries:
71.44% api_overhead_be  [kernel.kallsyms]   [k] clear_page_erms
6.34%  api_overhead_be  [kernel.kallsyms]   [k] __pageblock_pfn_to_page
2.24%  api_overhead_be  [kernel.kallsyms]   [k] cpa_flush
2.15%  api_overhead_be  [kernel.kallsyms]   [k] pages_are_mergeable
1.94%  api_overhead_be  [kernel.kallsyms]   [k] find_next_iomem_res

With the patch:
api_overhead_benchmark_l0 --testFilter=UsmMemoryAllocation:
UsmMemoryAllocation(api=l0 type=Host size=4KB) 79.439 us
UsmMemoryAllocation(api=l0 type=Host size=1GB) 98677.75 us
Perf tool top 5 entries:
11.16% api_overhead_be  [kernel.kallsyms]   [k] __pageblock_pfn_to_page
7.85%  api_overhead_be  [kernel.kallsyms]   [k] cpa_flush
7.59%  api_overhead_be  [kernel.kallsyms]   [k] find_next_iomem_res
7.24%  api_overhead_be  [kernel.kallsyms]   [k] pages_are_mergeable
5.53%  api_overhead_be  [kernel.kallsyms]   [k] lookup_address_in_pgd_attr

Without this patch clear_page_erms() dominates execution time which is
also not pipelined with migration jobs. With this patch page clearing
will get pipelined with migration job and will free CPU for more work.

v2: Handle regression on dgfx(Himal)
    Update commit message as no ttm API changes needed.
v3: Fix Kunit test.
v4: handle data leak on cpu mmap(Thomas)
v5: s/gpu_page_clear/gpu_page_clear_sys and move setting
    it to xe_ttm_sys_mgr_init() and other nits (Matt Auld)
v6: Disable it when init_on_alloc and/or init_on_free is active(Matt)
    Use compute-benchmarks as reporter used it to report this
    allocation latency issue also a proper test application than mime.
    In v5, the test showed significant reduction in alloc latency but
    that is not the case any more, I think this was mostly because
    previous test was done on IFWI which had low mem BW from CPU.

Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/xe/xe_bo.c           | 26 ++++++++++++++++++++++++--
 drivers/gpu/drm/xe/xe_device_types.h |  2 ++
 drivers/gpu/drm/xe/xe_ttm_sys_mgr.c  | 12 ++++++++++++
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
index 800119c8fc8d..1248acbdacfc 100644
--- a/drivers/gpu/drm/xe/xe_bo.c
+++ b/drivers/gpu/drm/xe/xe_bo.c
@@ -396,6 +396,14 @@ static struct ttm_tt *xe_ttm_tt_create(struct ttm_buffer_object *ttm_bo,
 		caching = ttm_uncached;
 	}
 
+	/*
+	 * If the device can support gpu clear system pages then set proper ttm
+	 * flag. Zeroed pages are only required for ttm_bo_type_device so
+	 * unwanted data is not leaked to userspace.
+	 */
+	if (ttm_bo->type == ttm_bo_type_device && xe->mem.gpu_page_clear_sys)
+		page_flags |= TTM_TT_FLAG_CLEARED_ON_FREE;
+
 	err = ttm_tt_init(&tt->ttm, &bo->ttm, page_flags, caching, extra_pages);
 	if (err) {
 		kfree(tt);
@@ -417,6 +425,10 @@ static int xe_ttm_tt_populate(struct ttm_device *ttm_dev, struct ttm_tt *tt,
 	if (tt->page_flags & TTM_TT_FLAG_EXTERNAL)
 		return 0;
 
+	/* Clear TTM_TT_FLAG_ZERO_ALLOC when GPU is set to clear system pages */
+	if (tt->page_flags & TTM_TT_FLAG_CLEARED_ON_FREE)
+		tt->page_flags &= ~TTM_TT_FLAG_ZERO_ALLOC;
+
 	err = ttm_pool_alloc(&ttm_dev->pool, tt, ctx);
 	if (err)
 		return err;
@@ -659,8 +671,16 @@ static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
 	bool needs_clear;
 	bool handle_system_ccs = (!IS_DGFX(xe) && xe_bo_needs_ccs_pages(bo) &&
 				  ttm && ttm_tt_is_populated(ttm)) ? true : false;
+	bool clear_system_pages;
 	int ret = 0;
 
+	/*
+	 * Clear TTM_TT_FLAG_CLEARED_ON_FREE on bo creation path when
+	 * moving to system as the bo doesn't have dma_mapping.
+	 */
+	if (!old_mem && ttm && !ttm_tt_is_populated(ttm))
+		ttm->page_flags &= ~TTM_TT_FLAG_CLEARED_ON_FREE;
+
 	/* Bo creation path, moving to system or TT. */
 	if ((!old_mem && ttm) && !handle_system_ccs) {
 		if (new_mem->mem_type == XE_PL_TT)
@@ -683,8 +703,10 @@ static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
 	move_lacks_source = handle_system_ccs ? (!bo->ccs_cleared)  :
 						(!mem_type_is_vram(old_mem_type) && !tt_has_data);
 
+	clear_system_pages = ttm && (ttm->page_flags & TTM_TT_FLAG_CLEARED_ON_FREE);
 	needs_clear = (ttm && ttm->page_flags & TTM_TT_FLAG_ZERO_ALLOC) ||
-		(!ttm && ttm_bo->type == ttm_bo_type_device);
+		(!ttm && ttm_bo->type == ttm_bo_type_device) ||
+		clear_system_pages;
 
 	if (new_mem->mem_type == XE_PL_TT) {
 		ret = xe_tt_map_sg(ttm);
@@ -796,7 +818,7 @@ static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
 		if (move_lacks_source) {
 			u32 flags = 0;
 
-			if (mem_type_is_vram(new_mem->mem_type))
+			if (mem_type_is_vram(new_mem->mem_type) || clear_system_pages)
 				flags |= XE_MIGRATE_CLEAR_FLAG_FULL;
 			else if (handle_system_ccs)
 				flags |= XE_MIGRATE_CLEAR_FLAG_CCS_DATA;
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 16a24eadd94b..2aa18e32b32f 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -339,6 +339,8 @@ struct xe_device {
 		struct xe_mem_region vram;
 		/** @mem.sys_mgr: system TTM manager */
 		struct ttm_resource_manager sys_mgr;
+		/** @mem.gpu_page_clear_sys: clear system pages offloaded to GPU */
+		bool gpu_page_clear_sys;
 	} mem;
 
 	/** @sriov: device level virtualization data */
diff --git a/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
index 9844a8edbfe1..e0ac20f20758 100644
--- a/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
+++ b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
@@ -117,5 +117,17 @@ int xe_ttm_sys_mgr_init(struct xe_device *xe)
 	ttm_resource_manager_init(man, &xe->ttm, gtt_size >> PAGE_SHIFT);
 	ttm_set_driver_manager(&xe->ttm, XE_PL_TT, man);
 	ttm_resource_manager_set_used(man, true);
+
+	/*
+	 * On iGFX device with flat CCS, we clear CCS metadata, let's extend that
+	 * and use GPU to clear pages as well.
+	 *
+	 * Disable this when init_on_free and/or init_on_alloc is on to avoid double
+	 * zeroing pages with CPU and GPU.
+	 */
+	if (xe_device_has_flat_ccs(xe) && !IS_DGFX(xe) &&
+	    !want_init_on_alloc(GFP_USER) && !want_init_on_free())
+		xe->mem.gpu_page_clear_sys = true;
+
 	return drmm_add_action_or_reset(&xe->drm, ttm_sys_mgr_fini, xe);
 }
-- 
2.42.0

Re: [PATCH v6 2/2] drm/xe/lnl: Offload system clear page activity to GPU

[Matthew Auld]

On 16/08/2024 14:51, Nirmoy Das wrote:
> On LNL because of flat CCS, driver creates migrates job to clear
> CCS meta data. Extend that to also clear system pages using GPU.
> Inform TTM to allocate pages without __GFP_ZERO to avoid double page
> clearing by clearing out TTM_TT_FLAG_ZERO_ALLOC flag and set
> TTM_TT_FLAG_CLEARED_ON_FREE while freeing to skip ttm pool's clear
> on free as XE now takes care of clearing pages. If a bo is in system
> placement such as BO created with  DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING
> and there is a cpu map then for such BO gpu clear will be avoided as
> there is no dma mapping for such BO at that moment to create migration
> jobs.
> 
> Tested this patch api_overhead_benchmark_l0 from
> https://github.com/intel/compute-benchmarks
> 
> Without the patch:
> api_overhead_benchmark_l0 --testFilter=UsmMemoryAllocation:
> UsmMemoryAllocation(api=l0 type=Host size=4KB) 84.206 us
> UsmMemoryAllocation(api=l0 type=Host size=1GB) 105775.56 us
> erf tool top 5 entries:
> 71.44% api_overhead_be  [kernel.kallsyms]   [k] clear_page_erms
> 6.34%  api_overhead_be  [kernel.kallsyms]   [k] __pageblock_pfn_to_page
> 2.24%  api_overhead_be  [kernel.kallsyms]   [k] cpa_flush
> 2.15%  api_overhead_be  [kernel.kallsyms]   [k] pages_are_mergeable
> 1.94%  api_overhead_be  [kernel.kallsyms]   [k] find_next_iomem_res
> 
> With the patch:
> api_overhead_benchmark_l0 --testFilter=UsmMemoryAllocation:
> UsmMemoryAllocation(api=l0 type=Host size=4KB) 79.439 us
> UsmMemoryAllocation(api=l0 type=Host size=1GB) 98677.75 us
> Perf tool top 5 entries:
> 11.16% api_overhead_be  [kernel.kallsyms]   [k] __pageblock_pfn_to_page
> 7.85%  api_overhead_be  [kernel.kallsyms]   [k] cpa_flush
> 7.59%  api_overhead_be  [kernel.kallsyms]   [k] find_next_iomem_res
> 7.24%  api_overhead_be  [kernel.kallsyms]   [k] pages_are_mergeable
> 5.53%  api_overhead_be  [kernel.kallsyms]   [k] lookup_address_in_pgd_attr
> 
> Without this patch clear_page_erms() dominates execution time which is
> also not pipelined with migration jobs. With this patch page clearing
> will get pipelined with migration job and will free CPU for more work.
> 
> v2: Handle regression on dgfx(Himal)
>      Update commit message as no ttm API changes needed.
> v3: Fix Kunit test.
> v4: handle data leak on cpu mmap(Thomas)
> v5: s/gpu_page_clear/gpu_page_clear_sys and move setting
>      it to xe_ttm_sys_mgr_init() and other nits (Matt Auld)
> v6: Disable it when init_on_alloc and/or init_on_free is active(Matt)
>      Use compute-benchmarks as reporter used it to report this
>      allocation latency issue also a proper test application than mime.
>      In v5, the test showed significant reduction in alloc latency but
>      that is not the case any more, I think this was mostly because
>      previous test was done on IFWI which had low mem BW from CPU.
> 
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

Reviewed-by: Matthew Auld <matthew.auld@intel.com>

Re: [PATCH v6 2/2] drm/xe/lnl: Offload system clear page activity to GPU

[Nirmoy Das]

On 8/19/2024 1:05 PM, Matthew Auld wrote:
> On 16/08/2024 14:51, Nirmoy Das wrote:
>> On LNL because of flat CCS, driver creates migrates job to clear
>> CCS meta data. Extend that to also clear system pages using GPU.
>> Inform TTM to allocate pages without __GFP_ZERO to avoid double page
>> clearing by clearing out TTM_TT_FLAG_ZERO_ALLOC flag and set
>> TTM_TT_FLAG_CLEARED_ON_FREE while freeing to skip ttm pool's clear
>> on free as XE now takes care of clearing pages. If a bo is in system
>> placement such as BO created with DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING
>> and there is a cpu map then for such BO gpu clear will be avoided as
>> there is no dma mapping for such BO at that moment to create migration
>> jobs.
>>
>> Tested this patch api_overhead_benchmark_l0 from
>> https://github.com/intel/compute-benchmarks
>>
>> Without the patch:
>> api_overhead_benchmark_l0 --testFilter=UsmMemoryAllocation:
>> UsmMemoryAllocation(api=l0 type=Host size=4KB) 84.206 us
>> UsmMemoryAllocation(api=l0 type=Host size=1GB) 105775.56 us
>> erf tool top 5 entries:
>> 71.44% api_overhead_be  [kernel.kallsyms]   [k] clear_page_erms
>> 6.34%  api_overhead_be  [kernel.kallsyms]   [k] __pageblock_pfn_to_page
>> 2.24%  api_overhead_be  [kernel.kallsyms]   [k] cpa_flush
>> 2.15%  api_overhead_be  [kernel.kallsyms]   [k] pages_are_mergeable
>> 1.94%  api_overhead_be  [kernel.kallsyms]   [k] find_next_iomem_res
>>
>> With the patch:
>> api_overhead_benchmark_l0 --testFilter=UsmMemoryAllocation:
>> UsmMemoryAllocation(api=l0 type=Host size=4KB) 79.439 us
>> UsmMemoryAllocation(api=l0 type=Host size=1GB) 98677.75 us
>> Perf tool top 5 entries:
>> 11.16% api_overhead_be  [kernel.kallsyms]   [k] __pageblock_pfn_to_page
>> 7.85%  api_overhead_be  [kernel.kallsyms]   [k] cpa_flush
>> 7.59%  api_overhead_be  [kernel.kallsyms]   [k] find_next_iomem_res
>> 7.24%  api_overhead_be  [kernel.kallsyms]   [k] pages_are_mergeable
>> 5.53%  api_overhead_be  [kernel.kallsyms]   [k] 
>> lookup_address_in_pgd_attr
>>
>> Without this patch clear_page_erms() dominates execution time which is
>> also not pipelined with migration jobs. With this patch page clearing
>> will get pipelined with migration job and will free CPU for more work.
>>
>> v2: Handle regression on dgfx(Himal)
>>      Update commit message as no ttm API changes needed.
>> v3: Fix Kunit test.
>> v4: handle data leak on cpu mmap(Thomas)
>> v5: s/gpu_page_clear/gpu_page_clear_sys and move setting
>>      it to xe_ttm_sys_mgr_init() and other nits (Matt Auld)
>> v6: Disable it when init_on_alloc and/or init_on_free is active(Matt)
>>      Use compute-benchmarks as reporter used it to report this
>>      allocation latency issue also a proper test application than mime.
>>      In v5, the test showed significant reduction in alloc latency but
>>      that is not the case any more, I think this was mostly because
>>      previous test was done on IFWI which had low mem BW from CPU.
>>
>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>


Thanks Matt.

Pushed this to drm-xe-next. The series contains a ttm pool change which 
as agreed with Christian

is small enough to not cause any issue so can be pulled though drm-xe-next.


Regards,

Nirmoy

Re: [PATCH v6 2/2] drm/xe/lnl: Offload system clear page activity to GPU

[Thomas Hellström]

Hi, Nirmoy,

On Mon, 2024-08-19 at 18:01 +0200, Nirmoy Das wrote:
> 
> On 8/19/2024 1:05 PM, Matthew Auld wrote:
> > On 16/08/2024 14:51, Nirmoy Das wrote:
> > > On LNL because of flat CCS, driver creates migrates job to clear
> > > CCS meta data. Extend that to also clear system pages using GPU.
> > > Inform TTM to allocate pages without __GFP_ZERO to avoid double
> > > page
> > > clearing by clearing out TTM_TT_FLAG_ZERO_ALLOC flag and set
> > > TTM_TT_FLAG_CLEARED_ON_FREE while freeing to skip ttm pool's
> > > clear
> > > on free as XE now takes care of clearing pages. If a bo is in
> > > system
> > > placement such as BO created with
> > > DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING
> > > and there is a cpu map then for such BO gpu clear will be avoided
> > > as
> > > there is no dma mapping for such BO at that moment to create
> > > migration
> > > jobs.
> > > 
> > > Tested this patch api_overhead_benchmark_l0 from
> > > https://github.com/intel/compute-benchmarks
> > > 
> > > Without the patch:
> > > api_overhead_benchmark_l0 --testFilter=UsmMemoryAllocation:
> > > UsmMemoryAllocation(api=l0 type=Host size=4KB) 84.206 us
> > > UsmMemoryAllocation(api=l0 type=Host size=1GB) 105775.56 us
> > > erf tool top 5 entries:
> > > 71.44% api_overhead_be  [kernel.kallsyms]   [k] clear_page_erms
> > > 6.34%  api_overhead_be  [kernel.kallsyms]   [k]
> > > __pageblock_pfn_to_page
> > > 2.24%  api_overhead_be  [kernel.kallsyms]   [k] cpa_flush
> > > 2.15%  api_overhead_be  [kernel.kallsyms]   [k]
> > > pages_are_mergeable
> > > 1.94%  api_overhead_be  [kernel.kallsyms]   [k]
> > > find_next_iomem_res
> > > 
> > > With the patch:
> > > api_overhead_benchmark_l0 --testFilter=UsmMemoryAllocation:
> > > UsmMemoryAllocation(api=l0 type=Host size=4KB) 79.439 us
> > > UsmMemoryAllocation(api=l0 type=Host size=1GB) 98677.75 us
> > > Perf tool top 5 entries:
> > > 11.16% api_overhead_be  [kernel.kallsyms]   [k]
> > > __pageblock_pfn_to_page
> > > 7.85%  api_overhead_be  [kernel.kallsyms]   [k] cpa_flush
> > > 7.59%  api_overhead_be  [kernel.kallsyms]   [k]
> > > find_next_iomem_res
> > > 7.24%  api_overhead_be  [kernel.kallsyms]   [k]
> > > pages_are_mergeable
> > > 5.53%  api_overhead_be  [kernel.kallsyms]   [k] 
> > > lookup_address_in_pgd_attr
> > > 
> > > Without this patch clear_page_erms() dominates execution time
> > > which is
> > > also not pipelined with migration jobs. With this patch page
> > > clearing
> > > will get pipelined with migration job and will free CPU for more
> > > work.
> > > 
> > > v2: Handle regression on dgfx(Himal)
> > >      Update commit message as no ttm API changes needed.
> > > v3: Fix Kunit test.
> > > v4: handle data leak on cpu mmap(Thomas)
> > > v5: s/gpu_page_clear/gpu_page_clear_sys and move setting
> > >      it to xe_ttm_sys_mgr_init() and other nits (Matt Auld)
> > > v6: Disable it when init_on_alloc and/or init_on_free is
> > > active(Matt)
> > >      Use compute-benchmarks as reporter used it to report this
> > >      allocation latency issue also a proper test application than
> > > mime.
> > >      In v5, the test showed significant reduction in alloc
> > > latency but
> > >      that is not the case any more, I think this was mostly
> > > because
> > >      previous test was done on IFWI which had low mem BW from
> > > CPU.
> > > 
> > > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > 
> > Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> 
> 
> Thanks Matt.
> 
> Pushed this to drm-xe-next. The series contains a ttm pool change
> which 
> as agreed with Christian
> 
> is small enough to not cause any issue so can be pulled though drm-
> xe-next.

I have a question that was sent as a reply-to on that patch.

Thanks,
Thomas

> 
> 
> Regards,
> 
> Nirmoy
> 

Re: [PATCH v6 1/2] drm/ttm: Add a flag to allow drivers to skip clear-on-free

[Thomas Hellström]

Hi, Nirmoy, Christian

On Fri, 2024-08-16 at 15:51 +0200, Nirmoy Das wrote:
> Add TTM_TT_FLAG_CLEARED_ON_FREE, which DRM drivers can set before
> releasing backing stores if they want to skip clear-on-free.
> 
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>

What happens if two devices share the same global TTM pool
 type and one that does its own clearing. Wouldn't there be a pretty
high chance that the the device that doesn't clear its own pages
allocate non-cleared memory from the pool?

/Thomas

> ---
>  drivers/gpu/drm/ttm/ttm_pool.c | 18 +++++++++++-------
>  include/drm/ttm/ttm_tt.h       |  6 +++++-
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
> b/drivers/gpu/drm/ttm/ttm_pool.c
> index 8504dbe19c1a..935ab3cfd046 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -222,15 +222,18 @@ static void ttm_pool_unmap(struct ttm_pool
> *pool, dma_addr_t dma_addr,
>  }
>  
>  /* Give pages into a specific pool_type */
> -static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page
> *p)
> +static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page
> *p,
> +			       bool cleared)
>  {
>  	unsigned int i, num_pages = 1 << pt->order;
>  
> -	for (i = 0; i < num_pages; ++i) {
> -		if (PageHighMem(p))
> -			clear_highpage(p + i);
> -		else
> -			clear_page(page_address(p + i));
> +	if (!cleared) {
> +		for (i = 0; i < num_pages; ++i) {
> +			if (PageHighMem(p))
> +				clear_highpage(p + i);
> +			else
> +				clear_page(page_address(p + i));
> +		}
>  	}
>  
>  	spin_lock(&pt->lock);
> @@ -394,6 +397,7 @@ static void ttm_pool_free_range(struct ttm_pool
> *pool, struct ttm_tt *tt,
>  				pgoff_t start_page, pgoff_t
> end_page)
>  {
>  	struct page **pages = &tt->pages[start_page];
> +	bool cleared = tt->page_flags & TTM_TT_FLAG_CLEARED_ON_FREE;
>  	unsigned int order;
>  	pgoff_t i, nr;
>  
> @@ -407,7 +411,7 @@ static void ttm_pool_free_range(struct ttm_pool
> *pool, struct ttm_tt *tt,
>  
>  		pt = ttm_pool_select_type(pool, caching, order);
>  		if (pt)
> -			ttm_pool_type_give(pt, *pages);
> +			ttm_pool_type_give(pt, *pages, cleared);
>  		else
>  			ttm_pool_free_page(pool, caching, order,
> *pages);
>  	}
> diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
> index 2b9d856ff388..cfaf49de2419 100644
> --- a/include/drm/ttm/ttm_tt.h
> +++ b/include/drm/ttm/ttm_tt.h
> @@ -85,6 +85,9 @@ struct ttm_tt {
>  	 * fault handling abuses the DMA api a bit and dma_map_attrs
> can't be
>  	 * used to assure pgprot always matches.
>  	 *
> +	 * TTM_TT_FLAG_CLEARED_ON_FREE: Set this if a drm driver
> handles
> +	 * clearing backing store
> +	 *
>  	 * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only. DO NOT
> USE. This is
>  	 * set by TTM after ttm_tt_populate() has successfully
> returned, and is
>  	 * then unset when TTM calls ttm_tt_unpopulate().
> @@ -94,8 +97,9 @@ struct ttm_tt {
>  #define TTM_TT_FLAG_EXTERNAL		BIT(2)
>  #define TTM_TT_FLAG_EXTERNAL_MAPPABLE	BIT(3)
>  #define TTM_TT_FLAG_DECRYPTED		BIT(4)
> +#define TTM_TT_FLAG_CLEARED_ON_FREE	BIT(5)
>  
> -#define TTM_TT_FLAG_PRIV_POPULATED	BIT(5)
> +#define TTM_TT_FLAG_PRIV_POPULATED	BIT(6)
>  	uint32_t page_flags;
>  	/** @num_pages: Number of pages in the page array. */
>  	uint32_t num_pages;

Re: [PATCH v6 1/2] drm/ttm: Add a flag to allow drivers to skip clear-on-free

[Nirmoy Das]

On 8/20/2024 3:33 PM, Thomas Hellström wrote:
> Hi, Nirmoy, Christian
>
> On Fri, 2024-08-16 at 15:51 +0200, Nirmoy Das wrote:
>> Add TTM_TT_FLAG_CLEARED_ON_FREE, which DRM drivers can set before
>> releasing backing stores if they want to skip clear-on-free.
>>
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Suggested-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
> What happens if two devices share the same global TTM pool
>   type and one that does its own clearing. Wouldn't there be a pretty
> high chance that the the device that doesn't clear its own pages
> allocate non-cleared memory from the pool?


You are right, mixing such devices will poison the global pool. 
Unfortunately, I fully concentrated on single device

use-case. This is problematic mainly because on XE, we are doing clear 
on alloc so from ttm prospective the flag is correct.


A quick option would be to limit this for non-pooled allocations on XE 
or even create a separate pool for XE when have this 
gpu-system-page-clear is enabled.

Thanks for catching this, I will test above options and send patches to 
fix the xe commit.


Regards,

Nirmoy


>
> /Thomas
>
>> ---
>>   drivers/gpu/drm/ttm/ttm_pool.c | 18 +++++++++++-------
>>   include/drm/ttm/ttm_tt.h       |  6 +++++-
>>   2 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
>> b/drivers/gpu/drm/ttm/ttm_pool.c
>> index 8504dbe19c1a..935ab3cfd046 100644
>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>> @@ -222,15 +222,18 @@ static void ttm_pool_unmap(struct ttm_pool
>> *pool, dma_addr_t dma_addr,
>>   }
>>   
>>   /* Give pages into a specific pool_type */
>> -static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page
>> *p)
>> +static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page
>> *p,
>> +			       bool cleared)
>>   {
>>   	unsigned int i, num_pages = 1 << pt->order;
>>   
>> -	for (i = 0; i < num_pages; ++i) {
>> -		if (PageHighMem(p))
>> -			clear_highpage(p + i);
>> -		else
>> -			clear_page(page_address(p + i));
>> +	if (!cleared) {
>> +		for (i = 0; i < num_pages; ++i) {
>> +			if (PageHighMem(p))
>> +				clear_highpage(p + i);
>> +			else
>> +				clear_page(page_address(p + i));
>> +		}
>>   	}
>>   
>>   	spin_lock(&pt->lock);
>> @@ -394,6 +397,7 @@ static void ttm_pool_free_range(struct ttm_pool
>> *pool, struct ttm_tt *tt,
>>   				pgoff_t start_page, pgoff_t
>> end_page)
>>   {
>>   	struct page **pages = &tt->pages[start_page];
>> +	bool cleared = tt->page_flags & TTM_TT_FLAG_CLEARED_ON_FREE;
>>   	unsigned int order;
>>   	pgoff_t i, nr;
>>   
>> @@ -407,7 +411,7 @@ static void ttm_pool_free_range(struct ttm_pool
>> *pool, struct ttm_tt *tt,
>>   
>>   		pt = ttm_pool_select_type(pool, caching, order);
>>   		if (pt)
>> -			ttm_pool_type_give(pt, *pages);
>> +			ttm_pool_type_give(pt, *pages, cleared);
>>   		else
>>   			ttm_pool_free_page(pool, caching, order,
>> *pages);
>>   	}
>> diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
>> index 2b9d856ff388..cfaf49de2419 100644
>> --- a/include/drm/ttm/ttm_tt.h
>> +++ b/include/drm/ttm/ttm_tt.h
>> @@ -85,6 +85,9 @@ struct ttm_tt {
>>   	 * fault handling abuses the DMA api a bit and dma_map_attrs
>> can't be
>>   	 * used to assure pgprot always matches.
>>   	 *
>> +	 * TTM_TT_FLAG_CLEARED_ON_FREE: Set this if a drm driver
>> handles
>> +	 * clearing backing store
>> +	 *
>>   	 * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only. DO NOT
>> USE. This is
>>   	 * set by TTM after ttm_tt_populate() has successfully
>> returned, and is
>>   	 * then unset when TTM calls ttm_tt_unpopulate().
>> @@ -94,8 +97,9 @@ struct ttm_tt {
>>   #define TTM_TT_FLAG_EXTERNAL		BIT(2)
>>   #define TTM_TT_FLAG_EXTERNAL_MAPPABLE	BIT(3)
>>   #define TTM_TT_FLAG_DECRYPTED		BIT(4)
>> +#define TTM_TT_FLAG_CLEARED_ON_FREE	BIT(5)
>>   
>> -#define TTM_TT_FLAG_PRIV_POPULATED	BIT(5)
>> +#define TTM_TT_FLAG_PRIV_POPULATED	BIT(6)
>>   	uint32_t page_flags;
>>   	/** @num_pages: Number of pages in the page array. */
>>   	uint32_t num_pages;

Re: [PATCH v6 1/2] drm/ttm: Add a flag to allow drivers to skip clear-on-free

[Christian König]

Am 20.08.24 um 15:33 schrieb Thomas Hellström:
> Hi, Nirmoy, Christian
>
> On Fri, 2024-08-16 at 15:51 +0200, Nirmoy Das wrote:
>> Add TTM_TT_FLAG_CLEARED_ON_FREE, which DRM drivers can set before
>> releasing backing stores if they want to skip clear-on-free.
>>
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Suggested-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
> What happens if two devices share the same global TTM pool
>   type and one that does its own clearing. Wouldn't there be a pretty
> high chance that the the device that doesn't clear its own pages
> allocate non-cleared memory from the pool?

That's completely unproblematic. The flag indicates that the released 
pages are already cleared, if that isn't the case then the flag 
shouldn't be set on the TT object.

If one device clear it's pages and another device doesn't clear it's 
pages then we would just clear the pages of the device which doesn't do 
it with a hardware DMA.

Regards,
Christian.

>
> /Thomas
>
>> ---
>>   drivers/gpu/drm/ttm/ttm_pool.c | 18 +++++++++++-------
>>   include/drm/ttm/ttm_tt.h       |  6 +++++-
>>   2 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
>> b/drivers/gpu/drm/ttm/ttm_pool.c
>> index 8504dbe19c1a..935ab3cfd046 100644
>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>> @@ -222,15 +222,18 @@ static void ttm_pool_unmap(struct ttm_pool
>> *pool, dma_addr_t dma_addr,
>>   }
>>   
>>   /* Give pages into a specific pool_type */
>> -static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page
>> *p)
>> +static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page
>> *p,
>> +			       bool cleared)
>>   {
>>   	unsigned int i, num_pages = 1 << pt->order;
>>   
>> -	for (i = 0; i < num_pages; ++i) {
>> -		if (PageHighMem(p))
>> -			clear_highpage(p + i);
>> -		else
>> -			clear_page(page_address(p + i));
>> +	if (!cleared) {
>> +		for (i = 0; i < num_pages; ++i) {
>> +			if (PageHighMem(p))
>> +				clear_highpage(p + i);
>> +			else
>> +				clear_page(page_address(p + i));
>> +		}
>>   	}
>>   
>>   	spin_lock(&pt->lock);
>> @@ -394,6 +397,7 @@ static void ttm_pool_free_range(struct ttm_pool
>> *pool, struct ttm_tt *tt,
>>   				pgoff_t start_page, pgoff_t
>> end_page)
>>   {
>>   	struct page **pages = &tt->pages[start_page];
>> +	bool cleared = tt->page_flags & TTM_TT_FLAG_CLEARED_ON_FREE;
>>   	unsigned int order;
>>   	pgoff_t i, nr;
>>   
>> @@ -407,7 +411,7 @@ static void ttm_pool_free_range(struct ttm_pool
>> *pool, struct ttm_tt *tt,
>>   
>>   		pt = ttm_pool_select_type(pool, caching, order);
>>   		if (pt)
>> -			ttm_pool_type_give(pt, *pages);
>> +			ttm_pool_type_give(pt, *pages, cleared);
>>   		else
>>   			ttm_pool_free_page(pool, caching, order,
>> *pages);
>>   	}
>> diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
>> index 2b9d856ff388..cfaf49de2419 100644
>> --- a/include/drm/ttm/ttm_tt.h
>> +++ b/include/drm/ttm/ttm_tt.h
>> @@ -85,6 +85,9 @@ struct ttm_tt {
>>   	 * fault handling abuses the DMA api a bit and dma_map_attrs
>> can't be
>>   	 * used to assure pgprot always matches.
>>   	 *
>> +	 * TTM_TT_FLAG_CLEARED_ON_FREE: Set this if a drm driver
>> handles
>> +	 * clearing backing store
>> +	 *
>>   	 * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only. DO NOT
>> USE. This is
>>   	 * set by TTM after ttm_tt_populate() has successfully
>> returned, and is
>>   	 * then unset when TTM calls ttm_tt_unpopulate().
>> @@ -94,8 +97,9 @@ struct ttm_tt {
>>   #define TTM_TT_FLAG_EXTERNAL		BIT(2)
>>   #define TTM_TT_FLAG_EXTERNAL_MAPPABLE	BIT(3)
>>   #define TTM_TT_FLAG_DECRYPTED		BIT(4)
>> +#define TTM_TT_FLAG_CLEARED_ON_FREE	BIT(5)
>>   
>> -#define TTM_TT_FLAG_PRIV_POPULATED	BIT(5)
>> +#define TTM_TT_FLAG_PRIV_POPULATED	BIT(6)
>>   	uint32_t page_flags;
>>   	/** @num_pages: Number of pages in the page array. */
>>   	uint32_t num_pages;

Re: [PATCH v6 1/2] drm/ttm: Add a flag to allow drivers to skip clear-on-free

[Thomas Hellström]

On Tue, 2024-08-20 at 17:30 +0200, Christian König wrote:
> Am 20.08.24 um 15:33 schrieb Thomas Hellström:
> > Hi, Nirmoy, Christian
> > 
> > On Fri, 2024-08-16 at 15:51 +0200, Nirmoy Das wrote:
> > > Add TTM_TT_FLAG_CLEARED_ON_FREE, which DRM drivers can set before
> > > releasing backing stores if they want to skip clear-on-free.
> > > 
> > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > Suggested-by: Christian König <christian.koenig@amd.com>
> > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > > Reviewed-by: Christian König <christian.koenig@amd.com>
> > What happens if two devices share the same global TTM pool
> >   type and one that does its own clearing. Wouldn't there be a
> > pretty
> > high chance that the the device that doesn't clear its own pages
> > allocate non-cleared memory from the pool?
> 
> That's completely unproblematic. The flag indicates that the released
> pages are already cleared, if that isn't the case then the flag 
> shouldn't be set on the TT object.

Yeah, this patch is OK, but the way the follow-up xe patch uses it is
problematic since, AFAICT, xe dma clears on alloc, meaning the pool
pages are not cleared after use.

/Thomas

> 
> If one device clear it's pages and another device doesn't clear it's 
> pages then we would just clear the pages of the device which doesn't
> do 
> it with a hardware DMA.
> 
> Regards,
> Christian.
> 
> > 
> > /Thomas
> > 
> > > ---
> > >   drivers/gpu/drm/ttm/ttm_pool.c | 18 +++++++++++-------
> > >   include/drm/ttm/ttm_tt.h       |  6 +++++-
> > >   2 files changed, 16 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
> > > b/drivers/gpu/drm/ttm/ttm_pool.c
> > > index 8504dbe19c1a..935ab3cfd046 100644
> > > --- a/drivers/gpu/drm/ttm/ttm_pool.c
> > > +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> > > @@ -222,15 +222,18 @@ static void ttm_pool_unmap(struct ttm_pool
> > > *pool, dma_addr_t dma_addr,
> > >   }
> > >   
> > >   /* Give pages into a specific pool_type */
> > > -static void ttm_pool_type_give(struct ttm_pool_type *pt, struct
> > > page
> > > *p)
> > > +static void ttm_pool_type_give(struct ttm_pool_type *pt, struct
> > > page
> > > *p,
> > > +			       bool cleared)
> > >   {
> > >   	unsigned int i, num_pages = 1 << pt->order;
> > >   
> > > -	for (i = 0; i < num_pages; ++i) {
> > > -		if (PageHighMem(p))
> > > -			clear_highpage(p + i);
> > > -		else
> > > -			clear_page(page_address(p + i));
> > > +	if (!cleared) {
> > > +		for (i = 0; i < num_pages; ++i) {
> > > +			if (PageHighMem(p))
> > > +				clear_highpage(p + i);
> > > +			else
> > > +				clear_page(page_address(p + i));
> > > +		}
> > >   	}
> > >   
> > >   	spin_lock(&pt->lock);
> > > @@ -394,6 +397,7 @@ static void ttm_pool_free_range(struct
> > > ttm_pool
> > > *pool, struct ttm_tt *tt,
> > >   				pgoff_t start_page, pgoff_t
> > > end_page)
> > >   {
> > >   	struct page **pages = &tt->pages[start_page];
> > > +	bool cleared = tt->page_flags &
> > > TTM_TT_FLAG_CLEARED_ON_FREE;
> > >   	unsigned int order;
> > >   	pgoff_t i, nr;
> > >   
> > > @@ -407,7 +411,7 @@ static void ttm_pool_free_range(struct
> > > ttm_pool
> > > *pool, struct ttm_tt *tt,
> > >   
> > >   		pt = ttm_pool_select_type(pool, caching, order);
> > >   		if (pt)
> > > -			ttm_pool_type_give(pt, *pages);
> > > +			ttm_pool_type_give(pt, *pages, cleared);
> > >   		else
> > >   			ttm_pool_free_page(pool, caching, order,
> > > *pages);
> > >   	}
> > > diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
> > > index 2b9d856ff388..cfaf49de2419 100644
> > > --- a/include/drm/ttm/ttm_tt.h
> > > +++ b/include/drm/ttm/ttm_tt.h
> > > @@ -85,6 +85,9 @@ struct ttm_tt {
> > >   	 * fault handling abuses the DMA api a bit and
> > > dma_map_attrs
> > > can't be
> > >   	 * used to assure pgprot always matches.
> > >   	 *
> > > +	 * TTM_TT_FLAG_CLEARED_ON_FREE: Set this if a drm driver
> > > handles
> > > +	 * clearing backing store
> > > +	 *
> > >   	 * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only. DO NOT
> > > USE. This is
> > >   	 * set by TTM after ttm_tt_populate() has successfully
> > > returned, and is
> > >   	 * then unset when TTM calls ttm_tt_unpopulate().
> > > @@ -94,8 +97,9 @@ struct ttm_tt {
> > >   #define TTM_TT_FLAG_EXTERNAL		BIT(2)
> > >   #define TTM_TT_FLAG_EXTERNAL_MAPPABLE	BIT(3)
> > >   #define TTM_TT_FLAG_DECRYPTED		BIT(4)
> > > +#define TTM_TT_FLAG_CLEARED_ON_FREE	BIT(5)
> > >   
> > > -#define TTM_TT_FLAG_PRIV_POPULATED	BIT(5)
> > > +#define TTM_TT_FLAG_PRIV_POPULATED	BIT(6)
> > >   	uint32_t page_flags;
> > >   	/** @num_pages: Number of pages in the page array. */
> > >   	uint32_t num_pages;
> 

Re: [PATCH v6 1/2] drm/ttm: Add a flag to allow drivers to skip clear-on-free

[Christian König]

Am 20.08.24 um 17:45 schrieb Thomas Hellström:
> On Tue, 2024-08-20 at 17:30 +0200, Christian König wrote:
>> Am 20.08.24 um 15:33 schrieb Thomas Hellström:
>>> Hi, Nirmoy, Christian
>>>
>>> On Fri, 2024-08-16 at 15:51 +0200, Nirmoy Das wrote:
>>>> Add TTM_TT_FLAG_CLEARED_ON_FREE, which DRM drivers can set before
>>>> releasing backing stores if they want to skip clear-on-free.
>>>>
>>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>> What happens if two devices share the same global TTM pool
>>>    type and one that does its own clearing. Wouldn't there be a
>>> pretty
>>> high chance that the the device that doesn't clear its own pages
>>> allocate non-cleared memory from the pool?
>> That's completely unproblematic. The flag indicates that the released
>> pages are already cleared, if that isn't the case then the flag
>> shouldn't be set on the TT object.
> Yeah, this patch is OK, but the way the follow-up xe patch uses it is
> problematic since, AFAICT, xe dma clears on alloc, meaning the pool
> pages are not cleared after use.

Yeah that is clearly invalid behavior.

Regards,
Christian.

>
> /Thomas
>
>> If one device clear it's pages and another device doesn't clear it's
>> pages then we would just clear the pages of the device which doesn't
>> do
>> it with a hardware DMA.
>>
>> Regards,
>> Christian.
>>
>>> /Thomas
>>>
>>>> ---
>>>>    drivers/gpu/drm/ttm/ttm_pool.c | 18 +++++++++++-------
>>>>    include/drm/ttm/ttm_tt.h       |  6 +++++-
>>>>    2 files changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
>>>> b/drivers/gpu/drm/ttm/ttm_pool.c
>>>> index 8504dbe19c1a..935ab3cfd046 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>>>> @@ -222,15 +222,18 @@ static void ttm_pool_unmap(struct ttm_pool
>>>> *pool, dma_addr_t dma_addr,
>>>>    }
>>>>    
>>>>    /* Give pages into a specific pool_type */
>>>> -static void ttm_pool_type_give(struct ttm_pool_type *pt, struct
>>>> page
>>>> *p)
>>>> +static void ttm_pool_type_give(struct ttm_pool_type *pt, struct
>>>> page
>>>> *p,
>>>> +			       bool cleared)
>>>>    {
>>>>    	unsigned int i, num_pages = 1 << pt->order;
>>>>    
>>>> -	for (i = 0; i < num_pages; ++i) {
>>>> -		if (PageHighMem(p))
>>>> -			clear_highpage(p + i);
>>>> -		else
>>>> -			clear_page(page_address(p + i));
>>>> +	if (!cleared) {
>>>> +		for (i = 0; i < num_pages; ++i) {
>>>> +			if (PageHighMem(p))
>>>> +				clear_highpage(p + i);
>>>> +			else
>>>> +				clear_page(page_address(p + i));
>>>> +		}
>>>>    	}
>>>>    
>>>>    	spin_lock(&pt->lock);
>>>> @@ -394,6 +397,7 @@ static void ttm_pool_free_range(struct
>>>> ttm_pool
>>>> *pool, struct ttm_tt *tt,
>>>>    				pgoff_t start_page, pgoff_t
>>>> end_page)
>>>>    {
>>>>    	struct page **pages = &tt->pages[start_page];
>>>> +	bool cleared = tt->page_flags &
>>>> TTM_TT_FLAG_CLEARED_ON_FREE;
>>>>    	unsigned int order;
>>>>    	pgoff_t i, nr;
>>>>    
>>>> @@ -407,7 +411,7 @@ static void ttm_pool_free_range(struct
>>>> ttm_pool
>>>> *pool, struct ttm_tt *tt,
>>>>    
>>>>    		pt = ttm_pool_select_type(pool, caching, order);
>>>>    		if (pt)
>>>> -			ttm_pool_type_give(pt, *pages);
>>>> +			ttm_pool_type_give(pt, *pages, cleared);
>>>>    		else
>>>>    			ttm_pool_free_page(pool, caching, order,
>>>> *pages);
>>>>    	}
>>>> diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
>>>> index 2b9d856ff388..cfaf49de2419 100644
>>>> --- a/include/drm/ttm/ttm_tt.h
>>>> +++ b/include/drm/ttm/ttm_tt.h
>>>> @@ -85,6 +85,9 @@ struct ttm_tt {
>>>>    	 * fault handling abuses the DMA api a bit and
>>>> dma_map_attrs
>>>> can't be
>>>>    	 * used to assure pgprot always matches.
>>>>    	 *
>>>> +	 * TTM_TT_FLAG_CLEARED_ON_FREE: Set this if a drm driver
>>>> handles
>>>> +	 * clearing backing store
>>>> +	 *
>>>>    	 * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only. DO NOT
>>>> USE. This is
>>>>    	 * set by TTM after ttm_tt_populate() has successfully
>>>> returned, and is
>>>>    	 * then unset when TTM calls ttm_tt_unpopulate().
>>>> @@ -94,8 +97,9 @@ struct ttm_tt {
>>>>    #define TTM_TT_FLAG_EXTERNAL		BIT(2)
>>>>    #define TTM_TT_FLAG_EXTERNAL_MAPPABLE	BIT(3)
>>>>    #define TTM_TT_FLAG_DECRYPTED		BIT(4)
>>>> +#define TTM_TT_FLAG_CLEARED_ON_FREE	BIT(5)
>>>>    
>>>> -#define TTM_TT_FLAG_PRIV_POPULATED	BIT(5)
>>>> +#define TTM_TT_FLAG_PRIV_POPULATED	BIT(6)
>>>>    	uint32_t page_flags;
>>>>    	/** @num_pages: Number of pages in the page array. */
>>>>    	uint32_t num_pages;

Re: [PATCH v6 1/2] drm/ttm: Add a flag to allow drivers to skip clear-on-free

[Nirmoy Das]

Hi Thomas, Christian,

On 8/20/2024 5:47 PM, Christian König wrote:
> Am 20.08.24 um 17:45 schrieb Thomas Hellström:
>> On Tue, 2024-08-20 at 17:30 +0200, Christian König wrote:
>>> Am 20.08.24 um 15:33 schrieb Thomas Hellström:
>>>> Hi, Nirmoy, Christian
>>>>
>>>> On Fri, 2024-08-16 at 15:51 +0200, Nirmoy Das wrote:
>>>>> Add TTM_TT_FLAG_CLEARED_ON_FREE, which DRM drivers can set before
>>>>> releasing backing stores if they want to skip clear-on-free.
>>>>>
>>>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>> What happens if two devices share the same global TTM pool
>>>>    type and one that does its own clearing. Wouldn't there be a
>>>> pretty
>>>> high chance that the the device that doesn't clear its own pages
>>>> allocate non-cleared memory from the pool?
>>> That's completely unproblematic. The flag indicates that the released
>>> pages are already cleared, if that isn't the case then the flag
>>> shouldn't be set on the TT object.
>> Yeah, this patch is OK, but the way the follow-up xe patch uses it is
>> problematic since, AFAICT, xe dma clears on alloc, meaning the pool
>> pages are not cleared after use.
>
> Yeah that is clearly invalid behavior.


I was only thinking about one device use-case which won't leak any data 
though I am now miss-using the flag.

If I skip dma clear for pooled BO then this flag  is not really needed. 
Shall I revert the this and usage of TTM_TT_FLAG_CLEARED_ON_FREE

and re-introduce it after I get a working clear on free implementation 
for XE?


Regards,

Nirmoy


>
> Regards,
> Christian.
>
>>
>> /Thomas
>>
>>> If one device clear it's pages and another device doesn't clear it's
>>> pages then we would just clear the pages of the device which doesn't
>>> do
>>> it with a hardware DMA.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> /Thomas
>>>>
>>>>> ---
>>>>>    drivers/gpu/drm/ttm/ttm_pool.c | 18 +++++++++++-------
>>>>>    include/drm/ttm/ttm_tt.h       |  6 +++++-
>>>>>    2 files changed, 16 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
>>>>> b/drivers/gpu/drm/ttm/ttm_pool.c
>>>>> index 8504dbe19c1a..935ab3cfd046 100644
>>>>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>>>>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>>>>> @@ -222,15 +222,18 @@ static void ttm_pool_unmap(struct ttm_pool
>>>>> *pool, dma_addr_t dma_addr,
>>>>>    }
>>>>>       /* Give pages into a specific pool_type */
>>>>> -static void ttm_pool_type_give(struct ttm_pool_type *pt, struct
>>>>> page
>>>>> *p)
>>>>> +static void ttm_pool_type_give(struct ttm_pool_type *pt, struct
>>>>> page
>>>>> *p,
>>>>> +                   bool cleared)
>>>>>    {
>>>>>        unsigned int i, num_pages = 1 << pt->order;
>>>>>    -    for (i = 0; i < num_pages; ++i) {
>>>>> -        if (PageHighMem(p))
>>>>> -            clear_highpage(p + i);
>>>>> -        else
>>>>> -            clear_page(page_address(p + i));
>>>>> +    if (!cleared) {
>>>>> +        for (i = 0; i < num_pages; ++i) {
>>>>> +            if (PageHighMem(p))
>>>>> +                clear_highpage(p + i);
>>>>> +            else
>>>>> +                clear_page(page_address(p + i));
>>>>> +        }
>>>>>        }
>>>>>           spin_lock(&pt->lock);
>>>>> @@ -394,6 +397,7 @@ static void ttm_pool_free_range(struct
>>>>> ttm_pool
>>>>> *pool, struct ttm_tt *tt,
>>>>>                    pgoff_t start_page, pgoff_t
>>>>> end_page)
>>>>>    {
>>>>>        struct page **pages = &tt->pages[start_page];
>>>>> +    bool cleared = tt->page_flags &
>>>>> TTM_TT_FLAG_CLEARED_ON_FREE;
>>>>>        unsigned int order;
>>>>>        pgoff_t i, nr;
>>>>>    @@ -407,7 +411,7 @@ static void ttm_pool_free_range(struct
>>>>> ttm_pool
>>>>> *pool, struct ttm_tt *tt,
>>>>>               pt = ttm_pool_select_type(pool, caching, order);
>>>>>            if (pt)
>>>>> -            ttm_pool_type_give(pt, *pages);
>>>>> +            ttm_pool_type_give(pt, *pages, cleared);
>>>>>            else
>>>>>                ttm_pool_free_page(pool, caching, order,
>>>>> *pages);
>>>>>        }
>>>>> diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
>>>>> index 2b9d856ff388..cfaf49de2419 100644
>>>>> --- a/include/drm/ttm/ttm_tt.h
>>>>> +++ b/include/drm/ttm/ttm_tt.h
>>>>> @@ -85,6 +85,9 @@ struct ttm_tt {
>>>>>         * fault handling abuses the DMA api a bit and
>>>>> dma_map_attrs
>>>>> can't be
>>>>>         * used to assure pgprot always matches.
>>>>>         *
>>>>> +     * TTM_TT_FLAG_CLEARED_ON_FREE: Set this if a drm driver
>>>>> handles
>>>>> +     * clearing backing store
>>>>> +     *
>>>>>         * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only. DO NOT
>>>>> USE. This is
>>>>>         * set by TTM after ttm_tt_populate() has successfully
>>>>> returned, and is
>>>>>         * then unset when TTM calls ttm_tt_unpopulate().
>>>>> @@ -94,8 +97,9 @@ struct ttm_tt {
>>>>>    #define TTM_TT_FLAG_EXTERNAL        BIT(2)
>>>>>    #define TTM_TT_FLAG_EXTERNAL_MAPPABLE    BIT(3)
>>>>>    #define TTM_TT_FLAG_DECRYPTED        BIT(4)
>>>>> +#define TTM_TT_FLAG_CLEARED_ON_FREE    BIT(5)
>>>>>    -#define TTM_TT_FLAG_PRIV_POPULATED    BIT(5)
>>>>> +#define TTM_TT_FLAG_PRIV_POPULATED    BIT(6)
>>>>>        uint32_t page_flags;
>>>>>        /** @num_pages: Number of pages in the page array. */
>>>>>        uint32_t num_pages;
>

Re: [PATCH v6 1/2] drm/ttm: Add a flag to allow drivers to skip clear-on-free

[Christian König]

Am 20.08.24 um 18:46 schrieb Nirmoy Das:
> Hi Thomas, Christian,
>
> On 8/20/2024 5:47 PM, Christian König wrote:
>> Am 20.08.24 um 17:45 schrieb Thomas Hellström:
>>> On Tue, 2024-08-20 at 17:30 +0200, Christian König wrote:
>>>> Am 20.08.24 um 15:33 schrieb Thomas Hellström:
>>>>> Hi, Nirmoy, Christian
>>>>>
>>>>> On Fri, 2024-08-16 at 15:51 +0200, Nirmoy Das wrote:
>>>>>> Add TTM_TT_FLAG_CLEARED_ON_FREE, which DRM drivers can set before
>>>>>> releasing backing stores if they want to skip clear-on-free.
>>>>>>
>>>>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>> What happens if two devices share the same global TTM pool
>>>>>    type and one that does its own clearing. Wouldn't there be a
>>>>> pretty
>>>>> high chance that the the device that doesn't clear its own pages
>>>>> allocate non-cleared memory from the pool?
>>>> That's completely unproblematic. The flag indicates that the released
>>>> pages are already cleared, if that isn't the case then the flag
>>>> shouldn't be set on the TT object.
>>> Yeah, this patch is OK, but the way the follow-up xe patch uses it is
>>> problematic since, AFAICT, xe dma clears on alloc, meaning the pool
>>> pages are not cleared after use.
>>
>> Yeah that is clearly invalid behavior.
>
>
> I was only thinking about one device use-case which won't leak any 
> data though I am now miss-using the flag.
>
> If I skip dma clear for pooled BO then this flag  is not really 
> needed. Shall I revert the this and usage of TTM_TT_FLAG_CLEARED_ON_FREE
>
> and re-introduce it after I get a working clear on free implementation 
> for XE?

Yes absolutely.

I though that I made it clear that the handling should be that the 
driver clears the pages and *then* sets the TTM_TT_FLAG_CLEARED_ON_FREE 
flag.

So if you don't have the handling implemented like that then that's 
clearly invalid behavior.

Regards,
Christian.

>
>
> Regards,
>
> Nirmoy
>
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> /Thomas
>>>
>>>> If one device clear it's pages and another device doesn't clear it's
>>>> pages then we would just clear the pages of the device which doesn't
>>>> do
>>>> it with a hardware DMA.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> /Thomas
>>>>>
>>>>>> ---
>>>>>>    drivers/gpu/drm/ttm/ttm_pool.c | 18 +++++++++++-------
>>>>>>    include/drm/ttm/ttm_tt.h       |  6 +++++-
>>>>>>    2 files changed, 16 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
>>>>>> b/drivers/gpu/drm/ttm/ttm_pool.c
>>>>>> index 8504dbe19c1a..935ab3cfd046 100644
>>>>>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>>>>>> @@ -222,15 +222,18 @@ static void ttm_pool_unmap(struct ttm_pool
>>>>>> *pool, dma_addr_t dma_addr,
>>>>>>    }
>>>>>>       /* Give pages into a specific pool_type */
>>>>>> -static void ttm_pool_type_give(struct ttm_pool_type *pt, struct
>>>>>> page
>>>>>> *p)
>>>>>> +static void ttm_pool_type_give(struct ttm_pool_type *pt, struct
>>>>>> page
>>>>>> *p,
>>>>>> +                   bool cleared)
>>>>>>    {
>>>>>>        unsigned int i, num_pages = 1 << pt->order;
>>>>>>    -    for (i = 0; i < num_pages; ++i) {
>>>>>> -        if (PageHighMem(p))
>>>>>> -            clear_highpage(p + i);
>>>>>> -        else
>>>>>> -            clear_page(page_address(p + i));
>>>>>> +    if (!cleared) {
>>>>>> +        for (i = 0; i < num_pages; ++i) {
>>>>>> +            if (PageHighMem(p))
>>>>>> +                clear_highpage(p + i);
>>>>>> +            else
>>>>>> +                clear_page(page_address(p + i));
>>>>>> +        }
>>>>>>        }
>>>>>>           spin_lock(&pt->lock);
>>>>>> @@ -394,6 +397,7 @@ static void ttm_pool_free_range(struct
>>>>>> ttm_pool
>>>>>> *pool, struct ttm_tt *tt,
>>>>>>                    pgoff_t start_page, pgoff_t
>>>>>> end_page)
>>>>>>    {
>>>>>>        struct page **pages = &tt->pages[start_page];
>>>>>> +    bool cleared = tt->page_flags &
>>>>>> TTM_TT_FLAG_CLEARED_ON_FREE;
>>>>>>        unsigned int order;
>>>>>>        pgoff_t i, nr;
>>>>>>    @@ -407,7 +411,7 @@ static void ttm_pool_free_range(struct
>>>>>> ttm_pool
>>>>>> *pool, struct ttm_tt *tt,
>>>>>>               pt = ttm_pool_select_type(pool, caching, order);
>>>>>>            if (pt)
>>>>>> -            ttm_pool_type_give(pt, *pages);
>>>>>> +            ttm_pool_type_give(pt, *pages, cleared);
>>>>>>            else
>>>>>>                ttm_pool_free_page(pool, caching, order,
>>>>>> *pages);
>>>>>>        }
>>>>>> diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
>>>>>> index 2b9d856ff388..cfaf49de2419 100644
>>>>>> --- a/include/drm/ttm/ttm_tt.h
>>>>>> +++ b/include/drm/ttm/ttm_tt.h
>>>>>> @@ -85,6 +85,9 @@ struct ttm_tt {
>>>>>>         * fault handling abuses the DMA api a bit and
>>>>>> dma_map_attrs
>>>>>> can't be
>>>>>>         * used to assure pgprot always matches.
>>>>>>         *
>>>>>> +     * TTM_TT_FLAG_CLEARED_ON_FREE: Set this if a drm driver
>>>>>> handles
>>>>>> +     * clearing backing store
>>>>>> +     *
>>>>>>         * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only. DO NOT
>>>>>> USE. This is
>>>>>>         * set by TTM after ttm_tt_populate() has successfully
>>>>>> returned, and is
>>>>>>         * then unset when TTM calls ttm_tt_unpopulate().
>>>>>> @@ -94,8 +97,9 @@ struct ttm_tt {
>>>>>>    #define TTM_TT_FLAG_EXTERNAL        BIT(2)
>>>>>>    #define TTM_TT_FLAG_EXTERNAL_MAPPABLE    BIT(3)
>>>>>>    #define TTM_TT_FLAG_DECRYPTED        BIT(4)
>>>>>> +#define TTM_TT_FLAG_CLEARED_ON_FREE    BIT(5)
>>>>>>    -#define TTM_TT_FLAG_PRIV_POPULATED    BIT(5)
>>>>>> +#define TTM_TT_FLAG_PRIV_POPULATED    BIT(6)
>>>>>>        uint32_t page_flags;
>>>>>>        /** @num_pages: Number of pages in the page array. */
>>>>>>        uint32_t num_pages;
>>

Re: [PATCH v6 1/2] drm/ttm: Add a flag to allow drivers to skip clear-on-free

[Thomas Hellström]

On Wed, 2024-08-21 at 09:47 +0200, Christian König wrote:
> Am 20.08.24 um 18:46 schrieb Nirmoy Das:
> > Hi Thomas, Christian,
> > 
> > On 8/20/2024 5:47 PM, Christian König wrote:
> > > Am 20.08.24 um 17:45 schrieb Thomas Hellström:
> > > > On Tue, 2024-08-20 at 17:30 +0200, Christian König wrote:
> > > > > Am 20.08.24 um 15:33 schrieb Thomas Hellström:
> > > > > > Hi, Nirmoy, Christian
> > > > > > 
> > > > > > On Fri, 2024-08-16 at 15:51 +0200, Nirmoy Das wrote:
> > > > > > > Add TTM_TT_FLAG_CLEARED_ON_FREE, which DRM drivers can
> > > > > > > set before
> > > > > > > releasing backing stores if they want to skip clear-on-
> > > > > > > free.
> > > > > > > 
> > > > > > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > > > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > > > > > Suggested-by: Christian König <christian.koenig@amd.com>
> > > > > > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > > > > > > Reviewed-by: Christian König <christian.koenig@amd.com>
> > > > > > What happens if two devices share the same global TTM pool
> > > > > >    type and one that does its own clearing. Wouldn't there
> > > > > > be a
> > > > > > pretty
> > > > > > high chance that the the device that doesn't clear its own
> > > > > > pages
> > > > > > allocate non-cleared memory from the pool?
> > > > > That's completely unproblematic. The flag indicates that the
> > > > > released
> > > > > pages are already cleared, if that isn't the case then the
> > > > > flag
> > > > > shouldn't be set on the TT object.
> > > > Yeah, this patch is OK, but the way the follow-up xe patch uses
> > > > it is
> > > > problematic since, AFAICT, xe dma clears on alloc, meaning the
> > > > pool
> > > > pages are not cleared after use.
> > > 
> > > Yeah that is clearly invalid behavior.
> > 
> > 
> > I was only thinking about one device use-case which won't leak any 
> > data though I am now miss-using the flag.
> > 
> > If I skip dma clear for pooled BO then this flag  is not really 
> > needed. Shall I revert the this and usage of
> > TTM_TT_FLAG_CLEARED_ON_FREE
> > 
> > and re-introduce it after I get a working clear on free
> > implementation 
> > for XE?
> 
> Yes absolutely.
> 
> I though that I made it clear that the handling should be that the 
> driver clears the pages and *then* sets the
> TTM_TT_FLAG_CLEARED_ON_FREE 
> flag.
> 
> So if you don't have the handling implemented like that then that's 
> clearly invalid behavior.
> 
> Regards,
> Christian.

I agree.
Revert and re-introduce as needed, and obtain an ack from Christian to
merge through drm-xe-next before re-introduction so that it doesn't
clash with anything planned elsewhere.

Thanks,
Thomas



> 
> > 
> > 
> > Regards,
> > 
> > Nirmoy
> > 
> > 
> > > 
> > > Regards,
> > > Christian.
> > > 
> > > > 
> > > > /Thomas
> > > > 
> > > > > If one device clear it's pages and another device doesn't
> > > > > clear it's
> > > > > pages then we would just clear the pages of the device which
> > > > > doesn't
> > > > > do
> > > > > it with a hardware DMA.
> > > > > 
> > > > > Regards,
> > > > > Christian.
> > > > > 
> > > > > > /Thomas
> > > > > > 
> > > > > > > ---
> > > > > > >    drivers/gpu/drm/ttm/ttm_pool.c | 18 +++++++++++-------
> > > > > > >    include/drm/ttm/ttm_tt.h       |  6 +++++-
> > > > > > >    2 files changed, 16 insertions(+), 8 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
> > > > > > > b/drivers/gpu/drm/ttm/ttm_pool.c
> > > > > > > index 8504dbe19c1a..935ab3cfd046 100644
> > > > > > > --- a/drivers/gpu/drm/ttm/ttm_pool.c
> > > > > > > +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> > > > > > > @@ -222,15 +222,18 @@ static void ttm_pool_unmap(struct
> > > > > > > ttm_pool
> > > > > > > *pool, dma_addr_t dma_addr,
> > > > > > >    }
> > > > > > >       /* Give pages into a specific pool_type */
> > > > > > > -static void ttm_pool_type_give(struct ttm_pool_type *pt,
> > > > > > > struct
> > > > > > > page
> > > > > > > *p)
> > > > > > > +static void ttm_pool_type_give(struct ttm_pool_type *pt,
> > > > > > > struct
> > > > > > > page
> > > > > > > *p,
> > > > > > > +                   bool cleared)
> > > > > > >    {
> > > > > > >        unsigned int i, num_pages = 1 << pt->order;
> > > > > > >    -    for (i = 0; i < num_pages; ++i) {
> > > > > > > -        if (PageHighMem(p))
> > > > > > > -            clear_highpage(p + i);
> > > > > > > -        else
> > > > > > > -            clear_page(page_address(p + i));
> > > > > > > +    if (!cleared) {
> > > > > > > +        for (i = 0; i < num_pages; ++i) {
> > > > > > > +            if (PageHighMem(p))
> > > > > > > +                clear_highpage(p + i);
> > > > > > > +            else
> > > > > > > +                clear_page(page_address(p + i));
> > > > > > > +        }
> > > > > > >        }
> > > > > > >           spin_lock(&pt->lock);
> > > > > > > @@ -394,6 +397,7 @@ static void
> > > > > > > ttm_pool_free_range(struct
> > > > > > > ttm_pool
> > > > > > > *pool, struct ttm_tt *tt,
> > > > > > >                    pgoff_t start_page, pgoff_t
> > > > > > > end_page)
> > > > > > >    {
> > > > > > >        struct page **pages = &tt->pages[start_page];
> > > > > > > +    bool cleared = tt->page_flags &
> > > > > > > TTM_TT_FLAG_CLEARED_ON_FREE;
> > > > > > >        unsigned int order;
> > > > > > >        pgoff_t i, nr;
> > > > > > >    @@ -407,7 +411,7 @@ static void
> > > > > > > ttm_pool_free_range(struct
> > > > > > > ttm_pool
> > > > > > > *pool, struct ttm_tt *tt,
> > > > > > >               pt = ttm_pool_select_type(pool, caching,
> > > > > > > order);
> > > > > > >            if (pt)
> > > > > > > -            ttm_pool_type_give(pt, *pages);
> > > > > > > +            ttm_pool_type_give(pt, *pages, cleared);
> > > > > > >            else
> > > > > > >                ttm_pool_free_page(pool, caching, order,
> > > > > > > *pages);
> > > > > > >        }
> > > > > > > diff --git a/include/drm/ttm/ttm_tt.h
> > > > > > > b/include/drm/ttm/ttm_tt.h
> > > > > > > index 2b9d856ff388..cfaf49de2419 100644
> > > > > > > --- a/include/drm/ttm/ttm_tt.h
> > > > > > > +++ b/include/drm/ttm/ttm_tt.h
> > > > > > > @@ -85,6 +85,9 @@ struct ttm_tt {
> > > > > > >         * fault handling abuses the DMA api a bit and
> > > > > > > dma_map_attrs
> > > > > > > can't be
> > > > > > >         * used to assure pgprot always matches.
> > > > > > >         *
> > > > > > > +     * TTM_TT_FLAG_CLEARED_ON_FREE: Set this if a drm
> > > > > > > driver
> > > > > > > handles
> > > > > > > +     * clearing backing store
> > > > > > > +     *
> > > > > > >         * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only.
> > > > > > > DO NOT
> > > > > > > USE. This is
> > > > > > >         * set by TTM after ttm_tt_populate() has
> > > > > > > successfully
> > > > > > > returned, and is
> > > > > > >         * then unset when TTM calls ttm_tt_unpopulate().
> > > > > > > @@ -94,8 +97,9 @@ struct ttm_tt {
> > > > > > >    #define TTM_TT_FLAG_EXTERNAL        BIT(2)
> > > > > > >    #define TTM_TT_FLAG_EXTERNAL_MAPPABLE    BIT(3)
> > > > > > >    #define TTM_TT_FLAG_DECRYPTED        BIT(4)
> > > > > > > +#define TTM_TT_FLAG_CLEARED_ON_FREE    BIT(5)
> > > > > > >    -#define TTM_TT_FLAG_PRIV_POPULATED    BIT(5)
> > > > > > > +#define TTM_TT_FLAG_PRIV_POPULATED    BIT(6)
> > > > > > >        uint32_t page_flags;
> > > > > > >        /** @num_pages: Number of pages in the page array.
> > > > > > > */
> > > > > > >        uint32_t num_pages;
> > > 
> 

Re: [PATCH v6 1/2] drm/ttm: Add a flag to allow drivers to skip clear-on-free

[Nirmoy Das]

[-- Attachment #1: Type: text/plain, Size: 7400 bytes --]


On 8/21/2024 10:08 AM, Thomas Hellström wrote:
> On Wed, 2024-08-21 at 09:47 +0200, Christian König wrote:
>> Am 20.08.24 um 18:46 schrieb Nirmoy Das:
>>> Hi Thomas, Christian,
>>>
>>> On 8/20/2024 5:47 PM, Christian König wrote:
>>>> Am 20.08.24 um 17:45 schrieb Thomas Hellström:
>>>>> On Tue, 2024-08-20 at 17:30 +0200, Christian König wrote:
>>>>>> Am 20.08.24 um 15:33 schrieb Thomas Hellström:
>>>>>>> Hi, Nirmoy, Christian
>>>>>>>
>>>>>>> On Fri, 2024-08-16 at 15:51 +0200, Nirmoy Das wrote:
>>>>>>>> Add TTM_TT_FLAG_CLEARED_ON_FREE, which DRM drivers can
>>>>>>>> set before
>>>>>>>> releasing backing stores if they want to skip clear-on-
>>>>>>>> free.
>>>>>>>>
>>>>>>>> Cc: Matthew Auld<matthew.auld@intel.com>
>>>>>>>> Cc: Thomas Hellström<thomas.hellstrom@linux.intel.com>
>>>>>>>> Suggested-by: Christian König<christian.koenig@amd.com>
>>>>>>>> Signed-off-by: Nirmoy Das<nirmoy.das@intel.com>
>>>>>>>> Reviewed-by: Christian König<christian.koenig@amd.com>
>>>>>>> What happens if two devices share the same global TTM pool
>>>>>>>     type and one that does its own clearing. Wouldn't there
>>>>>>> be a
>>>>>>> pretty
>>>>>>> high chance that the the device that doesn't clear its own
>>>>>>> pages
>>>>>>> allocate non-cleared memory from the pool?
>>>>>> That's completely unproblematic. The flag indicates that the
>>>>>> released
>>>>>> pages are already cleared, if that isn't the case then the
>>>>>> flag
>>>>>> shouldn't be set on the TT object.
>>>>> Yeah, this patch is OK, but the way the follow-up xe patch uses
>>>>> it is
>>>>> problematic since, AFAICT, xe dma clears on alloc, meaning the
>>>>> pool
>>>>> pages are not cleared after use.
>>>> Yeah that is clearly invalid behavior.
>>>
>>> I was only thinking about one device use-case which won't leak any
>>> data though I am now miss-using the flag.
>>>
>>> If I skip dma clear for pooled BO then this flag  is not really
>>> needed. Shall I revert the this and usage of
>>> TTM_TT_FLAG_CLEARED_ON_FREE
>>>
>>> and re-introduce it after I get a working clear on free
>>> implementation
>>> for XE?
>> Yes absolutely.
>>
>> I though that I made it clear that the handling should be that the
>> driver clears the pages and *then* sets the
>> TTM_TT_FLAG_CLEARED_ON_FREE
>> flag.
>>
>> So if you don't have the handling implemented like that then that's
>> clearly invalid behavior.
>>
>> Regards,
>> Christian.
> I agree.
> Revert and re-introduce as needed, and obtain an ack from Christian to
> merge through drm-xe-next before re-introduction so that it doesn't
> clash with anything planned elsewhere.


Sent a series to revert the usages TTM_TT_FLAG_CLEARED_ON_FREE.

Thanks both of you for your time and patience,

Nirmoy

>
> Thanks,
> Thomas
>
>
>
>>>
>>> Regards,
>>>
>>> Nirmoy
>>>
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> /Thomas
>>>>>
>>>>>> If one device clear it's pages and another device doesn't
>>>>>> clear it's
>>>>>> pages then we would just clear the pages of the device which
>>>>>> doesn't
>>>>>> do
>>>>>> it with a hardware DMA.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> /Thomas
>>>>>>>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/ttm/ttm_pool.c | 18 +++++++++++-------
>>>>>>>>     include/drm/ttm/ttm_tt.h       |  6 +++++-
>>>>>>>>     2 files changed, 16 insertions(+), 8 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
>>>>>>>> b/drivers/gpu/drm/ttm/ttm_pool.c
>>>>>>>> index 8504dbe19c1a..935ab3cfd046 100644
>>>>>>>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>>>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>>>>>>>> @@ -222,15 +222,18 @@ static void ttm_pool_unmap(struct
>>>>>>>> ttm_pool
>>>>>>>> *pool, dma_addr_t dma_addr,
>>>>>>>>     }
>>>>>>>>        /* Give pages into a specific pool_type */
>>>>>>>> -static void ttm_pool_type_give(struct ttm_pool_type *pt,
>>>>>>>> struct
>>>>>>>> page
>>>>>>>> *p)
>>>>>>>> +static void ttm_pool_type_give(struct ttm_pool_type *pt,
>>>>>>>> struct
>>>>>>>> page
>>>>>>>> *p,
>>>>>>>> +                   bool cleared)
>>>>>>>>     {
>>>>>>>>         unsigned int i, num_pages = 1 << pt->order;
>>>>>>>>     -    for (i = 0; i < num_pages; ++i) {
>>>>>>>> -        if (PageHighMem(p))
>>>>>>>> -            clear_highpage(p + i);
>>>>>>>> -        else
>>>>>>>> -            clear_page(page_address(p + i));
>>>>>>>> +    if (!cleared) {
>>>>>>>> +        for (i = 0; i < num_pages; ++i) {
>>>>>>>> +            if (PageHighMem(p))
>>>>>>>> +                clear_highpage(p + i);
>>>>>>>> +            else
>>>>>>>> +                clear_page(page_address(p + i));
>>>>>>>> +        }
>>>>>>>>         }
>>>>>>>>            spin_lock(&pt->lock);
>>>>>>>> @@ -394,6 +397,7 @@ static void
>>>>>>>> ttm_pool_free_range(struct
>>>>>>>> ttm_pool
>>>>>>>> *pool, struct ttm_tt *tt,
>>>>>>>>                     pgoff_t start_page, pgoff_t
>>>>>>>> end_page)
>>>>>>>>     {
>>>>>>>>         struct page **pages = &tt->pages[start_page];
>>>>>>>> +    bool cleared = tt->page_flags &
>>>>>>>> TTM_TT_FLAG_CLEARED_ON_FREE;
>>>>>>>>         unsigned int order;
>>>>>>>>         pgoff_t i, nr;
>>>>>>>>     @@ -407,7 +411,7 @@ static void
>>>>>>>> ttm_pool_free_range(struct
>>>>>>>> ttm_pool
>>>>>>>> *pool, struct ttm_tt *tt,
>>>>>>>>                pt = ttm_pool_select_type(pool, caching,
>>>>>>>> order);
>>>>>>>>             if (pt)
>>>>>>>> -            ttm_pool_type_give(pt, *pages);
>>>>>>>> +            ttm_pool_type_give(pt, *pages, cleared);
>>>>>>>>             else
>>>>>>>>                 ttm_pool_free_page(pool, caching, order,
>>>>>>>> *pages);
>>>>>>>>         }
>>>>>>>> diff --git a/include/drm/ttm/ttm_tt.h
>>>>>>>> b/include/drm/ttm/ttm_tt.h
>>>>>>>> index 2b9d856ff388..cfaf49de2419 100644
>>>>>>>> --- a/include/drm/ttm/ttm_tt.h
>>>>>>>> +++ b/include/drm/ttm/ttm_tt.h
>>>>>>>> @@ -85,6 +85,9 @@ struct ttm_tt {
>>>>>>>>          * fault handling abuses the DMA api a bit and
>>>>>>>> dma_map_attrs
>>>>>>>> can't be
>>>>>>>>          * used to assure pgprot always matches.
>>>>>>>>          *
>>>>>>>> +     * TTM_TT_FLAG_CLEARED_ON_FREE: Set this if a drm
>>>>>>>> driver
>>>>>>>> handles
>>>>>>>> +     * clearing backing store
>>>>>>>> +     *
>>>>>>>>          * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only.
>>>>>>>> DO NOT
>>>>>>>> USE. This is
>>>>>>>>          * set by TTM after ttm_tt_populate() has
>>>>>>>> successfully
>>>>>>>> returned, and is
>>>>>>>>          * then unset when TTM calls ttm_tt_unpopulate().
>>>>>>>> @@ -94,8 +97,9 @@ struct ttm_tt {
>>>>>>>>     #define TTM_TT_FLAG_EXTERNAL        BIT(2)
>>>>>>>>     #define TTM_TT_FLAG_EXTERNAL_MAPPABLE    BIT(3)
>>>>>>>>     #define TTM_TT_FLAG_DECRYPTED        BIT(4)
>>>>>>>> +#define TTM_TT_FLAG_CLEARED_ON_FREE    BIT(5)
>>>>>>>>     -#define TTM_TT_FLAG_PRIV_POPULATED    BIT(5)
>>>>>>>> +#define TTM_TT_FLAG_PRIV_POPULATED    BIT(6)
>>>>>>>>         uint32_t page_flags;
>>>>>>>>         /** @num_pages: Number of pages in the page array.
>>>>>>>> */
>>>>>>>>         uint32_t num_pages;

[-- Attachment #2: Type: text/html, Size: 10994 bytes --]