Hello Thomas, On Tue, 27 Jan 2026 14:16:36 +0100 Thomas Zimmermann <[email protected]> wrote:
> Gem-shmem operates on pages instead of I/O memory ranges, so use them > for mmap. This will allow for tracking page dirty/accessed flags. If > hugepage support is available, insert the page's folio if possible. > Otherwise fall back to mapping individual pages. > > As the PFN is no longer required for hugepage mappings, simplify the > related code and make it depend on CONFIG_TRANSPARENT_HUGEPAGE. Prepare > for tracking folio status. > > Signed-off-by: Thomas Zimmermann <[email protected]> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 58 ++++++++++++++++---------- > 1 file changed, 35 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c > b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 3871a6d92f77..b6ddabbfcc52 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -553,17 +553,18 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_dumb_create); > static bool drm_gem_shmem_try_map_pmd(struct vm_fault *vmf, unsigned long > addr, > struct page *page) > { > -#ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP > - unsigned long pfn = page_to_pfn(page); > - unsigned long paddr = pfn << PAGE_SHIFT; > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE > + phys_addr_t paddr = page_to_phys(page); > bool aligned = (addr & ~PMD_MASK) == (paddr & ~PMD_MASK); > > - if (aligned && > - pmd_none(*vmf->pmd) && > - folio_test_pmd_mappable(page_folio(page))) { > - pfn &= PMD_MASK >> PAGE_SHIFT; > - if (vmf_insert_pfn_pmd(vmf, pfn, false) == VM_FAULT_NOPAGE) > - return true; > + if (aligned && pmd_none(*vmf->pmd)) { > + struct folio *folio = page_folio(page); > + > + if (folio_test_pmd_mappable(folio)) { > + /* Read-only mapping; split upon write fault */ > + if (vmf_insert_folio_pmd(vmf, folio, false) == > VM_FAULT_NOPAGE) > + return true; > + } > } > #endif > > @@ -576,13 +577,10 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault > *vmf) > struct drm_gem_object *obj = vma->vm_private_data; > struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > loff_t num_pages = obj->size >> PAGE_SHIFT; > - vm_fault_t ret; > struct page **pages = shmem->pages; > - pgoff_t page_offset; > - unsigned long pfn; > - > - /* Offset to faulty address in the VMA. */ > - page_offset = vmf->pgoff - vma->vm_pgoff; > + pgoff_t page_offset = vmf->pgoff - vma->vm_pgoff; /* page offset within > VMA */ > + struct page *page = pages[page_offset]; > + vm_fault_t ret; > > dma_resv_lock(shmem->base.resv, NULL); > > @@ -590,21 +588,35 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault > *vmf) > drm_WARN_ON_ONCE(obj->dev, !shmem->pages) || > shmem->madv < 0) { > ret = VM_FAULT_SIGBUS; > - goto out; > + goto err_dma_resv_unlock; > } > > - if (drm_gem_shmem_try_map_pmd(vmf, vmf->address, pages[page_offset])) { > - ret = VM_FAULT_NOPAGE; > - goto out; > + page = pages[page_offset]; > + if (!page) { > + ret = VM_FAULT_SIGBUS; > + goto err_dma_resv_unlock; > } > > - pfn = page_to_pfn(pages[page_offset]); > - ret = vmf_insert_pfn(vma, vmf->address, pfn); > + if (drm_gem_shmem_try_map_pmd(vmf, vmf->address, page)) { > + ret = VM_FAULT_NOPAGE; > + } else { > + struct folio *folio = page_folio(page); > + > + get_page(page); > + > + folio_lock(folio); > + > + vmf->page = page; > + ret = VM_FAULT_LOCKED; > + } > > - out: > dma_resv_unlock(shmem->base.resv); > > return ret; > + > +err_dma_resv_unlock: > + dma_resv_unlock(shmem->base.resv); > + return ret; Why do we need an error path that's exactly the same as the success path? > } > > static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) > @@ -691,7 +703,7 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object > *shmem, struct vm_area_struct > if (ret) > return ret; > > - vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP); > + vm_flags_mod(vma, VM_DONTEXPAND | VM_DONTDUMP, VM_PFNMAP); > vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); > if (shmem->map_wc) > vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); The rest looks reasonable, if everyone is happy with the less restrictive aspect that !PFNMAP implies, as mentioned by Matthew.
