| Message ID | 1506082405-1556-1-git-send-email-deathsimple@vodafone.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Nice patch series. I wasn't expecting that so quickly. Patches 4-7 change the ttm_page_alloc allocator, which isn't used by amdgpu (as far as I can tell). Did you test those changes with a different driver? I didn't review them in as much detail as the rest. Someone with a stake in them should probably do a proper review. They are Acked-by: Felix Kuehling <Felix.Kuehling@amd.com> The rest of the series is Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com> As a possible follow up, I think ttm_vm_bo.c would need more work to do proper huge-page mappings for CPU access. Regards, Felix On 2017-09-22 08:13 AM, Christian König wrote: > From: Christian König <christian.koenig@amd.com> > > Nobody is actually using that, remove it. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/ttm/ttm_memory.c | 6 ++---- > drivers/gpu/drm/ttm/ttm_page_alloc.c | 3 +-- > drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 3 +-- > include/drm/ttm/ttm_memory.h | 3 +-- > 4 files changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_memory.c b/drivers/gpu/drm/ttm/ttm_memory.c > index 29855be..4f9978c 100644 > --- a/drivers/gpu/drm/ttm/ttm_memory.c > +++ b/drivers/gpu/drm/ttm/ttm_memory.c > @@ -546,8 +546,7 @@ int ttm_mem_global_alloc(struct ttm_mem_global *glob, uint64_t memory, > EXPORT_SYMBOL(ttm_mem_global_alloc); > > int ttm_mem_global_alloc_page(struct ttm_mem_global *glob, > - struct page *page, > - bool no_wait, bool interruptible) > + struct page *page) > { > > struct ttm_mem_zone *zone = NULL; > @@ -564,8 +563,7 @@ int ttm_mem_global_alloc_page(struct ttm_mem_global *glob, > if (glob->zone_dma32 && page_to_pfn(page) > 0x00100000UL) > zone = glob->zone_kernel; > #endif > - return ttm_mem_global_alloc_zone(glob, zone, PAGE_SIZE, no_wait, > - interruptible); > + return ttm_mem_global_alloc_zone(glob, zone, PAGE_SIZE, false, false); > } > > void ttm_mem_global_free_page(struct ttm_mem_global *glob, struct page *page) > diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c > index 052e1f1..74f465e 100644 > --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c > +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c > @@ -882,8 +882,7 @@ int ttm_pool_populate(struct ttm_tt *ttm) > return -ENOMEM; > } > > - ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i], > - false, false); > + ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i]); > if (unlikely(ret != 0)) { > ttm_pool_unpopulate(ttm); > return -ENOMEM; > diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > index 06bc14b..b8905bd 100644 > --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > @@ -902,8 +902,7 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev) > return -ENOMEM; > } > > - ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i], > - false, false); > + ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i]); > if (unlikely(ret != 0)) { > ttm_dma_unpopulate(ttm_dma, dev); > return -ENOMEM; > diff --git a/include/drm/ttm/ttm_memory.h b/include/drm/ttm/ttm_memory.h > index c452089..8184ff1 100644 > --- a/include/drm/ttm/ttm_memory.h > +++ b/include/drm/ttm/ttm_memory.h > @@ -150,8 +150,7 @@ extern int ttm_mem_global_alloc(struct ttm_mem_global *glob, uint64_t memory, > extern void ttm_mem_global_free(struct ttm_mem_global *glob, > uint64_t amount); > extern int ttm_mem_global_alloc_page(struct ttm_mem_global *glob, > - struct page *page, > - bool no_wait, bool interruptible); > + struct page *page); > extern void ttm_mem_global_free_page(struct ttm_mem_global *glob, > struct page *page); > extern size_t ttm_round_pot(size_t size);
Am 23.09.2017 um 02:01 schrieb Felix Kuehling: > Nice patch series. I wasn't expecting that so quickly. Actually I'm working on this for a few month now. > Patches 4-7 change the ttm_page_alloc allocator, which isn't used by > amdgpu (as far as I can tell). Well it is heavily used. See amdgpu_ttm_tt_populate(), if swiotlb is active we use the coherent allocator and if it isn't we assume we have iommu and use the mapping allocator. That is actually not 100% correct, cause the mapping allocator won't work on pre gfx9 if you have any memory above the 1TB limit. We already had a couple of bug reports about that. > Did you test those changes with a > different driver? I didn't review them in as much detail as the rest. > Someone with a stake in them should probably do a proper review. They > are Acked-by: Felix Kuehling <Felix.Kuehling@amd.com> > > The rest of the series is Reviewed-by: Felix Kuehling > <Felix.Kuehling@amd.com> > > As a possible follow up, I think ttm_vm_bo.c would need more work to do > proper huge-page mappings for CPU access. Correct, actually it looks like the huge page is at some point decomposed. That is not a problem for GPU huge page access (because we don't look at the compound order there), but it is rather suboptimal for CPU access. Regards, Christian. > > Regards, > Felix > > > On 2017-09-22 08:13 AM, Christian König wrote: >> From: Christian König <christian.koenig@amd.com> >> >> Nobody is actually using that, remove it. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/gpu/drm/ttm/ttm_memory.c | 6 ++---- >> drivers/gpu/drm/ttm/ttm_page_alloc.c | 3 +-- >> drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 3 +-- >> include/drm/ttm/ttm_memory.h | 3 +-- >> 4 files changed, 5 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/ttm/ttm_memory.c b/drivers/gpu/drm/ttm/ttm_memory.c >> index 29855be..4f9978c 100644 >> --- a/drivers/gpu/drm/ttm/ttm_memory.c >> +++ b/drivers/gpu/drm/ttm/ttm_memory.c >> @@ -546,8 +546,7 @@ int ttm_mem_global_alloc(struct ttm_mem_global *glob, uint64_t memory, >> EXPORT_SYMBOL(ttm_mem_global_alloc); >> >> int ttm_mem_global_alloc_page(struct ttm_mem_global *glob, >> - struct page *page, >> - bool no_wait, bool interruptible) >> + struct page *page) >> { >> >> struct ttm_mem_zone *zone = NULL; >> @@ -564,8 +563,7 @@ int ttm_mem_global_alloc_page(struct ttm_mem_global *glob, >> if (glob->zone_dma32 && page_to_pfn(page) > 0x00100000UL) >> zone = glob->zone_kernel; >> #endif >> - return ttm_mem_global_alloc_zone(glob, zone, PAGE_SIZE, no_wait, >> - interruptible); >> + return ttm_mem_global_alloc_zone(glob, zone, PAGE_SIZE, false, false); >> } >> >> void ttm_mem_global_free_page(struct ttm_mem_global *glob, struct page *page) >> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c >> index 052e1f1..74f465e 100644 >> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c >> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c >> @@ -882,8 +882,7 @@ int ttm_pool_populate(struct ttm_tt *ttm) >> return -ENOMEM; >> } >> >> - ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i], >> - false, false); >> + ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i]); >> if (unlikely(ret != 0)) { >> ttm_pool_unpopulate(ttm); >> return -ENOMEM; >> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c >> index 06bc14b..b8905bd 100644 >> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c >> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c >> @@ -902,8 +902,7 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev) >> return -ENOMEM; >> } >> >> - ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i], >> - false, false); >> + ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i]); >> if (unlikely(ret != 0)) { >> ttm_dma_unpopulate(ttm_dma, dev); >> return -ENOMEM; >> diff --git a/include/drm/ttm/ttm_memory.h b/include/drm/ttm/ttm_memory.h >> index c452089..8184ff1 100644 >> --- a/include/drm/ttm/ttm_memory.h >> +++ b/include/drm/ttm/ttm_memory.h >> @@ -150,8 +150,7 @@ extern int ttm_mem_global_alloc(struct ttm_mem_global *glob, uint64_t memory, >> extern void ttm_mem_global_free(struct ttm_mem_global *glob, >> uint64_t amount); >> extern int ttm_mem_global_alloc_page(struct ttm_mem_global *glob, >> - struct page *page, >> - bool no_wait, bool interruptible); >> + struct page *page); >> extern void ttm_mem_global_free_page(struct ttm_mem_global *glob, >> struct page *page); >> extern size_t ttm_round_pot(size_t size);
diff --git a/drivers/gpu/drm/ttm/ttm_memory.c b/drivers/gpu/drm/ttm/ttm_memory.c index 29855be..4f9978c 100644 --- a/drivers/gpu/drm/ttm/ttm_memory.c +++ b/drivers/gpu/drm/ttm/ttm_memory.c @@ -546,8 +546,7 @@ int ttm_mem_global_alloc(struct ttm_mem_global *glob, uint64_t memory, EXPORT_SYMBOL(ttm_mem_global_alloc); int ttm_mem_global_alloc_page(struct ttm_mem_global *glob, - struct page *page, - bool no_wait, bool interruptible) + struct page *page) { struct ttm_mem_zone *zone = NULL; @@ -564,8 +563,7 @@ int ttm_mem_global_alloc_page(struct ttm_mem_global *glob, if (glob->zone_dma32 && page_to_pfn(page) > 0x00100000UL) zone = glob->zone_kernel; #endif - return ttm_mem_global_alloc_zone(glob, zone, PAGE_SIZE, no_wait, - interruptible); + return ttm_mem_global_alloc_zone(glob, zone, PAGE_SIZE, false, false); } void ttm_mem_global_free_page(struct ttm_mem_global *glob, struct page *page) diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c index 052e1f1..74f465e 100644 --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c @@ -882,8 +882,7 @@ int ttm_pool_populate(struct ttm_tt *ttm) return -ENOMEM; } - ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i], - false, false); + ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i]); if (unlikely(ret != 0)) { ttm_pool_unpopulate(ttm); return -ENOMEM; diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c index 06bc14b..b8905bd 100644 --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c @@ -902,8 +902,7 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev) return -ENOMEM; } - ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i], - false, false); + ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i]); if (unlikely(ret != 0)) { ttm_dma_unpopulate(ttm_dma, dev); return -ENOMEM; diff --git a/include/drm/ttm/ttm_memory.h b/include/drm/ttm/ttm_memory.h index c452089..8184ff1 100644 --- a/include/drm/ttm/ttm_memory.h +++ b/include/drm/ttm/ttm_memory.h @@ -150,8 +150,7 @@ extern int ttm_mem_global_alloc(struct ttm_mem_global *glob, uint64_t memory, extern void ttm_mem_global_free(struct ttm_mem_global *glob, uint64_t amount); extern int ttm_mem_global_alloc_page(struct ttm_mem_global *glob, - struct page *page, - bool no_wait, bool interruptible); + struct page *page); extern void ttm_mem_global_free_page(struct ttm_mem_global *glob, struct page *page); extern size_t ttm_round_pot(size_t size);