Re: [PATCH v9 05/11] drm/i915: Use huge tmpfs mountpoint helpers

["Loïc Molinari" <loic.molinari@collabora.com>]

Hi Tvrtko,

On 02/12/2025 12:01, Tvrtko Ursulin wrote:
> 
> On 28/11/2025 18:41, Loïc Molinari wrote:
>> On 20/11/2025 10:31, Tvrtko Ursulin wrote:
>>>
>>> On 14/11/2025 17:02, Loïc Molinari wrote:
>>>> Make use of the new drm_gem_huge_mnt_create() and
>>>> drm_gem_get_huge_mnt() helpers to avoid code duplication. Now that
>>>> it's just a few lines long, the single function in i915_gemfs.c is
>>>> moved into v3d_gem_shmem.c.
>>>>
>>>> v3:
>>>> - use huge tmpfs mountpoint in drm_device
>>>> - move i915_gemfs.c into i915_gem_shmem.c
>>>>
>>>> v4:
>>>> - clean up mountpoint creation error handling
>>>>
>>>> v5:
>>>> - use drm_gem_has_huge_mnt() helper
>>>>
>>>> v7:
>>>> - include <drm/drm_print.h> in i915_gem_shmem.c
>>>>
>>>> v8:
>>>> - keep logging notice message with CONFIG_TRANSPARENT_HUGEPAGE=n
>>>> - don't access huge_mnt field with CONFIG_TRANSPARENT_HUGEPAGE=n
>>>>
>>>> v9:
>>>> - replace drm_gem_has_huge_mnt() by drm_gem_get_huge_mnt()
>>>> - remove useless ternary op test in selftests/huge_pages.c
>>>>
>>>> Signed-off-by: Loïc Molinari <loic.molinari@collabora.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/Makefile                 |  3 +-
>>>>   drivers/gpu/drm/i915/gem/i915_gem_shmem.c     | 48 +++++++++----
>>>>   drivers/gpu/drm/i915/gem/i915_gemfs.c         | 71 
>>>> -------------------
>>>>   drivers/gpu/drm/i915/gem/i915_gemfs.h         | 14 ----
>>>>   .../gpu/drm/i915/gem/selftests/huge_pages.c   | 16 +++--
>>>>   drivers/gpu/drm/i915/i915_drv.h               |  5 --
>>>>   6 files changed, 47 insertions(+), 110 deletions(-)
>>>>   delete mode 100644 drivers/gpu/drm/i915/gem/i915_gemfs.c
>>>>   delete mode 100644 drivers/gpu/drm/i915/gem/i915_gemfs.h
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/ 
>>>> Makefile
>>>> index 84ec79b64960..b5a8c0a6b747 100644
>>>> --- a/drivers/gpu/drm/i915/Makefile
>>>> +++ b/drivers/gpu/drm/i915/Makefile
>>>> @@ -169,8 +169,7 @@ gem-y += \
>>>>       gem/i915_gem_ttm_move.o \
>>>>       gem/i915_gem_ttm_pm.o \
>>>>       gem/i915_gem_userptr.o \
>>>> -    gem/i915_gem_wait.o \
>>>> -    gem/i915_gemfs.o
>>>> +    gem/i915_gem_wait.o
>>>>   i915-y += \
>>>>       $(gem-y) \
>>>>       i915_active.o \
>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/ 
>>>> gpu/ drm/i915/gem/i915_gem_shmem.c
>>>> index 26dda55a07ff..15c2c6fde2ac 100644
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>>>> @@ -9,14 +9,16 @@
>>>>   #include <linux/uio.h>
>>>>   #include <drm/drm_cache.h>
>>>> +#include <drm/drm_gem.h>
>>>> +#include <drm/drm_print.h>
>>>>   #include "gem/i915_gem_region.h"
>>>>   #include "i915_drv.h"
>>>>   #include "i915_gem_object.h"
>>>>   #include "i915_gem_tiling.h"
>>>> -#include "i915_gemfs.h"
>>>>   #include "i915_scatterlist.h"
>>>>   #include "i915_trace.h"
>>>> +#include "i915_utils.h"
>>>>   /*
>>>>    * Move folios to appropriate lru and release the batch, 
>>>> decrementing the
>>>> @@ -497,6 +499,7 @@ static int __create_shmem(struct 
>>>> drm_i915_private *i915,
>>>>                 resource_size_t size)
>>>>   {
>>>>       unsigned long flags = VM_NORESERVE;
>>>> +    struct vfsmount *huge_mnt;
>>>>       struct file *filp;
>>>>       drm_gem_private_object_init(&i915->drm, obj, size);
>>>> @@ -515,9 +518,9 @@ static int __create_shmem(struct 
>>>> drm_i915_private *i915,
>>>>       if (BITS_PER_LONG == 64 && size > MAX_LFS_FILESIZE)
>>>>           return -E2BIG;
>>>> -    if (i915->mm.gemfs)
>>>> -        filp = shmem_file_setup_with_mnt(i915->mm.gemfs, "i915", size,
>>>> -                         flags);
>>>> +    huge_mnt = drm_gem_get_huge_mnt(&i915->drm);
>>>> +    if (huge_mnt)
>>>> +        filp = shmem_file_setup_with_mnt(huge_mnt, "i915", size, 
>>>> flags);
>>>>       else
>>>>           filp = shmem_file_setup("i915", size, flags);
>>>>       if (IS_ERR(filp))
>>>> @@ -644,21 +647,40 @@ i915_gem_object_create_shmem_from_data(struct 
>>>> drm_i915_private *i915,
>>>>   static int init_shmem(struct intel_memory_region *mem)
>>>>   {
>>>> -    i915_gemfs_init(mem->i915);
>>>> -    intel_memory_region_set_name(mem, "system");
>>>> +    struct drm_i915_private *i915 = mem->i915;
>>>> -    return 0; /* We have fallback to the kernel mnt if gemfs init 
>>>> failed. */
>>>> -}
>>>> +    /*
>>>> +     * By creating our own shmemfs mountpoint, we can pass in
>>>> +     * mount flags that better match our usecase.
>>>> +     *
>>>> +     * One example, although it is probably better with a per-file
>>>> +     * control, is selecting huge page allocations 
>>>> ("huge=within_size").
>>>> +     * However, we only do so on platforms which benefit from it, 
>>>> or to
>>>> +     * offset the overhead of iommu lookups, where with latter it 
>>>> is a net
>>>> +     * win even on platforms which would otherwise see some 
>>>> performance
>>>> +     * regressions such a slow reads issue on Broadwell and Skylake.
>>>> +     */
>>>> -static int release_shmem(struct intel_memory_region *mem)
>>>> -{
>>>> -    i915_gemfs_fini(mem->i915);
>>>> -    return 0;
>>>> +    if (GRAPHICS_VER(i915) < 11 && !i915_vtd_active(i915))
>>>> +        goto no_thp;
>>>> +
>>>> +    drm_gem_huge_mnt_create(&i915->drm, "within_size");
>>>> +    if (drm_gem_get_huge_mnt(&i915->drm))
>>>> +        drm_info(&i915->drm, "Using Transparent Hugepages\n");
>>>> +    else
>>>> +        drm_notice(&i915->drm,
>>>> +               "Transparent Hugepage support is recommended for 
>>>> optimal performance%s\n",
>>>> +               GRAPHICS_VER(i915) >= 11 ? " on this platform!" :
>>>> +                              " when IOMMU is enabled!");
>>>> +
>>>> + no_thp:
>>>> +    intel_memory_region_set_name(mem, "system");
>>>> +
>>>> +    return 0; /* We have fallback to the kernel mnt if huge mnt 
>>>> failed. */
>>>>   }
>>>>   static const struct intel_memory_region_ops shmem_region_ops = {
>>>>       .init = init_shmem,
>>>> -    .release = release_shmem,
>>>>       .init_object = shmem_object_init,
>>>>   };
>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/ 
>>>> drm/ i915/gem/i915_gemfs.c
>>>> deleted file mode 100644
>>>> index 1f1290214031..000000000000
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c
>>>> +++ /dev/null
>>>> @@ -1,71 +0,0 @@
>>>> -// SPDX-License-Identifier: MIT
>>>> -/*
>>>> - * Copyright © 2017 Intel Corporation
>>>> - */
>>>> -
>>>> -#include <linux/fs.h>
>>>> -#include <linux/mount.h>
>>>> -#include <linux/fs_context.h>
>>>> -
>>>> -#include <drm/drm_print.h>
>>>> -
>>>> -#include "i915_drv.h"
>>>> -#include "i915_gemfs.h"
>>>> -#include "i915_utils.h"
>>>> -
>>>> -void i915_gemfs_init(struct drm_i915_private *i915)
>>>> -{
>>>> -    struct file_system_type *type;
>>>> -    struct fs_context *fc;
>>>> -    struct vfsmount *gemfs;
>>>> -    int ret;
>>>> -
>>>> -    /*
>>>> -     * By creating our own shmemfs mountpoint, we can pass in
>>>> -     * mount flags that better match our usecase.
>>>> -     *
>>>> -     * One example, although it is probably better with a per-file
>>>> -     * control, is selecting huge page allocations 
>>>> ("huge=within_size").
>>>> -     * However, we only do so on platforms which benefit from it, 
>>>> or to
>>>> -     * offset the overhead of iommu lookups, where with latter it 
>>>> is a net
>>>> -     * win even on platforms which would otherwise see some 
>>>> performance
>>>> -     * regressions such a slow reads issue on Broadwell and Skylake.
>>>> -     */
>>>> -
>>>> -    if (GRAPHICS_VER(i915) < 11 && !i915_vtd_active(i915))
>>>> -        return;
>>>> -
>>>> -    if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
>>>> -        goto err;
>>>> -
>>>> -    type = get_fs_type("tmpfs");
>>>> -    if (!type)
>>>> -        goto err;
>>>> -
>>>> -    fc = fs_context_for_mount(type, SB_KERNMOUNT);
>>>> -    if (IS_ERR(fc))
>>>> -        goto err;
>>>> -    ret = vfs_parse_fs_string(fc, "source", "tmpfs");
>>>> -    if (!ret)
>>>> -        ret = vfs_parse_fs_string(fc, "huge", "within_size");
>>>> -    if (!ret)
>>>> -        gemfs = fc_mount_longterm(fc);
>>>> -    put_fs_context(fc);
>>>> -    if (ret)
>>>> -        goto err;
>>>> -
>>>> -    i915->mm.gemfs = gemfs;
>>>> -    drm_info(&i915->drm, "Using Transparent Hugepages\n");
>>>> -    return;
>>>> -
>>>> -err:
>>>> -    drm_notice(&i915->drm,
>>>> -           "Transparent Hugepage support is recommended for optimal 
>>>> performance%s\n",
>>>> -           GRAPHICS_VER(i915) >= 11 ? " on this platform!" :
>>>> -                          " when IOMMU is enabled!");
>>>> -}
>>>> -
>>>> -void i915_gemfs_fini(struct drm_i915_private *i915)
>>>> -{
>>>> -    kern_unmount(i915->mm.gemfs);
>>>> -}
>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.h b/drivers/gpu/ 
>>>> drm/ i915/gem/i915_gemfs.h
>>>> deleted file mode 100644
>>>> index 16d4333c9a4e..000000000000
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gemfs.h
>>>> +++ /dev/null
>>>> @@ -1,14 +0,0 @@
>>>> -/* SPDX-License-Identifier: MIT */
>>>> -/*
>>>> - * Copyright © 2017 Intel Corporation
>>>> - */
>>>> -
>>>> -#ifndef __I915_GEMFS_H__
>>>> -#define __I915_GEMFS_H__
>>>> -
>>>> -struct drm_i915_private;
>>>> -
>>>> -void i915_gemfs_init(struct drm_i915_private *i915);
>>>> -void i915_gemfs_fini(struct drm_i915_private *i915);
>>>> -
>>>> -#endif
>>>> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/ 
>>>> drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>>>> index bd08605a1611..28aef75630a2 100644
>>>> --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>>>> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>>>> @@ -1316,7 +1316,7 @@ typedef struct drm_i915_gem_object *
>>>>   static inline bool igt_can_allocate_thp(struct drm_i915_private 
>>>> *i915)
>>>>   {
>>>> -    return i915->mm.gemfs && has_transparent_hugepage();
>>>> +    return !!drm_gem_get_huge_mnt(&i915->drm);
>>>>   }
>>>>   static struct drm_i915_gem_object *
>>>> @@ -1761,7 +1761,9 @@ static int igt_tmpfs_fallback(void *arg)
>>>>       struct drm_i915_private *i915 = arg;
>>>>       struct i915_address_space *vm;
>>>>       struct i915_gem_context *ctx;
>>>> -    struct vfsmount *gemfs = i915->mm.gemfs;
>>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>> +    struct vfsmount *huge_mnt = i915->drm.huge_mnt;
>>>> +#endif
>>>>       struct drm_i915_gem_object *obj;
>>>>       struct i915_vma *vma;
>>>>       struct file *file;
>>>> @@ -1782,10 +1784,12 @@ static int igt_tmpfs_fallback(void *arg)
>>>>       /*
>>>>        * Make sure that we don't burst into a ball of flames upon 
>>>> falling back
>>>>        * to tmpfs, which we rely on if on the off-chance we 
>>>> encounter a failure
>>>> -     * when setting up gemfs.
>>>> +     * when setting up a huge mountpoint.
>>>>        */
>>>> -    i915->mm.gemfs = NULL;
>>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>> +    i915->drm.huge_mnt = NULL;
>>>> +#endif
>>>>       obj = i915_gem_object_create_shmem(i915, PAGE_SIZE);
>>>>       if (IS_ERR(obj)) {
>>>> @@ -1819,7 +1823,9 @@ static int igt_tmpfs_fallback(void *arg)
>>>>   out_put:
>>>>       i915_gem_object_put(obj);
>>>>   out_restore:
>>>> -    i915->mm.gemfs = gemfs;
>>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>> +    i915->drm.huge_mnt = huge_mnt;
>>>> +#endif
>>>
>>> Apart from this layering violation in the selftest, this version 
>>> looks good to me. I am just wondering if we could somehow improve 
>>> this aspect. I was thinking a self-test builds only special version 
>>> of i915_gem_object_create_shmem. Call chain is deep but there are 
>>> flags passed on:
>>>
>>> i915_gem_object_create_shmem
>>>    i915_gem_object_create_region
>>>      __i915_gem_object_create_region
>>>        err = mem->ops->init_object(
>>>
>>> So we could add a new helper like:
>>>
>>> selftests_create_shmem
>>>    i915_gem_object_create_region(...flags = 
>>> I915_BO_ALLOC_SELFTESTS_NOTHP...)
>>>
>>> And in __create_shmem we just make it:
>>>
>>> ...
>>> huge_mnt = drm_gem_get_huge_mnt(&i915->drm) &&
>>> if (IS_ENABLED(..SELFTESTS..) &&
>>>      (flags & I915_BO_ALLOC_SELFTESTS_NOTHP))
>>>      huge_mnt = NULL;
>>> ...
>>>
>>> It would avoid the ifdef and needing to play games with the DRM 
>>> internals.
>>>
>>> How does that sound to you?
>>
>> That sounds better to me but I'm not very familiar with the i915 
>> testing process. Would you be ready to accept the currect ifdef'd 
>> version for now and let me take a better look at that proposal later 
>> for a follow-up patch series?
> 
> I would rather we do it in one go. I assume you are compile testing the 
> i915 part? I so, would you be happy to integrate something like this in 
> your patch (adjusted for your changes):
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/ 
> gpu/drm/i915/gem/i915_gem_object_types.h
> index 465ce94aee76..4dbd61280c93 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -348,12 +348,14 @@ struct drm_i915_gem_object {
>    */
>   #define I915_BO_ALLOC_GPU_ONLY      BIT(6)
>   #define I915_BO_ALLOC_CCS_AUX      BIT(7)
> +#define I915_BO_ALLOC_NOTHP       BIT(8)
>   /*
>    * Object is allowed to retain its initial data and will not be 
> cleared on first
>    * access if used along with I915_BO_ALLOC_USER. This is mainly to keep
>    * preallocated framebuffer data intact while transitioning it to 
> i915drmfb.
>    */
> -#define I915_BO_PREALLOC      BIT(8)
> +#define I915_BO_PREALLOC      BIT(9)
> +
>   #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
>                    I915_BO_ALLOC_VOLATILE | \
>                    I915_BO_ALLOC_CPU_CLEAR | \
> @@ -362,10 +364,12 @@ struct drm_i915_gem_object {
>                    I915_BO_ALLOC_PM_EARLY | \
>                    I915_BO_ALLOC_GPU_ONLY | \
>                    I915_BO_ALLOC_CCS_AUX | \
> +                 I915_BO_ALLOC_NOTHP | \
>                    I915_BO_PREALLOC)
> -#define I915_BO_READONLY          BIT(9)
> -#define I915_TILING_QUIRK_BIT     10 /* unknown swizzling; do not 
> release! */
> -#define I915_BO_PROTECTED         BIT(11)
> +#define I915_BO_READONLY          BIT(10)
> +#define I915_TILING_QUIRK_BIT     11 /* unknown swizzling; do not 
> release! */
> +#define I915_BO_PROTECTED         BIT(12)
> +
>       /**
>        * @mem_flags - Mutable placement-related flags
>        *
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/ 
> drm/i915/gem/i915_gem_shmem.c
> index 26dda55a07ff..a1e876ce7bb9 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -494,7 +494,8 @@ const struct drm_i915_gem_object_ops 
> i915_gem_shmem_ops = {
> 
>   static int __create_shmem(struct drm_i915_private *i915,
>                 struct drm_gem_object *obj,
> -              resource_size_t size)
> +              resource_size_t size,
> +              unsigned int flags)
>   {
>       unsigned long flags = VM_NORESERVE;
>       struct file *filp;
> @@ -515,7 +516,7 @@ static int __create_shmem(struct drm_i915_private 
> *i915,
>       if (BITS_PER_LONG == 64 && size > MAX_LFS_FILESIZE)
>           return -E2BIG;
> 
> -    if (i915->mm.gemfs)
> +    if (!(flags & I915_BO_ALLOC_NOTHP) && i915->mm.gemfs)
>           filp = shmem_file_setup_with_mnt(i915->mm.gemfs, "i915", size,
>                            flags);
>       else
> @@ -548,7 +549,7 @@ static int shmem_object_init(struct 
> intel_memory_region *mem,
>       gfp_t mask;
>       int ret;
> 
> -    ret = __create_shmem(i915, &obj->base, size);
> +    ret = __create_shmem(i915, &obj->base, size, flags);
>       if (ret)
>           return ret;
> 
> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/ 
> gpu/drm/i915/gem/selftests/huge_pages.c
> index bd08605a1611..c296af381007 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> @@ -1787,7 +1787,8 @@ static int igt_tmpfs_fallback(void *arg)
> 
>       i915->mm.gemfs = NULL;
> 
> -    obj = i915_gem_object_create_shmem(i915, PAGE_SIZE);
> +    obj = i915_gem_object_create_region(i915- 
>  >mm.regions[INTEL_REGION_SMEM],
> +                        PAGE_SIZE, 0, I915_BO_ALLOC_NOTHP);
>       if (IS_ERR(obj)) {
>           err = PTR_ERR(obj);
>           goto out_restore;
> 
> 
> If it compiles I would take that and will handle any CI fall out.

Thanks for that! I've integrated it into my i915 patch and compile 
tested it. I'll send a new version.

Regards,
Loïc

> Regards,
> 
> Tvrtko
> 
> 
>> Regards,
>>
>> Loïc
>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>       i915_vm_put(vm);
>>>>   out:
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/ 
>>>> i915_drv.h
>>>> index 95f9ddf22ce4..93a5af3de334 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -141,11 +141,6 @@ struct i915_gem_mm {
>>>>        */
>>>>       atomic_t free_count;
>>>> -    /**
>>>> -     * tmpfs instance used for shmem backed objects
>>>> -     */
>>>> -    struct vfsmount *gemfs;
>>>> -
>>>>       struct intel_memory_region *regions[INTEL_REGION_UNKNOWN];
>>>>       struct notifier_block oom_notifier;
>>
>