Re: [PATCH] drm/i915: Change context lifecycle

[Nick Hoath <nicholas.hoath@intel.com>]

On 25/11/2015 01:11, Dai, Yu wrote:
>
>
> On 11/24/2015 08:23 AM, Nick Hoath wrote:
>> Use the first retired request on a new context to unpin
>> the old context. This ensures that the hw context remains
>> bound until it has been saved.
>> Now that the context is pinned until later in the request/context
>> lifecycle, it no longer needs to be pinned from context_queue to
>> retire_requests.
>> This is to solve a hang with GuC submission, and a theoretical
>> issue with execlist submission.
>>
>> v2: Moved the new pin to cover GuC submission (Alex Dai)
>>       Moved the new unpin to request_retire to fix coverage leak
>> v3: Added switch to default context if freeing a still pinned
>>       context just in case the hw was actually still using it
>> v4: Unwrapped context unpin to allow calling without a request
>>
>> Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
>> Issue: VIZ-4277
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: David Gordon <david.s.gordon@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Alex Dai <yu.dai@intel.com>
>> ---
>>    drivers/gpu/drm/i915/i915_drv.h  |  1 +
>>    drivers/gpu/drm/i915/i915_gem.c  |  9 ++++-
>>    drivers/gpu/drm/i915/intel_lrc.c | 73 ++++++++++++++++++++++++++++++----------
>>    drivers/gpu/drm/i915/intel_lrc.h |  1 +
>>    4 files changed, 65 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index d5cf30b..4d2f44c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -889,6 +889,7 @@ struct intel_context {
>>    	struct {
>>    		struct drm_i915_gem_object *state;
>>    		struct intel_ringbuffer *ringbuf;
>> +		bool unsaved;
>>    		int pin_count;
>>    	} engine[I915_NUM_RINGS];
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index e955499..6fee473 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1354,6 +1354,14 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
>>    {
>>    	trace_i915_gem_request_retire(request);
>>
>> +	if (i915.enable_execlists) {
>> +		unsigned long flags;
>> +
>> +		spin_lock_irqsave(&request->ring->execlist_lock, flags);
>> +		intel_lr_context_complete_check(request);
>> +		spin_unlock_irqrestore(&request->ring->execlist_lock, flags);
>> +	}
>> +
>>    	/* We know the GPU must have read the request to have
>>    	 * sent us the seqno + interrupt, so use the position
>>    	 * of tail of the request to update the last known position
>> @@ -1384,7 +1392,6 @@ __i915_gem_request_retire__upto(struct drm_i915_gem_request *req)
>>    	do {
>>    		tmp = list_first_entry(&engine->request_list,
>>    				       typeof(*tmp), list);
>> -
>>    		i915_gem_request_retire(tmp);
>>    	} while (tmp != req);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 06180dc..a527c21 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -566,9 +566,6 @@ static int execlists_context_queue(struct drm_i915_gem_request *request)
>>    	struct drm_i915_gem_request *cursor;
>>    	int num_elements = 0;
>>
>> -	if (request->ctx != ring->default_context)
>> -		intel_lr_context_pin(request);
>> -
>>    	i915_gem_request_reference(request);
>>
>>    	spin_lock_irq(&ring->execlist_lock);
>> @@ -728,10 +725,16 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
>>    	intel_logical_ring_advance(request->ringbuf);
>>
>>    	request->tail = request->ringbuf->tail;
>> -
>>    	if (intel_ring_stopped(ring))
>>    		return;
>>
>> +	if (request->ctx != ring->default_context) {
>> +		if (!request->ctx->engine[ring->id].unsaved) {
>> +			intel_lr_context_pin(request);
>> +			request->ctx->engine[ring->id].unsaved = true;
>> +		}
>> +	}
>> +
>>    	if (dev_priv->guc.execbuf_client)
>>    		i915_guc_submit(dev_priv->guc.execbuf_client, request);
>>    	else
>> @@ -958,12 +961,6 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
>>    	spin_unlock_irq(&ring->execlist_lock);
>>
>>    	list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
>> -		struct intel_context *ctx = req->ctx;
>> -		struct drm_i915_gem_object *ctx_obj =
>> -				ctx->engine[ring->id].state;
>> -
>> -		if (ctx_obj && (ctx != ring->default_context))
>> -			intel_lr_context_unpin(req);
>>    		list_del(&req->execlist_link);
>>    		i915_gem_request_unreference(req);
>>    	}
>> @@ -1058,21 +1055,41 @@ reset_pin_count:
>>    	return ret;
>>    }
>>
>> -void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
>> +static void intel_lr_context_unpin_no_req(struct intel_engine_cs *ring,
>> +		struct intel_context *ctx)
>>    {
>> -	struct intel_engine_cs *ring = rq->ring;
>> -	struct drm_i915_gem_object *ctx_obj = rq->ctx->engine[ring->id].state;
>> -	struct intel_ringbuffer *ringbuf = rq->ringbuf;
>> -
>> +	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
>> +	struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
>>    	if (ctx_obj) {
>>    		WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
>> -		if (--rq->ctx->engine[ring->id].pin_count == 0) {
>> +		if (--ctx->engine[ring->id].pin_count == 0) {
>>    			intel_unpin_ringbuffer_obj(ringbuf);
>>    			i915_gem_object_ggtt_unpin(ctx_obj);
>>    		}
>>    	}
>>    }
>>
>> +void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
>> +{
>> +	intel_lr_context_unpin_no_req(rq->ring, rq->ctx);
>> +}
>> +
>> +void intel_lr_context_complete_check(struct drm_i915_gem_request *req)
>> +{
>> +	struct intel_engine_cs *ring = req->ring;
>> +
>> +	assert_spin_locked(&ring->execlist_lock);
>> +
>> +	if (ring->last_context && ring->last_context != req->ctx &&
>> +			ring->last_context->engine[ring->id].unsaved) {
>
> Context pin is done for every submission but unpin is done with
> condition. I think the engine.pin_count will be out of sync.
The context pin is only done when the unsaved flag is NOT set. The 
pin_count does not go out of sync.
>
>> +		intel_lr_context_unpin_no_req(
>> +				ring,
>> +				ring->last_context);
>> +		ring->last_context->engine[ring->id].unsaved = false;
>> +	}
>> +	ring->last_context = req->ctx;
>> +}
>> +
>>    static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
>>    {
>>    	int ret, i;
>> @@ -2378,7 +2395,7 @@ void intel_lr_context_free(struct intel_context *ctx)
>>    {
>>    	int i;
>>
>> -	for (i = 0; i < I915_NUM_RINGS; i++) {
>> +	for (i = 0; i < I915_NUM_RINGS; ++i) {
>>    		struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
>>
>>    		if (ctx_obj) {
>> @@ -2390,7 +2407,20 @@ void intel_lr_context_free(struct intel_context *ctx)
>>    				intel_unpin_ringbuffer_obj(ringbuf);
>>    				i915_gem_object_ggtt_unpin(ctx_obj);
>>    			}
>> -			WARN_ON(ctx->engine[ring->id].pin_count);
>> +
>> +			if (ctx->engine[ring->id].unsaved) {
>> +				int ret;
>> +				struct drm_i915_gem_request *req;
>> +
>> +				ret = i915_gem_request_alloc(ring,
>> +					ring->default_context, &req);
>> +				if (!ret) {
>> +					i915_add_request(req);
>> +					i915_wait_request(req);
>> +				}
>
> The will wait for all requests to be finished. Waiting for the head is
> good enough. If no request in queue, then add one from default context.
>
I'll make this change & upload a new patch.
> Alex
>> +			}
>> +
>> +			WARN_ON(ctx->engine[i].pin_count);
>>    			intel_ringbuffer_free(ringbuf);
>>    			drm_gem_object_unreference(&ctx_obj->base);
>>    		}
>> @@ -2554,5 +2584,12 @@ void intel_lr_context_reset(struct drm_device *dev,
>>
>>    		ringbuf->head = 0;
>>    		ringbuf->tail = 0;
>> +
>> +		if (ctx->engine[ring->id].unsaved) {
>> +			intel_lr_context_unpin_no_req(
>> +					ring,
>> +					ctx);
>> +			ctx->engine[ring->id].unsaved = false;
>> +		}
>>    	}
>>    }
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
>> index 4e60d54..cbc42b8 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.h
>> +++ b/drivers/gpu/drm/i915/intel_lrc.h
>> @@ -86,6 +86,7 @@ void intel_lr_context_reset(struct drm_device *dev,
>>    			struct intel_context *ctx);
>>    uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
>>    				     struct intel_engine_cs *ring);
>> +void intel_lr_context_complete_check(struct drm_i915_gem_request *req);
>>
>>    /* Execlists */
>>    int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists);
>

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