Re: [PATCH v2 01/18] drm/i915/lrc: Update PDPx registers with lri commands

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Michel Thierry <michel.thierry@intel.com> writes:

> A safer way to update the PDPx registers is sending lri commands, added
> in the ring before the batchbuffer start. Otherwise, the ctx must be idle
> before trying to change anything (but the ring-tail) in the ctx image. An
> example where the ctx won't be idle is lite-restore.
>
> This patch depends on [1], and has the advantage that it doesn't require
> to pre-allocate the top pdps like here [2].
>
> [1] http://mid.gmane.org/1432314314-23530-2-git-send-email-mika.kuoppala@intel.com
> [2] http://mid.gmane.org/1432314314-23530-3-git-send-email-mika.kuoppala@intel.com
>
> v2: Combine lri writes (and save 8 commands). (Mika)
>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 43 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 626949a..51c0e06 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1116,13 +1116,56 @@ static int gen9_init_render_ring(struct intel_engine_cs *ring)
>  	return init_workarounds_ring(ring);
>  }
>  
> +static int intel_logical_ring_emit_pdps(struct intel_engine_cs *ring,
> +					struct intel_context *ctx)
> +{
> +	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
> +	struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
> +	const int num_lri_cmds = GEN8_LEGACY_PDPES * 2;
> +	int i, ret;
> +
> +	ret = intel_logical_ring_begin(ringbuf, ctx, num_lri_cmds * 2 + 2);
> +	if (ret)
> +		return ret;
> +
> +	intel_logical_ring_emit(ringbuf, MI_LOAD_REGISTER_IMM(num_lri_cmds));
> +	for (i = GEN8_LEGACY_PDPES - 1; i >= 0; i--) {
> +		const dma_addr_t pd_daddr = i915_page_dir_dma_addr(ppgtt, i);
> +
> +		intel_logical_ring_emit(ringbuf, GEN8_RING_PDP_UDW(ring, i));
> +		intel_logical_ring_emit(ringbuf, upper_32_bits(pd_daddr));
> +		intel_logical_ring_emit(ringbuf, GEN8_RING_PDP_LDW(ring, i));
> +		intel_logical_ring_emit(ringbuf, lower_32_bits(pd_daddr));
> +	}
> +
> +	intel_logical_ring_emit(ringbuf, MI_NOOP);
> +	intel_logical_ring_advance(ringbuf);
> +
> +	return 0;
> +}
> +
>  static int gen8_emit_bb_start(struct intel_ringbuffer *ringbuf,
>  			      struct intel_context *ctx,
>  			      u64 offset, unsigned dispatch_flags)
>  {
> +	struct intel_engine_cs *ring = ringbuf->ring;
>  	bool ppgtt = !(dispatch_flags & I915_DISPATCH_SECURE);
>  	int ret;
>  
> +	/* Don't rely in hw updating PDPs, specially in lite-restore.
> +	 * Ideally, we should set Force PD Restore in ctx descriptor,
> +	 * but we can't. Force Restore would be a second option, but
> +	 * it is unsafe in case of lite-restore (because the ctx is
> +	 * not idle). */
> +	if (ctx->ppgtt &&

Is this superfluous? Can the ctx->ppgtt ever be null with
execlists?
-Mika


> +	    (intel_ring_flag(ring) & ctx->ppgtt->pd_dirty_rings)) {
> +		ret = intel_logical_ring_emit_pdps(ring, ctx);
> +		if (ret)
> +			return ret;
> +
> +		ctx->ppgtt->pd_dirty_rings &= ~intel_ring_flag(ring);
> +	}
> +
>  	ret = intel_logical_ring_begin(ringbuf, ctx, 4);
>  	if (ret)
>  		return ret;
> -- 
> 2.4.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx