<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 8/21/2024 10:08 AM, Thomas Hellström wrote: </div> <blockquote type="cite" cite="mid:3f2c8223b92facfa99c760dacd116fa93afada53.camel@linux.intel.com"> <pre wrap="" class="moz-quote-pre">On Wed, 2024-08-21 at 09:47 +0200, Christian König wrote: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">Am 20.08.24 um 18:46 schrieb Nirmoy Das: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">Hi Thomas, Christian, On 8/20/2024 5:47 PM, Christian König wrote: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">Am 20.08.24 um 17:45 schrieb Thomas Hellström: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">On Tue, 2024-08-20 at 17:30 +0200, Christian König wrote: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">Am 20.08.24 um 15:33 schrieb Thomas Hellström: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">Hi, Nirmoy, Christian On Fri, 2024-08-16 at 15:51 +0200, Nirmoy Das wrote: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">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 <a class="moz-txt-link-rfc2396E" href="mailto:matthew.auld@intel.com"><matthew.auld@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> Suggested-by: Christian König <a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com"><christian.koenig@amd.com></a> Signed-off-by: Nirmoy Das <a class="moz-txt-link-rfc2396E" href="mailto:nirmoy.das@intel.com"><nirmoy.das@intel.com></a> Reviewed-by: Christian König <a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com"><christian.koenig@amd.com></a> </pre> </blockquote> <pre wrap="" class="moz-quote-pre">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? </pre> </blockquote> <pre wrap="" class="moz-quote-pre">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. </pre> </blockquote> <pre wrap="" class="moz-quote-pre">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. </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> Yeah that is clearly invalid behavior. </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> 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? </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> 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. </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> 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.</pre> </blockquote> Sent a series to revert the usages TTM_TT_FLAG_CLEARED_ON_FREE. Thanks both of you for your time and patience, Nirmoy <blockquote type="cite" cite="mid:3f2c8223b92facfa99c760dacd116fa93afada53.camel@linux.intel.com"> <pre wrap="" class="moz-quote-pre"> Thanks, Thomas </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> Regards, Nirmoy </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> Regards, Christian. </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> /Thomas </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">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. </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">/Thomas </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">--- 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; </pre> </blockquote> </blockquote> </blockquote> </blockquote> <pre wrap="" class="moz-quote-pre"> </pre> </blockquote> </blockquote> <pre wrap="" class="moz-quote-pre"> </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> </pre> </blockquote> </body> </html>