Re: [PATCH v2 09/37] drm/i915/lmem: support kernel mapping

Chris Wilson
2019-06-27T23:27:07Z

Quoting Matthew Auld (2019-06-27 21:56:05)
> From: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
>
> We can create LMEM objects, but we also need to support mapping them
> into kernel space for internal use.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_pages.c | 18 ++++-
> drivers/gpu/drm/i915/intel_region_lmem.c | 24 ++++++
> drivers/gpu/drm/i915/intel_region_lmem.h | 6 ++
> .../drm/i915/selftests/intel_memory_region.c | 77 +++++++++++++++++++
> 4 files changed, 121 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index b36ad269f4ea..15eaaedffc46 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -176,7 +176,9 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
> void *ptr;
>
> ptr = page_mask_bits(obj->mm.mapping);
> - if (is_vmalloc_addr(ptr))
> + if (i915_gem_object_is_lmem(obj))
> + io_mapping_unmap(ptr);
> + else if (is_vmalloc_addr(ptr))
> vunmap(ptr);
> else
> kunmap(kmap_to_page(ptr));
> @@ -235,7 +237,7 @@ int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
> }
>
> /* The 'mapping' part of i915_gem_object_pin_map() below */
> -static void *i915_gem_object_map(const struct drm_i915_gem_object *obj,
> +static void *i915_gem_object_map(struct drm_i915_gem_object *obj,
> enum i915_map_type type)
> {
> unsigned long n_pages = obj->base.size >> PAGE_SHIFT;
> @@ -248,6 +250,11 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj,
> pgprot_t pgprot;
> void *addr;
>
> + if (i915_gem_object_is_lmem(obj)) {
> + /* XXX: we are ignoring the type here -- this is simply wc */

Yeah, don't. The callers certainly do not expect that.

> + return i915_gem_object_lmem_io_map(obj, 0, obj->base.size);
> + }
> +
> /* A single page can always be kmapped */
> if (n_pages == 1 && type == I915_MAP_WB)
> return kmap(sg_page(sgt->sgl));
> @@ -293,7 +300,8 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
> void *ptr;
> int err;
>
> - if (unlikely(!i915_gem_object_has_struct_page(obj)))
> + if (unlikely(!i915_gem_object_has_struct_page(obj) &&
> + !i915_gem_object_is_lmem(obj)))

Redefine the feature bit in the obj->ops->flags.

> return ERR_PTR(-ENXIO);
>
> err = mutex_lock_interruptible(&obj->mm.lock);
> @@ -325,7 +333,9 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
> goto err_unpin;
> }
>
> - if (is_vmalloc_addr(ptr))
> + if (i915_gem_object_is_lmem(obj))
> + io_mapping_unmap(ptr);
> + else if (is_vmalloc_addr(ptr))
> vunmap(ptr);
> else
> kunmap(kmap_to_page(ptr));
> diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c b/drivers/gpu/drm/i915/intel_region_lmem.c
> index 15655cc5013f..701bcac3479e 100644
> --- a/drivers/gpu/drm/i915/intel_region_lmem.c
> +++ b/drivers/gpu/drm/i915/intel_region_lmem.c
> @@ -73,6 +73,30 @@ static const struct intel_memory_region_ops region_lmem_ops = {
> .create_object = lmem_create_object,
> };
>
> +/* XXX: Time to vfunc your life up? */
> +void __iomem *i915_gem_object_lmem_io_map_page(struct drm_i915_gem_object *obj,
> + unsigned long n)
> +{
> + resource_size_t offset;
> +
> + offset = i915_gem_object_get_dma_address(obj, n);

That seems dubious. So dubious that I again say do not mix terms.

> + return io_mapping_map_atomic_wc(&obj->memory_region->iomap, offset);

Ahem. The caller may not be ready to abide by the terms of the atomic
contract.

> +}