Re: [igt-dev] [PATCH i-g-t] lib/i915/gem_mman.c: add cpu coherency mapping wrapper

Chris Wilson
2020-01-14T11:15:02Z

Quoting Dixit, Ashutosh (2020-01-14 05:59:54)
> On Mon, 13 Jan 2020 21:27:28 -0800, Zbigniew Kempczyński wrote:
> >
> > For reduce code redundancy adding a wrapper for cpu memory mapping.
> >
> > +void *__gem_mmap__cpu_coherent(int fd, uint32_t handle, uint64_t offset,
> > + uint64_t size, unsigned prot)
> > +{
> > + void *ptr = __gem_mmap_offset__cpu(fd, handle, offset, size, prot);
> > +
> > + if (!ptr)
> > + ptr = __gem_mmap__cpu(fd, handle, offset, size, prot);
> > +
> > + return ptr;
> > +}
>
> We need similar wrappers for WC and GTT too. So why don't we put this code

That's gem_mmap__device_coherent.

> in __gem_mmap__cpu() itself and we can do the same for __gem_mmap__wc() and
> __gem_mmap__gtt() too? Otherwise what are we going to call those functions?
> Something like:
>
> void *__gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot)
> {
> if (gem_has_mmap_offset(fd))
> return __gem_mmap_offset(fd, handle, offset, size, prot, I915_MMAP_OFFSET_WB);
> else
> return __gem_mmap(fd, handle, offset, size, prot, 0);
> }
>
> So I am not sure of the point of introducing new wrappers, this code could
> just be put in the old existing wrappers.

The point is that we are separating intent from implementation. Where a
test is not looking at the mmap ioctl, but just wants access to a
buffer, how it wants to access that buffer is the important bit of
information.
-Chris