[Intel-gfx] [PATCH 4/5] drm/i915: Subsume intel_ctx_submit_request in to drm_i915_gem_request

[Daniel Vetter]

On Wed, Nov 12, 2014 at 12:13:03PM +0000, Nick Hoath wrote:
> 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.

I guess we'll eventually need to subsume this into the scheduler's list of
ctxs. Or reuse it there. Imo ok for now, might just need a rename
longterm.

> >>+	/** 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.

I guess this will drop out on a rebase now that everything has landed?

> >>+	/** 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.

Yeah this one is still a bit crazy but guess that's just how it is ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch