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

[Hugh Dickins <hughd@google.com>]

[-- 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