[Intel-gfx] [PATCH] drm/i915: Use i915_gem_object_pin_map_unlocked function for lmem allocation

[Matthew Auld]

On Fri, 14 Jan 2022 at 08:25, Stanislav Lisovskiy
<stanislav.lisovskiy at intel.com> wrote:
>
> Using i915_gem_object_pin_map_unlocked instead of
> i915_gem_object_lmem_io_map, would eliminate the need
> of using I915_BO_ALLOC_CONTIGUOUS, when calling
> i915_vma_pin_iomap, because it supports non-contiguous
> allocation as well.
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_vma.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 1f15c3298112..194ad92013f6 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -547,10 +547,16 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
>                  * of pages, that way we can also drop the
>                  * I915_BO_ALLOC_CONTIGUOUS when allocating the object.
>                  */

Remove the TODO above also?

> -               if (i915_gem_object_is_lmem(vma->obj))
> -                       ptr = i915_gem_object_lmem_io_map(vma->obj, 0,
> -                                                         vma->obj->base.size);
> -               else
> +               if (i915_gem_object_is_lmem(vma->obj)) {
> +                       ptr = (void __iomem *)
> +                              i915_gem_object_pin_map_unlocked(vma->obj,
> +                                                               I915_MAP_WC);

Do you know if we need some kind of sanity check here to ensure that
the vma->pages == obj->mm.pages, when dealing with LMEM? AFAIK the vma
could in theory remap the pages, and here pin_map only cares about the
obj->mm.pages? Maybe check if the vma is VIEW_NORMAL or something?

> +                       if (IS_ERR(ptr)) {
> +                               err = PTR_ERR(ptr);
> +                               goto err;
> +                       }
> +                       ptr = page_pack_bits(ptr, 1);

AFAIK, the guidance is to move away from pointer packing. Can we just
make the iounmap check for is_lmem()?

> +               } else
>                         ptr = io_mapping_map_wc(&i915_vm_to_ggtt(vma->vm)->iomap,
>                                                 vma->node.start,
>                                                 vma->node.size);
> @@ -560,7 +566,10 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
>                 }
>
>                 if (unlikely(cmpxchg(&vma->iomap, NULL, ptr))) {
> -                       io_mapping_unmap(ptr);
> +                       if (page_unmask_bits(ptr))
> +                               __i915_gem_object_release_map(vma->obj);
> +                       else
> +                               io_mapping_unmap(ptr);
>                         ptr = vma->iomap;
>                 }
>         }
> @@ -574,7 +583,7 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
>         i915_vma_set_ggtt_write(vma);
>
>         /* NB Access through the GTT requires the device to be awake. */
> -       return ptr;
> +       return page_mask_bits(ptr);
>
>  err_unpin:
>         __i915_vma_unpin(vma);
> @@ -1687,7 +1696,11 @@ static void __i915_vma_iounmap(struct i915_vma *vma)
>         if (vma->iomap == NULL)
>                 return;
>
> -       io_mapping_unmap(vma->iomap);
> +       if (page_unmask_bits(vma->iomap))
> +               __i915_gem_object_release_map(vma->obj);
> +       else
> +               io_mapping_unmap(vma->iomap);
> +
>         vma->iomap = NULL;
>  }
>
> --
> 2.24.1.485.gad05a3d8e5
>