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

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Tue, Jan 18, 2022 at 01:45:10PM +0000, Matthew Auld wrote:
> On Fri, 14 Jan 2022 at 08:25, Stanislav Lisovskiy
> <stanislav.lisovskiy@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@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?

Yep, should remove

> 
> > -               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?

As I see i915_gem_object_pin_map_unlocked is basically just
calling i915_gem_object_pin_pages, which is also being called
by i915_vma_get_pages, which handles this by calling __i915_vma_get_pages.
It then checks if those need to be remapped based on ggtt_view.type.
So wondering if I just need to call __i915_vma_get_pages as well,
afterwards.

> 
> > +                       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()?

Yes, will check this.

Stan

> 
> > +               } 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
> >