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

From: Michel Thierry <michel.thierry@intel.com>
To: Mika Kuoppala <mika.kuoppala@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 01/18] drm/i915/lrc: Update PDPx registers with lri commands
Date: Mon, 22 Jun 2015 10:18:43 +0100	[thread overview]
Message-ID: <5587D2F3.3000309@intel.com> (raw)
In-Reply-To: <87r3pidsv4.fsf@gaia.fi.intel.com>

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