Re: [PATCH 49/50] drm/i915/bdw: Help out the ctx switch interrupt handler

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, May 09, 2014 at 01:09:19PM +0100, oscar.mateo@intel.com wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> If we receive a storm of requests for the same context (see gem_storedw_loop_*)
> we might end up iterating over too many elements in interrupt time, looking for
> contexts to squash together. Instead, share the burden by giving more
> intelligence to the queue function. At most, the interrupt will iterate over
> three elements.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d9edd10..0aad721 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -410,9 +410,11 @@ int gen8_switch_context_queue(struct intel_engine *ring,
>  			      struct i915_hw_context *to,
>  			      u32 tail)
>  {
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  	struct drm_i915_gem_request *req = NULL;
>  	unsigned long flags;
> -	bool was_empty;
> +	struct drm_i915_gem_request *cursor;
> +	int num_elements = 0;
>  
>  	req = kzalloc(sizeof(*req), GFP_KERNEL);
>  	if (req == NULL)
> @@ -425,9 +427,24 @@ int gen8_switch_context_queue(struct intel_engine *ring,
>  
>  	spin_lock_irqsave(&ring->execlist_lock, flags);
>  
> -	was_empty = list_empty(&ring->execlist_queue);
> +	list_for_each_entry(cursor, &ring->execlist_queue, execlist_link)
> +		if (++num_elements > 2)
> +			break;
> +
> +	if (num_elements > 2) {
> +		struct drm_i915_gem_request *tail_req =
> +				list_last_entry(&ring->execlist_queue,
> +					struct drm_i915_gem_request, execlist_link);
> +		if (to == tail_req->ctx) {
> +			WARN(tail_req->elsp_submitted != 0,
> +					"More than 2 already-submitted reqs queued\n");
> +			list_del(&tail_req->execlist_link);
> +			queue_work(dev_priv->wq, &tail_req->work);
> +		}
> +	}

Completely forgotten to mention this: Chris&I discussed this on irc and I
guess this issue will disappear if we track contexts instead of requests
in the scheduler. I guess this is an artifact of the gen7 scheduler you've
based this on, but even for that I think scheduling contexts (with preempt
point after each batch) is the right approach. But I haven't dug out the
scheduler patches again so might be wrong with that.
-Daniel

> +
>  	list_add_tail(&req->execlist_link, &ring->execlist_queue);
> -	if (was_empty)
> +	if (num_elements == 0)
>  		gen8_switch_context_unqueue(ring);
>  
>  	spin_unlock_irqrestore(&ring->execlist_lock, flags);
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch