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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
