Re: [PATCH] drm/ttm: nuke VM_MIXEDMAP on BO mappings

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 02.06.21 um 20:38 schrieb Daniel Vetter:
> On Wed, Jun 02, 2021 at 10:30:13AM +0200, Christian König wrote:
>> We discussed if that is really the right approach for quite a while now, but
>> digging deeper into a bug report on arm turned out that this is actually
>> horrible broken right now.
>>
>> The reason for this is that vmf_insert_mixed_prot() always tries to grab
>> a reference to the underlaying page on architectures without
>> ARCH_HAS_PTE_SPECIAL and as far as I can see also enabled GUP.
>>
>> So nuke using VM_MIXEDMAP here and use VM_PFNMAP instead.
>>
>> Also set VM_SHARED, not 100% sure if that is needed with VM_PFNMAP, but better
>> save than sorry.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Bugs: https://gitlab.freedesktop.org/drm/amd/-/issues/1606#note_936174
> I thought we still have the same issue open for ttm_bo_vm_insert_huge()?
> Or at least a potentially pretty big bug, because our current huge entries
> don't stop gup (because there's no pmd_mkspecial right now in the kernel).
>
> So I think if you want to close this for good then we also need to
> (temporarily at least) disable the huge entry code?

That's already done (at least for ~vmwgfx) because we ran into problems 
we couldn't explain.

Going to add something which explicitly disable it with a comment.

What's the conclusion on VM_SHARED? Should I enforce this, warn about it 
or just ignore it because it doesn't matter for VM_PFNMAP?

Thanks,
Christian.

> -Daniel
>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo_vm.c | 29 +++++++----------------------
>>   1 file changed, 7 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>> index 9bd15cb39145..bf86ae849340 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>> @@ -359,12 +359,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
>>   		 * at arbitrary times while the data is mmap'ed.
>>   		 * See vmf_insert_mixed_prot() for a discussion.
>>   		 */
>> -		if (vma->vm_flags & VM_MIXEDMAP)
>> -			ret = vmf_insert_mixed_prot(vma, address,
>> -						    __pfn_to_pfn_t(pfn, PFN_DEV),
>> -						    prot);
>> -		else
>> -			ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
>> +		ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
>>   
>>   		/* Never error on prefaulted PTEs */
>>   		if (unlikely((ret & VM_FAULT_ERROR))) {
>> @@ -411,15 +406,9 @@ vm_fault_t ttm_bo_vm_dummy_page(struct vm_fault *vmf, pgprot_t prot)
>>   	pfn = page_to_pfn(page);
>>   
>>   	/* Prefault the entire VMA range right away to avoid further faults */
>> -	for (address = vma->vm_start; address < vma->vm_end; address += PAGE_SIZE) {
>> -
>> -		if (vma->vm_flags & VM_MIXEDMAP)
>> -			ret = vmf_insert_mixed_prot(vma, address,
>> -						    __pfn_to_pfn_t(pfn, PFN_DEV),
>> -						    prot);
>> -		else
>> -			ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
>> -	}
>> +	for (address = vma->vm_start; address < vma->vm_end;
>> +	     address += PAGE_SIZE)
>> +		ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
>>   
>>   	return ret;
>>   }
>> @@ -576,14 +565,10 @@ static void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_s
>>   
>>   	vma->vm_private_data = bo;
>>   
>> -	/*
>> -	 * We'd like to use VM_PFNMAP on shared mappings, where
>> -	 * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
>> -	 * but for some reason VM_PFNMAP + x86 PAT + write-combine is very
>> -	 * bad for performance. Until that has been sorted out, use
>> -	 * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
>> +	/* Enforce VM_SHARED here since no driver backend actually supports COW
>> +	 * on TTM buffer object mappings.
>>   	 */
>> -	vma->vm_flags |= VM_MIXEDMAP;
>> +	vma->vm_flags |= VM_PFNMAP | VM_SHARED;
>>   	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>>   }
>>   
>> -- 
>> 2.25.1
>>