| Message ID | 1413810355-28062-1-git-send-email-thomas.daniel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 20, 2014 at 02:05:55PM +0100, Thomas Daniel wrote: > No longer create a work item to clean each execlist queue item. > Instead, move retired execlist requests to a queue and clean up the > items during retire_requests. > > v2: Fix legacy ring path broken during overzealous cleanup At some point I presume you noticed that you broke the idle detection... Please do read http://patchwork.freedesktop.org/patch/33175/ and reconsider. -Chris
> -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Monday, October 20, 2014 2:11 PM > To: Daniel, Thomas > Cc: intel-gfx@lists.freedesktop.org; shuang.he@linux.intel.com > Subject: Re: [Intel-gfx] [PATCH] drm/i915/bdw: Clean up execlist queue > items in retire_work > > On Mon, Oct 20, 2014 at 02:05:55PM +0100, Thomas Daniel wrote: > > No longer create a work item to clean each execlist queue item. > > Instead, move retired execlist requests to a queue and clean up the > > items during retire_requests. > > > > v2: Fix legacy ring path broken during overzealous cleanup > > At some point I presume you noticed that you broke the idle detection... This patch doesn't change the idle detection. Did you want the idle flag to now take into account the state of the execlists requests as well? > Please do read http://patchwork.freedesktop.org/patch/33175/ and > reconsider. Are you saying that my patch is not required because that patch reworks the code? Cheers, Thomas. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=273/273->272/273
PNV: pass/total=269/271->271/271
ILK: pass/total=271/271->271/271
IVB: pass/total=271/271->271/271
SNB: pass/total=271/271->271/271
HSW: pass/total=271/271->271/271
BDW: pass/total=271/271->269/271
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly->result_with_patch_applied
BYT: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, PASS->DMESG_WARN
PNV: Intel_gpu_tools, igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-forked, TIMEOUT->PASS
PNV: Intel_gpu_tools, igt_gem_concurrent_blit_gttX-bcs-gpu-read-after-write-forked, TIMEOUT->PASS
BDW: Intel_gpu_tools, igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-forked, PASS->TIMEOUT
BDW: Intel_gpu_tools, igt_gem_concurrent_blit_gttX-bcs-gpu-read-after-write-forked, PASS->TIMEOUT
On Mon, Oct 20, 2014 at 01:29:59PM +0000, Daniel, Thomas wrote: > > -----Original Message----- > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > > Sent: Monday, October 20, 2014 2:11 PM > > To: Daniel, Thomas > > Cc: intel-gfx@lists.freedesktop.org; shuang.he@linux.intel.com > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/bdw: Clean up execlist queue > > items in retire_work > > > > On Mon, Oct 20, 2014 at 02:05:55PM +0100, Thomas Daniel wrote: > > > No longer create a work item to clean each execlist queue item. > > > Instead, move retired execlist requests to a queue and clean up the > > > items during retire_requests. > > > > > > v2: Fix legacy ring path broken during overzealous cleanup > > > > At some point I presume you noticed that you broke the idle detection... > This patch doesn't change the idle detection. Did you want the idle flag to > now take into account the state of the execlists requests as well? I guess what Chris means is that execlist broke idle detection (by retiring in a separate work item, but not integrating with the idle handling in the existing one). This patch gets us half-way there but still doesn't really integrate things back fully. We probably need (again) the s/execlist_item/request/ refactor to make this happen for real. Since that's what Chris' patch does he featured the link to it as an endpoint guideline I think. Or am I completely missing the point again? Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 895f988..65cfe9a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2718,6 +2718,8 @@ i915_gem_retire_requests(struct drm_device *dev) for_each_ring(ring, dev_priv, i) { i915_gem_retire_requests_ring(ring); idle &= list_empty(&ring->request_list); + if (i915.enable_execlists) + intel_execlists_retire_requests(ring); } if (idle) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 803fc38..666cb28 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -386,7 +386,6 @@ static void execlists_context_unqueue(struct intel_engine_cs *ring) { struct intel_ctx_submit_request *req0 = NULL, *req1 = NULL; struct intel_ctx_submit_request *cursor = NULL, *tmp = NULL; - struct drm_i915_private *dev_priv = ring->dev->dev_private; assert_spin_locked(&ring->execlist_lock); @@ -403,7 +402,8 @@ static void execlists_context_unqueue(struct intel_engine_cs *ring) * will update tail past first request's workload */ cursor->elsp_submitted = req0->elsp_submitted; list_del(&req0->execlist_link); - queue_work(dev_priv->wq, &req0->work); + list_add_tail(&req0->execlist_link, + &ring->execlist_retired_req_list); req0 = cursor; } else { req1 = cursor; @@ -425,7 +425,6 @@ static void execlists_context_unqueue(struct intel_engine_cs *ring) static bool execlists_check_remove_request(struct intel_engine_cs *ring, u32 request_id) { - struct drm_i915_private *dev_priv = ring->dev->dev_private; struct intel_ctx_submit_request *head_req; assert_spin_locked(&ring->execlist_lock); @@ -443,7 +442,8 @@ static bool execlists_check_remove_request(struct intel_engine_cs *ring, if (--head_req->elsp_submitted <= 0) { list_del(&head_req->execlist_link); - queue_work(dev_priv->wq, &head_req->work); + list_add_tail(&head_req->execlist_link, + &ring->execlist_retired_req_list); return true; } } @@ -512,22 +512,6 @@ void intel_execlists_handle_ctx_events(struct intel_engine_cs *ring) ((u32)ring->next_context_status_buffer & 0x07) << 8); } -static void execlists_free_request_task(struct work_struct *work) -{ - struct intel_ctx_submit_request *req = - container_of(work, struct intel_ctx_submit_request, work); - struct drm_device *dev = req->ring->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - - intel_runtime_pm_put(dev_priv); - - mutex_lock(&dev->struct_mutex); - i915_gem_context_unreference(req->ctx); - mutex_unlock(&dev->struct_mutex); - - kfree(req); -} - static int execlists_context_queue(struct intel_engine_cs *ring, struct intel_context *to, u32 tail) @@ -544,7 +528,6 @@ static int execlists_context_queue(struct intel_engine_cs *ring, i915_gem_context_reference(req->ctx); req->ring = ring; req->tail = tail; - INIT_WORK(&req->work, execlists_free_request_task); intel_runtime_pm_get(dev_priv); @@ -565,7 +548,8 @@ static int execlists_context_queue(struct intel_engine_cs *ring, 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); + list_add_tail(&tail_req->execlist_link, + &ring->execlist_retired_req_list); } } @@ -733,6 +717,29 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file, return 0; } +void intel_execlists_retire_requests(struct intel_engine_cs *ring) +{ + struct intel_ctx_submit_request *req, *tmp; + struct drm_i915_private *dev_priv = ring->dev->dev_private; + unsigned long flags; + struct list_head retired_list; + + WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex)); + if (list_empty(&ring->execlist_retired_req_list)) + return; + + INIT_LIST_HEAD(&retired_list); + spin_lock_irqsave(&ring->execlist_lock, flags); + list_replace_init(&ring->execlist_retired_req_list, &retired_list); + spin_unlock_irqrestore(&ring->execlist_lock, flags); + + list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) { + intel_runtime_pm_put(dev_priv); + i915_gem_context_unreference(req->ctx); + list_del(&req->execlist_link); + } +} + void intel_logical_ring_stop(struct intel_engine_cs *ring) { struct drm_i915_private *dev_priv = ring->dev->dev_private; @@ -1248,6 +1255,7 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin init_waitqueue_head(&ring->irq_queue); INIT_LIST_HEAD(&ring->execlist_queue); + INIT_LIST_HEAD(&ring->execlist_retired_req_list); spin_lock_init(&ring->execlist_lock); ring->next_context_status_buffer = 0; diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h index 33c3b4b..84bbf19 100644 --- a/drivers/gpu/drm/i915/intel_lrc.h +++ b/drivers/gpu/drm/i915/intel_lrc.h @@ -104,11 +104,11 @@ struct intel_ctx_submit_request { u32 tail; struct list_head execlist_link; - struct work_struct work; int elsp_submitted; }; void intel_execlists_handle_ctx_events(struct intel_engine_cs *ring); +void intel_execlists_retire_requests(struct intel_engine_cs *ring); #endif /* _INTEL_LRC_H_ */ diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 96479c8..8c002d2 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -235,6 +235,7 @@ struct intel_engine_cs { /* Execlists */ spinlock_t execlist_lock; struct list_head execlist_queue; + struct list_head execlist_retired_req_list; u8 next_context_status_buffer; u32 irq_keep_mask; /* bitmask for interrupts that should not be masked */ int (*emit_request)(struct intel_ringbuffer *ringbuf);
No longer create a work item to clean each execlist queue item. Instead, move retired execlist requests to a queue and clean up the items during retire_requests. v2: Fix legacy ring path broken during overzealous cleanup Issue: VIZ-4274 Signed-off-by: Thomas Daniel <thomas.daniel@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 2 ++ drivers/gpu/drm/i915/intel_lrc.c | 52 ++++++++++++++++++------------- drivers/gpu/drm/i915/intel_lrc.h | 2 +- drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + 4 files changed, 34 insertions(+), 23 deletions(-)