| Message ID | 20221028155029.494736-2-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/4] drm/i915/dmabuf: fix sg_table handling in map_dma_buf | expand |
>-----Original Message----- >From: Auld, Matthew <matthew.auld@intel.com> >Sent: Friday, October 28, 2022 11:50 AM >To: intel-gfx@lists.freedesktop.org >Cc: Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>; Tvrtko Ursulin ><tvrtko.ursulin@linux.intel.com>; Ville Syrjälä <ville.syrjala@linux.intel.com>; >Ruhl, Michael J <michael.j.ruhl@intel.com> >Subject: [PATCH v2 2/4] drm/i915/selftests: exercise GPU access from the >importer > >Using PAGE_SIZE here potentially hides issues so bump that to something >larger. This should also make it possible for iommu to coalesce entries >for us. With that in place verify we can write from the GPU using the >importers sg_table, followed by checking that our writes match when read >from the CPU side. > >v2: Switch over to igt_gpu_fill_dw(), which looks to be more widely >supported than the migrate stuff (at least OOTB). > >References: https://gitlab.freedesktop.org/drm/intel/-/issues/7306 >Signed-off-by: Matthew Auld <matthew.auld@intel.com> >Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >Cc: Michael J. Ruhl <michael.j.ruhl@intel.com> >--- > .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 79 >++++++++++++++++++- > 1 file changed, 78 insertions(+), 1 deletion(-) > >diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c >b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c >index f2f3cfad807b..e57f9390076c 100644 >--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c >+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c >@@ -6,8 +6,12 @@ > > #include "i915_drv.h" > #include "i915_selftest.h" >+#include "gem/i915_gem_context.h" > >+#include "mock_context.h" > #include "mock_dmabuf.h" >+#include "igt_gem_utils.h" >+#include "selftests/mock_drm.h" > #include "selftests/mock_gem_device.h" > > static int igt_dmabuf_export(void *arg) >@@ -140,6 +144,75 @@ static int >igt_dmabuf_import_same_driver_lmem(void *arg) > return err; > } > >+static int verify_access(struct drm_i915_private *i915, >+ struct drm_i915_gem_object *native_obj, >+ struct drm_i915_gem_object *import_obj) >+{ >+ struct i915_gem_engines_iter it; >+ struct i915_gem_context *ctx; >+ struct intel_context *ce; >+ struct i915_vma *vma; >+ struct file *file; >+ u32 *vaddr; >+ int err = 0, i; >+ >+ file = mock_file(i915); >+ if (IS_ERR(file)) >+ return PTR_ERR(file); >+ >+ ctx = live_context(i915, file); >+ if (IS_ERR(ctx)) { >+ err = PTR_ERR(ctx); >+ goto out_file; >+ } >+ >+ for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) { >+ if (intel_engine_can_store_dword(ce->engine)) >+ break; >+ } >+ i915_gem_context_unlock_engines(ctx); >+ if (!ce) >+ goto out_file; >+ >+ vma = i915_vma_instance(import_obj, ce->vm, NULL); >+ if (IS_ERR(vma)) { >+ err = PTR_ERR(vma); >+ goto out_file; >+ } >+ >+ err = i915_vma_pin(vma, 0, 0, PIN_USER); >+ if (err) >+ goto out_file; >+ >+ err = igt_gpu_fill_dw(ce, vma, 0, >+ vma->size >> PAGE_SHIFT, 0xdeadbeaf); >+ i915_vma_unpin(vma); >+ if (err) >+ goto out_file; >+ >+ err = i915_gem_object_wait(import_obj, 0, >MAX_SCHEDULE_TIMEOUT); >+ if (err) >+ goto out_file; >+ >+ vaddr = i915_gem_object_pin_map_unlocked(native_obj, >I915_MAP_WB); >+ if (IS_ERR(vaddr)) { >+ err = PTR_ERR(vaddr); >+ goto out_file; >+ } >+ >+ for (i = 0; i < native_obj->base.size / sizeof(u32); i += PAGE_SIZE / >sizeof(u32)) { >+ if (vaddr[i] != 0xdeadbeaf) { >+ pr_err("Data mismatch [%d]=%u\n", i, vaddr[i]); >+ err = -EINVAL; >+ goto out_file; >+ } Not sure what timing issues are related to this test, but this loop could have some impact (takes a long time, assuming the object is LMEM). Would checking the beginning, middle and end of each page be any less beneficial than the current check? Not a suggested change, but just a thought to ponder if the timing becomes an issue... This test looks reasonable to me. Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> M >+ } >+ >+out_file: >+ fput(file); >+ return err; >+} >+ > static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915, > struct intel_memory_region >**regions, > unsigned int num_regions) >@@ -154,7 +227,7 @@ static int igt_dmabuf_import_same_driver(struct >drm_i915_private *i915, > > force_different_devices = true; > >- obj = __i915_gem_object_create_user(i915, PAGE_SIZE, >+ obj = __i915_gem_object_create_user(i915, SZ_8M, > regions, num_regions); > if (IS_ERR(obj)) { > pr_err("__i915_gem_object_create_user failed with >err=%ld\n", >@@ -206,6 +279,10 @@ static int igt_dmabuf_import_same_driver(struct >drm_i915_private *i915, > > i915_gem_object_unlock(import_obj); > >+ err = verify_access(i915, obj, import_obj); >+ if (err) >+ goto out_import; >+ > /* Now try a fake an importer */ > import_attach = dma_buf_attach(dmabuf, obj->base.dev->dev); > if (IS_ERR(import_attach)) { >-- >2.37.3
On 28/10/2022 17:10, Ruhl, Michael J wrote: >> -----Original Message----- >> From: Auld, Matthew <matthew.auld@intel.com> >> Sent: Friday, October 28, 2022 11:50 AM >> To: intel-gfx@lists.freedesktop.org >> Cc: Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>; Tvrtko Ursulin >> <tvrtko.ursulin@linux.intel.com>; Ville Syrjälä <ville.syrjala@linux.intel.com>; >> Ruhl, Michael J <michael.j.ruhl@intel.com> >> Subject: [PATCH v2 2/4] drm/i915/selftests: exercise GPU access from the >> importer >> >> Using PAGE_SIZE here potentially hides issues so bump that to something >> larger. This should also make it possible for iommu to coalesce entries >> for us. With that in place verify we can write from the GPU using the >> importers sg_table, followed by checking that our writes match when read >>from the CPU side. >> >> v2: Switch over to igt_gpu_fill_dw(), which looks to be more widely >> supported than the migrate stuff (at least OOTB). >> >> References: https://gitlab.freedesktop.org/drm/intel/-/issues/7306 >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Michael J. Ruhl <michael.j.ruhl@intel.com> >> --- >> .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 79 >> ++++++++++++++++++- >> 1 file changed, 78 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c >> b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c >> index f2f3cfad807b..e57f9390076c 100644 >> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c >> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c >> @@ -6,8 +6,12 @@ >> >> #include "i915_drv.h" >> #include "i915_selftest.h" >> +#include "gem/i915_gem_context.h" >> >> +#include "mock_context.h" >> #include "mock_dmabuf.h" >> +#include "igt_gem_utils.h" >> +#include "selftests/mock_drm.h" >> #include "selftests/mock_gem_device.h" >> >> static int igt_dmabuf_export(void *arg) >> @@ -140,6 +144,75 @@ static int >> igt_dmabuf_import_same_driver_lmem(void *arg) >> return err; >> } >> >> +static int verify_access(struct drm_i915_private *i915, >> + struct drm_i915_gem_object *native_obj, >> + struct drm_i915_gem_object *import_obj) >> +{ >> + struct i915_gem_engines_iter it; >> + struct i915_gem_context *ctx; >> + struct intel_context *ce; >> + struct i915_vma *vma; >> + struct file *file; >> + u32 *vaddr; >> + int err = 0, i; >> + >> + file = mock_file(i915); >> + if (IS_ERR(file)) >> + return PTR_ERR(file); >> + >> + ctx = live_context(i915, file); >> + if (IS_ERR(ctx)) { >> + err = PTR_ERR(ctx); >> + goto out_file; >> + } >> + >> + for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) { >> + if (intel_engine_can_store_dword(ce->engine)) >> + break; >> + } >> + i915_gem_context_unlock_engines(ctx); >> + if (!ce) >> + goto out_file; >> + >> + vma = i915_vma_instance(import_obj, ce->vm, NULL); >> + if (IS_ERR(vma)) { >> + err = PTR_ERR(vma); >> + goto out_file; >> + } >> + >> + err = i915_vma_pin(vma, 0, 0, PIN_USER); >> + if (err) >> + goto out_file; >> + >> + err = igt_gpu_fill_dw(ce, vma, 0, >> + vma->size >> PAGE_SHIFT, 0xdeadbeaf); >> + i915_vma_unpin(vma); >> + if (err) >> + goto out_file; >> + >> + err = i915_gem_object_wait(import_obj, 0, >> MAX_SCHEDULE_TIMEOUT); >> + if (err) >> + goto out_file; >> + >> + vaddr = i915_gem_object_pin_map_unlocked(native_obj, >> I915_MAP_WB); >> + if (IS_ERR(vaddr)) { >> + err = PTR_ERR(vaddr); >> + goto out_file; >> + } >> + >> + for (i = 0; i < native_obj->base.size / sizeof(u32); i += PAGE_SIZE / >> sizeof(u32)) { >> + if (vaddr[i] != 0xdeadbeaf) { >> + pr_err("Data mismatch [%d]=%u\n", i, vaddr[i]); >> + err = -EINVAL; >> + goto out_file; >> + } > > Not sure what timing issues are related to this test, but this loop could have > some impact (takes a long time, assuming the object is LMEM). > > Would checking the beginning, middle and end of each page be any less > beneficial than the current check? We are currently just sampling the first dword of each page (that could perhaps be random instead). We could go further and sample a selection of random pages instead, if speed is a concern. Although here the pages must be in system memory. dma-buf in upstream will currently force migrate to system memory. > > Not a suggested change, but just a thought to ponder if the timing becomes > an issue... > > This test looks reasonable to me. > > Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> > > M > >> + } >> + >> +out_file: >> + fput(file); >> + return err; >> +} >> + >> static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915, >> struct intel_memory_region >> **regions, >> unsigned int num_regions) >> @@ -154,7 +227,7 @@ static int igt_dmabuf_import_same_driver(struct >> drm_i915_private *i915, >> >> force_different_devices = true; >> >> - obj = __i915_gem_object_create_user(i915, PAGE_SIZE, >> + obj = __i915_gem_object_create_user(i915, SZ_8M, >> regions, num_regions); >> if (IS_ERR(obj)) { >> pr_err("__i915_gem_object_create_user failed with >> err=%ld\n", >> @@ -206,6 +279,10 @@ static int igt_dmabuf_import_same_driver(struct >> drm_i915_private *i915, >> >> i915_gem_object_unlock(import_obj); >> >> + err = verify_access(i915, obj, import_obj); >> + if (err) >> + goto out_import; >> + >> /* Now try a fake an importer */ >> import_attach = dma_buf_attach(dmabuf, obj->base.dev->dev); >> if (IS_ERR(import_attach)) { >> -- >> 2.37.3 >
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c index f2f3cfad807b..e57f9390076c 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c @@ -6,8 +6,12 @@ #include "i915_drv.h" #include "i915_selftest.h" +#include "gem/i915_gem_context.h" +#include "mock_context.h" #include "mock_dmabuf.h" +#include "igt_gem_utils.h" +#include "selftests/mock_drm.h" #include "selftests/mock_gem_device.h" static int igt_dmabuf_export(void *arg) @@ -140,6 +144,75 @@ static int igt_dmabuf_import_same_driver_lmem(void *arg) return err; } +static int verify_access(struct drm_i915_private *i915, + struct drm_i915_gem_object *native_obj, + struct drm_i915_gem_object *import_obj) +{ + struct i915_gem_engines_iter it; + struct i915_gem_context *ctx; + struct intel_context *ce; + struct i915_vma *vma; + struct file *file; + u32 *vaddr; + int err = 0, i; + + file = mock_file(i915); + if (IS_ERR(file)) + return PTR_ERR(file); + + ctx = live_context(i915, file); + if (IS_ERR(ctx)) { + err = PTR_ERR(ctx); + goto out_file; + } + + for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) { + if (intel_engine_can_store_dword(ce->engine)) + break; + } + i915_gem_context_unlock_engines(ctx); + if (!ce) + goto out_file; + + vma = i915_vma_instance(import_obj, ce->vm, NULL); + if (IS_ERR(vma)) { + err = PTR_ERR(vma); + goto out_file; + } + + err = i915_vma_pin(vma, 0, 0, PIN_USER); + if (err) + goto out_file; + + err = igt_gpu_fill_dw(ce, vma, 0, + vma->size >> PAGE_SHIFT, 0xdeadbeaf); + i915_vma_unpin(vma); + if (err) + goto out_file; + + err = i915_gem_object_wait(import_obj, 0, MAX_SCHEDULE_TIMEOUT); + if (err) + goto out_file; + + vaddr = i915_gem_object_pin_map_unlocked(native_obj, I915_MAP_WB); + if (IS_ERR(vaddr)) { + err = PTR_ERR(vaddr); + goto out_file; + } + + for (i = 0; i < native_obj->base.size / sizeof(u32); i += PAGE_SIZE / sizeof(u32)) { + if (vaddr[i] != 0xdeadbeaf) { + pr_err("Data mismatch [%d]=%u\n", i, vaddr[i]); + err = -EINVAL; + goto out_file; + } + } + +out_file: + fput(file); + return err; +} + static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915, struct intel_memory_region **regions, unsigned int num_regions) @@ -154,7 +227,7 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915, force_different_devices = true; - obj = __i915_gem_object_create_user(i915, PAGE_SIZE, + obj = __i915_gem_object_create_user(i915, SZ_8M, regions, num_regions); if (IS_ERR(obj)) { pr_err("__i915_gem_object_create_user failed with err=%ld\n", @@ -206,6 +279,10 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915, i915_gem_object_unlock(import_obj); + err = verify_access(i915, obj, import_obj); + if (err) + goto out_import; + /* Now try a fake an importer */ import_attach = dma_buf_attach(dmabuf, obj->base.dev->dev); if (IS_ERR(import_attach)) {
Using PAGE_SIZE here potentially hides issues so bump that to something larger. This should also make it possible for iommu to coalesce entries for us. With that in place verify we can write from the GPU using the importers sg_table, followed by checking that our writes match when read from the CPU side. v2: Switch over to igt_gpu_fill_dw(), which looks to be more widely supported than the migrate stuff (at least OOTB). References: https://gitlab.freedesktop.org/drm/intel/-/issues/7306 Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Michael J. Ruhl <michael.j.ruhl@intel.com> --- .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 79 ++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-)