| Message ID | 1353934099-18685-3-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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.
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.
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); }