Re: [RFC 21/21] drm/i915: Remove 'obj->ring'

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 28, 2014 at 03:09:07PM +0000, John Harrison wrote:
> On 19/10/2014 15:12, Daniel Vetter wrote:
> >On Mon, Oct 06, 2014 at 03:15:25PM +0100, John.C.Harrison@Intel.com wrote:
> >>From: John Harrison <John.C.Harrison@Intel.com>
> >>
> >>For: VIZ-4377
> >>Signed-off-by: John.C.Harrison@Intel.com
> >I think this should be split up into the different parts:
> >
> >- s/obj->ring/obj->last_read_req->ring/ for all the cases that just want
> >   the current ring.
> >- s/obj->ring/obj->last_read_req/ I think in a bunch of places the code
> >   would actually be more readable if we'd check for obj->active instead.
> >- All the oddball special cases probably deserve their own commit + nice
> >   explanation in the commit message about why the change is correct.
> 
> Can you explain which is which? As for why the change is correct, it is not
> a functional change. obj->ring was assigned with obj->last_read_seqno and
> cleared with obj->last_read_seqno. Thus querying obj->ring is equivalent to
> querying obj->last_read_req->ring in all cases. Given that the ring is now
> available from obj->lrr, it seemed redundant to also have it explicitly
> saved in obj->ring, hence the patch to remove it.
> 
> AFAICT, there are two examples in debugfs that should just be querying
> obj->active. The rest are pretty much wanting the currently in use ring
> and/or are about to use last_read_req anyway. It seems more sensible to say
> 'if(obj->lrr) { do_stuff(obj->lrr); }' than 'if(obj->active) {
> do_stuff(obj->lrr); }'. Nothing looks particularly 'oddball' to me!

[snip]

> >>+			if (obj->last_read_req) /* XXX per-vma statistic */
> >>  				stats->active += obj->base.size;

Stuff like this here is what I've meant. Checking for obj->active here is
better, and you might as well ditch the XXX comment too.

You're right that it's all semantically equivalent and your change here is
the less risky since there's no way behaviour can change. But code is
mostly written so that other humans can understand it (when debugging it),
so conveying as much meaning as possible is important. And this looks like
a good opportunity to review all the various places and make sure the code
is sane. We have accumulated a bit of cruft from the per-vma rework and
there's more cruft on the horizon with potentially multiple read fences,
so I think this is worth it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx