[PATCH 1/4] drm/radeon: stop using pages with drm_prime_sg_to_page_addr_arrays

[Christian König]

Am 04.11.20 um 18:38 schrieb Daniel Vetter:
> On Wed, Nov 04, 2020 at 02:00:21PM +0100, Christian König wrote:
>> This is deprecated.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
> So I tried to prove to myself that ttm doesn't access ->pages for these
> cases, and kinda couldn't. We still seem to allocate the pages array and
> all that, and there's lots of code using ->pages all over. And between
> ttm_bo_type_sg and TTM_PAGE_FLAG_SG I didn't manage to chase a whole lot
> of paths to their full conclusion.

Nope, see the amdgpu code:

> if (ttm_sg_tt_init(&gtt->ttm, bo, page_flags, caching)) {

And then what ttm_sg_tt_init() does:

>         if (page_flags & TTM_PAGE_FLAG_SG)
>                 ret = ttm_sg_tt_alloc_page_directory(ttm);
>         else
>                 ret = ttm_dma_tt_alloc_page_directory(ttm);

And then finally what ttm_sg_tt_alloc_page_directory() does:

ttm->dma_address = kvmalloc_array(ttm->num_pages,....

ttm->pages should be NULL in this case if I'm not completely mistaken.

For or imported DMA-buf s we shouldn't have a ttm->pages in amdgpu for 
quite a while and that works perfectly fine.




> So I reduced my ambitions and wanted to prove that at least for dma-buf
> imports aka ttm_bo_type_sg, we're guaranteed that we don't try to mmap
> these to userspace. And also failed to find that check.

See ttm_bo_vm_reserve():
>         /*
>          * Refuse to fault imported pages. This should be handled
>          * (if at all) by redirecting mmap to the exporter.
>          */
>         if (bo->ttm && (bo->ttm->page_flags & TTM_PAGE_FLAG_SG)) {
>                 dma_resv_unlock(bo->base.resv);
>                 return VM_FAULT_SIGBUS;
>         }


> btw this is across all drivers, mostly ttm code, not radeon specific.

Well instead of those patches we could as well switch over radeon and 
nouveau to using ttm_sg_tt_init() as well (or merge ttm_sg_tt_init() 
into ttm_dma_tt_init).

That's probably the much cleaner approach, but when I wrote 
ttm_sg_tt_init() I wasn't sure if radeon/nouveau where using ttm->pages 
for something, e.g. basically the same concern you have.

Regards,
Christian.

> So conclusion, still a mess here that at least I can't see throug clearly
> :-/ here = ttm_tt and the entire backing storage handling and everything
> that ties into it. Probably the area that still has the most midlayer feel
> to ttm with all the refactoring in-flight in still.
>
> tldr; tried to review patches 1-3, gave up.
>
> Cheers, Daniel
>
>> ---
>>   drivers/gpu/drm/radeon/radeon_ttm.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
>> index 95038ac3382e..f41fcee35f70 100644
>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>> @@ -494,8 +494,8 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_bo_device *bdev, struct ttm_tt *
>>   	if (r)
>>   		goto release_sg;
>>   
>> -	drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
>> -					 gtt->ttm.dma_address, ttm->num_pages);
>> +	drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL, gtt->ttm.dma_address,
>> +					 ttm->num_pages);
>>   
>>   	return 0;
>>   
>> @@ -673,8 +673,9 @@ static int radeon_ttm_tt_populate(struct ttm_bo_device *bdev,
>>   	}
>>   
>>   	if (slave && ttm->sg) {
>> -		drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
>> -						 gtt->ttm.dma_address, ttm->num_pages);
>> +		drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL,
>> +						 gtt->ttm.dma_address,
>> +						 ttm->num_pages);
>>   		return 0;
>>   	}
>>   
>> -- 
>> 2.25.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel