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

Christian König <ckoenig.leichtzumerken@xxxxxxxxx> · Wed, 2 Jun 2021 20:46:30 +0200

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@xxxxxxx>
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








