Re: [PATCH v5 04/12] drm/gem: Introduce drm_gem_get_unmapped_area() fop

Hugh Dickins
2025-10-27T11:38:23Z

[-- Attachment #1: Type: text/plain, Size: 9550 bytes --]

On Tue, 21 Oct 2025, Loïc Molinari wrote:

> mmap() calls on the DRM file pointer currently always end up using
> mm_get_unmapped_area() to get a free mapping region. On builds with
> CONFIG_TRANSPARENT_HUGEPAGE enabled, this isn't ideal for GEM objects
> backed by shmem buffers on mountpoints setting the 'huge=' option
> because it can't correctly figure out the potentially huge address
> alignment required.
>
> This commit introduces the drm_gem_get_unmapped_area() function which
> is meant to be used as a get_unmapped_area file operation on the DRM
> file pointer to lookup GEM objects based on their fake offsets and get
> a properly aligned region by calling shmem_get_unmapped_area() with
> the right file pointer. If a GEM object isn't available at the given
> offset or if the caller isn't granted access to it, the function falls
> back to mm_get_unmapped_area().
>
> This also makes drm_gem_get_unmapped_area() part of the default GEM
> file operations so that all the DRM drivers can benefit from more
> efficient mappings thanks to the huge page fault handler introduced in
> previous commit 'drm/shmem-helper: Add huge page fault handler'.
>
> The shmem_get_unmapped_area() function needs to be exported so that
> it can be used from the DRM subsystem.
>
> v3:
> - add missing include: 'linux/sched/mm.h'
> - forward to shmem layer in builds with CONFIG_TRANSPARENT_HUGEPAGE=n
>
> Signed-off-by: Loïc Molinari <loic.molinari@collabora.com>

Seems reasonable, but a couple of minor remarks below.

> ---
> drivers/gpu/drm/drm_gem.c | 107 ++++++++++++++++++++++++++++++--------
> include/drm/drm_gem.h | 4 ++
> mm/shmem.c | 1 +
> 3 files changed, 90 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index a1a9c828938b..a98d5744cc6c 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -36,6 +36,7 @@
> #include <linux/module.h>
> #include <linux/pagemap.h>
> #include <linux/pagevec.h>
> +#include <linux/sched/mm.h>
> #include <linux/shmem_fs.h>
> #include <linux/slab.h>
> #include <linux/string_helpers.h>
> @@ -1187,36 +1188,27 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
> }
> EXPORT_SYMBOL(drm_gem_mmap_obj);
>
> -/**
> - * drm_gem_mmap - memory map routine for GEM objects
> - * @filp: DRM file pointer
> - * @vma: VMA for the area to be mapped
> - *
> - * If a driver supports GEM object mapping, mmap calls on the DRM file
> - * descriptor will end up here.
> - *
> - * Look up the GEM object based on the offset passed in (vma->vm_pgoff will
> - * contain the fake offset we created when the GTT map ioctl was called on
> - * the object) and map it with a call to drm_gem_mmap_obj().
> - *
> - * If the caller is not granted access to the buffer object, the mmap will fail
> - * with EACCES. Please see the vma manager for more information.
> +/*
> + * Look up a GEM object in offset space based on the exact start address. The
> + * caller must be granted access to the object. Returns a GEM object on success
> + * or a negative error code on failure. The returned GEM object needs to be
> + * released with drm_gem_object_put().
> */
> -int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> +static struct drm_gem_object *
> +drm_gem_object_lookup_from_offset(struct file *filp, unsigned long start,
> + unsigned long pages)
> {
> struct drm_file *priv = filp->private_data;
> struct drm_device *dev = priv->minor->dev;
> struct drm_gem_object *obj = NULL;
> struct drm_vma_offset_node *node;
> - int ret;
>
> if (drm_dev_is_unplugged(dev))
> - return -ENODEV;
> + return ERR_PTR(-ENODEV);
>
> drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> - vma->vm_pgoff,
> - vma_pages(vma));
> + start, pages);
> if (likely(node)) {
> obj = container_of(node, struct drm_gem_object, vma_node);
> /*
> @@ -1235,14 +1227,85 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
>
> if (!obj)
> - return -EINVAL;
> + return ERR_PTR(-EINVAL);
>
> if (!drm_vma_node_is_allowed(node, priv)) {
> drm_gem_object_put(obj);
> - return -EACCES;
> + return ERR_PTR(-EACCES);
> }
>
> - ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT,
> + return obj;
> +}
> +
> +/**
> + * drm_gem_get_unmapped_area - get memory mapping region routine for GEM objects
> + * @filp: DRM file pointer
> + * @uaddr: User address hint
> + * @len: Mapping length
> + * @pgoff: Offset (in pages)
> + * @flags: Mapping flags
> + *
> + * If a driver supports GEM object mapping, before ending up in drm_gem_mmap(),
> + * mmap calls on the DRM file descriptor will first try to find a free linear
> + * address space large enough for a mapping. Since GEM objects are backed by
> + * shmem buffers, this should preferably be handled by the shmem virtual memory
> + * filesystem which can appropriately align addresses to huge page sizes when
> + * needed.
> + *
> + * Look up the GEM object based on the offset passed in (vma->vm_pgoff will
> + * contain the fake offset we created) and call shmem_get_unmapped_area() with
> + * the right file pointer.
> + *
> + * If a GEM object is not available at the given offset or if the caller is not
> + * granted access to it, fall back to mm_get_unmapped_area().
> + */
> +unsigned long drm_gem_get_unmapped_area(struct file *filp, unsigned long uaddr,
> + unsigned long len, unsigned long pgoff,
> + unsigned long flags)
> +{
> + struct drm_gem_object *obj;
> + unsigned long ret;
> +
> + obj = drm_gem_object_lookup_from_offset(filp, pgoff, len >> PAGE_SHIFT);
> + if (IS_ERR(obj))
> + return mm_get_unmapped_area(current->mm, filp, uaddr, len, 0,
> + flags);
> +
> + ret = shmem_get_unmapped_area(obj->filp, uaddr, len, 0, flags);
> +
> + drm_gem_object_put(obj);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_gem_get_unmapped_area);

Not something I'll make an issue of, but this does look rather like
a drm EXPORT_SYMBOL() of a shmem EXPORT_SYMBOL_GPL().

Not your intention, I think, and a quick look around suggests some
inconsistency as to whether symbols here are exported _GPL() or not.

Maybe there's good (historical?) reason for which is which, or
maybe it's something maintainers would like to clean up one day.

Please make this one EXPORT_SYMBOL_GPL() if you can.

> +
> +/**
> + * drm_gem_mmap - memory map routine for GEM objects
> + * @filp: DRM file pointer
> + * @vma: VMA for the area to be mapped
> + *
> + * If a driver supports GEM object mapping, mmap calls on the DRM file
> + * descriptor will end up here.
> + *
> + * Look up the GEM object based on the offset passed in (vma->vm_pgoff will
> + * contain the fake offset we created) and map it with a call to
> + * drm_gem_mmap_obj().
> + *
> + * If the caller is not granted access to the buffer object, the mmap will fail
> + * with EACCES. Please see the vma manager for more information.
> + */
> +int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> + struct drm_gem_object *obj;
> + int ret;
> +
> + obj = drm_gem_object_lookup_from_offset(filp, vma->vm_pgoff,
> + vma_pages(vma));
> + if (IS_ERR(obj))
> + return PTR_ERR(obj);
> +
> + ret = drm_gem_mmap_obj(obj,
> + drm_vma_node_size(&obj->vma_node) << PAGE_SHIFT,
> vma);
>
> drm_gem_object_put(obj);
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 8d48d2af2649..7c8bd67d087c 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -469,6 +469,7 @@ struct drm_gem_object {
> .poll = drm_poll,\
> .read = drm_read,\
> .llseek = noop_llseek,\
> + .get_unmapped_area = drm_gem_get_unmapped_area,\
> .mmap = drm_gem_mmap, \
> .fop_flags = FOP_UNSIGNED_OFFSET
>
> @@ -506,6 +507,9 @@ void drm_gem_vm_close(struct vm_area_struct *vma);
> int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
> struct vm_area_struct *vma);
> int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> +unsigned long drm_gem_get_unmapped_area(struct file *filp, unsigned long uaddr,
> + unsigned long len, unsigned long pgoff,
> + unsigned long flags);
>
> /**
> * drm_gem_object_get - acquire a GEM buffer object reference
> diff --git a/mm/shmem.c b/mm/shmem.c
> index b9081b817d28..612218fc95cb 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2851,6 +2851,7 @@ unsigned long shmem_get_unmapped_area(struct file *file,
> return addr;
> return inflated_addr;
> }
> +EXPORT_SYMBOL_GPL(shmem_get_unmapped_area);

As you have it, that export comes under #ifdef CONFIG_SHMEM.

I know parts of drm do "select SHMEM" these days, but you might not
be covered by those: maybe you need a "select SHMEM" in the relevant
Kconfig, or maybe you prefer to duplicate that export line after the
later !CONFIG_SHMEM definitiion of shmem_get_unmapped_area().

Or better, make no export here at all, but drm_gem_get_unmapped_area()
use obj->filp->f_op->get_unmapped_area()?

>
> #ifdef CONFIG_NUMA
> static int shmem_set_policy(struct vm_area_struct *vma, struct mempolicy *mpol)
> --
> 2.47.3