Re: [PATCH 06/50] drm/i915: s/intel_ring_buffer/intel_engine

["Volkin, Bradley D" <bradley.d.volkin@intel.com>]

On Mon, May 19, 2014 at 09:33:37AM -0700, Mateo Lozano, Oscar wrote:
> > -----Original Message-----
> > From: Volkin, Bradley D
> > Sent: Monday, May 19, 2014 5:24 PM
> > To: Mateo Lozano, Oscar
> > Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 06/50] drm/i915:
> > s/intel_ring_buffer/intel_engine
> > 
> > On Mon, May 19, 2014 at 09:12:26AM -0700, Mateo Lozano, Oscar wrote:
> > > BTW: do you want me to kill private_default_ctx as well? It doesn´t look very
> > useful...
> > 
> > Isn't private_default_ctx the one that's actually used when userspace specifies
> > DEFAULT_CONTEXT_ID?
> 
> What I see is a normal idr_find:

Right, but i915_gem_context_open() does:
	idr_init(&file_priv->context_idr);
	file_priv->private_default_ctx =
		i915_gem_create_context(dev, file_priv, USES_FULL_PPGTT(dev));

And i915_gem_create_context() calls __create_hw_context(), which does:
	if (file_priv != NULL) {
		ret = idr_alloc(&file_priv->context_idr, ctx,
				DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
		if (ret < 0)
			goto err_out;
	} else
		ret = DEFAULT_CONTEXT_ID;

So I think the idr_find() should indirectly give us private_default_ctx.

Brad

> 
> struct i915_hw_context *
> i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
> {
> 	struct i915_hw_context *ctx;
> 
> 	ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> 	if (!ctx)
> 		return ERR_PTR(-ENOENT);
> 
> 	return ctx;
> }
> 
> I think Chris has almost killed it off completely:
> 
> commit 691e6415c891b8b2b082a120b896b443531c4d45
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Wed Apr 9 09:07:36 2014 +0100
> 
>     drm/i915: Always use kref tracking for all contexts.
>     
>     If we always initialize kref for the context, even if we are using fake
>     contexts for hangstats when there is no hw support, we can forgo the
>     dance to dereference the ctx->obj and inspect whether we are permitted
>     to use kref inside i915_gem_context_reference() and _unreference().
>     
>     My ulterior motive here is to improve the debugging of a use-after-free
>     of ctx->obj. This patch avoids the dereference here and instead forces
>     the assertion checks associated with kref.
>     
>     v2: Refactor the fake contexts to being even more like the real
>     contexts, so that there is much less duplicated and special case code.
>     
>     v3: Tweaks.
>     v4: Tweaks, minor.