Re: [PATCH 4/5] drm/i915: Subsume intel_ctx_submit_request in to drm_i915_gem_request

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

On 12/11/2014 11:24, Chris Wilson wrote:
> On Wed, Nov 12, 2014 at 10:53:26AM +0000, Nick Hoath wrote:
>    		seq_putc(m, '\n');
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index afa9c35..0fe238c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2027,6 +2027,28 @@ struct drm_i915_gem_request {
>>   	struct list_head free_list;
>>
>>   	uint32_t uniq;
>> +
>> +	/**
>> +	 * The ELSP only accepts two elements at a time, so we queue context/tail
>> +	 * pairs on a given queue (ring->execlist_queue) until the hardware is
>> +	 * available. The queue serves a double purpose: we also use it to keep track
>> +	 * of the up to 2 contexts currently in the hardware (usually one in execution
>> +	 * and the other queued up by the GPU): We only remove elements from the head
>> +	 * of the queue when the hardware informs us that an element has been
>> +	 * completed.
>> +	 *
>> +	 * All accesses to the queue are mediated by a spinlock (ring->execlist_lock).
>> +	 */
>> +
>> +	/** Execlist link in the submission queue.*/
>> +	struct list_head execlist_link;
>
> This is redundant. The request should only be one of the pending or active
> lists at any time.
>
This is used by the pending execlist requests list owned by the 
intel_engine_cs. The request isn't in both the active and pending 
execlist engine lists.
>> +	/** Execlists workqueue for processing this request in a bottom half */
>> +	struct work_struct work;
>
> For what purpose? This is not needed.
This worker is currently used to free up execlist requests. This goes 
away when Thomas Daniel's patchset is merged.
I have spotted a bug in the cleanup handler with the merged 
requests/execlists cleanup though.
>
>> +	/** Execlists no. of times this request has been sent to the ELSP */
>> +	int elsp_submitted;
>
> A request can only be submitted exactly once at any time. This
> bookkeeping is not part of the request.
This is a refcount to preserve the request if it has been resubmitted 
due to preemption or TDR, due to a race condition between the HW 
finishing with the item and the cleanup/resubmission. Have a look at
e1fee72c2ea2e9c0c6e6743d32a6832f21337d6c which contains a much better
description of why this exists.
>
> Still not detangled I am afraid.
> -Chris
>

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