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

* Re: [igt-dev] [PATCH i-g-t 1/2] lib/i915/gem_mman: Add coherent mmap api support with set domain
  2020-02-18  0:17 ` [igt-dev] [PATCH i-g-t 1/2] lib/i915/gem_mman: Add coherent mmap api support with set domain Sreedhar Telukuntla
@ 2020-02-17 16:29   ` Chris Wilson
  2020-02-18  3:53     ` Telukuntla, Sreedhar
  2020-02-28 11:06     ` Tvrtko Ursulin
  0 siblings, 2 replies; 8+ messages in thread
From: Chris Wilson @ 2020-02-17 16:29 UTC (permalink / raw)
  To: Sreedhar Telukuntla, igt-dev; +Cc: Tvrtko Ursulin

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@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 8+ messages in thread