[igt-dev] [PATCH i-g-t 1/2] lib/i915/gem_mman: Add coherent mmap api support with set domain

[Telukuntla, Sreedhar]

> -----Original Message-----
> From: Chris Wilson <chris at chris-wilson.co.uk>
> Sent: Monday, February 17, 2020 9:59 PM
> To: Telukuntla, Sreedhar <sreedhar.telukuntla at intel.com>; igt-
> dev at lists.freedesktop.org
> Cc: Ursulin, Tvrtko <tvrtko.ursulin at intel.com>
> Subject: Re: [igt-dev] [PATCH i-g-t 1/2] lib/i915/gem_mman: Add coherent
> mmap api support with set domain
> 
> Quoting Sreedhar Telukuntla (2020-02-18 00:17:09)
> > gem_mmap__device_coherent_domain api maps buffer object and returns
> > the memory domain to be used by the caller for subsequent set_domain
> > call for cpu access.
> >
> > gem_mmap__device_coherent_sync api maps buffer object and also does
> > set_domain to make the memory ready for cpu access.
> >
> > Signed-off-by: Sreedhar Telukuntla <sreedhar.telukuntla at intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >  lib/i915/gem_mman.c | 76
> > ++++++++++++++++++++++++++++++++++++++++++---
> >  lib/i915/gem_mman.h |  6 +++-
> >  2 files changed, 77 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c index
> > 08ae6769..b8d20c07 100644
> > --- a/lib/i915/gem_mman.c
> > +++ b/lib/i915/gem_mman.c
> > @@ -345,21 +345,25 @@ void *gem_mmap_offset__wc(int fd, uint32_t
> handle, uint64_t offset,
> >   * @offset: offset in the gem buffer of the mmap arena
> >   * @size: size of the mmap arena
> >   * @prot: memory protection bits as used by mmap()
> > + * @domain: cache  domain used for mmap
> >   *
> >   * Returns: A pointer to a block of linear device memory mapped into the
> >   * process with WC semantics. When no WC is available try to mmap using
> GGTT.
> >   */
> >  void *__gem_mmap__device_coherent(int fd, uint32_t handle, uint64_t
> offset,
> > -                                 uint64_t size, unsigned prot)
> > +                                 uint64_t size, unsigned prot,
> > + uint32_t *domain)
> >  {
> >         void *ptr = __gem_mmap_offset(fd, handle, offset, size, prot,
> >                                       I915_MMAP_OFFSET_WC);
> >         if (!ptr)
> >                 ptr = __gem_mmap__wc(fd, handle, offset, size, prot);
> >
> > -       if (!ptr)
> > -               ptr = __gem_mmap__gtt(fd, handle, size, prot);
> > +       *domain = I915_GEM_DOMAIN_WC;
> >
> > +       if (!ptr) {
> > +               ptr = __gem_mmap__gtt(fd, handle, size, prot);
> > +               *domain = I915_GEM_DOMAIN_GTT;
> > +       }
> >         return ptr;
> >  }
> >
> > @@ -382,15 +386,79 @@ void *gem_mmap__device_coherent(int fd,
> uint32_t handle, uint64_t offset,
> >                                 uint64_t size, unsigned prot)  {
> >         void *ptr;
> > +       uint32_t domain;
> >
> >         igt_assert(offset == 0);
> >
> > -       ptr = __gem_mmap__device_coherent(fd, handle, offset, size, prot);
> > +       ptr = __gem_mmap__device_coherent(fd, handle, offset, size,
> > + prot, &domain);
> >         igt_assert(ptr);
> >
> >         return ptr;
> >  }
> >
> > +/**
> > + * gem_mmap__device_coherent_domain:
> > + * @fd: open i915 drm file descriptor
> > + * @handle: gem buffer object handle
> > + * @offset: offset in the gem buffer of the mmap arena
> > + * @size: size of the mmap arena
> > + * @prot: memory protection bits as used by mmap()
> > + * @domain: memory domain to be used by the caller for set_domain
> > +after mmap
> > + *
> > + * Call __gem_mmap__device__coherent_domain(), asserts on fail.
> > + * Offset argument passed in function call must be 0. In the future
> > + * when driver will allow slice mapping of buffer object this
> > +restriction
> > + * will be removed.
> > + *
> > + * Returns: A pointer to the created memory mapping.
> > + */
> > +void *gem_mmap__device_coherent_domain(int fd, uint32_t handle,
> uint64_t offset,
> > +                               uint64_t size, unsigned prot, uint32_t
> > +*domain) {
> > +       void *ptr;
> > +
> > +       igt_assert(offset == 0);
> > +
> > +       ptr = __gem_mmap__device_coherent(fd, handle, offset, size, prot,
> domain);
> > +       igt_assert(ptr);
> > +
> > +       return ptr;
> > +}
> > +
> > +/**
> > + * gem_mmap__device_coherent_sync:
> > + * @fd: open i915 drm file descriptor
> > + * @handle: gem buffer object handle
> > + * @offset: offset in the gem buffer of the mmap arena
> > + * @size: size of the mmap arena
> > + * @prot: memory protection bits as used by mmap()
> > + *
> > + * Call __gem_mmap__device__coherent_sync(), asserts on fail.
> > + * Offset argument passed in function call must be 0. In the future
> > + * when driver will allow slice mapping of buffer object this
> > +restriction
> > + * will be removed.
> > + *
> > + * This function sets appropriate memory domain as well once the
> > +mapping is
> > + * complete and makes the memory ready for cpu access
> > + *
> > + * Returns: A pointer to the created memory mapping.
> > + */
> > +void *gem_mmap__device_coherent_sync(int fd, uint32_t handle, uint64_t
> offset,
> > +                               uint64_t size, unsigned prot) {
> > +       void *ptr;
> > +       uint32_t domain;
> > +
> > +       igt_assert(offset == 0);
> > +
> > +       ptr = gem_mmap__device_coherent_domain(fd, handle, offset, size,
> prot, &domain);
> > +       igt_assert(ptr);
> > +
> > +       gem_set_domain(fd, handle, domain, domain);
> 
> No. Read/write is important, especially when there are comments indicating
> that the api is being subverted and abused.

I didn’t understand your comment. Is it about read/write domain usage here? Are you suggesting not to use gem_set_domain in this way?

Regards,
Sreedhar

> -Chris