| Message ID | 1396503023-3114-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 02, 2014 at 10:30:23PM -0700, Ben Widawsky wrote: > We have been setting the bit which was originally BIOS dependent since: > commit f05bb0c7b624252a5e768287e340e8e45df96e42 > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Sun Jan 20 16:33:32 2013 +0000 > > drm/i915: GFX_MODE Flush TLB Invalidate Mode must be '1' for scanline waits > > Therefore, we do not need to try to figure it out dynamically and we can > just always invalidate the TLBs. > > It's a partial revert of: > commit 12b0286f49947a6cdc9285032d918466a8c3f5f9 > Author: Ben Widawsky <ben@bwidawsk.net> > Date: Mon Jun 4 14:42:50 2012 -0700 > > drm/i915: possibly invalidate TLB before context switch > > The original commit attempted to only invalidate when necessary > (very much a relic from the old days). Now, we can just always invalidate. > > I guess the old TODO still exists. Since we seem to have abandoned ILK > contexts however, there isn't much point in even remembering. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> Seems reasonable, except in most cases (execbuffer) there will be a following cache-invalidate as part of the move-to-gpu. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> ILK ctx, never forget. -Chris
On Thu, Apr 03, 2014 at 08:05:35AM +0100, Chris Wilson wrote: > On Wed, Apr 02, 2014 at 10:30:23PM -0700, Ben Widawsky wrote: > > We have been setting the bit which was originally BIOS dependent since: > > commit f05bb0c7b624252a5e768287e340e8e45df96e42 > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > Date: Sun Jan 20 16:33:32 2013 +0000 > > > > drm/i915: GFX_MODE Flush TLB Invalidate Mode must be '1' for scanline waits > > > > Therefore, we do not need to try to figure it out dynamically and we can > > just always invalidate the TLBs. > > > > It's a partial revert of: > > commit 12b0286f49947a6cdc9285032d918466a8c3f5f9 > > Author: Ben Widawsky <ben@bwidawsk.net> > > Date: Mon Jun 4 14:42:50 2012 -0700 > > > > drm/i915: possibly invalidate TLB before context switch > > > > The original commit attempted to only invalidate when necessary > > (very much a relic from the old days). Now, we can just always invalidate. > > > > I guess the old TODO still exists. Since we seem to have abandoned ILK > > contexts however, there isn't much point in even remembering. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > Seems reasonable, except in most cases (execbuffer) there will be > a following cache-invalidate as part of the move-to-gpu. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Queued for -next, thanks for the patch. > ILK ctx, never forget. Iirc those patches still hang in there ;-) Cheers, Daniel
On Thu, Apr 03, 2014 at 08:05:35AM +0100, Chris Wilson wrote: > On Wed, Apr 02, 2014 at 10:30:23PM -0700, Ben Widawsky wrote: > > We have been setting the bit which was originally BIOS dependent since: > > commit f05bb0c7b624252a5e768287e340e8e45df96e42 > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > Date: Sun Jan 20 16:33:32 2013 +0000 > > > > drm/i915: GFX_MODE Flush TLB Invalidate Mode must be '1' for scanline waits > > > > Therefore, we do not need to try to figure it out dynamically and we can > > just always invalidate the TLBs. > > > > It's a partial revert of: > > commit 12b0286f49947a6cdc9285032d918466a8c3f5f9 > > Author: Ben Widawsky <ben@bwidawsk.net> > > Date: Mon Jun 4 14:42:50 2012 -0700 > > > > drm/i915: possibly invalidate TLB before context switch > > > > The original commit attempted to only invalidate when necessary > > (very much a relic from the old days). Now, we can just always invalidate. > > > > I guess the old TODO still exists. Since we seem to have abandoned ILK > > contexts however, there isn't much point in even remembering. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > Seems reasonable, except in most cases (execbuffer) there will be > a following cache-invalidate as part of the move-to-gpu. Except we still move_to_gpu() before the context switch. My fbc related patch to change that order never got merged. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > ILK ctx, never forget. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Apr 03, 2014 at 01:21:38PM +0300, Ville Syrjälä wrote: > On Thu, Apr 03, 2014 at 08:05:35AM +0100, Chris Wilson wrote: > > On Wed, Apr 02, 2014 at 10:30:23PM -0700, Ben Widawsky wrote: > > > We have been setting the bit which was originally BIOS dependent since: > > > commit f05bb0c7b624252a5e768287e340e8e45df96e42 > > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > > Date: Sun Jan 20 16:33:32 2013 +0000 > > > > > > drm/i915: GFX_MODE Flush TLB Invalidate Mode must be '1' for scanline waits > > > > > > Therefore, we do not need to try to figure it out dynamically and we can > > > just always invalidate the TLBs. > > > > > > It's a partial revert of: > > > commit 12b0286f49947a6cdc9285032d918466a8c3f5f9 > > > Author: Ben Widawsky <ben@bwidawsk.net> > > > Date: Mon Jun 4 14:42:50 2012 -0700 > > > > > > drm/i915: possibly invalidate TLB before context switch > > > > > > The original commit attempted to only invalidate when necessary > > > (very much a relic from the old days). Now, we can just always invalidate. > > > > > > I guess the old TODO still exists. Since we seem to have abandoned ILK > > > contexts however, there isn't much point in even remembering. > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > > Seems reasonable, except in most cases (execbuffer) there will be > > a following cache-invalidate as part of the move-to-gpu. > > Except we still move_to_gpu() before the context switch. My fbc related > patch to change that order never got merged. Hm, pointer to that thing? I guess we need to sign up someone to dig out all your fbc patches and get them in line in prep for bdw. At least another opportunity to beat all that into better shape ... -Daniel
On Thu, Apr 03, 2014 at 05:28:54PM +0200, Daniel Vetter wrote: > On Thu, Apr 03, 2014 at 01:21:38PM +0300, Ville Syrjälä wrote: > > On Thu, Apr 03, 2014 at 08:05:35AM +0100, Chris Wilson wrote: > > > On Wed, Apr 02, 2014 at 10:30:23PM -0700, Ben Widawsky wrote: > > > > We have been setting the bit which was originally BIOS dependent since: > > > > commit f05bb0c7b624252a5e768287e340e8e45df96e42 > > > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > > > Date: Sun Jan 20 16:33:32 2013 +0000 > > > > > > > > drm/i915: GFX_MODE Flush TLB Invalidate Mode must be '1' for scanline waits > > > > > > > > Therefore, we do not need to try to figure it out dynamically and we can > > > > just always invalidate the TLBs. > > > > > > > > It's a partial revert of: > > > > commit 12b0286f49947a6cdc9285032d918466a8c3f5f9 > > > > Author: Ben Widawsky <ben@bwidawsk.net> > > > > Date: Mon Jun 4 14:42:50 2012 -0700 > > > > > > > > drm/i915: possibly invalidate TLB before context switch > > > > > > > > The original commit attempted to only invalidate when necessary > > > > (very much a relic from the old days). Now, we can just always invalidate. > > > > > > > > I guess the old TODO still exists. Since we seem to have abandoned ILK > > > > contexts however, there isn't much point in even remembering. > > > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > > > > Seems reasonable, except in most cases (execbuffer) there will be > > > a following cache-invalidate as part of the move-to-gpu. > > > > Except we still move_to_gpu() before the context switch. My fbc related > > patch to change that order never got merged. > > Hm, pointer to that thing? http://lists.freedesktop.org/archives/intel-gfx/2013-November/036339.html > I guess we need to sign up someone to dig out > all your fbc patches and get them in line in prep for bdw. At least > another opportunity to beat all that into better shape ... If someone is serious about fixing FBC, I did start reworking the whole locking mess, but it's sitting somewhere in a branch on my laptop in half assed state. The main new idea I had was to compute a "fbc score" for the current crtc at modeset/setplane time (we hold the appropriate crtc lock here) and then update/consult those scores under a new fbc lock. This way almost all of the FBC state is protected by the fbc lock. Whenever we need to make a decision which pipe gets to use fbc, we grab the lock and pick the one with the highest score. And as we always reduce the score to 0 before we do anything that requires fbc to be disabled, and only set the score to anything but 0 only after fbc can be enabled, we don't need to grab any crtc locks in any fbc specific functions, which is nice. The main extra complication is that the render tracking stuff still needs to use struct_mutex as that's what execbuffer uses. And as an extra complication whenever we decide to enable FBC on a specific plane, we need to: 1) disable fbc if it's active on another plane. this needs to happen before step 2 since we can only do render tracking for one front buffer (with the gen7+ nuke mechanism I guess we might be able to track more that one actually). 2) enable render tracking for the new front buffer so any new rendering to the buffer will invalidate fbc 3) sample the current write seqno and wait for it, as there may be rendering already in flight w/o render tracking enabled (this is something our current tests fail to exercise 4) do the vblank wait to make sure we don't enable FBC too soon after enabling the plane. Alternatively we could keep the fbc score at 0 until at least one vblank has passed after the plane has been enabled, and then we won't even consider the plane for fbc until it's really ready for it 5) finally enable fbc I've been hoping to find a bit of time and motivation to finish this off. But so far that's not happened.
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 855ad1e..d4da864 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -596,7 +596,7 @@ mi_set_context(struct intel_ring_buffer *ring, * explicitly, so we rely on the value at ring init, stored in * itlb_before_ctx_switch. */ - if (IS_GEN6(ring->dev) && ring->itlb_before_ctx_switch) { + if (IS_GEN6(ring->dev)) { ret = ring->flush(ring, I915_GEM_GPU_DOMAINS, 0); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 5ceb3c5..218d9cc 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -612,13 +612,6 @@ static int init_render_ring(struct intel_ring_buffer *ring) */ I915_WRITE(CACHE_MODE_0, _MASKED_BIT_DISABLE(CM0_STC_EVICT_DISABLE_LRA_SNB)); - - /* This is not explicitly set for GEN6, so read the register. - * see intel_ring_mi_set_context() for why we care. - * TODO: consider explicitly setting the bit for GEN5 - */ - ring->itlb_before_ctx_switch = - !!(I915_READ(GFX_MODE) & GFX_TLB_INVALIDATE_EXPLICIT); } if (INTEL_INFO(dev)->gen >= 6) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 4243fc2..1d04636 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -223,10 +223,6 @@ struct intel_ring_buffer { wait_queue_head_t irq_queue; - /** - * Do an explicit TLB flush before MI_SET_CONTEXT - */ - bool itlb_before_ctx_switch; struct i915_hw_context *default_context; struct i915_hw_context *last_context;
We have been setting the bit which was originally BIOS dependent since: commit f05bb0c7b624252a5e768287e340e8e45df96e42 Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Sun Jan 20 16:33:32 2013 +0000 drm/i915: GFX_MODE Flush TLB Invalidate Mode must be '1' for scanline waits Therefore, we do not need to try to figure it out dynamically and we can just always invalidate the TLBs. It's a partial revert of: commit 12b0286f49947a6cdc9285032d918466a8c3f5f9 Author: Ben Widawsky <ben@bwidawsk.net> Date: Mon Jun 4 14:42:50 2012 -0700 drm/i915: possibly invalidate TLB before context switch The original commit attempted to only invalidate when necessary (very much a relic from the old days). Now, we can just always invalidate. I guess the old TODO still exists. Since we seem to have abandoned ILK contexts however, there isn't much point in even remembering. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_context.c | 2 +- drivers/gpu/drm/i915/intel_ringbuffer.c | 7 ------- drivers/gpu/drm/i915/intel_ringbuffer.h | 4 ---- 3 files changed, 1 insertion(+), 12 deletions(-)