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

Michel Thierry
2015-06-22T09:18:46Z

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