Re: [PATCH 18/33] drm/i915: Use VMA as the primary object for context state

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, Aug 10, 2016 at 11:03:39AM +0300, Joonas Lahtinen wrote:
> On su, 2016-08-07 at 15:45 +0100, Chris Wilson wrote:
> > -	if (!i915_gem_obj_ggtt_bound(ctx_obj))
> > -		seq_puts(m, "\tNot bound in GGTT\n");
> > -	else
> > -		ggtt_offset = i915_gem_obj_ggtt_offset(ctx_obj);
> > +	if (vma->flags & I915_VMA_GLOBAL_BIND)
> > +		seq_printf(m, "\tBound in GGTT at 0x%x\n",
> 
> 0x%04x?

You mean 0x08.

> > -	if (i915_gem_object_get_pages(ctx_obj)) {
> > -		seq_puts(m, "\tFailed to get pages for context object\n");
> > +	if (i915_gem_object_get_pages(vma->obj)) {
> > +		seq_puts(m, "\tFailed to get pages for context object\n\n");
> >  		return;
> >  	}
> >  
> > -	page = i915_gem_object_get_page(ctx_obj, LRC_STATE_PN);
> > -	if (!WARN_ON(page == NULL)) {
> > -		reg_state = kmap_atomic(page);
> > +	page = i915_gem_object_get_page(vma->obj, LRC_STATE_PN);
> > +	if (page) {
> 
> Dropped the WARN_ON? No mention in the commit message.

It's a redundant warn that should have been thrown out before. It
doesn't even deserve mentioning.

> > @@ -620,9 +631,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> >  
> >  	intel_ring_emit(ring, MI_NOOP);
> >  	intel_ring_emit(ring, MI_SET_CONTEXT);
> > -	intel_ring_emit(ring,
> > -			i915_gem_obj_ggtt_offset(req->ctx->engine[RCS].state) |
> > -			flags);
> > +	intel_ring_emit(ring, req->ctx->engine[RCS].state->node.start | flags);
> 
> Do we somewhere make sure flags do not collide with address? Not
> related to this patch, though.

Bspec vs alignment request, with warns that the allocation meets the
requst.

> > @@ -778,16 +788,12 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
> >  	from = engine->last_context;
> >  
> >  	/*
> > -	 * Clear this page out of any CPU caches for coherent swap-in/out. Note
> > -	 * that thanks to write = false in this call and us not setting any gpu
> > -	 * write domains when putting a context object onto the active list
> > -	 * (when switching away from it), this won't block.
> > -	 *
> > -	 * XXX: We need a real interface to do this instead of trickery.
> 
> What is changed to make this comment obsolete or should have it been
> removed earlier?

I had written a custom routine to do it, and then removed it to keep
this patch concise. In the next patch it is obsolete.

> >  	return 0;
> >  
> > -unpin_out:
> > -	i915_gem_object_ggtt_unpin(to->engine[RCS].state);
> > +unpin_vma:
> 
> sole error path; "err"
> 
> > +	i915_vma_unpin(to->engine[RCS].state);
> >  	return ret;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index c621fa23cd28..21a4d0220c17 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -1077,9 +1077,10 @@ static void i915_gem_record_rings(struct drm_i915_private *dev_priv,
> >  					i915_error_ggtt_object_create(dev_priv,
> >  								      engine->scratch.obj);
> >  
> > -			ee->ctx =
> > -				i915_error_ggtt_object_create(dev_priv,
> > -							      request->ctx->engine[i].state);
> > +			if (request->ctx->engine[i].state) {
> > +				ee->ctx = i915_error_ggtt_object_create(dev_priv,
> > +									request->ctx->engine[i].state->obj);
> > +			}
> 
> Why conditional now?

Because the code would otherwise dereference a NULL pointer.

It gets removed again in the next patches when we pass vma to error
object capture.

> >  	i915_gem_context_get(ctx);
> >  	return 0;
> >  
> >  unpin_map:
> > -	i915_gem_object_unpin_map(ce->state);
> > -unpin_ctx_obj:
> > -	i915_gem_object_ggtt_unpin(ce->state);
> > +	i915_gem_object_unpin_map(ce->state->obj);
> > +unpin_vma:
> > +	__i915_vma_unpin(ce->state);
> 
> err_vma while at it?
> 
> > @@ -2161,7 +2162,7 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> >  	}
> >  
> >  	ce->ring = ring;
> > -	ce->state = ctx_obj;
> > +	ce->state = vma;
> 
> Maybe the member name could be just ce->vma too?

No, it still contains the logical GPU state as opposed to the ring which
also has its own vma, and so potentially confusing.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx