Re: [PATCH 04/18] drm/i915: add context information to objects

[Ben Widawsky <ben@bwidawsk.net>]

On Thu, 29 Mar 2012 10:47:56 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Mar 28, 2012 at 05:20:11PM -0700, Ben Widawsky wrote:
> > On Thu, 29 Mar 2012 00:36:21 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > On Sun, Mar 18, 2012 at 01:39:44PM -0700, Ben Widawsky wrote:
> > > > Handy mostly for assertions.
> > > > 
> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > 
> > > I don't see the point of carrying around a obj->context_id -
> > > context_id's are file_priv, so they're not that useful in the
> > > tracepoint. I suggest you just drop the contex_id arg there - the
> > > obj pointers are global and imo good enough for identification.
> > > And the few BUG_ON's don't seem really useful either.
> > > -Daniel
> > 
> > obj->context_id was requested by Chris to clarify the trace events.
> > I vaguely recall telling him that you won't like it.
> 
> That's easily shot down on the grounds that:
> - we currently don't dump the id/handles of gem objects into our
> traces
> - your tracepoints dump the context_id at create/destroy time, so
> with a bit of python this can be re-added in userspace.

Yes, but gem handles are not specific to i915, whereas context id
numbers are.

Also the tracepoint that is a problem is switch, not create/destroy

> 
> > I personally dislike using object pointer though I do agree it
> > serves the same purpose.
> > 
> > The other nice thing about having the context id is it makes it
> > easy in debug situations to find out if a certain object is a
> > context object. But no use case for that yet.
> 
> My gripes with obj->context_id is that it smells like a layering
> violation. We don't have such special stuff for other special things
> like rings. The only exception is framebuffer when pageflipping, but
> I expect that we'll need to clean this up sooner or later (because we
> need to be able to move framebuffers sooner or later anyway, so they
> need better integration with the gem eviction code).
> -Daniel

I still think the pros (easy debug) outweigh the cons (not really on
exactly what you don't like). I don't see any harm that can come from
this. /me braces for harm

Anyway, as you said in the other email, I'm expecting more serious
feedback. If I end up redoing a bunch of stuff, this can go too if you
really don't like it.