Here is the actual review... _____________________________________________ From: Daniel, Thomas Sent: Wednesday, November 12, 2014 8:52 PM To: Goel, Akash Subject: RE: Execlists patches code review Hi Akash, I will put the WARN messages back in and remove the need_unpin. The elsp_submitted count does not behave exactly as you would expect because of some race condition. Have a look at the patch “Avoid non-lite-restore preemptions” by Oscar Mateo for a description. Thanks, Thomas. _____________________________________________ From: Goel, Akash Sent: Tuesday, November 11, 2014 4:37 PM To: Daniel, Thomas Subject: RE: Execlists patches code review Hi Thomas, Few comments on http://patchwork.freedesktop.org/patch/35830/ int elsp_submitted; + bool need_unpin; This new field has not been used anywhere. if (intel_execlists_ctx_id(ctx_obj) == request_id) { - WARN(head_req->elsp_submitted == 0, - "Never submitted head request\n"); Sorry couldn’t get this change. Even if a request has been merged, still the elsp_submitted count should not be 0 here, when this function is executed on arrival of Context switch interrupt. When a new request is merged with a previously submitted request, the original value of elsp_submitted is still retained. + /* If the request has been merged, it is possible to get + * here with an unsubmitted request. */ if (--head_req->elsp_submitted <= 0) { if (status & GEN8_CTX_STATUS_PREEMPTED) { if (status & GEN8_CTX_STATUS_LITE_RESTORE) { - if (execlists_check_remove_request(ring, status_id)) - WARN(1, "Lite Restored request removed from queue\n"); + execlists_check_remove_request(ring, status_id); Same doubt here, thought that in this case of interrupt due to Preemption (Lite restore), which will occur when the same Context is submitted as the one already being executed by the Hw, the count will not drop to 0. Count will drop to 0 when the context switch interrupt will be generated subsequently. Best regards Akash _____________________________________________ From: Goel, Akash Sent: Tuesday, November 11, 2014 8:58 PM To: Daniel, Thomas Subject: RE: Execlists patches code review Hi Thomas, I was OOP today, I will provide this review comment tomorrow on the GFX mailing list. Best regards Akash _____________________________________________ From: Daniel, Thomas Sent: Monday, November 10, 2014 10:41 PM To: Goel, Akash Subject: RE: Execlists patches code review Hi Akash, Please post this comment to the mailing list. Assuming nobody else comments I will remove the unpin_lock and replace the mutex_lock(&unpin_lock) with WARN_ON(!mutex_is_locked(&dev->struct_mutex)). Cheers, Thomas. _____________________________________________ From: Goel, Akash Sent: Monday, November 10, 2014 11:19 AM To: Daniel, Thomas Subject: RE: Execlists patches code review In context of the 3rd patch http://patchwork.freedesktop.org/patch/35829/ intel_lr_context_pin is being called from logical_ring_alloc_seqno function and intel_lr_context_unpin gets called from i915_gem_free_request & i915_gem_reset_ring_cleanup functions All these 3 paths are already protected by dev->struct_mutex (Global lock), so they will always execute sequentially with respect to each other. Do we need to have a new lock ? + struct mutex unpin_lock; Best regards Akash _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
