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

[Daniel, Thomas]

For the avoidance of confusion, I want to make it clear that the latest revisions to the patches in this set posted to the list (v5) address all the review comments from the VPG guys.

[v5 1/4] http://patchwork.freedesktop.org/patch/36716/
[2/4] already accepted
[v5 3/4] http://patchwork.freedesktop.org/patch/36717/
[v5 4/4] http://patchwork.freedesktop.org/patch/36718/

Thomas.

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf
> Of Daniel, Thomas
> Sent: Monday, November 17, 2014 2:56 PM
> To: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v5 3/4] drm/i915/bdw: Pin the context
> backing objects to GGTT on-demand
> 
> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx