| Message ID | 20191120164429.8441-1-janusz.krzysztofik@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t,v2] tests/prime_vgem: Skip basic-read/write subtests if no mappable GGTT | expand |
On Wed, 2019-11-20 at 17:44 +0100, Janusz Krzysztofik wrote: > As we've agreed that using I915_GEM_PREAD/PWRITE IOCTLs on dma-buf > objects doesn't make much sense, we are not going to extend their > handlers in the i915 driver with new processing paths required for > them > to work correctly with dma-buf objects on future hardware with no > mappable aperture. When running on that kind of hardware, just skip > subtests which use those IOCTLs. How is this handled for other dma-buf capable devices? Is there no opportunity for a working solution in i915 here? Also, is this a permanent limitation? Or just a workaround based on our current implementation? Thanks, Stuart > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com > > > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Joonas Lahtinen <joonas.lahtinen@intel.com> > --- > lib/ioctl_wrappers.c | 2 +- > lib/ioctl_wrappers.h | 1 + > tests/prime_vgem.c | 6 ++++++ > 3 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c > index 628f8b83..78b92bd2 100644 > --- a/lib/ioctl_wrappers.c > +++ b/lib/ioctl_wrappers.c > @@ -356,7 +356,7 @@ void gem_write(int fd, uint32_t handle, uint64_t > offset, const void *buf, uint64 > igt_assert_eq(__gem_write(fd, handle, offset, buf, length), 0); > } > > -static int __gem_read(int fd, uint32_t handle, uint64_t offset, void > *buf, uint64_t length) > +int __gem_read(int fd, uint32_t handle, uint64_t offset, void *buf, > uint64_t length) > { > struct drm_i915_gem_pread gem_pread; > int err; > diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h > index f2412d78..97f278ed 100644 > --- a/lib/ioctl_wrappers.h > +++ b/lib/ioctl_wrappers.h > @@ -69,6 +69,7 @@ uint32_t gem_open(int fd, uint32_t name); > void gem_close(int fd, uint32_t handle); > int __gem_write(int fd, uint32_t handle, uint64_t offset, const void > *buf, uint64_t length); > void gem_write(int fd, uint32_t handle, uint64_t offset, const void > *buf, uint64_t length); > +int __gem_read(int fd, uint32_t handle, uint64_t offset, void *buf, > uint64_t length); > void gem_read(int fd, uint32_t handle, uint64_t offset, void *buf, > uint64_t length); > int __gem_set_domain(int fd, uint32_t handle, uint32_t read, > uint32_t write); > void gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t > write); > diff --git a/tests/prime_vgem.c b/tests/prime_vgem.c > index 04cc913d..6595818c 100644 > --- a/tests/prime_vgem.c > +++ b/tests/prime_vgem.c > @@ -46,6 +46,8 @@ static void test_read(int vgem, int i915) > handle = prime_fd_to_handle(i915, dmabuf); > close(dmabuf); > > + igt_skip_on(__gem_read(i915, handle, 0, &i, sizeof(i))); > + > ptr = vgem_mmap(vgem, &scratch, PROT_WRITE); > for (i = 0; i < 1024; i++) > ptr[1024*i] = i; > @@ -81,6 +83,8 @@ static void test_fence_read(int i915, int vgem) > handle = prime_fd_to_handle(i915, dmabuf); > close(dmabuf); > > + igt_skip_on(__gem_read(i915, handle, 0, &i, sizeof(i))); > + > igt_fork(child, 1) { > close(master[0]); > close(slave[1]); > @@ -191,6 +195,8 @@ static void test_write(int vgem, int i915) > handle = prime_fd_to_handle(i915, dmabuf); > close(dmabuf); > > + igt_skip_on(__gem_write(i915, handle, 0, &i, sizeof(i))); > + > ptr = vgem_mmap(vgem, &scratch, PROT_READ); > gem_close(vgem, scratch.handle); >
On Wed, 2019-11-20 at 17:14 +0000, Summers, Stuart wrote: > On Wed, 2019-11-20 at 17:44 +0100, Janusz Krzysztofik wrote: > > As we've agreed that using I915_GEM_PREAD/PWRITE IOCTLs on dma-buf > > objects doesn't make much sense, we are not going to extend their > > handlers in the i915 driver with new processing paths required for > > them > > to work correctly with dma-buf objects on future hardware with no > > mappable aperture. When running on that kind of hardware, just > > skip > > subtests which use those IOCTLs. > > How is this handled for other dma-buf capable devices? Is there no > opportunity for a working solution in i915 here? > > Also, is this a permanent limitation? Or just a workaround based on > our > current implementation? Thinking a bit more about this, I think my main concern here is around the commit message. You are implying there is no chance we can change these DRM interfaces to work with dma-buf by referencing an I915 IOCTL. I don't personally have any issue adding the skip below, and honestly that seems like a good safeguard. But can you update the commit message to indicate that this is simply that, a safeguard, and not because we have no option to make this work from the kernel side? Of course please correct me if I'm mistaken here. Thanks, Stuart > > Thanks, > Stuart > > > > > Signed-off-by: Janusz Krzysztofik < > > janusz.krzysztofik@linux.intel.com > > > > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Joonas Lahtinen <joonas.lahtinen@intel.com> > > --- > > lib/ioctl_wrappers.c | 2 +- > > lib/ioctl_wrappers.h | 1 + > > tests/prime_vgem.c | 6 ++++++ > > 3 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c > > index 628f8b83..78b92bd2 100644 > > --- a/lib/ioctl_wrappers.c > > +++ b/lib/ioctl_wrappers.c > > @@ -356,7 +356,7 @@ void gem_write(int fd, uint32_t handle, > > uint64_t > > offset, const void *buf, uint64 > > igt_assert_eq(__gem_write(fd, handle, offset, buf, length), 0); > > } > > > > -static int __gem_read(int fd, uint32_t handle, uint64_t offset, > > void > > *buf, uint64_t length) > > +int __gem_read(int fd, uint32_t handle, uint64_t offset, void > > *buf, > > uint64_t length) > > { > > struct drm_i915_gem_pread gem_pread; > > int err; > > diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h > > index f2412d78..97f278ed 100644 > > --- a/lib/ioctl_wrappers.h > > +++ b/lib/ioctl_wrappers.h > > @@ -69,6 +69,7 @@ uint32_t gem_open(int fd, uint32_t name); > > void gem_close(int fd, uint32_t handle); > > int __gem_write(int fd, uint32_t handle, uint64_t offset, const > > void > > *buf, uint64_t length); > > void gem_write(int fd, uint32_t handle, uint64_t offset, const > > void > > *buf, uint64_t length); > > +int __gem_read(int fd, uint32_t handle, uint64_t offset, void > > *buf, > > uint64_t length); > > void gem_read(int fd, uint32_t handle, uint64_t offset, void *buf, > > uint64_t length); > > int __gem_set_domain(int fd, uint32_t handle, uint32_t read, > > uint32_t write); > > void gem_set_domain(int fd, uint32_t handle, uint32_t read, > > uint32_t > > write); > > diff --git a/tests/prime_vgem.c b/tests/prime_vgem.c > > index 04cc913d..6595818c 100644 > > --- a/tests/prime_vgem.c > > +++ b/tests/prime_vgem.c > > @@ -46,6 +46,8 @@ static void test_read(int vgem, int i915) > > handle = prime_fd_to_handle(i915, dmabuf); > > close(dmabuf); > > > > + igt_skip_on(__gem_read(i915, handle, 0, &i, sizeof(i))); > > + > > ptr = vgem_mmap(vgem, &scratch, PROT_WRITE); > > for (i = 0; i < 1024; i++) > > ptr[1024*i] = i; > > @@ -81,6 +83,8 @@ static void test_fence_read(int i915, int vgem) > > handle = prime_fd_to_handle(i915, dmabuf); > > close(dmabuf); > > > > + igt_skip_on(__gem_read(i915, handle, 0, &i, sizeof(i))); > > + > > igt_fork(child, 1) { > > close(master[0]); > > close(slave[1]); > > @@ -191,6 +195,8 @@ static void test_write(int vgem, int i915) > > handle = prime_fd_to_handle(i915, dmabuf); > > close(dmabuf); > > > > + igt_skip_on(__gem_write(i915, handle, 0, &i, sizeof(i))); > > + > > ptr = vgem_mmap(vgem, &scratch, PROT_READ); > > gem_close(vgem, scratch.handle); > > > > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev
Hi Stuart, On Wednesday, November 20, 2019 11:04:59 PM CET Summers, Stuart wrote: > On Wed, 2019-11-20 at 17:14 +0000, Summers, Stuart wrote: > > On Wed, 2019-11-20 at 17:44 +0100, Janusz Krzysztofik wrote: > > > As we've agreed that using I915_GEM_PREAD/PWRITE IOCTLs on dma-buf > > > objects doesn't make much sense, we are not going to extend their > > > handlers in the i915 driver with new processing paths required for > > > them > > > to work correctly with dma-buf objects on future hardware with no > > > mappable aperture. When running on that kind of hardware, just > > > skip > > > subtests which use those IOCTLs. > > > > How is this handled for other dma-buf capable devices? Is there no > > opportunity for a working solution in i915 here? > > > > Also, is this a permanent limitation? Or just a workaround based on > > our > > current implementation? > > Thinking a bit more about this, I think my main concern here is around > the commit message. You are implying there is no chance we can I think my commit message doesn't imply we can't, it says that there is no need to ... > change > these DRM interfaces to work with dma-buf by referencing an I915 IOCTL. because that interface is not useful. > I don't personally have any issue adding the skip below, and honestly > that seems like a good safeguard. But can you update the commit message > to indicate that this is simply that, a safeguard, and not because we > have no option As my commit message states, we are not going ... > to make this work from the kernel side? even if we have an option, simply because there are no users of such interface. Thanks, Janusz > Of course please correct me if I'm mistaken here. > > Thanks, > Stuart > > > > > Thanks, > > Stuart > > > > > > > > Signed-off-by: Janusz Krzysztofik < > > > janusz.krzysztofik@linux.intel.com > > > > > > > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > Cc: Joonas Lahtinen <joonas.lahtinen@intel.com> > > > --- > > > lib/ioctl_wrappers.c | 2 +- > > > lib/ioctl_wrappers.h | 1 + > > > tests/prime_vgem.c | 6 ++++++ > > > 3 files changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c > > > index 628f8b83..78b92bd2 100644 > > > --- a/lib/ioctl_wrappers.c > > > +++ b/lib/ioctl_wrappers.c > > > @@ -356,7 +356,7 @@ void gem_write(int fd, uint32_t handle, > > > uint64_t > > > offset, const void *buf, uint64 > > > igt_assert_eq(__gem_write(fd, handle, offset, buf, length), 0); > > > } > > > > > > -static int __gem_read(int fd, uint32_t handle, uint64_t offset, > > > void > > > *buf, uint64_t length) > > > +int __gem_read(int fd, uint32_t handle, uint64_t offset, void > > > *buf, > > > uint64_t length) > > > { > > > struct drm_i915_gem_pread gem_pread; > > > int err; > > > diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h > > > index f2412d78..97f278ed 100644 > > > --- a/lib/ioctl_wrappers.h > > > +++ b/lib/ioctl_wrappers.h > > > @@ -69,6 +69,7 @@ uint32_t gem_open(int fd, uint32_t name); > > > void gem_close(int fd, uint32_t handle); > > > int __gem_write(int fd, uint32_t handle, uint64_t offset, const > > > void > > > *buf, uint64_t length); > > > void gem_write(int fd, uint32_t handle, uint64_t offset, const > > > void > > > *buf, uint64_t length); > > > +int __gem_read(int fd, uint32_t handle, uint64_t offset, void > > > *buf, > > > uint64_t length); > > > void gem_read(int fd, uint32_t handle, uint64_t offset, void *buf, > > > uint64_t length); > > > int __gem_set_domain(int fd, uint32_t handle, uint32_t read, > > > uint32_t write); > > > void gem_set_domain(int fd, uint32_t handle, uint32_t read, > > > uint32_t > > > write); > > > diff --git a/tests/prime_vgem.c b/tests/prime_vgem.c > > > index 04cc913d..6595818c 100644 > > > --- a/tests/prime_vgem.c > > > +++ b/tests/prime_vgem.c > > > @@ -46,6 +46,8 @@ static void test_read(int vgem, int i915) > > > handle = prime_fd_to_handle(i915, dmabuf); > > > close(dmabuf); > > > > > > + igt_skip_on(__gem_read(i915, handle, 0, &i, sizeof(i))); > > > + > > > ptr = vgem_mmap(vgem, &scratch, PROT_WRITE); > > > for (i = 0; i < 1024; i++) > > > ptr[1024*i] = i; > > > @@ -81,6 +83,8 @@ static void test_fence_read(int i915, int vgem) > > > handle = prime_fd_to_handle(i915, dmabuf); > > > close(dmabuf); > > > > > > + igt_skip_on(__gem_read(i915, handle, 0, &i, sizeof(i))); > > > + > > > igt_fork(child, 1) { > > > close(master[0]); > > > close(slave[1]); > > > @@ -191,6 +195,8 @@ static void test_write(int vgem, int i915) > > > handle = prime_fd_to_handle(i915, dmabuf); > > > close(dmabuf); > > > > > > + igt_skip_on(__gem_write(i915, handle, 0, &i, sizeof(i))); > > > + > > > ptr = vgem_mmap(vgem, &scratch, PROT_READ); > > > gem_close(vgem, scratch.handle); > > > > > > > _______________________________________________ > > igt-dev mailing list > > igt-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/igt-dev >
diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c index 628f8b83..78b92bd2 100644 --- a/lib/ioctl_wrappers.c +++ b/lib/ioctl_wrappers.c @@ -356,7 +356,7 @@ void gem_write(int fd, uint32_t handle, uint64_t offset, const void *buf, uint64 igt_assert_eq(__gem_write(fd, handle, offset, buf, length), 0); } -static int __gem_read(int fd, uint32_t handle, uint64_t offset, void *buf, uint64_t length) +int __gem_read(int fd, uint32_t handle, uint64_t offset, void *buf, uint64_t length) { struct drm_i915_gem_pread gem_pread; int err; diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h index f2412d78..97f278ed 100644 --- a/lib/ioctl_wrappers.h +++ b/lib/ioctl_wrappers.h @@ -69,6 +69,7 @@ uint32_t gem_open(int fd, uint32_t name); void gem_close(int fd, uint32_t handle); int __gem_write(int fd, uint32_t handle, uint64_t offset, const void *buf, uint64_t length); void gem_write(int fd, uint32_t handle, uint64_t offset, const void *buf, uint64_t length); +int __gem_read(int fd, uint32_t handle, uint64_t offset, void *buf, uint64_t length); void gem_read(int fd, uint32_t handle, uint64_t offset, void *buf, uint64_t length); int __gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write); void gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write); diff --git a/tests/prime_vgem.c b/tests/prime_vgem.c index 04cc913d..6595818c 100644 --- a/tests/prime_vgem.c +++ b/tests/prime_vgem.c @@ -46,6 +46,8 @@ static void test_read(int vgem, int i915) handle = prime_fd_to_handle(i915, dmabuf); close(dmabuf); + igt_skip_on(__gem_read(i915, handle, 0, &i, sizeof(i))); + ptr = vgem_mmap(vgem, &scratch, PROT_WRITE); for (i = 0; i < 1024; i++) ptr[1024*i] = i; @@ -81,6 +83,8 @@ static void test_fence_read(int i915, int vgem) handle = prime_fd_to_handle(i915, dmabuf); close(dmabuf); + igt_skip_on(__gem_read(i915, handle, 0, &i, sizeof(i))); + igt_fork(child, 1) { close(master[0]); close(slave[1]); @@ -191,6 +195,8 @@ static void test_write(int vgem, int i915) handle = prime_fd_to_handle(i915, dmabuf); close(dmabuf); + igt_skip_on(__gem_write(i915, handle, 0, &i, sizeof(i))); + ptr = vgem_mmap(vgem, &scratch, PROT_READ); gem_close(vgem, scratch.handle);
As we've agreed that using I915_GEM_PREAD/PWRITE IOCTLs on dma-buf objects doesn't make much sense, we are not going to extend their handlers in the i915 driver with new processing paths required for them to work correctly with dma-buf objects on future hardware with no mappable aperture. When running on that kind of hardware, just skip subtests which use those IOCTLs. Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Joonas Lahtinen <joonas.lahtinen@intel.com> --- lib/ioctl_wrappers.c | 2 +- lib/ioctl_wrappers.h | 1 + tests/prime_vgem.c | 6 ++++++ 3 files changed, 8 insertions(+), 1 deletion(-)