Re: [PATCH 03/11] drm/i915: initialization/teardown for the aliasing ppgtt

[Ben Widawsky <ben@bwidawsk.net>]

On Wed, Nov 30, 2011 at 09:09:20AM +0100, Daniel Vetter wrote:
> On Tue, Nov 29, 2011 at 03:41:40PM -0800, Ben Widawsky wrote:
> > On Mon, Nov 28, 2011 at 09:35:30PM +0100, Daniel Vetter wrote:
> > > This just adds the setup and teardown code for the ppgtt PDE and the
> > > last-level pagetables, which are fixed for the entire lifetime, at
> > > least for the moment.
> > > 
> > > v2: Kill the stray debug printk noted by and improve the pte
> > > definitions as suggested by Chris Wilson.
> > > 
> > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Crap... one more point
> > 
> > > +/* PPGTT support for Sandybdrige/Gen6 and later */
> > > +static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
> > > +				   unsigned first_entry,
> > > +				   unsigned num_entries)
> > > +{
> > > +	int i, j;
> > > +	uint32_t *pt_vaddr;
> > > +	uint32_t scratch_pte;
> > > +
> > > +	scratch_pte = GEN6_PTE_ADDR_ENCODE(ppgtt->scratch_page_dma_addr);
> > > +	scratch_pte |= GEN6_PTE_VALID | GEN6_PTE_CACHE_LLC;
> > > +
> > > +	for (i = 0; i < ppgtt->num_pd_entries; i++) {
> > > +		pt_vaddr = kmap_atomic(ppgtt->pt_pages[i]);
> > > +
> > > +		for (j = 0; j < I915_PPGTT_PT_ENTRIES; j++)
> > > +			pt_vaddr[j] = scratch_pte;
> > > +
> > > +		kunmap_atomic(pt_vaddr);
> > > +	}
> > > +
> > > +}
> > 
> > Don't you want to clflush here (unless I missed it somewhere else).
> > Ideally I think you'd also flush the TLB with a PIPE_CONTROL, but I
> > guess so long as we have that bit set that always flushes we're okay on
> > that one... Still feel we need a clflush though.
> 
> Afaics ppgtt pte fetches are coherent with the cpu irrespective of the pte
> cacheability control setting. So like for llc cached buffers, no flushing
> needed at all. We also have a fair share of tests that trash mappings like
> crazy (and easily hit all the historical tlb issues we've had), so I'm
> fairly sure it works.

Well that's convenient. In that case, given how often we invalidate
TLBs, it'd be really nice if we could be smart about managing these
cache lines. Perhaps as part of execbuf we could warm up the the ptes?