Re: [PATCH 14/21] drm/i915: Only apply one barrier after a breadcrumb interrupt is posted

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

On 08/06/16 10:35, Chris Wilson wrote:
> On Mon, Jun 06, 2016 at 04:34:27PM +0100, Tvrtko Ursulin wrote:
>>
>> On 03/06/16 17:08, Chris Wilson wrote:
>>> If we flag the seqno as potentially stale upon receiving an interrupt,
>>> we can use that information to reduce the frequency that we apply the
>>> heavyweight coherent seqno read (i.e. if we wake up a chain of waiters).
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h          | 15 ++++++++++++++-
>>>   drivers/gpu/drm/i915/i915_irq.c          |  1 +
>>>   drivers/gpu/drm/i915/intel_breadcrumbs.c | 16 ++++++++++------
>>>   drivers/gpu/drm/i915/intel_ringbuffer.h  |  1 +
>>>   4 files changed, 26 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 4ddb9ff319cb..a71d08199d57 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -3935,7 +3935,20 @@ static inline bool __i915_request_irq_complete(struct drm_i915_gem_request *req)
>>>   	 * but it is easier and safer to do it every time the waiter
>>>   	 * is woken.
>>>   	 */
>>> -	if (engine->irq_seqno_barrier) {
>>> +	if (engine->irq_seqno_barrier && READ_ONCE(engine->irq_posted)) {
>>> +		/* The ordering of irq_posted versus applying the barrier
>>> +		 * is crucial. The clearing of the current irq_posted must
>>> +		 * be visible before we perform the barrier operation,
>>> +		 * such that if a subsequent interrupt arrives, irq_posted
>>> +		 * is reasserted and our task rewoken (which causes us to
>>> +		 * do another __i915_request_irq_complete() immediately
>>> +		 * and reapply the barrier). Conversely, if the clear
>>> +		 * occurs after the barrier, then an interrupt that arrived
>>> +		 * whilst we waited on the barrier would not trigger a
>>> +		 * barrier on the next pass, and the read may not see the
>>> +		 * seqno update.
>>> +		 */
>>> +		WRITE_ONCE(engine->irq_posted, false);
>>
>> Why is this not smp_store_mb ?
>
> We only require the ordering wrt to irq_seqno_barrier().
>
> How about:
>
> if (engine->irq_seqno_barrier &&
>      cmpxchg_relaxed(&engine->irq_post, 1, 0)) {
>
> Less shouty?

I think so.

>>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> index 44346de39794..0f5fe114c204 100644
>>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> @@ -43,12 +43,18 @@ static void intel_breadcrumbs_fake_irq(unsigned long data)
>>>
>>>   static void irq_enable(struct intel_engine_cs *engine)
>>>   {
>>> +	/* Enabling the IRQ may miss the generation of the interrupt, but
>>> +	 * we still need to force the barrier before reading the seqno,
>>> +	 * just in case.
>>> +	 */
>>> +	engine->irq_posted = true;
>>
>> Should it be smp_store_mb here as well?
>
> No, this is written/read on the same callchain.

Ah true.

Regards,

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