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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
> >
