[Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context backing objects to GGTT on-demand

[Daniel, Thomas]

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