Re: [PATCH 43/49] drm/i915/bdw: Handle context switch events

[Robert Beckett <robert.beckett@intel.com>]

On 27/03/2014 18:00, oscar.mateo@intel.com wrote:
> From: Thomas Daniel <thomas.daniel@intel.com>
>
> Handle all context status events in the context status buffer on every
> context switch interrupt. We only remove work from the execlist queue
> after a context status buffer reports that it has completed and we only
> attempt to schedule new contexts on interrupt when a previously submitted
> context completes (unless no contexts are queued, which means the GPU is
> free).
>
> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
>
> v2: Unreferencing the context when we are freeing the request might free
> the backing bo, which requires the struct_mutex to be grabbed, so defer
> unreferencing and freeing to a bottom half.
>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         |   3 +
>   drivers/gpu/drm/i915/i915_irq.c         |  28 ++++++---
>   drivers/gpu/drm/i915/i915_lrc.c         | 101 +++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/i915/intel_ringbuffer.c |   1 +
>   drivers/gpu/drm/i915/intel_ringbuffer.h |   1 +
>   5 files changed, 123 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2607664..4c8cf52 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1679,6 +1679,8 @@ struct drm_i915_gem_request {
>
>   	/** execlist queue entry for this request */
>   	struct list_head execlist_link;
> +	/** Struct to handle this request in the bottom half of an interrupt */
> +	struct work_struct work;
>   };
>
>   struct drm_i915_file_private {
> @@ -2344,6 +2346,7 @@ void gen8_gem_context_free(struct i915_hw_context *ctx);
>   int gen8_switch_context_queue(struct intel_engine *ring,
>   			      struct i915_hw_context *to,
>   			      u32 tail);
> +void gen8_handle_context_events(struct intel_engine *ring);
>
>   /* i915_gem_evict.c */
>   int __must_check i915_gem_evict_something(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 56657b5..6e0f456 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1334,6 +1334,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>   				       struct drm_i915_private *dev_priv,
>   				       u32 master_ctl)
>   {
> +	struct intel_engine *ring;
>   	u32 rcs, bcs, vcs, vecs;
>   	uint32_t tmp = 0;
>   	irqreturn_t ret = IRQ_NONE;
> @@ -1342,14 +1343,21 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>   		tmp = I915_READ(GEN8_GT_IIR(0));
>   		if (tmp) {
>   			ret = IRQ_HANDLED;
> +
>   			rcs = tmp >> GEN8_RCS_IRQ_SHIFT;
> -			bcs = tmp >> GEN8_BCS_IRQ_SHIFT;
> +			ring = &dev_priv->ring[RCS];
>   			if (rcs & GT_RENDER_USER_INTERRUPT)
> -				notify_ring(dev, &dev_priv->ring[RCS]);
> +				notify_ring(dev, ring);
> +			if (rcs & GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
> +				gen8_handle_context_events(ring);

Handling the context events here can generate a new execlist submission, 
which if a small enough workload, can finish and generate a new context 
event interrupt before we ack this interrupt.

When we ack this interrupt, we clear the new one too, loosing an interrupt.

Moving the

I915_WRITE(GEN8_GT_IIR(0), tmp);

to just inside the if (tmp) { conditional (or anywhere before this call) 
fixes this issue. There is no harm in acking the interrupt immediately 
as we have the read stored in tmp.


> +
> +			bcs = tmp >> GEN8_BCS_IRQ_SHIFT;
> +			ring = &dev_priv->ring[BCS];
>   			if (bcs & GT_RENDER_USER_INTERRUPT)
> -				notify_ring(dev, &dev_priv->ring[BCS]);
> -			if ((rcs | bcs) & GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
> -			     DRM_DEBUG_DRIVER("TODO: Context switch\n");
> +				notify_ring(dev, ring);
> +			if (bcs & GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
> +				gen8_handle_context_events(ring);
> +
>   			I915_WRITE(GEN8_GT_IIR(0), tmp);
>   		} else
>   			DRM_ERROR("The master control interrupt lied (GT0)!\n");
> @@ -1360,10 +1368,11 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>   		if (tmp) {
>   			ret = IRQ_HANDLED;
>   			vcs = tmp >> GEN8_VCS1_IRQ_SHIFT;
> +			ring = &dev_priv->ring[VCS];
>   			if (vcs & GT_RENDER_USER_INTERRUPT)
> -				notify_ring(dev, &dev_priv->ring[VCS]);
> +				notify_ring(dev, ring);
>   			if (vcs & GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
> -			     DRM_DEBUG_DRIVER("TODO: Context switch\n");
> +				gen8_handle_context_events(ring);
>   			I915_WRITE(GEN8_GT_IIR(1), tmp);
>   		} else
>   			DRM_ERROR("The master control interrupt lied (GT1)!\n");
> @@ -1374,10 +1383,11 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>   		if (tmp) {
>   			ret = IRQ_HANDLED;
>   			vecs = tmp >> GEN8_VECS_IRQ_SHIFT;
> +			ring = &dev_priv->ring[VECS];
>   			if (vecs & GT_RENDER_USER_INTERRUPT)
> -				notify_ring(dev, &dev_priv->ring[VECS]);
> +				notify_ring(dev, ring);
>   			if (vecs & GEN8_GT_CONTEXT_SWITCH_INTERRUPT)
> -			     DRM_DEBUG_DRIVER("TODO: Context switch\n");
> +				gen8_handle_context_events(ring);
>   			I915_WRITE(GEN8_GT_IIR(3), tmp);
>   		} else
>   			DRM_ERROR("The master control interrupt lied (GT3)!\n");
> diff --git a/drivers/gpu/drm/i915/i915_lrc.c b/drivers/gpu/drm/i915/i915_lrc.c
> index 4cacabb..440da11 100644
> --- a/drivers/gpu/drm/i915/i915_lrc.c
> +++ b/drivers/gpu/drm/i915/i915_lrc.c
> @@ -46,7 +46,24 @@
>   #define GEN8_LR_CONTEXT_SIZE (21 * PAGE_SIZE)
>
>   #define RING_ELSP(ring)			((ring)->mmio_base+0x230)
> +#define RING_EXECLIST_STATUS(ring)	((ring)->mmio_base+0x234)
>   #define RING_CONTEXT_CONTROL(ring)	((ring)->mmio_base+0x244)
> +#define RING_CONTEXT_STATUS_BUF(ring)	((ring)->mmio_base+0x370)
> +#define RING_CONTEXT_STATUS_PTR(ring)	((ring)->mmio_base+0x3a0)
> +
> +#define RING_EXECLIST_QFULL		(1 << 0x2)
> +#define RING_EXECLIST1_VALID		(1 << 0x3)
> +#define RING_EXECLIST0_VALID		(1 << 0x4)
> +#define RING_EXECLIST_ACTIVE_STATUS	(3 << 0xE)
> +#define RING_EXECLIST1_ACTIVE		(1 << 0x11)
> +#define RING_EXECLIST0_ACTIVE		(1 << 0x12)
> +
> +#define GEN8_CTX_STATUS_IDLE_ACTIVE	(1 << 0)
> +#define GEN8_CTX_STATUS_PREEMPTED	(1 << 1)
> +#define GEN8_CTX_STATUS_ELEMENT_SWITCH	(1 << 2)
> +#define GEN8_CTX_STATUS_ACTIVE_IDLE	(1 << 3)
> +#define GEN8_CTX_STATUS_COMPLETE	(1 << 4)
> +#define GEN8_CTX_STATUS_LITE_RESTORE	(1 << 15)
>
>   #define CTX_LRI_HEADER_0		0x01
>   #define CTX_CONTEXT_CONTROL		0x02
> @@ -237,6 +254,9 @@ static void gen8_switch_context_unqueue(struct intel_engine *ring)
>   {
>   	struct drm_i915_gem_request *req0 = NULL, *req1 = NULL;
>   	struct drm_i915_gem_request *cursor = NULL, *tmp = NULL;
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +
> +	assert_spin_locked(&ring->execlist_lock);
>
>   	if (list_empty(&ring->execlist_queue))
>   		return;
> @@ -249,8 +269,7 @@ static void gen8_switch_context_unqueue(struct intel_engine *ring)
>   			/* Same ID: ignore first request, as second request
>   			 * will update tail past first request's workload */
>   			list_del(&req0->execlist_link);
> -			i915_gem_context_unreference(req0->ctx);
> -			kfree(req0);
> +			queue_work(dev_priv->wq, &req0->work);
>   			req0 = cursor;
>   		} else {
>   			req1 = cursor;
> @@ -262,6 +281,83 @@ static void gen8_switch_context_unqueue(struct intel_engine *ring)
>   			req1? req1->ctx : NULL, req1? req1->tail : 0));
>   }
>
> +static bool check_remove_request(struct intel_engine *ring, u32 request_id)
> +{
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct drm_i915_gem_request *head_req;
> +
> +	assert_spin_locked(&ring->execlist_lock);
> +
> +	head_req = list_first_entry_or_null(&ring->execlist_queue,
> +			struct drm_i915_gem_request, execlist_link);
> +	if (head_req != NULL) {
> +		if (get_submission_id(head_req->ctx) == request_id) {
> +			list_del(&head_req->execlist_link);
> +			queue_work(dev_priv->wq, &head_req->work);
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
> +void gen8_handle_context_events(struct intel_engine *ring)
> +{
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	u32 status_pointer;
> +	u8 read_pointer;
> +	u8 write_pointer;
> +	u32 status;
> +	u32 status_id;
> +	u32 submit_contexts = 0;
> +
> +	status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(ring));
> +
> +	read_pointer = ring->next_context_status_buffer;
> +	write_pointer = status_pointer & 0x07;
> +	if (read_pointer > write_pointer)
> +		write_pointer += 6;
> +
> +	spin_lock(&ring->execlist_lock);
> +
> +	while (read_pointer < write_pointer) {
> +		read_pointer++;
> +		status = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
> +				(read_pointer % 6) * 8);
> +		status_id = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
> +				(read_pointer % 6) * 8 + 4);
> +
> +		if (status & GEN8_CTX_STATUS_ELEMENT_SWITCH) {
> +			if (check_remove_request(ring, status_id))
> +				submit_contexts++;
> +		} else if (status & GEN8_CTX_STATUS_COMPLETE) {
> +			if (check_remove_request(ring, status_id))
> +				submit_contexts++;
> +		}
> +	}
> +
> +	if (submit_contexts != 0)
> +		gen8_switch_context_unqueue(ring);
> +
> +	spin_unlock(&ring->execlist_lock);
> +
> +	WARN(submit_contexts > 2, "More than two context complete events?\n");
> +	ring->next_context_status_buffer = write_pointer % 6;
> +}
> +
> +static void free_request_task(struct work_struct *work)
> +{
> +	struct drm_i915_gem_request *req =
> +			container_of(work, struct drm_i915_gem_request, work);
> +	struct drm_device *dev = req->ring->dev;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	i915_gem_context_unreference(req->ctx);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	kfree(req);
> +}
> +
>   int gen8_switch_context_queue(struct intel_engine *ring,
>   			      struct i915_hw_context *to,
>   			      u32 tail)
> @@ -276,6 +372,7 @@ int gen8_switch_context_queue(struct intel_engine *ring,
>   	req->ctx = to;
>   	i915_gem_context_reference(req->ctx);
>   	req->tail = tail;
> +	INIT_WORK(&req->work, free_request_task);
>
>   	spin_lock_irqsave(&ring->execlist_lock, flags);
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index a92bede..ee5a220 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1464,6 +1464,7 @@ static int intel_init_ring(struct drm_device *dev,
>   		if (ring->status_page.page_addr == NULL)
>   			return -ENOMEM;
>   		ring->status_page.obj = obj;
> +		ring->next_context_status_buffer = 0;
>   	} else if (I915_NEED_GFX_HWS(dev)) {
>   		ret = init_status_page(ring);
>   		if (ret)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 5f4fd3c..daca04e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -173,6 +173,7 @@ struct intel_engine {
>
>   	struct i915_hw_context *default_context;
>   	struct i915_hw_context *last_context;
> +	u8 next_context_status_buffer;
>
>   	struct intel_ring_hangcheck hangcheck;
>
>