Re: [PATCH 02/20] drm/i915: Force PD restore on dirty ppGTTs

Ville Syrjälä
2015-05-21T15:09:38Z

On Thu, May 21, 2015 at 05:37:30PM +0300, Mika Kuoppala wrote:
> Force page directory reload when ppgtt va->pa
> mapping has changed. Extend dirty rings mechanism
> for gen > 7 and use it to force pd restore in execlist
> mode when vm has been changed.
>
> Some parts of execlist context update cleanup based on
> work by Chris Wilson.
>
> v2: Add comment about lite restore (Chris)
>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 65 ++++++++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0413b8f..5ee2a8c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -264,9 +264,10 @@ u32 intel_execlists_ctx_id(struct drm_i915_gem_object *ctx_obj)
> }
>
> static uint64_t execlists_ctx_descriptor(struct intel_engine_cs *ring,
> - struct drm_i915_gem_object *ctx_obj)
> + struct intel_context *ctx)
> {
> struct drm_device *dev = ring->dev;
> + struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
> uint64_t desc;
> uint64_t lrca = i915_gem_obj_ggtt_offset(ctx_obj);
>
> @@ -284,6 +285,14 @@ static uint64_t execlists_ctx_descriptor(struct intel_engine_cs *ring,
> * signalling between Command Streamers */
> /* desc |= GEN8_CTX_FORCE_RESTORE; */
>
> + /* When performing a LiteRestore but with updated PD we need
> + * to force the GPU to reload the PD
> + */
> + if (intel_ring_flag(ring) & ctx->ppgtt->pd_dirty_rings) {
> + desc |= GEN8_CTX_FORCE_PD_RESTORE;

Wasn't there a hardware issue which basically meant you are not
allowed to actually set this bit?

Rafael had some details on that as far as I recall so adding cc...

> + ctx->ppgtt->pd_dirty_rings &= ~intel_ring_flag(ring);
> + }
> +
> /* WaEnableForceRestoreInCtxtDescForVCS:skl */
> if (IS_GEN9(dev) &&
> INTEL_REVID(dev) <= SKL_REVID_B0 &&
> @@ -295,8 +304,8 @@ static uint64_t execlists_ctx_descriptor(struct intel_engine_cs *ring,
> }
>
> static void execlists_elsp_write(struct intel_engine_cs *ring,
> - struct drm_i915_gem_object *ctx_obj0,
> - struct drm_i915_gem_object *ctx_obj1)
> + struct intel_context *to0,
> + struct intel_context *to1)
> {
> struct drm_device *dev = ring->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -304,14 +313,15 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
> uint32_t desc[4];
>
> /* XXX: You must always write both descriptors in the order below. */
> - if (ctx_obj1)
> - temp = execlists_ctx_descriptor(ring, ctx_obj1);
> + if (to1)
> + temp = execlists_ctx_descriptor(ring, to1);
> else
> temp = 0;
> +
> desc[1] = (u32)(temp >> 32);
> desc[0] = (u32)temp;
>
> - temp = execlists_ctx_descriptor(ring, ctx_obj0);
> + temp = execlists_ctx_descriptor(ring, to0);
> desc[3] = (u32)(temp >> 32);
> desc[2] = (u32)temp;
>
> @@ -330,14 +340,20 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
> spin_unlock(&dev_priv->uncore.lock);
> }
>
> -static int execlists_update_context(struct drm_i915_gem_object *ctx_obj,
> - struct drm_i915_gem_object *ring_obj,
> - struct i915_hw_ppgtt *ppgtt,
> - u32 tail)
> +static void execlists_update_context(struct intel_engine_cs *ring,
> + struct intel_context *ctx,
> + u32 tail)
> {
> + struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
> + struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
> + struct drm_i915_gem_object *ring_obj = ringbuf->obj;
> + struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
> struct page *page;
> uint32_t *reg_state;
>
> + WARN_ON(!i915_gem_obj_is_pinned(ctx_obj));
> + WARN_ON(!i915_gem_obj_is_pinned(ring_obj));
> +
> page = i915_gem_object_get_page(ctx_obj, 1);
> reg_state = kmap_atomic(page);
>
> @@ -347,7 +363,7 @@ static int execlists_update_context(struct drm_i915_gem_object *ctx_obj,
> /* True PPGTT with dynamic page allocation: update PDP registers and
> * point the unallocated PDPs to the scratch page
> */
> - if (ppgtt) {
> + if (ppgtt && intel_ring_flag(ring) & ctx->ppgtt->pd_dirty_rings) {
> ASSIGN_CTX_PDP(ppgtt, reg_state, 3);
> ASSIGN_CTX_PDP(ppgtt, reg_state, 2);
> ASSIGN_CTX_PDP(ppgtt, reg_state, 1);
> @@ -355,36 +371,21 @@ static int execlists_update_context(struct drm_i915_gem_object *ctx_obj,
> }
>
> kunmap_atomic(reg_state);
> -
> - return 0;
> }
>
> static void execlists_submit_contexts(struct intel_engine_cs *ring,
> struct intel_context *to0, u32 tail0,
> struct intel_context *to1, u32 tail1)
> {
> - struct drm_i915_gem_object *ctx_obj0 = to0->engine[ring->id].state;
> - struct intel_ringbuffer *ringbuf0 = to0->engine[ring->id].ringbuf;
> - struct drm_i915_gem_object *ctx_obj1 = NULL;
> - struct intel_ringbuffer *ringbuf1 = NULL;
> -
> - BUG_ON(!ctx_obj0);
> - WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0));
> - WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj));
> -
> - execlists_update_context(ctx_obj0, ringbuf0->obj, to0->ppgtt, tail0);
> + if (WARN_ON(to0 == NULL))
> + return;
>
> - if (to1) {
> - ringbuf1 = to1->engine[ring->id].ringbuf;
> - ctx_obj1 = to1->engine[ring->id].state;
> - BUG_ON(!ctx_obj1);
> - WARN_ON(!i915_gem_obj_is_pinned(ctx_obj1));
> - WARN_ON(!i915_gem_obj_is_pinned(ringbuf1->obj));
> + execlists_update_context(ring, to0, tail0);
>
> - execlists_update_context(ctx_obj1, ringbuf1->obj, to1->ppgtt, tail1);
> - }
> + if (to1)
> + execlists_update_context(ring, to1, tail1);
>
> - execlists_elsp_write(ring, ctx_obj0, ctx_obj1);
> + execlists_elsp_write(ring, to0, to1);
> }
>
> static void execlists_context_unqueue(struct intel_engine_cs *ring)
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrjälä
Intel OTC