Re: [igt-dev] [PATCH i-g-t] lib/ioctl_wrappers: Keep IGT working without pread/pwrite ioctls

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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ński <zbigniew.kempczynski@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >   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 <valgrind/valgrind.h>
> > @@ -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) != I915_CACHING_NONE;
> > +}
> > +
> > +static void mmap_write(int fd, uint32_t handle, uint64_t offset,
> > +		       const void *buf, uint64_t length)
> > +{
> > +	void *map = 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/pwrite
> and which can just work. I trust that has been established through CI runs
> and manual inspection.

Correct.

> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks.

> Regards,
>
> Tvrtko
>
> > +		map = __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 = __gem_mmap_offset__wc(fd, handle, 0, offset + length,
> > +					    PROT_READ | PROT_WRITE);
> > +		if (!map)
> > +			map = 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