[PATCH] drm/i915/selftests: Keep mock file open during unfaultable migrate with fill
Krzysztof Niemiec
krzysztof.niemiec at intel.com
Tue Jun 10 11:42:57 UTC 2025
Hi Krzysztof,
On 2025-06-10 at 10:21:24 GMT, Krzysztof Karas wrote:
> igt_mmap_migrate() tests migration with various parameters.
> In one of the cases, where FILL and UNFAULTABLE flags are set,
> during first stages of this test a mock file is opened in
> igt_mmap_offset(), which results in allocating some data in the
> GPU memory. Then, also in igt_mmap_offset(), the file is closed
> (fput) and the cleanup of that data is scheduled. Right after
> that, the test calls igt_fill_mappable() to fill all available
> GPU memory. At this point, three scenarios are possible
> (N = max size of GPU memory for this test in MiB):
> 1) the data cleanup does not fire until the whole test is over,
> so the memory is fully occupied by 1 MiB with that data and
> N - 1 MiB added by igt_fill_mappable(),
> 2) the data cleanup fires before igt_fill_mappable() completes,
> so the whole memory is populated with N MiB by
> igt_fill_mappable(),
> 3) the data cleanup is performed right after fill is done,
> so only N - 1 MiB are in the GPU memory, preventing the test
> from properly faulting - we'd expect no space left, but an
> object was able to fit in the remaining 1 MiB.
>
> Amend the problem by keeping the mock file open throughout the
> duration of this test and calling fput() from the test itself.
>
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13929
> Signed-off-by: Krzysztof Karas <krzysztof.karas at intel.com>
> ---
>
> On DG2 platforms the memory for data allocated as a result of
> opening a mock file remains occupied until the test is done
> (scenario 1), but on ATSM cards the data is freed earlier
> (scenarios 2 and 3), which leads to sporadic failures.
>
> During testing I observed that the max memory was equal
> to either 4096 or 2048 and igt_fill_mappable() tries to allocate
> as many 1k objects as possible before halving allocation size.
>
> .../drm/i915/gem/selftests/i915_gem_mman.c | 6 ++-
> drivers/gpu/drm/i915/selftests/igt_mmap.c | 54 +++++++++++++------
> drivers/gpu/drm/i915/selftests/igt_mmap.h | 8 +++
> 3 files changed, 51 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> index 9c3f17e51885..1fe4a45d3efb 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> @@ -1176,6 +1176,7 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements,
> struct drm_i915_private *i915 = placements[0]->i915;
> struct drm_i915_gem_object *obj;
> struct i915_request *rq = NULL;
> + struct file *mock_file;
> unsigned long addr;
> LIST_HEAD(objects);
> u64 offset;
> @@ -1200,8 +1201,8 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements,
> * level paging structures(and perhaps scratch), so make sure we
> * allocate early, to avoid tears.
> */
> - addr = igt_mmap_offset(i915, offset, obj->base.size,
> - PROT_WRITE, MAP_SHARED);
> + addr = igt_mmap_offset_get_file(i915, offset, obj->base.size,
> + PROT_WRITE, MAP_SHARED, &mock_file);
> if (IS_ERR_VALUE(addr)) {
> err = addr;
> goto out_put;
> @@ -1299,6 +1300,7 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements,
> }
>
> out_put:
> + fput(mock_file);
> i915_gem_object_put(obj);
> igt_close_objects(i915, &objects);
> return err;
> diff --git a/drivers/gpu/drm/i915/selftests/igt_mmap.c b/drivers/gpu/drm/i915/selftests/igt_mmap.c
> index e920a461bd36..237ad91cd009 100644
> --- a/drivers/gpu/drm/i915/selftests/igt_mmap.c
> +++ b/drivers/gpu/drm/i915/selftests/igt_mmap.c
> @@ -9,14 +9,14 @@
> #include "i915_drv.h"
> #include "igt_mmap.h"
>
> -unsigned long igt_mmap_offset(struct drm_i915_private *i915,
> - u64 offset,
> - unsigned long size,
> - unsigned long prot,
> - unsigned long flags)
> +static unsigned long __igt_mmap_offset(struct drm_i915_private *i915,
> + u64 offset,
> + unsigned long size,
> + unsigned long prot,
> + unsigned long flags,
> + struct file **file)
Keep note of here...
> {
> struct drm_vma_offset_node *node;
> - struct file *file;
> unsigned long addr;
> int err;
>
> @@ -32,21 +32,45 @@ unsigned long igt_mmap_offset(struct drm_i915_private *i915,
> }
>
> /* Pretend to open("/dev/dri/card0") */
> - file = mock_drm_getfile(i915->drm.primary, O_RDWR);
> - if (IS_ERR(file))
> - return PTR_ERR(file);
> + *file = mock_drm_getfile(i915->drm.primary, O_RDWR);
> + if (IS_ERR(*file))
> + return PTR_ERR(*file);
and here...
>
> - err = drm_vma_node_allow(node, file->private_data);
> + err = drm_vma_node_allow(node, (*file)->private_data);
> if (err) {
> - addr = err;
> - goto out_file;
> + fput(*file);
> + return err;
> }
>
> - addr = vm_mmap(file, 0, drm_vma_node_size(node) << PAGE_SHIFT,
> + addr = vm_mmap(*file, 0, drm_vma_node_size(node) << PAGE_SHIFT,
> prot, flags, drm_vma_node_offset_addr(node));
>
> - drm_vma_node_revoke(node, file->private_data);
> -out_file:
> + drm_vma_node_revoke(node, (*file)->private_data);
> +
> + return addr;
> +}
> +
> +unsigned long igt_mmap_offset(struct drm_i915_private *i915,
> + u64 offset,
> + unsigned long size,
> + unsigned long prot,
> + unsigned long flags)
> +{
> + struct file *file;
> + unsigned long addr;
> +
> + addr = __igt_mmap_offset(i915, offset, size, prot, flags, &file);
> fput(file);
> +
> return addr;
> }
> +
and here.
To me, this has a bit of code smell. The double pointer needs to be passed
to the helper function if we want to acquire the file inside it. However
we can also just let the caller handle that acquisition, especially since
we have it call fput() anyway. That way it's clear that it's the caller
who actually has to manage the resource during its lifetime. This will
also allow the helper function to take just a copy of the file pointer,
getting rid of all the redundant dereferencing inside.
So IMO the usage should be more like:
<in caller function>
unsigned long addr;
struct file *file;
file = acquire_in_some_manner();
addr = __igt_mmap_offset(i915, offset, size, prot, flags, file);
/* do some operations */
fput(file);
</in caller function>
> +unsigned long igt_mmap_offset_get_file(struct drm_i915_private *i915,
> + u64 offset,
> + unsigned long size,
> + unsigned long prot,
> + unsigned long flags,
> + struct file **file)
> +{
> + return __igt_mmap_offset(i915, offset, size, prot, flags, file);
> +}
Also given the above I'd reconsider this function. Since we're now
expecting the caller to acquire the file anyway, we could probably just
put the body of __igt_mmap_offset() into igt_mmap_offset_get_file() and
call _get_file() from igt_mmap_offset().
> diff --git a/drivers/gpu/drm/i915/selftests/igt_mmap.h b/drivers/gpu/drm/i915/selftests/igt_mmap.h
> index acbe34d81a6d..756ccdf6fd69 100644
> --- a/drivers/gpu/drm/i915/selftests/igt_mmap.h
> +++ b/drivers/gpu/drm/i915/selftests/igt_mmap.h
> @@ -11,6 +11,7 @@
>
> struct drm_i915_private;
> struct drm_vma_offset_node;
> +struct file;
>
> unsigned long igt_mmap_offset(struct drm_i915_private *i915,
> u64 offset,
> @@ -18,4 +19,11 @@ unsigned long igt_mmap_offset(struct drm_i915_private *i915,
> unsigned long prot,
> unsigned long flags);
>
> +unsigned long igt_mmap_offset_get_file(struct drm_i915_private *i915,
> + u64 offset,
> + unsigned long size,
> + unsigned long prot,
> + unsigned long flags,
> + struct file **file);
> +
> #endif /* IGT_MMAP_H */
> --
Thanks
Krzysztof
> 2.43.0
>
More information about the Intel-gfx
mailing list