From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0308E89F27 for ; Mon, 15 Mar 2021 23:23:31 +0000 (UTC) Date: Mon, 15 Mar 2021 16:19:06 -0700 Message-ID: <87lfaohuol.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: References: <20210121083742.46592-1-ashutosh.dixit@intel.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Subject: Re: [igt-dev] [PATCH i-g-t] lib/ioctl_wrappers: Keep IGT working without pread/pwrite ioctls List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-2" Content-Transfer-Encoding: quoted-printable Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Tvrtko Ursulin Cc: igt-dev@lists.freedesktop.org List-ID: On Mon, 15 Mar 2021 09:51:04 -0700, Tvrtko Ursulin wrote: > > > On 21/01/2021 08:37, Ashutosh Dixit wrote: > > The general direction at this time is to phase out pread/write ioctls > > and not support them in future products. This means IGT must handle > > the absence of these ioctls. This patch does this by modifying > > gem_read() and gem_write() to do the read/write using the pread/pwrite > > ioctls first but when these ioctls are unavailable fall back to doing > > the read/write using a combination of mmap and memcpy. > > > > Callers who must absolutely use the pread/pwrite ioctls (such as tests > > which test these ioctls or must otherwise only use the pread/pwrite > > ioctls) must use gem_require_pread_pwrite() to skip when these ioctls > > are not available. > > > > v1: Removed __gem_pread, gem_pread, __gem_pwrite and gem_pwrite > > introduced previously since they are not necessary, > > gem_require_pread_pwrite is sufficient > > v2: Fix CI failures in gem_advise and gen9_exec_parse by introducing > > gem_require_pread_pwrite > > v3: Skip mmap for 0 length read/write's > > v4: Remove redundant igt_assert's > > v5: Re-run > > v6: s/EOPNOTSUPP/-EOPNOTSUPP/ > > v7: Rebase on latest master, skip gem_exec_parallel@userptr with > > gem_require_pread_pwrite > > > > Reviewed-by: Zbigniew Kempczy=F1ski > > Signed-off-by: Ashutosh Dixit > > --- > > lib/ioctl_wrappers.c | 135 +++++++++++++++++++- > > lib/ioctl_wrappers.h | 3 + > > tests/i915/gem_exec_parallel.c | 3 + > > tests/i915/gem_madvise.c | 2 + > > tests/i915/gem_partial_pwrite_pread.c | 1 + > > tests/i915/gem_pread.c | 1 + > > tests/i915/gem_pread_after_blit.c | 1 + > > tests/i915/gem_pwrite.c | 1 + > > tests/i915/gem_pwrite_snooped.c | 1 + > > tests/i915/gem_readwrite.c | 1 + > > tests/i915/gem_set_tiling_vs_pwrite.c | 1 + > > tests/i915/gem_tiled_partial_pwrite_pread.c | 1 + > > tests/i915/gem_tiled_pread_basic.c | 1 + > > tests/i915/gem_tiled_pread_pwrite.c | 1 + > > tests/i915/gem_userptr_blits.c | 2 + > > tests/i915/gen9_exec_parse.c | 4 +- > > 16 files changed, 152 insertions(+), 7 deletions(-) > > > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c > > index 45415621b7..4440004c42 100644 > > --- a/lib/ioctl_wrappers.c > > +++ b/lib/ioctl_wrappers.c > > @@ -56,6 +56,7 @@ > > #include "igt_debugfs.h" > > #include "igt_sysfs.h" > > #include "config.h" > > +#include "i915/gem_mman.h" > > #ifdef HAVE_VALGRIND > > #include > > @@ -324,6 +325,70 @@ void gem_close(int fd, uint32_t handle) > > do_ioctl(fd, DRM_IOCTL_GEM_CLOSE, &close_bo); > > } > > +static bool is_cache_coherent(int fd, uint32_t handle) > > +{ > > + return gem_get_caching(fd, handle) !=3D I915_CACHING_NONE; > > +} > > + > > +static void mmap_write(int fd, uint32_t handle, uint64_t offset, > > + const void *buf, uint64_t length) > > +{ > > + void *map =3D NULL; > > + > > + if (!length) > > + return; > > + > > + if (is_cache_coherent(fd, handle)) { > > + /* offset arg for mmap functions must be 0 */ > > Offset and lenght I think. So I guess none of the tests used non-aligned > access? Might be worth putting those two as asserts at the top of > mmap_write and mmap_read. Hi Tvrtko, sorry I think you missed the logic. offset and length coming into mmap_write() and mmap_read() are non-zero (so tests are using non-aligned access). However __gem_mmap_offset() (called from __gem_mmap__cpu_coherent() etc.) has an assert that needs the offset to be 0 (which is what the comment above refers to). Therefore instead of doing mmap(offset, length) below we do mmap(0, offset + length). A similar logic is used in mmap_write() in lib/intel_bufops.c. > Patch looks good to me. I did not investigate which tests need pread/pwri= te > and which can just work. I trust that has been established through CI runs > and manual inspection. Correct. > Acked-by: Tvrtko Ursulin Thanks. > Regards, > > Tvrtko > > > + map =3D __gem_mmap__cpu_coherent(fd, handle, 0, offset + length, > > + PROT_READ | PROT_WRITE); > > + if (map) > > + gem_set_domain(fd, handle, > > + I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU); > > + } > > + > > + if (!map) { > > + map =3D __gem_mmap_offset__wc(fd, handle, 0, offset + length, > > + PROT_READ | PROT_WRITE); > > + if (!map) > > + map =3D gem_mmap__wc(fd, handle, 0, offset + length, > > + PROT_READ | PROT_WRITE); > > + gem_set_domain(fd, handle, > > + I915_GEM_DOMAIN_WC, I915_GEM_DOMAIN_WC); > > + } > > + > > + memcpy(map + offset, buf, length); > > + munmap(map, offset + length); > > +} _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev