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

[Dixit, Ashutosh]

On Tue, 14 Jan 2020 03:14:57 -0800, Chris Wilson wrote:
>
> 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.

After Chris' explanation:

Acked-by: Ashutosh Dixit <ashutosh.dixit at intel.com>