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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
