Re: [PATCH 2/3] drm/i915/guc: Keep the previous context pinned until the next one has been completed

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 15/04/16 13:12, Chris Wilson wrote:
> On Fri, Apr 15, 2016 at 12:54:34PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> In GuC mode submitting requests is only putting them into the
>> GuC queue with the actual submission to hardware following at
>> some future point. This makes the per engine last context
>> tracking insufficient for closing the premature context
>> unpin race.
>>
>> Instead we need to make requests track and pin the previous
>> context on the same engine, and only unpin them when they
>> themselves are retired. This will ensure contexts remain pinned
>> in all cases until the are fully saved by the GPU.
>>
>> v2: Only track contexts.
>> v3: Limit to GuC for now.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Alex Dai <yu.dai@intel.com>
>> Cc: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Nick Hoath <nicholas.hoath@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  |  4 +++-
>>   drivers/gpu/drm/i915/i915_gem.c  |  5 +++++
>>   drivers/gpu/drm/i915/intel_lrc.c | 15 +++++++++++++++
>>   3 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index b9ed1b305beb..80f2d959ed7b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2290,6 +2290,9 @@ struct drm_i915_gem_request {
>>   	struct intel_context *ctx;
>>   	struct intel_ringbuffer *ringbuf;
>>
>> +	/** Context preceding this one on the same engine. Can be NULL. */
>> +	struct intel_context *prev_ctx;
>> +
>>   	/** Batch buffer related to this request if any (used for
>>   	    error state dump only) */
>>   	struct drm_i915_gem_object *batch_obj;
>> @@ -2325,7 +2328,6 @@ struct drm_i915_gem_request {
>>
>>   	/** Execlists no. of times this request has been sent to the ELSP */
>>   	int elsp_submitted;
>> -
>>   };
>>
>>   struct drm_i915_gem_request * __must_check
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index e9db56c7a31f..18e747132ce5 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1413,6 +1413,11 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
>>   	list_del_init(&request->list);
>>   	i915_gem_request_remove_from_client(request);
>>
>> +	if (request->prev_ctx) {
>> +		intel_lr_context_unpin(request->prev_ctx, request->engine);
>> +		request->prev_ctx = NULL;
>> +	}
>> +
>>   	i915_gem_request_unreference(request);
>>   }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 2f49dfae5560..73be9eac8ecc 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -791,6 +791,21 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
>>   	if (intel_engine_stopped(engine))
>>   		return 0;
>>
>> +	/*
>> +	 * Pin the previous context on this engine to ensure it is not
>> +	 * prematurely unpinned in GuC mode.
>> +	 * Previous context will be unpinned when this request is retired,
>> +	 * ensuring the GPU has switched out from the previous context and
>> +	 * into a new context at that point.
>> +	 */
>> +	if (dev_priv->guc.execbuf_client && engine->last_context) {
>> +		intel_lr_context_pin(engine->last_context, engine);
>> +		request->prev_ctx = engine->last_context;
>> +	}
>
> Nak.
>
> We just want,
>
> request->pinned_ctx = engine->last_context;
> engine->last_context = request->ctx;
>
> as that works for everyone (and replaces the current last_context dance)

Not sure if you are against the implementation or the idea.

It is deliberately limited to GuC for now, until the lazy ringbuffer 
unpin happens for all platforms.

Then - engine needs to hold the pin on the last context in case there 
are no subsequent requests to keep it pinned.

In your snippet where do you pin it before assigning to any object?

I could optimize my implementation by not doing the pin-unpin in the 
case when the context changes between current and last, but other than 
that it looks correct to me.

And I don't think pinned_ctx is a good name. Previous context is 
descriptive and self documenting so wins easily IMHO.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx