[Mesa-dev] [PATCH v2 07/11] intel: tools: aub_mem: reuse already mapped ppgtt buffers

[Rafael Antognolli]

On Wed, Aug 08, 2018 at 11:11:11PM +0100, Lionel Landwerlin wrote:
> On 08/08/18 20:07, Rafael Antognolli wrote:
> > On Tue, Aug 07, 2018 at 06:35:18PM +0100, Lionel Landwerlin wrote:
> > > When we map a PPGTT buffer into a continous address space of aubinator
> > > to be able to inspect it, we currently add it to the list of BOs to
> > > unmap once we're finished. An optimization we can apply it to look up
> > > that list before trying to remap PPGTT buffers again (we already do
> > > this for GGTT buffers).
> > > 
> > > We need to take some care before doing this because the list also
> > > contains GGTT BOs. As GGTT & PPGTT are 2 different address spaces, we
> > > can have matching addresses in both that point to different physical
> > > locations.
> > So, before this change, we could have the same address for PPGTT and
> > GGTT on the map list, but they never clashed because we only added the
> > PPGTT ones at the end, and then unmapped them? Or was there something
> > else preventing them from conflicting?
> 
> Before this change we could get clashes when asking for a GGTT address and
> get a PPGTT one.
> I think we got lucky so far because we use a very small amount of GGTT and
> that didn't happen.

You explained it, I understood and moved on, and forgot about it :-/

But this is:

Reviewed-by: Rafael Antognolli <rafael.antognolli at intel.com>

> 
> > > This changes adds a flag on the elements of the list of mapped BOs to
> > > differenciate between GGTT & PPGTT, which allows use to reuse that
> > > list when looking up both address spaces.
> > > 
> > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> > > ---
> > >   src/intel/tools/aub_mem.c | 16 +++++++++++-----
> > >   1 file changed, 11 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/intel/tools/aub_mem.c b/src/intel/tools/aub_mem.c
> > > index 2d29386e57c..3d4dc8061bd 100644
> > > --- a/src/intel/tools/aub_mem.c
> > > +++ b/src/intel/tools/aub_mem.c
> > > @@ -42,6 +42,7 @@ struct bo_map {
> > >      struct list_head link;
> > >      struct gen_batch_decode_bo bo;
> > >      bool unmap_after_use;
> > > +   bool ppgtt;
> > >   };
> > >   struct ggtt_entry {
> > > @@ -59,10 +60,11 @@ struct phys_mem {
> > >   };
> > >   static void
> > > -add_gtt_bo_map(struct aub_mem *mem, struct gen_batch_decode_bo bo, bool unmap_after_use)
> > > +add_gtt_bo_map(struct aub_mem *mem, struct gen_batch_decode_bo bo, bool ppgtt, bool unmap_after_use)
> > >   {
> > >      struct bo_map *m = calloc(1, sizeof(*m));
> > > +   m->ppgtt = ppgtt;
> > >      m->bo = bo;
> > >      m->unmap_after_use = unmap_after_use;
> > >      list_add(&m->link, &mem->maps);
> > > @@ -190,7 +192,7 @@ aub_mem_local_write(void *_mem, uint64_t address,
> > >         .addr = address,
> > >         .size = size,
> > >      };
> > > -   add_gtt_bo_map(mem, bo, false);
> > > +   add_gtt_bo_map(mem, bo, false, false);
> > >   }
> > >   void
> > > @@ -253,7 +255,7 @@ aub_mem_get_ggtt_bo(void *_mem, uint64_t address)
> > >      struct gen_batch_decode_bo bo = {0};
> > >      list_for_each_entry(struct bo_map, i, &mem->maps, link)
> > > -      if (i->bo.addr <= address && i->bo.addr + i->bo.size > address)
> > > +      if (!i->ppgtt && i->bo.addr <= address && i->bo.addr + i->bo.size > address)
> > >            return i->bo;
> > >      address &= ~0xfff;
> > > @@ -292,7 +294,7 @@ aub_mem_get_ggtt_bo(void *_mem, uint64_t address)
> > >         assert(res != MAP_FAILED);
> > >      }
> > > -   add_gtt_bo_map(mem, bo, true);
> > > +   add_gtt_bo_map(mem, bo, false, true);
> > >      return bo;
> > >   }
> > > @@ -328,6 +330,10 @@ aub_mem_get_ppgtt_bo(void *_mem, uint64_t address)
> > >      struct aub_mem *mem = _mem;
> > >      struct gen_batch_decode_bo bo = {0};
> > > +   list_for_each_entry(struct bo_map, i, &mem->maps, link)
> > > +      if (i->ppgtt && i->bo.addr <= address && i->bo.addr + i->bo.size > address)
> > > +         return i->bo;
> > > +
> > >      address &= ~0xfff;
> > >      if (!ppgtt_mapped(mem, mem->pml4, address))
> > > @@ -353,7 +359,7 @@ aub_mem_get_ppgtt_bo(void *_mem, uint64_t address)
> > >         assert(res != MAP_FAILED);
> > >      }
> > > -   add_gtt_bo_map(mem, bo, true);
> > > +   add_gtt_bo_map(mem, bo, true, true);
> > >      return bo;
> > >   }
> > > -- 
> > > 2.18.0
> > > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev