Re: [PATCH v2 10/15] drm/i915: Remove the preempted request from the execution queue

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 22/02/2017 13:40, Chris Wilson wrote:
> On Wed, Feb 22, 2017 at 01:33:22PM +0000, Tvrtko Ursulin wrote:
>>
>> On 22/02/2017 11:46, Chris Wilson wrote:
>>> +void __i915_gem_request_unsubmit(struct drm_i915_gem_request *request)
>>> +{
>>> +	struct intel_engine_cs *engine = request->engine;
>>> +	struct intel_timeline *timeline;
>>> +
>>> +	assert_spin_locked(&engine->timeline->lock);
>>> +
>>> +	/* Only unwind in reverse order, required so that the per-context list
>>> +	 * is kept in seqno/ring order.
>>> +	 */
>>> +	GEM_BUG_ON(request->global_seqno != engine->timeline->seqno);
>>> +	engine->timeline->seqno--;
>>> +
>>> +	/* We may be recursing from the signal callback of another i915 fence */
>>
>> Copy-paste of the comment of there will really be preemption
>> triggered from the signal callback?
>
> I believe it may be. Say an RCS request was waiting on a BCS request,
> and we decide to preempt, and can do so immediately. I think being
> prepared for the same recursion here is predundant.

Yeah OK, just wasn't sure at which level will we handle preemption.

>>> static int __i915_sw_fence_call
>>> submit_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
>>> {
>>> @@ -1034,9 +1083,11 @@ long i915_wait_request(struct drm_i915_gem_request *req,
>>> 	if (flags & I915_WAIT_LOCKED)
>>> 		add_wait_queue(errq, &reset);
>>>
>>> -	intel_wait_init(&wait, i915_gem_request_global_seqno(req));
>>> +	wait.tsk = current;
>>>
>>> +restart:
>>> 	reset_wait_queue(&req->execute, &exec);
>>> +	wait.seqno = i915_gem_request_global_seqno(req);
>>
>> Not sure if it is worth dropping intel_wait_init, I presume to avoid
>> assigning the task twice? It will still be the same task so just
>> moving the intel_wait_init here would be clearer.
>
> I was thinking the opposite, since we are looking at wait.seqno directly
> elsewhere, so wanted that to be clear. And current is in a special
> register, so why pay the cost to reload it onto stack :)

I can see that but intel_wait_init was so nice as a marker when reading 
the code.

Maybe leave it and add intel_wait_update_seqno?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx