Message-ID: <20250930123003.75370854@fedora>
Date: Tue, 30 Sep 2025 12:30:03 +0200
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Loïc Molinari <loic.molinari@...labora.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
 <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
 <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Jani Nikula
 <jani.nikula@...ux.intel.com>, Joonas Lahtinen
 <joonas.lahtinen@...ux.intel.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>,
 Tvrtko Ursulin <tursulin@...ulin.net>, Rob Herring <robh@...nel.org>,
 Steven Price <steven.price@....com>, Liviu Dudau <liviu.dudau@....com>,
 Melissa Wen <mwen@...lia.com>, Maíra Canal
 <mcanal@...lia.com>, Hugh Dickins <hughd@...gle.com>, Baolin Wang
 <baolin.wang@...ux.alibaba.com>, Andrew Morton <akpm@...ux-foundation.org>,
 Al Viro <viro@...iv.linux.org.uk>, Mikołaj Wasiak
 <mikolaj.wasiak@...el.com>, Christian Brauner <brauner@...nel.org>, Nitin
 Gote <nitin.r.gote@...el.com>, Andi Shyti <andi.shyti@...ux.intel.com>,
 linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 intel-gfx@...ts.freedesktop.org, linux-mm@...ck.org, kernel@...labora.com
Subject: Re: [PATCH 2/8] drm/gem: Introduce drm_gem_get_unmapped_area() fop

On Mon, 29 Sep 2025 22:03:10 +0200
Loïc Molinari <loic.molinari@...labora.com> 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 mount points 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.
> 
> Signed-off-by: Loïc Molinari <loic.molinari@...labora.com>
> ---
>  drivers/gpu/drm/drm_gem.c | 110 ++++++++++++++++++++++++++++++--------
>  include/drm/drm_gem.h     |   4 ++
>  mm/shmem.c                |   1 +
>  3 files changed, 93 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index cbeb76b2124f..d027db462c2d 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1187,36 +1187,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 +1226,89 @@ 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)
> +{
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +	struct drm_gem_object *obj;
> +	unsigned long ret;
> +
> +	obj = drm_gem_object_lookup_from_offset(filp, pgoff, len >> PAGE_SHIFT);
> +	if (IS_ERR(obj))

Is this supposed to happen? If not, I'd be tempted to add a
WARN_ON_ONCE().

> +		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;
> +#else
> +	return mm_get_unmapped_area(current->mm, filp, uaddr, len, 0, flags);

Looks like the above code covers the non-THP case too, do we really need
to specialize for !CONFIG_TRANSPARENT_HUGEPAGE here?

> +#endif
> +}
> +EXPORT_SYMBOL(drm_gem_get_unmapped_area);
> +
> +/**
> + * 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 e2c76a30802b..b2f41b430daa 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2915,6 +2915,7 @@ unsigned long shmem_get_unmapped_area(struct file *file,
>  		return addr;
>  	return inflated_addr;
>  }
> +EXPORT_SYMBOL_GPL(shmem_get_unmapped_area);
>  
>  #ifdef CONFIG_NUMA
>  static int shmem_set_policy(struct vm_area_struct *vma, struct mempolicy *mpol)

Powered by blists - more mailing lists