[igt-dev] [PATCH v3 1/3] lib/rendercopy: Add AUX page table support

[Chris Wilson]

Quoting Imre Deak (2019-11-06 16:36:49)
> On Wed, Nov 06, 2019 at 04:14:36PM +0000, Chris Wilson wrote:
> > Quoting Imre Deak (2019-11-06 16:00:16)
> > > On Wed, Nov 06, 2019 at 11:11:40AM +0000, Chris Wilson wrote:
> > > > What's the coherency model for the pgt->bo? There's a lot of writes here
> > > > from the CPU how are you making sure they are visible to the GPU?
> > > 
> > > My understanding: execbuf will do a clflush for the buf on non-LLC
> > > platforms, while on LLC platforms that's not needed; not sure if there
> > > will be any non-LLC platforms where the AUX pagetable will be needed.
> > 
> > No, we don't unless you tell us you modified it between batches. That
> > would be the case if you were using drm_intel_bo_map(true) everytime,
> > but again that will then stall the GPU between every batch -- which we
> > definitely do not want when testing [as it will hide every bug] :)
> 
> Do you see a problem with the way I program it here though? I call
> drm_intel_bo_map(true) for the purpose of programming a new table. If
> you see a problem with that what do you suggest instead?

You create a new pagetable for every batch. Good for solitary render
copies, not so good for validation :( I expect we are invoking too many
stalls to be able to detect missing flushes and invalidations.

I would not suggest this interface is suitable long term. There is a
large leap between this and userspace, that I feel like we should be
covering more of the stepping stones so that userspace can trust the
layers it is built on [for some value of trust].

> > > On top of that the AUX GAM has its own caches - found that now in an HSD
> > > doc - which will be invalidated whenever GAM's top level table base
> > > pointer register is set from the context image, which happens whenver
> > > switching to the context.
> > 
> > But not for a lite-restore?
> 
> We still write the top level table regsiter from the batch, so that
> could in itself trigger the invalidation.

I would not expect that to be true for real clients. Or is it the only
recommended programming model?

> > So we have a problem when updating existing entries between batches
> > inside the same context? We put a big hammer at the front of the
> > request for TLB invalidation. If there's a flush we can add, we need
> > it there.
> 
> Yes, there is, see the CCS_AUX_INV reg in bspec. But I suspect this
> would have the same effect as writing the table ptr register, so not
> sure when exactly this would be needed. 

But the top level base pointer will not be updated on a lite restore;
are you sure the invalidation will occur if sublevels of the tt are
updated?

The good thing is that this is all virtual addresses, though there is a
slight danger with using system pages if the TLB is not flushed before
release (i.e. it can still access the page contents after someone else
uses them).
-Chris