[Intel-gfx] [PATCH 2/2] drm/i915: Warn if ring tail is not qword aligned

[Ben Widawsky]

On Mon, 26 Nov 2012 20:09:00 +0200
Ville Syrjälä <ville.syrjala at 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 at 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 at linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala at 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