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

From: Ben Widawsky <ben@bwidawsk.net>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 04/25] drm/i915: add context information to objects
Date: Wed, 13 Jun 2012 16:26:07 -0700	[thread overview]
Message-ID: <20120613162607.1b93e053@bwidawsk.net> (raw)
In-Reply-To: <20120613220122.GR4829@phenom.ffwll.local>

On Thu, 14 Jun 2012 00:01:22 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Jun 04, 2012 at 02:42:44PM -0700, Ben Widawsky wrote:
> > Daniel originally shot this patch down as being a layering violation. In
> > fact, over time I've come to agree with him on this.
> > 
> > However, it is handy for debug information, and after the below commit,
> > I need some way to be able to not lazily bind objects to the gtt for
> > contexts.
> > 
> > commit 0ebb98299357e1dbeeea470eec29241263c8f244
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Wed Feb 15 23:50:24 2012 +0100
> > 
> >     drm/i915: enable lazy global-gtt binding
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> That is imo a really neat layering violation in the following patch. I'll
> shot down both patches. Just call i915_gem_gtt_bind_object right before
> using the object - object_bind_to_gtt only allocates the gtt area.
> -Daniel
> 

As long as we have a solution to the non-lazy binding of context
objects, we can certainly get rid of this.

> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |    5 +++++
> >  drivers/gpu/drm/i915/i915_gem.c         |    1 +
> >  drivers/gpu/drm/i915/i915_gem_context.c |    6 +++++-
> >  3 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f543679..9b309aa 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -993,6 +993,11 @@ struct drm_i915_gem_object {
> >  	 * reaches 0, dev_priv->pending_flip_queue will be woken up.
> >  	 */
> >  	atomic_t pending_flip;
> > +
> > +	/**
> > +	 * >= 0 if this object is the object for a context.
> > +	 */
> > +	int context_id;
> >  };
> >  
> >  #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 28f00dc..2142e9c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3439,6 +3439,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
> >  	obj->madv = I915_MADV_WILLNEED;
> >  	/* Avoid an unnecessary call to unbind on the first bind. */
> >  	obj->map_and_fenceable = true;
> > +	obj->context_id = -1;
> >  
> >  	return obj;
> >  }
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 2aca002..bf57123 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -162,7 +162,7 @@ create_hw_context(struct drm_device *dev,
> >  
> >  	/* Default context will never have a file_priv */
> >  	if (file_priv == NULL)
> > -		return 0;
> > +		goto out;
> >  
> >  	(*ctx_out)->file_priv = file_priv;
> >  
> > @@ -183,6 +183,8 @@ again:
> >  	else if (ret)
> >  		goto err_out;
> >  
> > +out:
> > +	(*ctx_out)->obj->context_id = (*ctx_out)->id;
> >  	return 0;
> >  
> >  err_out:
> > @@ -214,6 +216,8 @@ static int create_default_context(struct drm_i915_private *dev_priv)
> >  	 * default context.
> >  	 */
> >  	ctx = dev_priv->ring[RCS].default_context;
> > +	ctx->id = DEFAULT_CONTEXT_ID;
> > +	ctx->obj->context_id = DEFAULT_CONTEXT_ID;
> >  	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
> >  	if (ret) {
> >  		do_destroy(ctx);
> > -- 
> > 1.7.10.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

-- 
Ben Widawsky, Intel Open Source Technology Center