| Message ID | 1404424910-7234-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Is the requirement before or after the SET_CONTEXT? 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
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.
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)); + if (ring->id == RCS) return do_switch_rcs(ring, from, to); else
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(+)