Re: [PATCH 1/2] drm/i915/gen8: Invalidate TLBs before PDP reload

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, Jul 04, 2014 at 08:51:59AM +0100, Chris Wilson wrote:
> On Thu, Jul 03, 2014 at 03:01:49PM -0700, Ben Widawsky wrote:
> > This is a spec requirement for all rings.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 5b4a9a0..1ac648f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -870,6 +870,9 @@ int i915_switch_context(struct intel_engine_cs *ring,
> >  	if (from == to && !to->remap_slice)
> >  		return 0;
> >  
> > +	if (IS_GEN8(ring->dev))
> > +		WARN_ON(ring->flush(ring, I915_GEM_GPU_DOMAINS, 0));
> > +
> 
> That's intel_ring_invalidate_all_caches(). The only time we won't be
> doing this are the forced switches at startup/reset and close.

So there were three reasons for doing this.
1. AFAICT there were cases when we don't, which you seem to agree on.
2. I liked the explicit nature which /should/ have minimal impact given
that it's already done. It's future proof (to whatever extent such a
thing is possible). It's clear.
3. The worst of the reasons. On more than one occasion, I sent a trace
to certain people and they complained I was missing the TLB invalidate.
So I put it right there where nobody could miss it.

> Is the requirement before or after the SET_CONTEXT?

The requirement is before the PDP load. Which on GEN8 RCS means, before
MI_SET_CONTEXT. When we will RESTORE_INHIBIT, it must be before the
LRIs.

> What I would prefer doing
> is moving the invalidate-dispatch-flush-signal into one atomic operation
> (that would help simplify execlist as well) - would that be good enough
> here to be sure that an invalidate is performed after the context
> switch and before the next batch?
> -Chris
> 

As long as the ordering is correct based on the above, it sounds fine to
me. Assuming any of the other patches in the series gets merged though,
I'd propose sticking this one in as well, until the final solution
lands.

-- 
Ben Widawsky, Intel Open Source Technology Center