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

Submitted by Michel Thierry on June 10, 2015, 4:46 p.m.

Details

Message ID 1433954816-13787-2-git-send-email-michel.thierry@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Michel Thierry June 10, 2015, 4:46 p.m.
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(+)

Patch hide | download patch | download mbox

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 &&
+	    (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;

Comments

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
On 6/11/2015 7:04 PM, Mika Kuoppala wrote:
> 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?

It's for execlists with aliasing ppgtt. In that case ctx->ppgtt is null 
(and we shouldn't need to update the pdps).

-Michel

> -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