Re: [PATCH 4/4] drm/i915: Make next_seqno debugs entry to use i915_gem_seqno_init

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Mon, 17 Dec 2012 18:44:26 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
>> Hardware status page needs to have proper seqno set
>> as our initial seqno can be arbitrary. If initial seqno is close
>> to wrap boundary on init and i915_seqno_passed() (31bit space)
>> refers to hw status page which contains zero, errorneous result
>> will be returned.
>> 
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=58230
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Looks good baring the last chunk.
>
>
>> @@ -1435,8 +1439,18 @@ int intel_ring_handle_seqno_wrap(struct intel_ring_buffer *ring)
>>  	 * post-wrap semaphore waits completing immediately. Clear them. */
>>  	update_mboxes(ring, ring->signal_mbox[0]);
>>  	update_mboxes(ring, ring->signal_mbox[1]);
>> +	intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
>> +	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
>> +	intel_ring_emit(ring, seqno);
>> +	intel_ring_emit(ring, MI_USER_INTERRUPT);
>>  	intel_ring_advance(ring);
>>  
>> +	/* Wait until mboxes have actually cleared before pushing
>> +	 * anything to the rings */
>> +	ret = ring_wait_for_space(ring, ring->size - 8);
>> +	if (ret)
>> +		return ret;
>
> I don't this is well justified. Can you clearly explain the situation
> where it is required?

As the ring_add_request can it self cause seqno wrap due to
intel_ring_begin, and the fact that it will update the *other* rings
mboxes, we need to wait until all the rings have proceed with 
clearing the mboxes.


> How about if we assert that the ring is idle, and just blitz the
> registers and hws rather than go through the ring?
> -Chris

I have tried this but failed. I think the problem is ring_add_request.
It will still inject seqnos before the wrap boundary if intel_ring_begin
in itself just wrapped. This is why we need to clear mboxes and set 
the hws page through rings.

I have a patch which allocates seqnos explicitly early in
i915_gem_do_execbuffer, gets rid of outstanding_lazy_request and 
related i915_gem_check_olr completely thus making the wrap handling much more 
simpler as we don't need to be careful in ring_sync nor ring_add_request
as no cross wrap boundary stuff can no longer happen. But I got
the impression that you don't like this approach.

-Mika

> -- 
> Chris Wilson, Intel Open Source Technology Centre