| Message ID | lirio6uzqw4v62akcfcoo7w37gai24nbgefoyzxviysjape7aj@ck7iwwcnvpx4 (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/selftest/mmap_migrate: wait for clear memory | expand |
Hi Mikolaj, thanks for the series. On 2025-04-11 at 12:48:16 GMT, Mikolaj Wasiak wrote: > Mmap_migrate test runs multiple times filling GPU memory > with objects. Those objects are deleted after each run > but cleaning pages takes some time after the objects are > put. This patch lets tests to wait for cleanup after previous test > if they need to allocate whole memory with new objects. > I have a feeling that making sure that the GPU state is clean should be a responsibility of the running test. Meaning, the wait should be placed at the end of a finishing test, not at the start of the next test. That way any and all other code that wants to use the GPU doesn't have to worry about its state. Though I suppose that this is an edge case anyway, as it's rare to fill that much memory in back to back executions. > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13929 > > Signed-off-by: Mikolaj Wasiak <mikolaj.wasiak@intel.com> > --- > .../drm/i915/gem/selftests/i915_gem_mman.c | 35 +++++++++++++++++-- > 1 file changed, 32 insertions(+), 3 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..e486a52b855a 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > @@ -1047,8 +1047,36 @@ static void igt_make_evictable(struct list_head *objects) > cond_resched(); > } > > +static int igt_try_pinning_pages(struct drm_i915_gem_object *obj, > + bool force_pin) > +{ > + int retries; > + int max_retries; > + int err; Rearrange the declarations like this: int max_retries; int retries; int err; Kernel code style quirk. Declarations are sorted by length descending. I don't think it's specified in the coding style document per se, but this is generally followed at least around i915. > + > + retries = 0; > + max_retries = 10; Can max_retries be a #define symbol instead or is it overkill here? > + do { > + err = i915_gem_object_pin_pages_unlocked(obj); > + if (!err) > + break; > + > + if (err != -ENXIO && err != -ENOMEM) > + break; > + > + if (!force_pin) > + break; > + > + retries++; > + msleep(20); > + } while (retries < max_retries); > + > + return err; > +} Is a do while loop here really necessary? I think a simple for loop would be more idiomatic, as all this code is really doing is repeat the pin a set amount of times. Since the call to i915_gem_object_pin_pages() is the first statement in the body, it will execute at least once anyway. > + > static int igt_fill_mappable(struct intel_memory_region *mr, > - struct list_head *objects) > + struct list_head *objects, > + bool force_fill) > { > u64 size, total; > int err; > @@ -1066,7 +1094,7 @@ static int igt_fill_mappable(struct intel_memory_region *mr, > > list_add(&obj->st_link, objects); > > - err = i915_gem_object_pin_pages_unlocked(obj); > + err = igt_try_pinning_pages(obj, force_fill); > if (err) { > if (err != -ENXIO && err != -ENOMEM) > goto err_close; > @@ -1208,7 +1236,8 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements, > } > > if (flags & IGT_MMAP_MIGRATE_FILL) { > - err = igt_fill_mappable(placements[0], &objects); > + err = igt_fill_mappable(placements[0], &objects, > + flags & IGT_MMAP_MIGRATE_UNFAULTABLE); Align 'flags & ...' so it's exactly below 'placements[0]': err = igt_fill_mappable(placements[0], &objects, flags & IGT_MMAP_MIGRATE_UNFAULTABLE); > if (err) > goto out_put; > } Thanks Krzysztof > -- > 2.43.0 >
Quoting Mikolaj Wasiak (2025-04-11 12:48:16) > Mmap_migrate test runs multiple times filling GPU memory > with objects. Those objects are deleted after each run > but cleaning pages takes some time after the objects are > put. This patch lets tests to wait for cleanup after previous test > if they need to allocate whole memory with new objects. That's a fundamental bug if pages are available on the system but not being allocated [just because they are currently being freed]. That breaks the understanding that clients can allocate any and all local memory. -Chris
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..e486a52b855a 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c @@ -1047,8 +1047,36 @@ static void igt_make_evictable(struct list_head *objects) cond_resched(); } +static int igt_try_pinning_pages(struct drm_i915_gem_object *obj, + bool force_pin) +{ + int retries; + int max_retries; + int err; + + retries = 0; + max_retries = 10; + do { + err = i915_gem_object_pin_pages_unlocked(obj); + if (!err) + break; + + if (err != -ENXIO && err != -ENOMEM) + break; + + if (!force_pin) + break; + + retries++; + msleep(20); + } while (retries < max_retries); + + return err; +} + static int igt_fill_mappable(struct intel_memory_region *mr, - struct list_head *objects) + struct list_head *objects, + bool force_fill) { u64 size, total; int err; @@ -1066,7 +1094,7 @@ static int igt_fill_mappable(struct intel_memory_region *mr, list_add(&obj->st_link, objects); - err = i915_gem_object_pin_pages_unlocked(obj); + err = igt_try_pinning_pages(obj, force_fill); if (err) { if (err != -ENXIO && err != -ENOMEM) goto err_close; @@ -1208,7 +1236,8 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements, } if (flags & IGT_MMAP_MIGRATE_FILL) { - err = igt_fill_mappable(placements[0], &objects); + err = igt_fill_mappable(placements[0], &objects, + flags & IGT_MMAP_MIGRATE_UNFAULTABLE); if (err) goto out_put; }
Mmap_migrate test runs multiple times filling GPU memory with objects. Those objects are deleted after each run but cleaning pages takes some time after the objects are put. This patch lets tests to wait for cleanup after previous test if they need to allocate whole memory with new objects. Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13929 Signed-off-by: Mikolaj Wasiak <mikolaj.wasiak@intel.com> --- .../drm/i915/gem/selftests/i915_gem_mman.c | 35 +++++++++++++++++-- 1 file changed, 32 insertions(+), 3 deletions(-)