| Message ID | 1358699612-29606-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jan 20, 2013 at 04:33:32PM +0000, Chris Wilson wrote: > On SNB, if bit 13 of GFX_MODE, Flush TLB Invalidate Mode, is not set to 1, > the hardware can not program the scanline values. Those scanline values > then control when the signal is sent from the display engine to the render > ring for MI_WAIT_FOR_EVENTs. Note setting this bit means that TLB > invalidations must be performed explicitly through the appropriate bits > being set in PIPE_CONTROL. > > References: https://bugzilla.kernel.org/show_bug.cgi?id=52311 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: stable@vger.kernel.org This all sounds a bit hand-wavy to me. First, it's not clear from the commit message if this actually fixes anything. Is that connection between bit 13 of GFX_MODE and the scanline updates documented, or was it just "found." If it was found we should get a doc update, or clarification, because I can't find that in the docs, and perhaps more importantly, I can't even come up with a theory why the TLB would have any effect on the problem. OTOH, I've always disliked that this bit wasn't explicitly set. As a note there, we have a context workaround you could update as a result of this patch. My only real concern is over old userspace with this patch. Were there any released ddx, or mesa which didn't set the bit on the PIPE_CONTROL? Does anyone care? It'd be nice if we had some way to observe the TLBs weren't being invalidated as needed. If you can address my concerns over breaking old userspace, and don't feel like addressing my other comments, it is (and the next patch): Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
On Sun, Jan 20, 2013 at 07:36:39PM -0800, Ben Widawsky wrote: > On Sun, Jan 20, 2013 at 04:33:32PM +0000, Chris Wilson wrote: > > On SNB, if bit 13 of GFX_MODE, Flush TLB Invalidate Mode, is not set to 1, > > the hardware can not program the scanline values. Those scanline values > > then control when the signal is sent from the display engine to the render > > ring for MI_WAIT_FOR_EVENTs. Note setting this bit means that TLB > > invalidations must be performed explicitly through the appropriate bits > > being set in PIPE_CONTROL. > > > > References: https://bugzilla.kernel.org/show_bug.cgi?id=52311 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: stable@vger.kernel.org > > This all sounds a bit hand-wavy to me. First, it's not clear from the > commit message if this actually fixes anything. Is that connection > between bit 13 of GFX_MODE and the scanline updates documented, or was > it just "found." If it was found we should get a doc update, or > clarification, because I can't find that in the docs, and perhaps more > importantly, I can't even come up with a theory why the TLB would have > any effect on the problem. It was copied almost verbatim from a tiny little note in the updated SNB bspecs for the magic DISPLAY LOAD SCANLINES register. > OTOH, I've always disliked that this bit wasn't explicitly set. As a > note there, we have a context workaround you could update as a result of > this patch. > > My only real concern is over old userspace with this patch. Were there > any released ddx, or mesa which didn't set the bit on the PIPE_CONTROL? > Does anyone care? It'd be nice if we had some way to observe the TLBs > weren't being invalidated as needed. No, TLB invalidation is always a kernel problem as it is only required on moving a buffer into the GPU domain after PTE updates. Userspace is only concerned about ensuring that the contents of the render and texture caches are coherent within a batch. > If you can address my concerns over breaking old userspace, and don't > feel like addressing my other comments, it is (and the next patch): > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> Hopefully cleared that up, -Chris
On Sun, Jan 20, 2013 at 07:36:39PM -0800, Ben Widawsky wrote: > On Sun, Jan 20, 2013 at 04:33:32PM +0000, Chris Wilson wrote: > > On SNB, if bit 13 of GFX_MODE, Flush TLB Invalidate Mode, is not set to 1, > > the hardware can not program the scanline values. Those scanline values > > then control when the signal is sent from the display engine to the render > > ring for MI_WAIT_FOR_EVENTs. Note setting this bit means that TLB > > invalidations must be performed explicitly through the appropriate bits > > being set in PIPE_CONTROL. > > > > References: https://bugzilla.kernel.org/show_bug.cgi?id=52311 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: stable@vger.kernel.org > > This all sounds a bit hand-wavy to me. First, it's not clear from the > commit message if this actually fixes anything. Is that connection > between bit 13 of GFX_MODE and the scanline updates documented, or was > it just "found." If it was found we should get a doc update, or > clarification, because I can't find that in the docs, and perhaps more > importantly, I can't even come up with a theory why the TLB would have > any effect on the problem. > > OTOH, I've always disliked that this bit wasn't explicitly set. As a > note there, we have a context workaround you could update as a result of > this patch. > > My only real concern is over old userspace with this patch. Were there > any released ddx, or mesa which didn't set the bit on the PIPE_CONTROL? > Does anyone care? It'd be nice if we had some way to observe the TLBs > weren't being invalidated as needed. > > If you can address my concerns over breaking old userspace, and don't > feel like addressing my other comments, it is (and the next patch): > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> Picked up for -fixes, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index af7adb0..ae816fb 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -513,6 +513,11 @@ static int init_render_ring(struct intel_ring_buffer *ring) if (INTEL_INFO(dev)->gen >= 6) I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(ASYNC_FLIP_PERF_DISABLE)); + /* Required for the hardware to program scanline values for waiting */ + if (INTEL_INFO(dev)->gen == 6) + I915_WRITE(GFX_MODE, + _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_ALWAYS)); + if (IS_GEN7(dev)) I915_WRITE(GFX_MODE_GEN7, _MASKED_BIT_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) |
On SNB, if bit 13 of GFX_MODE, Flush TLB Invalidate Mode, is not set to 1, the hardware can not program the scanline values. Those scanline values then control when the signal is sent from the display engine to the render ring for MI_WAIT_FOR_EVENTs. Note setting this bit means that TLB invalidations must be performed explicitly through the appropriate bits being set in PIPE_CONTROL. References: https://bugzilla.kernel.org/show_bug.cgi?id=52311 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: stable@vger.kernel.org --- drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++ 1 file changed, 5 insertions(+)