Re: [PATCH 3/8] drm/i915: Wrap port cancellation into a function

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Michał Winiarski <michal.winiarski@intel.com> writes:

> On Wed, Sep 20, 2017 at 05:37:00PM +0300, Mika Kuoppala wrote:
>> On reset and wedged path, we want to release the requests
>> that are tied to ports and then mark the ports to be unset.
>> Introduce a function for this.
>> 
>> v2: rebase
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_lrc.c | 21 ++++++++++++---------
>>  1 file changed, 12 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index a4ece4c4f291..ffb9c900328b 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -568,6 +568,16 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>>  		execlists_submit_ports(engine);
>>  }
>>  
>> +static void execlist_cancel_port_requests(struct intel_engine_execlist *el)
>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(el->port); i++)
>> +		i915_gem_request_put(port_request(&el->port[i]));
>> +
>> +	memset(el->port, 0, sizeof(el->port));
>> +}
>> +
>>  static void execlists_cancel_requests(struct intel_engine_cs *engine)
>>  {
>>  	struct intel_engine_execlist * const el = &engine->execlist;
>> @@ -575,14 +585,11 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>>  	struct drm_i915_gem_request *rq, *rn;
>>  	struct rb_node *rb;
>>  	unsigned long flags;
>> -	unsigned long n;
>>  
>>  	spin_lock_irqsave(&engine->timeline->lock, flags);
>>  
>>  	/* Cancel the requests on the HW and clear the ELSP tracker. */
>> -	for (n = 0; n < ARRAY_SIZE(el->port); n++)
>> -		i915_gem_request_put(port_request(&port[n]));
>
> We could also drop the local variable for port.

Dropped.

> It's only used in GEM_BUG_ON(port_isset(&port[0])).
> Do we even need this assert when we're starting to treat ports in a more
> ring-like fashion?
>

The memset is, still, so close there in this version that it indeed
begs the question.

But it is there to ensure that we really did the port parts
properly.

-Mika

> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
>
> -Michał
>
>> -	memset(el->port, 0, sizeof(el->port));
>> +	execlist_cancel_port_requests(el);
>>  
>>  	/* Mark all executing requests as skipped. */
>>  	list_for_each_entry(rq, &engine->timeline->requests, link) {
>> @@ -1372,11 +1379,9 @@ static void reset_common_ring(struct intel_engine_cs *engine,
>>  			      struct drm_i915_gem_request *request)
>>  {
>>  	struct intel_engine_execlist * const el = &engine->execlist;
>> -	struct execlist_port *port = el->port;
>>  	struct drm_i915_gem_request *rq, *rn;
>>  	struct intel_context *ce;
>>  	unsigned long flags;
>> -	unsigned int n;
>>  
>>  	spin_lock_irqsave(&engine->timeline->lock, flags);
>>  
>> @@ -1389,9 +1394,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
>>  	 * guessing the missed context-switch events by looking at what
>>  	 * requests were completed.
>>  	 */
>> -	for (n = 0; n < ARRAY_SIZE(el->port); n++)
>> -		i915_gem_request_put(port_request(&port[n]));
>> -	memset(el->port, 0, sizeof(el->port));
>> +	execlist_cancel_port_requests(el);
>>  
>>  	/* Push back any incomplete requests for replay after the reset. */
>>  	list_for_each_entry_safe_reverse(rq, rn,
>> -- 
>> 2.11.0
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx