* [PATCH 0/2] drm/i915: Ring buffer stuff @ 2012-11-26 12:48 ville.syrjala 2012-11-26 12:48 ` [PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head ville.syrjala 2012-11-26 12:48 ` [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned ville.syrjala 0 siblings, 2 replies; 12+ messages in thread From: ville.syrjala @ 2012-11-26 12:48 UTC (permalink / raw) To: intel-gfx After getting slightly confused by the values of the ring registers in the error state dumps, I started to actually read the specs and compare what I learned with the code. I spotted one possible issue with the head and tail handling. But I don't know if my patch might fix some obscure bug, or not. gem_ringfill seems to work equally well with and without the patch. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head 2012-11-26 12:48 [PATCH 0/2] drm/i915: Ring buffer stuff ville.syrjala @ 2012-11-26 12:48 ` ville.syrjala 2012-11-26 16:28 ` Chris Wilson 2012-11-26 12:48 ` [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned ville.syrjala 1 sibling, 1 reply; 12+ messages in thread From: ville.syrjala @ 2012-11-26 12:48 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> According to BSpec the ring head and tail pointers must not be on the same cacheline when head > tail. The easiest way to enforce this is to reduce the reported ring space. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++-- drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 2cba7b4..d184727 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -168,7 +168,7 @@ void i915_kernel_lost_context(struct drm_device * dev) ring->head = I915_READ_HEAD(ring) & HEAD_ADDR; ring->tail = I915_READ_TAIL(ring) & TAIL_ADDR; - ring->space = ring->head - (ring->tail + 8); + ring->space = ring->head - (ring->tail + 64); if (ring->space < 0) ring->space += ring->size; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index c828169..70a184e 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -46,7 +46,7 @@ struct pipe_control { static inline int ring_space(struct intel_ring_buffer *ring) { - int space = (ring->head & HEAD_ADDR) - (ring->tail + 8); + int space = (ring->head & HEAD_ADDR) - (ring->tail + 64); if (space < 0) space += ring->size; return space; @@ -1163,7 +1163,7 @@ static int intel_ring_wait_request(struct intel_ring_buffer *ring, int n) if (request->tail == -1) continue; - space = request->tail - (ring->tail + 8); + space = request->tail - (ring->tail + 64); if (space < 0) space += ring->size; if (space >= n) { diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 2ea7a31..b15f896 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -187,7 +187,7 @@ void intel_cleanup_ring_buffer(struct intel_ring_buffer *ring); int __must_check intel_wait_ring_buffer(struct intel_ring_buffer *ring, int n); static inline int intel_wait_ring_idle(struct intel_ring_buffer *ring) { - return intel_wait_ring_buffer(ring, ring->size - 8); + return intel_wait_ring_buffer(ring, ring->size - 64); } int __must_check intel_ring_begin(struct intel_ring_buffer *ring, int n); -- 1.7.8.6 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head 2012-11-26 12:48 ` [PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head ville.syrjala @ 2012-11-26 16:28 ` Chris Wilson 2012-11-26 18:02 ` Ville Syrjälä 0 siblings, 1 reply; 12+ messages in thread From: Chris Wilson @ 2012-11-26 16:28 UTC (permalink / raw) To: ville.syrjala, intel-gfx [-- Attachment #1: Type: text/plain, Size: 542 bytes --] On Mon, 26 Nov 2012 14:48:18 +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > According to BSpec the ring head and tail pointers must not be > on the same cacheline when head > tail. The easiest way to enforce > this is to reduce the reported ring space. I'm going to admit blindness because I don't see that warning in the gen2-gen7 bspecs. Can you please give chapter and verse, and check to see if there is a rationale? -Chris -- Chris Wilson, Intel Open Source Technology Centre [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head 2012-11-26 16:28 ` Chris Wilson @ 2012-11-26 18:02 ` Ville Syrjälä 2012-11-26 20:24 ` Chris Wilson 0 siblings, 1 reply; 12+ messages in thread From: Ville Syrjälä @ 2012-11-26 18:02 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx On Mon, Nov 26, 2012 at 04:28:33PM +0000, Chris Wilson wrote: > On Mon, 26 Nov 2012 14:48:18 +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > According to BSpec the ring head and tail pointers must not be > > on the same cacheline when head > tail. The easiest way to enforce > > this is to reduce the reported ring space. > > I'm going to admit blindness because I don't see that warning in the > gen2-gen7 bspecs. Can you please give chapter and verse, and check to > see if there is a rationale? It's always the last thing in the section titled 'Ring Buffer Use'. I believe it's present in all the pre-snb internal bspecs, and it's also in all the public docs. I can't find it in the internal snb+ bspec but then again those don't seem to include the relevant chapter at all. Of course I can't be sure if it's a valid issue, or just something that got faithfully copypasted from one document to the next. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head 2012-11-26 18:02 ` Ville Syrjälä @ 2012-11-26 20:24 ` Chris Wilson 0 siblings, 0 replies; 12+ messages in thread From: Chris Wilson @ 2012-11-26 20:24 UTC (permalink / raw) Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 1107 bytes --] On Mon, 26 Nov 2012 20:02:00 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Mon, Nov 26, 2012 at 04:28:33PM +0000, Chris Wilson wrote: > > On Mon, 26 Nov 2012 14:48:18 +0200, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > According to BSpec the ring head and tail pointers must not be > > > on the same cacheline when head > tail. The easiest way to enforce > > > this is to reduce the reported ring space. > > > > I'm going to admit blindness because I don't see that warning in the > > gen2-gen7 bspecs. Can you please give chapter and verse, and check to > > see if there is a rationale? > > It's always the last thing in the section titled 'Ring Buffer Use'. I > believe it's present in all the pre-snb internal bspecs, and it's also > in all the public docs. I can't find it in the internal snb+ bspec but > then again those don't seem to include the relevant chapter at all. Gotcha. Yes, nice catch. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris -- Chris Wilson, Intel Open Source Technology Centre [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned 2012-11-26 12:48 [PATCH 0/2] drm/i915: Ring buffer stuff ville.syrjala 2012-11-26 12:48 ` [PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head ville.syrjala @ 2012-11-26 12:48 ` ville.syrjala 2012-11-26 15:55 ` Daniel Vetter 2012-11-26 16:25 ` Chris Wilson 1 sibling, 2 replies; 12+ messages in thread From: ville.syrjala @ 2012-11-26 12:48 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> Ringbuffer tail pointer must be qword aligned. Warn if someone makes a mistake and forgets to pad the ring when the commands inserted into the ring don't align to qword naturally. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 70a184e..79c8b13 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1275,6 +1275,8 @@ void intel_ring_advance(struct intel_ring_buffer *ring) ring->tail &= ring->size - 1; if (dev_priv->stop_rings & intel_ring_flag(ring)) return; + /* tail must be qword aligned */ + WARN_ON(ring->tail & 7); ring->write_tail(ring, ring->tail); } -- 1.7.8.6 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned 2012-11-26 12:48 ` [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned ville.syrjala @ 2012-11-26 15:55 ` Daniel Vetter 2012-11-26 16:25 ` Chris Wilson 1 sibling, 0 replies; 12+ messages in thread From: Daniel Vetter @ 2012-11-26 15:55 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx On Mon, Nov 26, 2012 at 1:48 PM, <ville.syrjala@linux.intel.com> wrote: > + /* tail must be qword aligned */ > + WARN_ON(ring->tail & 7); Minor bikeshed: Can you move the comment into the WARN argument so that if someone forgets to correctly pad stuff, dmesg will directly tell him wants wrong, instead of being forced to check the line numbers? Helps ridiculously lazy people like me ... ;-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned 2012-11-26 12:48 ` [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned ville.syrjala 2012-11-26 15:55 ` Daniel Vetter @ 2012-11-26 16:25 ` Chris Wilson 2012-11-26 17:40 ` Ville Syrjälä 1 sibling, 1 reply; 12+ messages in thread From: Chris Wilson @ 2012-11-26 16:25 UTC (permalink / raw) To: ville.syrjala, intel-gfx [-- Attachment #1: Type: text/plain, Size: 638 bytes --] On Mon, 26 Nov 2012 14:48:19 +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Ringbuffer tail pointer must be qword aligned. Warn if someone > makes a mistake and forgets to pad the ring when the commands > inserted into the ring don't align to qword naturally. The assertion should be that we wrote precisely the number of dwords we declared in intel_ring_begin(). Which is one of the important factors to check whenever reviewing such code. This assertion (which should be a BUG_ON) is no substitute for such review. -Chris -- Chris Wilson, Intel Open Source Technology Centre [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned 2012-11-26 16:25 ` Chris Wilson @ 2012-11-26 17:40 ` Ville Syrjälä 2012-11-26 17:46 ` Chris Wilson 0 siblings, 1 reply; 12+ messages in thread From: Ville Syrjälä @ 2012-11-26 17:40 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx On Mon, Nov 26, 2012 at 04:25:47PM +0000, Chris Wilson wrote: > On Mon, 26 Nov 2012 14:48:19 +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Ringbuffer tail pointer must be qword aligned. Warn if someone > > makes a mistake and forgets to pad the ring when the commands > > inserted into the ring don't align to qword naturally. > > The assertion should be that we wrote precisely the number of dwords we > declared in intel_ring_begin(). Which is one of the important factors to > check whenever reviewing such code. This assertion (which should be a > BUG_ON) is no substitute for such review. Yeah. I was considering adding some reserved_space field to the ring, populate it in ring_begin(), and and make sure it was correctly consumed at ring_advance(). If you think that sounds good, I can cook up a patch for it. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned 2012-11-26 17:40 ` Ville Syrjälä @ 2012-11-26 17:46 ` Chris Wilson 2012-11-26 18:09 ` Ville Syrjälä 0 siblings, 1 reply; 12+ messages in thread From: Chris Wilson @ 2012-11-26 17:46 UTC (permalink / raw) Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 1209 bytes --] On Mon, 26 Nov 2012 19:40:16 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Mon, Nov 26, 2012 at 04:25:47PM +0000, Chris Wilson wrote: > > On Mon, 26 Nov 2012 14:48:19 +0200, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Ringbuffer tail pointer must be qword aligned. Warn if someone > > > makes a mistake and forgets to pad the ring when the commands > > > inserted into the ring don't align to qword naturally. > > > > The assertion should be that we wrote precisely the number of dwords we > > declared in intel_ring_begin(). Which is one of the important factors to > > check whenever reviewing such code. This assertion (which should be a > > BUG_ON) is no substitute for such review. > > Yeah. I was considering adding some reserved_space field to the ring, > populate it in ring_begin(), and and make sure it was correctly > consumed at ring_advance(). If you think that sounds good, I can cook > up a patch for it. To be honest, I was thinking of a firing squad for the author and reviewers of any such patch that gets intel_ring_begin()..end() wrong. -Chris -- Chris Wilson, Intel Open Source Technology Centre [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned 2012-11-26 17:46 ` Chris Wilson @ 2012-11-26 18:09 ` Ville Syrjälä 2012-11-27 6:04 ` Ben Widawsky 0 siblings, 1 reply; 12+ messages in thread From: Ville Syrjälä @ 2012-11-26 18:09 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx On Mon, Nov 26, 2012 at 05:46:48PM +0000, Chris Wilson wrote: > On Mon, 26 Nov 2012 19:40:16 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Mon, Nov 26, 2012 at 04:25:47PM +0000, Chris Wilson wrote: > > > On Mon, 26 Nov 2012 14:48:19 +0200, ville.syrjala@linux.intel.com wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Ringbuffer tail pointer must be qword aligned. Warn if someone > > > > makes a mistake and forgets to pad the ring when the commands > > > > inserted into the ring don't align to qword naturally. > > > > > > The assertion should be that we wrote precisely the number of dwords we > > > declared in intel_ring_begin(). Which is one of the important factors to > > > check whenever reviewing such code. This assertion (which should be a > > > BUG_ON) is no substitute for such review. > > > > Yeah. I was considering adding some reserved_space field to the ring, > > populate it in ring_begin(), and and make sure it was correctly > > consumed at ring_advance(). If you think that sounds good, I can cook > > up a patch for it. > > To be honest, I was thinking of a firing squad for the author and > reviewers of any such patch that gets intel_ring_begin()..end() wrong. I was mainly thinking it might helpful during development, to catch obvious bugs. A compile time check would be even better though. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned 2012-11-26 18:09 ` Ville Syrjälä @ 2012-11-27 6:04 ` Ben Widawsky 0 siblings, 0 replies; 12+ messages in thread From: Ben Widawsky @ 2012-11-27 6:04 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Mon, 26 Nov 2012 20:09:00 +0200 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Mon, Nov 26, 2012 at 05:46:48PM +0000, Chris Wilson wrote: > > On Mon, 26 Nov 2012 19:40:16 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Mon, Nov 26, 2012 at 04:25:47PM +0000, Chris Wilson wrote: > > > > On Mon, 26 Nov 2012 14:48:19 +0200, ville.syrjala@linux.intel.com wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > Ringbuffer tail pointer must be qword aligned. Warn if someone > > > > > makes a mistake and forgets to pad the ring when the commands > > > > > inserted into the ring don't align to qword naturally. > > > > > > > > The assertion should be that we wrote precisely the number of dwords we > > > > declared in intel_ring_begin(). Which is one of the important factors to > > > > check whenever reviewing such code. This assertion (which should be a > > > > BUG_ON) is no substitute for such review. > > > > > > Yeah. I was considering adding some reserved_space field to the ring, > > > populate it in ring_begin(), and and make sure it was correctly > > > consumed at ring_advance(). If you think that sounds good, I can cook > > > up a patch for it. > > > > To be honest, I was thinking of a firing squad for the author and > > reviewers of any such patch that gets intel_ring_begin()..end() wrong. > > I was mainly thinking it might helpful during development, to catch > obvious bugs. A compile time check would be even better though. > It is nice for development (the version that Chris was hinting at), if the perf cost is provably low, or you can wrap it neatly in some #if blocks, I'd be in favor. Since you're fairly new to this world though, I'll mention that my opinion is almost always the minority, and the kiss of death in some cases... sorry. -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-11-27 6:03 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-26 12:48 [PATCH 0/2] drm/i915: Ring buffer stuff ville.syrjala 2012-11-26 12:48 ` [PATCH 1/2] drm/i915: Don't allow ring tail to reach the same cacheline as head ville.syrjala 2012-11-26 16:28 ` Chris Wilson 2012-11-26 18:02 ` Ville Syrjälä 2012-11-26 20:24 ` Chris Wilson 2012-11-26 12:48 ` [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned ville.syrjala 2012-11-26 15:55 ` Daniel Vetter 2012-11-26 16:25 ` Chris Wilson 2012-11-26 17:40 ` Ville Syrjälä 2012-11-26 17:46 ` Chris Wilson 2012-11-26 18:09 ` Ville Syrjälä 2012-11-27 6:04 ` Ben Widawsky
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.