Re: [PATCH] drm/i915/selftests: Keep mock file open during unfaultable migrate with fill

[Sebastian Brzezinka <sebastian.brzezinka@intel.com>]

On Tue Jun 10, 2025 at 10:21 AM UTC, 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@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)
>  {
>  	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);
>  
> -	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);
I'll try to avoid calling fput() here and instead handle it where the
file is created. This will simplify tracking the file's lifecycle.

-- 
Best regards,
Sebastian