[Intel-gfx] [PATCH v2 3/3] drm/i915: Consolidate ring freespace calculations

[Chris Wilson]

On Tue, Nov 25, 2014 at 12:41:16PM +0100, Daniel Vetter wrote:
> On Mon, Nov 24, 2014 at 02:32:25PM +0000, Dave Gordon wrote:
> > On 24/11/14 10:04, Daniel Vetter wrote:
> > > On Tue, Nov 18, 2014 at 08:07:22PM +0000, Dave Gordon wrote:
> > >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > >> index ae09258..a08ae65 100644
> > >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > >> @@ -52,16 +52,27 @@ intel_ring_initialized(struct intel_engine_cs *ring)
> > >>  
> > >>  int __intel_ring_space(int head, int tail, int size)
> > >>  {
> > >> -	int space = head - (tail + I915_RING_FREE_SPACE);
> > >> -	if (space < 0)
> > >> +	int space = head - tail;
> > >> +	if (space <= 0)
> > >>  		space += size;
> > >> -	return space;
> > >> +	return space - I915_RING_FREE_SPACE;
> > > 
> > > Changing the free space computation doesn't seem required, but resulting
> > > in Chris&me just discussing it on irc to convince ourselves it's not
> > > broken accidentally now. Can you please respin you patch with this change
> > > dropped?
> > > 
> > > In generally it's good practice to review the diff after committing a
> > > patch and hunt for any unecessary changes. Imo even when the endresult
> > > looks a bit ulgy (e.g. crazy ordering of static functions which requires
> > > tons of predeclarations) it's better if the resulting diff looks cleaner.
> > > And if things get out of hand we can always do a pure cleanup patch.
> > > -Daniel
> > 
> > This isn't an accidental change; it's to improve resilience in the case
> > that head and/or tail end up with values they shouldn't have.

Looks like a fun story. How about tackling a real issue like we should
prevent updating TAIL whilst in the same cacheline as HEAD?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre