[Intel-gfx] [PATCH] drm/i915: Always use kref tracking for all contexts.

[Chris Wilson]

On Thu, Apr 03, 2014 at 09:44:43PM -0700, Ben Widawsky wrote:
> On Thu, Apr 03, 2014 at 08:06:30AM +0100, Chris Wilson wrote:
> > @@ -377,7 +374,7 @@ void i915_gem_context_reset(struct drm_device *dev)
> >  		if (ring->last_context == dctx)
> >  			continue;
> >  
> > -		if (i == RCS) {
> > +		if (dctx->obj && i == RCS) {
> >  			WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
> >  						      get_context_alignment(dev), 0));
> >  			/* Fake a finish/inactive */
> 
> I am beginning to feel ambivalent now as to whether or not the
> simplicity gained by making fake contexts out weighs. For example,
> before, last_context was always equal to dctx without HW contexts, which
> made places like the above nice and simple.

Apart from that we use per-file defaults for hangstats, so this should
not be true today. If you look at the code after the patch, it does make
more sense I think - contexts always exists for bookkeeping, sometimes
they have hardware blobs as well.

> > @@ -559,14 +529,13 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> >  {
> >  	struct drm_i915_file_private *file_priv = file->driver_priv;
> >  
> > -	if (!HAS_HW_CONTEXTS(dev)) {
> > -		kfree(file_priv->private_default_ctx);
> > +	if (IS_ERR_OR_NULL(file_priv->private_default_ctx))
> >  		return;
> 
> I am not convinced. This should be
> BUG_ON(!file_priv->private_default_ctx). They don't get back an fd is
> context_open failed.

When I started, it wasn't a false condition, iirc. But now it is.

> >  int i915_switch_context(struct intel_ring_buffer *ring,
> > -			struct drm_file *file,
> >  			struct i915_hw_context *to)
> >  {
> >  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >  
> >  	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
> >  
> > -	BUG_ON(file && to == NULL);
> > -
> > -	/* We have the fake context */
> > -	if (!HAS_HW_CONTEXTS(ring->dev)) {
> > -		ring->last_context = to;
> > +	if (to->obj == NULL) { /* We have the fake context */
> > +		if (to != ring->last_context) {
> > +			if (to)
> > +				i915_gem_context_reference(to);
> > +			if (ring->last_context)
> > +				i915_gem_context_unreference(ring->last_context);
> > +			ring->last_context = to;
> > +		}
> >  		return 0;
> 
> Here's some more ambivalence. Adding complexity to i915_switch_context
> IMHO outweighs the gains elsewhere.

Apart from we have bugs in the current code where context (even fake
context) lifetimes are not tracked correctly, so using kref around each
assignment seems sensible.
 
> Other than Mika's comment about the unnecessary "if (to)" and mine about
> what I think is a false condition for context_close() it looks okay to
> me. I am a bit disappointed with the two spots I pointed out ambivalence,
> but I do think it's a net-win.

I can apply some more synatic sugar and see how you feel then.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre