Re: [Intel-gfx] [PATCH i-g-t v2 04/11] lib/i915/gem_mman: add fixed mode to gem_mmap__cpu

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

On Thu, 29 Jul 2021 01:50:45 -0700, Matthew Auld wrote:
>

Hi Matt,

> On 29/07/2021 00:07, Dixit, Ashutosh wrote:
> > On Wed, 28 Jul 2021 03:30:34 -0700, Matthew Auld wrote:
> >>
> >> diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
> >> index 337d28fb..6f5e6d72 100644
> >> --- a/lib/i915/gem_mman.c
> >> +++ b/lib/i915/gem_mman.c
> >> @@ -434,7 +434,13 @@ void *gem_mmap__device_coherent(int fd, uint32_t handle, uint64_t offset,
> >>    */
> >>   void *__gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot)
> >>   {
> >> -	return __gem_mmap(fd, handle, offset, size, prot, 0);
> >> +	void *ptr;
> >> +
> >> +	ptr = __gem_mmap(fd, handle, offset, size, prot, 0);
> >> +	if (!ptr)
> >> +		ptr = __gem_mmap_offset__fixed(fd, handle, offset, size, prot);
> >> +
> >> +	return ptr;
> >
> > What about __gem_mmap__wc? Also shouldn't we just fix the __gem_mmap_offset
> > fallback in __gem_mmap and that will take care of both __gem_mmap__cpu and
> > __gem_mmap__wc?
>
> For gem_mmap__wc it felt like slightly too much lying, since on discrete
> smem-only buffers are always wb, and so the __wc here is not what the user
> gets with the new FIXED mode.

Correct.

> gem_mmap__device_coherent() I think matches this new behaviour well,
> where we don't explicitly state what the mapping type is, but instead
> just guarantee that the returned mapping is device coherent. My rough
> thinking was to convert most users of __wc over to __device_coherent(),
> at least in the tests that we care about for discrete?

Correct, though note that ALL tests will run on discrete (though with
smem-only buffers with today's IGT since I don't believe we have added any
tests which create lmem buffers yet).

> On the other hand if we are happy with the lie, I don't think anything will
> break, and pretty much all testscases using mmap I think should just
> magically work on discrete, and it does mean a less less work vs converting
> to __device_coherent?

I lost you here since I really don't know what you mean by not changing to
__device_coherent because afaiu gem_mmap__wc and gem_mmap_offset__wc will
fail on discrete so we can't just not do anything.

Next, I have no idea what is working and what is not working with the
present series on DG1 (since the CI is showing all red on DG1 and I don't
believe DG1 is included in showing regressions) and whether or not we have
completed these FIXED related changes, so is this series complete or not?

Afais since this series hasn't done anything about wc (except
__device_coherent), any test which does wc will fail on discrete.

> >
> > (I think it will actually also fix __gem_mmap__device_coherent and
> > __gem_mmap__cpu_coherent but maybe we can still have those patches in this
> > series especially if they save a couple of system calls).

Last, I believe I have a very simple solution to this FIXED
conundrum. Which I believe works (you guys can tell me if it doesn't) but
what I am not sure is whether it is an acceptable programming paradigm.

Let us say, on discrete, we decide that we won't make any changes to any of
the tests (we'll change on the library), and the tests themselves will not
even be aware of the FIXED mode. The tests always ask for WC or WB but
under the hood we will convert those calls to FIXED on discrete. So the
tests ask for WC or WB but that will mean FIXED on discrete.

Then I believe all we need to do is make a single very simple change to the
function __gem_mmap_offset(). In this functions, if we see WC or WB we will
convert that to FIXED before issuing the ioctl (or alternative issue the
ioctl but if it returns error we will retry with FIXED).

This works even for __gem_mmap() because __gem_mmap() itself falls back to
__gem_mmap_offset(). So afais we don't need to make ANY changes to ANY of
the higher level functions because we have fixed it in the lowest level
function. If we do it this way we won't need most of the first 7 patches in
the series. (This is what I was saying in the previous reply too).

But like I said whether we want to do this or not we have to decide and I
want other CC'd reviewers to chime in on this. It doesn't mean that we
cannot make changes to higher level functions if it makes things more
efficient or if we want to proliferate the FIXED mode throughout IGT but I
think changing the lowest level function is sufficient to fix failing
tests.

Thanks.
--
Ashutosh