| Message ID | 20230215161405.187368-3-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a TTM shrinker | expand |
Am 15.02.23 um 17:13 schrieb Thomas Hellström: > When hitting an error, the error path forgot to unmap dma mappings and I don't see where this happens? > could call set_pages_wb() on already uncached pages. Yeah, but what's the problem? Regards, Christian. > > Fix this by introducing a common __ttm_pool_free() function that > does the right thing. > > Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3") > Cc: Christian König <christian.koenig@amd.com> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Madhav Chauhan <madhav.chauhan@amd.com> > Cc: Christian Koenig <christian.koenig@amd.com> > Cc: Huang Rui <ray.huang@amd.com> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/ttm/ttm_pool.c | 74 +++++++++++++++++++++------------- > 1 file changed, 45 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index aa116a7bbae3..1cc7591a9542 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > @@ -367,6 +367,39 @@ static int ttm_pool_page_allocated(struct ttm_pool *pool, unsigned int order, > return 0; > } > > +static void __ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt, > + struct page **caching_divide, > + enum ttm_caching initial_caching, > + enum ttm_caching subseq_caching, > + pgoff_t num_pages) > +{ > + enum ttm_caching caching = subseq_caching; > + struct page **pages = tt->pages; > + unsigned int order; > + pgoff_t i, nr; > + > + if (pool && caching_divide) > + caching = initial_caching; > + > + for (i = 0; i < num_pages; i += nr, pages += nr) { > + struct ttm_pool_type *pt = NULL; > + > + if (unlikely(caching_divide == pages)) > + caching = subseq_caching; > + > + order = ttm_pool_page_order(pool, *pages); > + nr = (1UL << order); > + if (tt->dma_address) > + ttm_pool_unmap(pool, tt->dma_address[i], nr); > + > + pt = ttm_pool_select_type(pool, caching, order); > + if (pt) > + ttm_pool_type_give(pt, *pages); > + else > + ttm_pool_free_page(pool, caching, order, *pages); > + } > +} > + > /** > * ttm_pool_alloc - Fill a ttm_tt object > * > @@ -386,8 +419,9 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, > dma_addr_t *dma_addr = tt->dma_address; > struct page **caching = tt->pages; > struct page **pages = tt->pages; > + enum ttm_caching page_caching; > gfp_t gfp_flags = GFP_USER; > - unsigned int i, order; > + unsigned int order; > struct page *p; > int r; > > @@ -410,6 +444,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, > order = min_t(unsigned int, order, __fls(num_pages))) { > struct ttm_pool_type *pt; > > + page_caching = tt->caching; > pt = ttm_pool_select_type(pool, tt->caching, order); > p = pt ? ttm_pool_type_take(pt) : NULL; > if (p) { > @@ -418,6 +453,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, > if (r) > goto error_free_page; > > + caching = pages; > do { > r = ttm_pool_page_allocated(pool, order, p, > &dma_addr, > @@ -426,14 +462,15 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, > if (r) > goto error_free_page; > > + caching = pages; > if (num_pages < (1 << order)) > break; > > p = ttm_pool_type_take(pt); > } while (p); > - caching = pages; > } > > + page_caching = ttm_cached; > while (num_pages >= (1 << order) && > (p = ttm_pool_alloc_page(pool, gfp_flags, order))) { > > @@ -442,6 +479,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, > tt->caching); > if (r) > goto error_free_page; > + caching = pages; > } > r = ttm_pool_page_allocated(pool, order, p, &dma_addr, > &num_pages, &pages); > @@ -468,15 +506,12 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, > return 0; > > error_free_page: > - ttm_pool_free_page(pool, tt->caching, order, p); > + ttm_pool_free_page(pool, page_caching, order, p); > > error_free_all: > num_pages = tt->num_pages - num_pages; > - for (i = 0; i < num_pages; ) { > - order = ttm_pool_page_order(pool, tt->pages[i]); > - ttm_pool_free_page(pool, tt->caching, order, tt->pages[i]); > - i += 1 << order; > - } > + __ttm_pool_free(pool, tt, caching, tt->caching, ttm_cached, > + num_pages); > > return r; > } > @@ -492,27 +527,8 @@ EXPORT_SYMBOL(ttm_pool_alloc); > */ > void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt) > { > - unsigned int i; > - > - for (i = 0; i < tt->num_pages; ) { > - struct page *p = tt->pages[i]; > - unsigned int order, num_pages; > - struct ttm_pool_type *pt; > - > - order = ttm_pool_page_order(pool, p); > - num_pages = 1ULL << order; > - if (tt->dma_address) > - ttm_pool_unmap(pool, tt->dma_address[i], num_pages); > - > - pt = ttm_pool_select_type(pool, tt->caching, order); > - if (pt) > - ttm_pool_type_give(pt, tt->pages[i]); > - else > - ttm_pool_free_page(pool, tt->caching, order, > - tt->pages[i]); > - > - i += num_pages; > - } > + __ttm_pool_free(pool, tt, NULL, tt->caching, tt->caching, > + tt->num_pages); > > while (atomic_long_read(&allocated_pages) > page_pool_size) > ttm_pool_shrink();
On Wed, 2023-02-15 at 18:31 +0100, Christian König wrote: > Am 15.02.23 um 17:13 schrieb Thomas Hellström: > > When hitting an error, the error path forgot to unmap dma mappings > > and > > I don't see where this happens? From what I can tell, ttm_pool_page_allocated() maps the page for dma, If we later hit an error, ttm_pool_free_page() will leak the mapping. > > > could call set_pages_wb() on already uncached pages. > > Yeah, but what's the problem? Umm, at least if you try to set WC on an already WC'd page, the set_pages_ code will spam dmesg with warnings. Not sure if set_pages_wb() on WB pages does the same, nor if it issues unnecessary global cache / tlb flushes or whether that will change in the future. The point of avoiding the set_pages_wb() when already WB is you don't have to check, and you don't have to care. That said, the __ttm_pool_free() is used also in upcoming patches. /Thomas > > Regards, > Christian. > > > > > Fix this by introducing a common __ttm_pool_free() function that > > does the right thing. > > > > Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3") > > Cc: Christian König <christian.koenig@amd.com> > > Cc: Dave Airlie <airlied@redhat.com> > > Cc: Madhav Chauhan <madhav.chauhan@amd.com> > > Cc: Christian Koenig <christian.koenig@amd.com> > > Cc: Huang Rui <ray.huang@amd.com> > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > --- > > drivers/gpu/drm/ttm/ttm_pool.c | 74 +++++++++++++++++++++-------- > > ----- > > 1 file changed, 45 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c > > b/drivers/gpu/drm/ttm/ttm_pool.c > > index aa116a7bbae3..1cc7591a9542 100644 > > --- a/drivers/gpu/drm/ttm/ttm_pool.c > > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > > @@ -367,6 +367,39 @@ static int ttm_pool_page_allocated(struct > > ttm_pool *pool, unsigned int order, > > return 0; > > } > > > > +static void __ttm_pool_free(struct ttm_pool *pool, struct ttm_tt > > *tt, > > + struct page **caching_divide, > > + enum ttm_caching initial_caching, > > + enum ttm_caching subseq_caching, > > + pgoff_t num_pages) > > +{ > > + enum ttm_caching caching = subseq_caching; > > + struct page **pages = tt->pages; > > + unsigned int order; > > + pgoff_t i, nr; > > + > > + if (pool && caching_divide) > > + caching = initial_caching; > > + > > + for (i = 0; i < num_pages; i += nr, pages += nr) { > > + struct ttm_pool_type *pt = NULL; > > + > > + if (unlikely(caching_divide == pages)) > > + caching = subseq_caching; > > + > > + order = ttm_pool_page_order(pool, *pages); > > + nr = (1UL << order); > > + if (tt->dma_address) > > + ttm_pool_unmap(pool, tt->dma_address[i], > > nr); > > + > > + pt = ttm_pool_select_type(pool, caching, order); > > + if (pt) > > + ttm_pool_type_give(pt, *pages); > > + else > > + ttm_pool_free_page(pool, caching, order, > > *pages); > > + } > > +} > > + > > /** > > * ttm_pool_alloc - Fill a ttm_tt object > > * > > @@ -386,8 +419,9 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > struct ttm_tt *tt, > > dma_addr_t *dma_addr = tt->dma_address; > > struct page **caching = tt->pages; > > struct page **pages = tt->pages; > > + enum ttm_caching page_caching; > > gfp_t gfp_flags = GFP_USER; > > - unsigned int i, order; > > + unsigned int order; > > struct page *p; > > int r; > > > > @@ -410,6 +444,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > struct ttm_tt *tt, > > order = min_t(unsigned int, order, __fls(num_pages))) > > { > > struct ttm_pool_type *pt; > > > > + page_caching = tt->caching; > > pt = ttm_pool_select_type(pool, tt->caching, > > order); > > p = pt ? ttm_pool_type_take(pt) : NULL; > > if (p) { > > @@ -418,6 +453,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > struct ttm_tt *tt, > > if (r) > > goto error_free_page; > > > > + caching = pages; > > do { > > r = ttm_pool_page_allocated(pool, > > order, p, > > > > &dma_addr, > > @@ -426,14 +462,15 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > struct ttm_tt *tt, > > if (r) > > goto error_free_page; > > > > + caching = pages; > > if (num_pages < (1 << order)) > > break; > > > > p = ttm_pool_type_take(pt); > > } while (p); > > - caching = pages; > > } > > > > + page_caching = ttm_cached; > > while (num_pages >= (1 << order) && > > (p = ttm_pool_alloc_page(pool, gfp_flags, > > order))) { > > > > @@ -442,6 +479,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > struct ttm_tt *tt, > > tt- > > >caching); > > if (r) > > goto error_free_page; > > + caching = pages; > > } > > r = ttm_pool_page_allocated(pool, order, p, > > &dma_addr, > > &num_pages, > > &pages); > > @@ -468,15 +506,12 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > struct ttm_tt *tt, > > return 0; > > > > error_free_page: > > - ttm_pool_free_page(pool, tt->caching, order, p); > > + ttm_pool_free_page(pool, page_caching, order, p); > > > > error_free_all: > > num_pages = tt->num_pages - num_pages; > > - for (i = 0; i < num_pages; ) { > > - order = ttm_pool_page_order(pool, tt->pages[i]); > > - ttm_pool_free_page(pool, tt->caching, order, tt- > > >pages[i]); > > - i += 1 << order; > > - } > > + __ttm_pool_free(pool, tt, caching, tt->caching, ttm_cached, > > + num_pages); > > > > return r; > > } > > @@ -492,27 +527,8 @@ EXPORT_SYMBOL(ttm_pool_alloc); > > */ > > void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt) > > { > > - unsigned int i; > > - > > - for (i = 0; i < tt->num_pages; ) { > > - struct page *p = tt->pages[i]; > > - unsigned int order, num_pages; > > - struct ttm_pool_type *pt; > > - > > - order = ttm_pool_page_order(pool, p); > > - num_pages = 1ULL << order; > > - if (tt->dma_address) > > - ttm_pool_unmap(pool, tt->dma_address[i], > > num_pages); > > - > > - pt = ttm_pool_select_type(pool, tt->caching, > > order); > > - if (pt) > > - ttm_pool_type_give(pt, tt->pages[i]); > > - else > > - ttm_pool_free_page(pool, tt->caching, > > order, > > - tt->pages[i]); > > - > > - i += num_pages; > > - } > > + __ttm_pool_free(pool, tt, NULL, tt->caching, tt->caching, > > + tt->num_pages); > > > > while (atomic_long_read(&allocated_pages) > page_pool_size) > > ttm_pool_shrink(); >
Am 15.02.23 um 19:02 schrieb Thomas Hellström: > On Wed, 2023-02-15 at 18:31 +0100, Christian König wrote: >> Am 15.02.23 um 17:13 schrieb Thomas Hellström: >>> When hitting an error, the error path forgot to unmap dma mappings >>> and >> I don't see where this happens? > From what I can tell, ttm_pool_page_allocated() maps the page for dma, > If we later hit an error, ttm_pool_free_page() will leak the mapping. Ah, I see. Good point. > >>> could call set_pages_wb() on already uncached pages. >> Yeah, but what's the problem? > Umm, at least if you try to set WC on an already WC'd page, the > set_pages_ code will spam dmesg with warnings. > Not sure if set_pages_wb() on WB pages does the same, nor if it > issues unnecessary global cache / tlb flushes or whether that will > change in the future. > The point of avoiding the set_pages_wb() when already WB is you don't > have to check, and you don't have to care. Please just open code the error handling then. That helper function looks horrible complicated to me. Alternatively we could have a free function for a range of pages. Regards, Christian. > > That said, the __ttm_pool_free() is used also in upcoming patches. > > /Thomas > > >> Regards, >> Christian. >> >>> Fix this by introducing a common __ttm_pool_free() function that >>> does the right thing. >>> >>> Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3") >>> Cc: Christian König <christian.koenig@amd.com> >>> Cc: Dave Airlie <airlied@redhat.com> >>> Cc: Madhav Chauhan <madhav.chauhan@amd.com> >>> Cc: Christian Koenig <christian.koenig@amd.com> >>> Cc: Huang Rui <ray.huang@amd.com> >>> Cc: dri-devel@lists.freedesktop.org >>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> --- >>> drivers/gpu/drm/ttm/ttm_pool.c | 74 +++++++++++++++++++++-------- >>> ----- >>> 1 file changed, 45 insertions(+), 29 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c >>> b/drivers/gpu/drm/ttm/ttm_pool.c >>> index aa116a7bbae3..1cc7591a9542 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_pool.c >>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c >>> @@ -367,6 +367,39 @@ static int ttm_pool_page_allocated(struct >>> ttm_pool *pool, unsigned int order, >>> return 0; >>> } >>> >>> +static void __ttm_pool_free(struct ttm_pool *pool, struct ttm_tt >>> *tt, >>> + struct page **caching_divide, >>> + enum ttm_caching initial_caching, >>> + enum ttm_caching subseq_caching, >>> + pgoff_t num_pages) >>> +{ >>> + enum ttm_caching caching = subseq_caching; >>> + struct page **pages = tt->pages; >>> + unsigned int order; >>> + pgoff_t i, nr; >>> + >>> + if (pool && caching_divide) >>> + caching = initial_caching; >>> + >>> + for (i = 0; i < num_pages; i += nr, pages += nr) { >>> + struct ttm_pool_type *pt = NULL; >>> + >>> + if (unlikely(caching_divide == pages)) >>> + caching = subseq_caching; >>> + >>> + order = ttm_pool_page_order(pool, *pages); >>> + nr = (1UL << order); >>> + if (tt->dma_address) >>> + ttm_pool_unmap(pool, tt->dma_address[i], >>> nr); >>> + >>> + pt = ttm_pool_select_type(pool, caching, order); >>> + if (pt) >>> + ttm_pool_type_give(pt, *pages); >>> + else >>> + ttm_pool_free_page(pool, caching, order, >>> *pages); >>> + } >>> +} >>> + >>> /** >>> * ttm_pool_alloc - Fill a ttm_tt object >>> * >>> @@ -386,8 +419,9 @@ int ttm_pool_alloc(struct ttm_pool *pool, >>> struct ttm_tt *tt, >>> dma_addr_t *dma_addr = tt->dma_address; >>> struct page **caching = tt->pages; >>> struct page **pages = tt->pages; >>> + enum ttm_caching page_caching; >>> gfp_t gfp_flags = GFP_USER; >>> - unsigned int i, order; >>> + unsigned int order; >>> struct page *p; >>> int r; >>> >>> @@ -410,6 +444,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, >>> struct ttm_tt *tt, >>> order = min_t(unsigned int, order, __fls(num_pages))) >>> { >>> struct ttm_pool_type *pt; >>> >>> + page_caching = tt->caching; >>> pt = ttm_pool_select_type(pool, tt->caching, >>> order); >>> p = pt ? ttm_pool_type_take(pt) : NULL; >>> if (p) { >>> @@ -418,6 +453,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, >>> struct ttm_tt *tt, >>> if (r) >>> goto error_free_page; >>> >>> + caching = pages; >>> do { >>> r = ttm_pool_page_allocated(pool, >>> order, p, >>> >>> &dma_addr, >>> @@ -426,14 +462,15 @@ int ttm_pool_alloc(struct ttm_pool *pool, >>> struct ttm_tt *tt, >>> if (r) >>> goto error_free_page; >>> >>> + caching = pages; >>> if (num_pages < (1 << order)) >>> break; >>> >>> p = ttm_pool_type_take(pt); >>> } while (p); >>> - caching = pages; >>> } >>> >>> + page_caching = ttm_cached; >>> while (num_pages >= (1 << order) && >>> (p = ttm_pool_alloc_page(pool, gfp_flags, >>> order))) { >>> >>> @@ -442,6 +479,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, >>> struct ttm_tt *tt, >>> tt- >>>> caching); >>> if (r) >>> goto error_free_page; >>> + caching = pages; >>> } >>> r = ttm_pool_page_allocated(pool, order, p, >>> &dma_addr, >>> &num_pages, >>> &pages); >>> @@ -468,15 +506,12 @@ int ttm_pool_alloc(struct ttm_pool *pool, >>> struct ttm_tt *tt, >>> return 0; >>> >>> error_free_page: >>> - ttm_pool_free_page(pool, tt->caching, order, p); >>> + ttm_pool_free_page(pool, page_caching, order, p); >>> >>> error_free_all: >>> num_pages = tt->num_pages - num_pages; >>> - for (i = 0; i < num_pages; ) { >>> - order = ttm_pool_page_order(pool, tt->pages[i]); >>> - ttm_pool_free_page(pool, tt->caching, order, tt- >>>> pages[i]); >>> - i += 1 << order; >>> - } >>> + __ttm_pool_free(pool, tt, caching, tt->caching, ttm_cached, >>> + num_pages); >>> >>> return r; >>> } >>> @@ -492,27 +527,8 @@ EXPORT_SYMBOL(ttm_pool_alloc); >>> */ >>> void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt) >>> { >>> - unsigned int i; >>> - >>> - for (i = 0; i < tt->num_pages; ) { >>> - struct page *p = tt->pages[i]; >>> - unsigned int order, num_pages; >>> - struct ttm_pool_type *pt; >>> - >>> - order = ttm_pool_page_order(pool, p); >>> - num_pages = 1ULL << order; >>> - if (tt->dma_address) >>> - ttm_pool_unmap(pool, tt->dma_address[i], >>> num_pages); >>> - >>> - pt = ttm_pool_select_type(pool, tt->caching, >>> order); >>> - if (pt) >>> - ttm_pool_type_give(pt, tt->pages[i]); >>> - else >>> - ttm_pool_free_page(pool, tt->caching, >>> order, >>> - tt->pages[i]); >>> - >>> - i += num_pages; >>> - } >>> + __ttm_pool_free(pool, tt, NULL, tt->caching, tt->caching, >>> + tt->num_pages); >>> >>> while (atomic_long_read(&allocated_pages) > page_pool_size) >>> ttm_pool_shrink();
On Wed, 2023-02-15 at 19:26 +0100, Christian König wrote: > Am 15.02.23 um 19:02 schrieb Thomas Hellström: > > On Wed, 2023-02-15 at 18:31 +0100, Christian König wrote: > > > Am 15.02.23 um 17:13 schrieb Thomas Hellström: > > > > When hitting an error, the error path forgot to unmap dma > > > > mappings > > > > and > > > I don't see where this happens? > > From what I can tell, ttm_pool_page_allocated() maps the page for > > dma, > > If we later hit an error, ttm_pool_free_page() will leak the > > mapping. > > Ah, I see. Good point. > > > > > > > could call set_pages_wb() on already uncached pages. > > > Yeah, but what's the problem? > > Umm, at least if you try to set WC on an already WC'd page, the > > set_pages_ code will spam dmesg with warnings. > > Not sure if set_pages_wb() on WB pages does the same, nor if it > > issues unnecessary global cache / tlb flushes or whether that will > > change in the future. > > The point of avoiding the set_pages_wb() when already WB is you > > don't > > have to check, and you don't have to care. > > Please just open code the error handling then. That helper function > looks horrible complicated to me. > > Alternatively we could have a free function for a range of pages. OK, I'll see if this is doable without adding a tremendous amount of code. /Thomas > > Regards, > Christian. > > > > > > That said, the __ttm_pool_free() is used also in upcoming patches. > > > > /Thomas > > > > > > > Regards, > > > Christian. > > > > > > > Fix this by introducing a common __ttm_pool_free() function > > > > that > > > > does the right thing. > > > > > > > > Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool > > > > v3") > > > > Cc: Christian König <christian.koenig@amd.com> > > > > Cc: Dave Airlie <airlied@redhat.com> > > > > Cc: Madhav Chauhan <madhav.chauhan@amd.com> > > > > Cc: Christian Koenig <christian.koenig@amd.com> > > > > Cc: Huang Rui <ray.huang@amd.com> > > > > Cc: dri-devel@lists.freedesktop.org > > > > Signed-off-by: Thomas Hellström > > > > <thomas.hellstrom@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/ttm/ttm_pool.c | 74 +++++++++++++++++++++--- > > > > ----- > > > > ----- > > > > 1 file changed, 45 insertions(+), 29 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c > > > > b/drivers/gpu/drm/ttm/ttm_pool.c > > > > index aa116a7bbae3..1cc7591a9542 100644 > > > > --- a/drivers/gpu/drm/ttm/ttm_pool.c > > > > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > > > > @@ -367,6 +367,39 @@ static int ttm_pool_page_allocated(struct > > > > ttm_pool *pool, unsigned int order, > > > > return 0; > > > > } > > > > > > > > +static void __ttm_pool_free(struct ttm_pool *pool, struct > > > > ttm_tt > > > > *tt, > > > > + struct page **caching_divide, > > > > + enum ttm_caching initial_caching, > > > > + enum ttm_caching subseq_caching, > > > > + pgoff_t num_pages) > > > > +{ > > > > + enum ttm_caching caching = subseq_caching; > > > > + struct page **pages = tt->pages; > > > > + unsigned int order; > > > > + pgoff_t i, nr; > > > > + > > > > + if (pool && caching_divide) > > > > + caching = initial_caching; > > > > + > > > > + for (i = 0; i < num_pages; i += nr, pages += nr) { > > > > + struct ttm_pool_type *pt = NULL; > > > > + > > > > + if (unlikely(caching_divide == pages)) > > > > + caching = subseq_caching; > > > > + > > > > + order = ttm_pool_page_order(pool, *pages); > > > > + nr = (1UL << order); > > > > + if (tt->dma_address) > > > > + ttm_pool_unmap(pool, tt- > > > > >dma_address[i], > > > > nr); > > > > + > > > > + pt = ttm_pool_select_type(pool, caching, > > > > order); > > > > + if (pt) > > > > + ttm_pool_type_give(pt, *pages); > > > > + else > > > > + ttm_pool_free_page(pool, caching, > > > > order, > > > > *pages); > > > > + } > > > > +} > > > > + > > > > /** > > > > * ttm_pool_alloc - Fill a ttm_tt object > > > > * > > > > @@ -386,8 +419,9 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > > > struct ttm_tt *tt, > > > > dma_addr_t *dma_addr = tt->dma_address; > > > > struct page **caching = tt->pages; > > > > struct page **pages = tt->pages; > > > > + enum ttm_caching page_caching; > > > > gfp_t gfp_flags = GFP_USER; > > > > - unsigned int i, order; > > > > + unsigned int order; > > > > struct page *p; > > > > int r; > > > > > > > > @@ -410,6 +444,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > > > struct ttm_tt *tt, > > > > order = min_t(unsigned int, order, > > > > __fls(num_pages))) > > > > { > > > > struct ttm_pool_type *pt; > > > > > > > > + page_caching = tt->caching; > > > > pt = ttm_pool_select_type(pool, tt->caching, > > > > order); > > > > p = pt ? ttm_pool_type_take(pt) : NULL; > > > > if (p) { > > > > @@ -418,6 +453,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > > > struct ttm_tt *tt, > > > > if (r) > > > > goto error_free_page; > > > > > > > > + caching = pages; > > > > do { > > > > r = > > > > ttm_pool_page_allocated(pool, > > > > order, p, > > > > > > > > &dma_addr, > > > > @@ -426,14 +462,15 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > > > struct ttm_tt *tt, > > > > if (r) > > > > goto error_free_page; > > > > > > > > + caching = pages; > > > > if (num_pages < (1 << order)) > > > > break; > > > > > > > > p = ttm_pool_type_take(pt); > > > > } while (p); > > > > - caching = pages; > > > > } > > > > > > > > + page_caching = ttm_cached; > > > > while (num_pages >= (1 << order) && > > > > (p = ttm_pool_alloc_page(pool, > > > > gfp_flags, > > > > order))) { > > > > > > > > @@ -442,6 +479,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > > > struct ttm_tt *tt, > > > > tt- > > > > > caching); > > > > if (r) > > > > goto error_free_page; > > > > + caching = pages; > > > > } > > > > r = ttm_pool_page_allocated(pool, > > > > order, p, > > > > &dma_addr, > > > > > > > > &num_pages, > > > > &pages); > > > > @@ -468,15 +506,12 @@ int ttm_pool_alloc(struct ttm_pool *pool, > > > > struct ttm_tt *tt, > > > > return 0; > > > > > > > > error_free_page: > > > > - ttm_pool_free_page(pool, tt->caching, order, p); > > > > + ttm_pool_free_page(pool, page_caching, order, p); > > > > > > > > error_free_all: > > > > num_pages = tt->num_pages - num_pages; > > > > - for (i = 0; i < num_pages; ) { > > > > - order = ttm_pool_page_order(pool, tt- > > > > >pages[i]); > > > > - ttm_pool_free_page(pool, tt->caching, order, > > > > tt- > > > > > pages[i]); > > > > - i += 1 << order; > > > > - } > > > > + __ttm_pool_free(pool, tt, caching, tt->caching, > > > > ttm_cached, > > > > + num_pages); > > > > > > > > return r; > > > > } > > > > @@ -492,27 +527,8 @@ EXPORT_SYMBOL(ttm_pool_alloc); > > > > */ > > > > void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt) > > > > { > > > > - unsigned int i; > > > > - > > > > - for (i = 0; i < tt->num_pages; ) { > > > > - struct page *p = tt->pages[i]; > > > > - unsigned int order, num_pages; > > > > - struct ttm_pool_type *pt; > > > > - > > > > - order = ttm_pool_page_order(pool, p); > > > > - num_pages = 1ULL << order; > > > > - if (tt->dma_address) > > > > - ttm_pool_unmap(pool, tt- > > > > >dma_address[i], > > > > num_pages); > > > > - > > > > - pt = ttm_pool_select_type(pool, tt->caching, > > > > order); > > > > - if (pt) > > > > - ttm_pool_type_give(pt, tt->pages[i]); > > > > - else > > > > - ttm_pool_free_page(pool, tt->caching, > > > > order, > > > > - tt->pages[i]); > > > > - > > > > - i += num_pages; > > > > - } > > > > + __ttm_pool_free(pool, tt, NULL, tt->caching, tt- > > > > >caching, > > > > + tt->num_pages); > > > > > > > > while (atomic_long_read(&allocated_pages) > > > > > page_pool_size) > > > > ttm_pool_shrink(); >
diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c index aa116a7bbae3..1cc7591a9542 100644 --- a/drivers/gpu/drm/ttm/ttm_pool.c +++ b/drivers/gpu/drm/ttm/ttm_pool.c @@ -367,6 +367,39 @@ static int ttm_pool_page_allocated(struct ttm_pool *pool, unsigned int order, return 0; } +static void __ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt, + struct page **caching_divide, + enum ttm_caching initial_caching, + enum ttm_caching subseq_caching, + pgoff_t num_pages) +{ + enum ttm_caching caching = subseq_caching; + struct page **pages = tt->pages; + unsigned int order; + pgoff_t i, nr; + + if (pool && caching_divide) + caching = initial_caching; + + for (i = 0; i < num_pages; i += nr, pages += nr) { + struct ttm_pool_type *pt = NULL; + + if (unlikely(caching_divide == pages)) + caching = subseq_caching; + + order = ttm_pool_page_order(pool, *pages); + nr = (1UL << order); + if (tt->dma_address) + ttm_pool_unmap(pool, tt->dma_address[i], nr); + + pt = ttm_pool_select_type(pool, caching, order); + if (pt) + ttm_pool_type_give(pt, *pages); + else + ttm_pool_free_page(pool, caching, order, *pages); + } +} + /** * ttm_pool_alloc - Fill a ttm_tt object * @@ -386,8 +419,9 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, dma_addr_t *dma_addr = tt->dma_address; struct page **caching = tt->pages; struct page **pages = tt->pages; + enum ttm_caching page_caching; gfp_t gfp_flags = GFP_USER; - unsigned int i, order; + unsigned int order; struct page *p; int r; @@ -410,6 +444,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, order = min_t(unsigned int, order, __fls(num_pages))) { struct ttm_pool_type *pt; + page_caching = tt->caching; pt = ttm_pool_select_type(pool, tt->caching, order); p = pt ? ttm_pool_type_take(pt) : NULL; if (p) { @@ -418,6 +453,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, if (r) goto error_free_page; + caching = pages; do { r = ttm_pool_page_allocated(pool, order, p, &dma_addr, @@ -426,14 +462,15 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, if (r) goto error_free_page; + caching = pages; if (num_pages < (1 << order)) break; p = ttm_pool_type_take(pt); } while (p); - caching = pages; } + page_caching = ttm_cached; while (num_pages >= (1 << order) && (p = ttm_pool_alloc_page(pool, gfp_flags, order))) { @@ -442,6 +479,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, tt->caching); if (r) goto error_free_page; + caching = pages; } r = ttm_pool_page_allocated(pool, order, p, &dma_addr, &num_pages, &pages); @@ -468,15 +506,12 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, return 0; error_free_page: - ttm_pool_free_page(pool, tt->caching, order, p); + ttm_pool_free_page(pool, page_caching, order, p); error_free_all: num_pages = tt->num_pages - num_pages; - for (i = 0; i < num_pages; ) { - order = ttm_pool_page_order(pool, tt->pages[i]); - ttm_pool_free_page(pool, tt->caching, order, tt->pages[i]); - i += 1 << order; - } + __ttm_pool_free(pool, tt, caching, tt->caching, ttm_cached, + num_pages); return r; } @@ -492,27 +527,8 @@ EXPORT_SYMBOL(ttm_pool_alloc); */ void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt) { - unsigned int i; - - for (i = 0; i < tt->num_pages; ) { - struct page *p = tt->pages[i]; - unsigned int order, num_pages; - struct ttm_pool_type *pt; - - order = ttm_pool_page_order(pool, p); - num_pages = 1ULL << order; - if (tt->dma_address) - ttm_pool_unmap(pool, tt->dma_address[i], num_pages); - - pt = ttm_pool_select_type(pool, tt->caching, order); - if (pt) - ttm_pool_type_give(pt, tt->pages[i]); - else - ttm_pool_free_page(pool, tt->caching, order, - tt->pages[i]); - - i += num_pages; - } + __ttm_pool_free(pool, tt, NULL, tt->caching, tt->caching, + tt->num_pages); while (atomic_long_read(&allocated_pages) > page_pool_size) ttm_pool_shrink();
When hitting an error, the error path forgot to unmap dma mappings and could call set_pages_wb() on already uncached pages. Fix this by introducing a common __ttm_pool_free() function that does the right thing. Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3") Cc: Christian König <christian.koenig@amd.com> Cc: Dave Airlie <airlied@redhat.com> Cc: Madhav Chauhan <madhav.chauhan@amd.com> Cc: Christian Koenig <christian.koenig@amd.com> Cc: Huang Rui <ray.huang@amd.com> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/ttm/ttm_pool.c | 74 +++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 29 deletions(-)