[Intel-gfx] [PATCH 20/33] drm/i915: Use VMA for ringbuffer tracking

[Chris Wilson]

On Thu, Aug 11, 2016 at 12:32:50PM +0300, Joonas Lahtinen wrote:
> On su, 2016-08-07 at 15:45 +0100, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index 03a4d2ae71db..761201ff6b34 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -343,7 +343,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
> >  	for_each_engine(engine, dev_priv) {
> >  		struct intel_context *ce = &ctx->engine[engine->id];
> >  		struct guc_execlist_context *lrc = &desc.lrc[engine->guc_id];
> > -		struct drm_i915_gem_object *obj;
> > +		struct i915_vma *vma;
> >  
> >  		/* TODO: We have a design issue to be solved here. Only when we
> >  		 * receive the first batch, we know which engine is used by the
> > @@ -358,17 +358,15 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
> >  		lrc->context_desc = lower_32_bits(ce->lrc_desc);
> >  
> >  		/* The state page is after PPHWSP */
> > -		gfx_addr = ce->state->node.start;
> > -		lrc->ring_lcra = gfx_addr + LRC_STATE_PN * PAGE_SIZE;
> > +		vma = ce->state;
> > +		lrc->ring_lcra = vma->node.start + LRC_STATE_PN * PAGE_SIZE;
> 
> An alias just for this line? Maybe not.

I was just trying to follow the conventions of the existing code.

> > @@ -1744,16 +1744,17 @@ logical_ring_default_irqs(struct intel_engine_cs *engine)
> >  static int
> >  lrc_setup_hws(struct intel_engine_cs *engine, struct i915_vma *vma)
> >  {
> > +#define HWS_OFFSET (LRC_PPHWSP_PN * PAGE_SIZE)
> 
> Wouldn't this go next to LRC_PPHWSP_PN?

I was going to undef it. Perhaps just make it a const local variable
instead.

> > @@ -1853,79 +1852,78 @@ static void cleanup_phys_status_page(struct intel_engine_cs *engine)
> >  
> >  static void cleanup_status_page(struct intel_engine_cs *engine)
> >  {
> > -	struct drm_i915_gem_object *obj;
> > +	struct i915_vma *vma;
> >  
> > -	obj = engine->status_page.obj;
> > -	if (obj == NULL)
> > +	vma = nullify(&engine->status_page.vma);
> > +	if (!vma)
> >  		return;
> >  
> > -	kunmap(sg_page(obj->pages->sgl));
> > -	i915_gem_object_ggtt_unpin(obj);
> > -	i915_gem_object_put(obj);
> > -	engine->status_page.obj = NULL;
> > +	i915_vma_unpin(vma);
> > +	i915_gem_object_unpin_map(vma->obj);
> > +	i915_gem_object_put(vma->obj);
> 
> This looks tad strange, because usually one first does all 'foo->bar'
> releases and then 'foo'. Just commenting here.

Next revision has both i915_vma_put() to hide the oddity, and
i915_vma_put_and_release for the common cases.

> > -	engine->status_page.gfx_addr = i915_gem_obj_ggtt_offset(obj);
> > -	engine->status_page.page_addr = kmap(sg_page(obj->pages->sgl));
> > -	memset(engine->status_page.page_addr, 0, PAGE_SIZE);
> > +	flags = PIN_GLOBAL;
> > +	if (!HAS_LLC(engine->i915))
> > +		/* On g33, we cannot place HWS above 256MiB, so
> > +		 * restrict its pinning to the low mappable arena.
> > +		 * Though this restriction is not documented for
> > +		 * gen4, gen5, or byt, they also behave similarly
> > +		 * and hang if the HWS is placed at the top of the
> > +		 * GTT. To generalise, it appears that all !llc
> > +		 * platforms have issues with us placing the HWS
> > +		 * above the mappable region (even though we never
> > +		 * actualy map it).
> > +		 */
> > +		flags |= PIN_MAPPABLE;
> 
> For readability, I'd move the comment one level up and before the if.

Just moving the comment, so I'd rather keep the stanza intact.

> > -		/* Access through the GTT requires the device to be awake. */
> > -		assert_rpm_wakelock_held(dev_priv);
> 
> This wakelock disappears in this patch.

Hmm, it was in the pin_iomap. Apparently not at this point in time.

> > +	if (HAS_LLC(dev_priv) && !obj->stolen)
> > +		ret = i915_gem_object_set_to_cpu_domain(obj, true);
> > +	else
> > +		ret = i915_gem_object_set_to_gtt_domain(obj, true);
> > +	if (ret) {
> > +		vma = ERR_PTR(ret);
> > +		goto err;
> > +	}
> 
> Might be worth mentioning that the ring objects are now moved to their
> domain at the time creation, not pinning. Any specific reason for the
> change?

Just saving a bit of complexity at pinning (and taking a risk doing so).
But since we have the is-bound? trick, we can use that instead.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre