Re: [PATCH 1/2] drm/i915: Introduce execlist_port_* accessors

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

Joonas Lahtinen <joonas.lahtinen@linux.intel.com> writes:

> On Thu, 2017-10-19 at 17:39 +0300, Mika Kuoppala wrote:
>> From: Mika Kuoppala <mika.kuoppala@intel.com>
>> 
>> Instead of trusting that first available port is at index 0,
>> use accessor to hide this. This is a preparation for a
>> following patches where head can be at arbitrary location
>> in the port array.
>> 
>> v2: improved commit message, elsp_ready readability (Chris)
>> v3: s/execlist_port_index/execlist_port (Chris)
>> v4: rebase to new naming
>> v5: fix port_next indexing
>> 
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> <SNIP>
>
>> @@ -561,15 +563,20 @@ static void port_assign(struct execlist_port *port,
>>  static void i915_guc_dequeue(struct intel_engine_cs *engine)
>>  {
>>  	struct intel_engine_execlists * const execlists = &engine->execlists;
>> -	struct execlist_port *port = execlists->port;
>> +	struct execlist_port *port;
>>  	struct drm_i915_gem_request *last = NULL;
>> -	const struct execlist_port * const last_port =
>> -		&execlists->port[execlists->port_mask];
>>  	bool submit = false;
>>  	struct rb_node *rb;
>>  
>> -	if (port_isset(port))
>> -		port++;
>> +	port = execlists_port_head(execlists);
>> +
>> +	/*
>> +	 * We don't coalesce into last submitted port with guc.
>> +	 * Find first free port, this is safe as we dont dequeue without
>> +	 * atleast last port free.
>
> "at least" + "the"
>
> <SNIP>
>
>> @@ -557,6 +557,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>>  	if (!rb)
>>  		goto unlock;
>>  
>> +	port = execlists_port_head(execlists);
>> +	last = port_request(port);
>> +
>>  	if (last) {
>>  		/*
>>  		 * Don't resubmit or switch until all outstanding
>> @@ -564,7 +567,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>>  		 * know the next preemption status we see corresponds
>>  		 * to this ELSP update.
>>  		 */
>> -		if (port_count(&port[0]) > 1)
>> +		if (port_count(port) > 1)
>>  			goto unlock;
>>  
>>  		if (can_preempt(engine) &&
>> @@ -598,7 +601,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>>  			 * the driver is unable to keep up the supply of new
>>  			 * work).
>>  			 */
>> -			if (port_count(&port[1]))
>> +			if (port_count(execlists_port_next(execlists, port)))
>>  				goto unlock;
>>  
>>  			/* WaIdleLiteRestore:bdw,skl
>> @@ -634,7 +637,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>>  				 * combine this request with the last, then we
>>  				 * are done.
>>  				 */
>> -				if (port == last_port) {
>> +				if (port == execlists_port_tail(execlists)) {
>>  					__list_del_many(&p->requests,
>>  							&rq->priotree.link);
>
> Nothing to fix related to this patch, but I was sure next hunk was
> going to escape my screen :) Maybe we need to cut the indents a bit.
>

I have noticed the same. But I didn't feel like attacking this loop
until everything is in place and working.

>> @@ -890,7 +902,7 @@ static void intel_lrc_irq_handler(unsigned long data)
>>  			}
>>  
>>  			/* After the final element, the hw should be idle */
>> -			GEM_BUG_ON(port_count(port) == 0 &&
>> +			GEM_BUG_ON(port_count(execlists_port_head(execlists)) == 0 &&
>>  				   !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
>
> Why did you stop trusting port variable here?
>

We did assing it pre loop before. Now we do it inside the loop. Also
I thought I made a favour for reader (and for the bug triager
as GEM_BUG_ON might soon show condition in dmesg) 
to note that it is always the first port count we assert
for idleness.

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