[PATCH] drm/i915: Break i915_spin_request() if we see an interrupt

[PATCH] drm/i915: Break i915_spin_request() if we see an interrupt

[Chris Wilson]

If an interrupt has been posted, and we were spinning on the active
seqno waiting for it to advance but it did not, then we can expect that
it will not see its advance in the immediate future and should call into
the irq-seqno barrier. We can stop spinning at this point, and leave the
difficulty of handling the coherency to the caller.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 7760d7481f85..9e42b2687cae 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -993,6 +993,9 @@ bool __i915_spin_request(const struct drm_i915_gem_request *req,
 				      seqno))
 			return true;
 
+		if (test_bit(ENGINE_IRQ_BREADCRUMB, &req->engine->irq_posted))
+			break;
+
 		if (signal_pending_state(state, current))
 			break;
 
-- 
2.11.0

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

Re: [PATCH] drm/i915: Break i915_spin_request() if we see an interrupt

[Mika Kuoppala]

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

> If an interrupt has been posted, and we were spinning on the active
> seqno waiting for it to advance but it did not, then we can expect that
> it will not see its advance in the immediate future

Why we can expect this?
-Mika

> and should call into
> the irq-seqno barrier. We can stop spinning at this point, and leave the
> difficulty of handling the coherency to the caller.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_request.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 7760d7481f85..9e42b2687cae 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -993,6 +993,9 @@ bool __i915_spin_request(const struct drm_i915_gem_request *req,
>  				      seqno))
>  			return true;
>  
> +		if (test_bit(ENGINE_IRQ_BREADCRUMB, &req->engine->irq_posted))
> +			break;
> +
>  		if (signal_pending_state(state, current))
>  			break;
>  
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Break i915_spin_request() if we see an interrupt

[Chris Wilson]

On Thu, Feb 16, 2017 at 03:17:30PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > If an interrupt has been posted, and we were spinning on the active
> > seqno waiting for it to advance but it did not, then we can expect that
> > it will not see its advance in the immediate future
> 
> Why we can expect this?

The seqno is meant to arrive *before* the interrupt. The check is lazy,
but assume it is

irq_posted = test_bit(ENGINE_IRQ_BREADCRUMB, &req->engine->irq_posted);
if (seqno_passed())
	return true;
if (irq_posted)
	return false;

with the strong read ordering imposed. The reason why I think we can be
lax is that we expect the caller to check after spin_request reports
false (just it may do some other work in the meantime).

Note that the interrupt will only be detected if there is already a
waiter (or equivalent).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Break i915_spin_request() if we see an interrupt

[Tvrtko Ursulin]

On 16/02/2017 13:17, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
>> If an interrupt has been posted, and we were spinning on the active
>> seqno waiting for it to advance but it did not, then we can expect that
>> it will not see its advance in the immediate future
>
> Why we can expect this?

Maybe it should be "if (engine->irq_seqno_barrier && test_bit(...))" if 
my thinking is right that this expectation applies on platforms with 
slow barriers?

Regards,

Tvrtko

>
>> and should call into
>> the irq-seqno barrier. We can stop spinning at this point, and leave the
>> difficulty of handling the coherency to the caller.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_request.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
>> index 7760d7481f85..9e42b2687cae 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_request.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
>> @@ -993,6 +993,9 @@ bool __i915_spin_request(const struct drm_i915_gem_request *req,
>>  				      seqno))
>>  			return true;
>>
>> +		if (test_bit(ENGINE_IRQ_BREADCRUMB, &req->engine->irq_posted))
>> +			break;
>> +
>>  		if (signal_pending_state(state, current))
>>  			break;
>>
>> --
>> 2.11.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Break i915_spin_request() if we see an interrupt

[Chris Wilson]

On Thu, Feb 16, 2017 at 01:26:58PM +0000, Tvrtko Ursulin wrote:
> 
> 
> On 16/02/2017 13:17, Mika Kuoppala wrote:
> >Chris Wilson <chris@chris-wilson.co.uk> writes:
> >
> >>If an interrupt has been posted, and we were spinning on the active
> >>seqno waiting for it to advance but it did not, then we can expect that
> >>it will not see its advance in the immediate future
> >
> >Why we can expect this?
> 
> Maybe it should be "if (engine->irq_seqno_barrier && test_bit(...))"
> if my thinking is right that this expectation applies on platforms
> with slow barriers?

True, those others don't do the clear. And even where we have the
barrier, we only sometimes clear the bit (we interpret the bit to really
mean whether or not we have applied the barrier).

That leaves me with:

unsinged int irq_count = atomic_read(&req->engine->irq_count);

...

if (atomic_read(&req->engine->irq_count) != irq_count))
	break;

Based on the new breadcrumbs hang/waitcheck.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Break i915_spin_request() if we see an interrupt

[Chris Wilson]

If an interrupt has been posted, and we were spinning on the active
seqno waiting for it to advance but it did not, then we can expect that
it will not see its advance in the immediate future and should call into
the irq-seqno barrier. We can stop spinning at this point, and leave the
difficulty of handling the coherency to the caller.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 7760d7481f85..de52ac18e215 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -972,7 +972,8 @@ static bool busywait_stop(unsigned long timeout, unsigned int cpu)
 bool __i915_spin_request(const struct drm_i915_gem_request *req,
 			 u32 seqno, int state, unsigned long timeout_us)
 {
-	unsigned int cpu;
+	struct intel_engine_cs *engine = req->engine;
+	unsigned int irq, cpu;
 
 	/* When waiting for high frequency requests, e.g. during synchronous
 	 * rendering split between the CPU and GPU, the finite amount of time
@@ -984,15 +985,23 @@ bool __i915_spin_request(const struct drm_i915_gem_request *req,
 	 * takes to sleep on a request, on the order of a microsecond.
 	 */
 
+	irq = atomic_read(&engine->irq_count);
 	timeout_us += local_clock_us(&cpu);
 	do {
 		if (seqno != i915_gem_request_global_seqno(req))
 			break;
 
-		if (i915_seqno_passed(intel_engine_get_seqno(req->engine),
-				      seqno))
+		if (i915_seqno_passed(intel_engine_get_seqno(engine), seqno))
 			return true;
 
+		/* Seqno are meant to be ordered *before* the interrupt. If
+		 * we see an interrupt without a corresponding seqno advance,
+		 * assume we won't see one in the near future but require
+		 * the engine->seqno_barrier() to fixup coherency.
+		 */
+		if (atomic_read(&engine->irq_count) != irq)
+			break;
+
 		if (signal_pending_state(state, current))
 			break;
 
-- 
2.11.0

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

Re: [PATCH v2] drm/i915: Break i915_spin_request() if we see an interrupt

[Tvrtko Ursulin]

On 16/02/2017 13:42, Chris Wilson wrote:
> If an interrupt has been posted, and we were spinning on the active
> seqno waiting for it to advance but it did not, then we can expect that
> it will not see its advance in the immediate future and should call into
> the irq-seqno barrier. We can stop spinning at this point, and leave the
> difficulty of handling the coherency to the caller.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_request.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 7760d7481f85..de52ac18e215 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -972,7 +972,8 @@ static bool busywait_stop(unsigned long timeout, unsigned int cpu)
>  bool __i915_spin_request(const struct drm_i915_gem_request *req,
>  			 u32 seqno, int state, unsigned long timeout_us)
>  {
> -	unsigned int cpu;
> +	struct intel_engine_cs *engine = req->engine;
> +	unsigned int irq, cpu;
>
>  	/* When waiting for high frequency requests, e.g. during synchronous
>  	 * rendering split between the CPU and GPU, the finite amount of time
> @@ -984,15 +985,23 @@ bool __i915_spin_request(const struct drm_i915_gem_request *req,
>  	 * takes to sleep on a request, on the order of a microsecond.
>  	 */
>
> +	irq = atomic_read(&engine->irq_count);
>  	timeout_us += local_clock_us(&cpu);
>  	do {
>  		if (seqno != i915_gem_request_global_seqno(req))
>  			break;
>
> -		if (i915_seqno_passed(intel_engine_get_seqno(req->engine),
> -				      seqno))
> +		if (i915_seqno_passed(intel_engine_get_seqno(engine), seqno))
>  			return true;
>
> +		/* Seqno are meant to be ordered *before* the interrupt. If
> +		 * we see an interrupt without a corresponding seqno advance,
> +		 * assume we won't see one in the near future but require
> +		 * the engine->seqno_barrier() to fixup coherency.
> +		 */
> +		if (atomic_read(&engine->irq_count) != irq)
> +			break;
> +
>  		if (signal_pending_state(state, current))
>  			break;
>
>

LGTM

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH v2] drm/i915: Break i915_spin_request() if we see an interrupt

[Mika Kuoppala]

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

> If an interrupt has been posted, and we were spinning on the active
> seqno waiting for it to advance but it did not, then we can expect that
> it will not see its advance in the immediate future and should call into
> the irq-seqno barrier. We can stop spinning at this point, and leave the
> difficulty of handling the coherency to the caller.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_request.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 7760d7481f85..de52ac18e215 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -972,7 +972,8 @@ static bool busywait_stop(unsigned long timeout, unsigned int cpu)
>  bool __i915_spin_request(const struct drm_i915_gem_request *req,
>  			 u32 seqno, int state, unsigned long timeout_us)
>  {
> -	unsigned int cpu;
> +	struct intel_engine_cs *engine = req->engine;
> +	unsigned int irq, cpu;
>  
>  	/* When waiting for high frequency requests, e.g. during synchronous
>  	 * rendering split between the CPU and GPU, the finite amount of time
> @@ -984,15 +985,23 @@ bool __i915_spin_request(const struct drm_i915_gem_request *req,
>  	 * takes to sleep on a request, on the order of a microsecond.
>  	 */
>  
> +	irq = atomic_read(&engine->irq_count);
>  	timeout_us += local_clock_us(&cpu);
>  	do {
>  		if (seqno != i915_gem_request_global_seqno(req))
>  			break;
>  
> -		if (i915_seqno_passed(intel_engine_get_seqno(req->engine),
> -				      seqno))
> +		if (i915_seqno_passed(intel_engine_get_seqno(engine), seqno))
>  			return true;
>  
> +		/* Seqno are meant to be ordered *before* the interrupt. If
> +		 * we see an interrupt without a corresponding seqno advance,
> +		 * assume we won't see one in the near future but require
> +		 * the engine->seqno_barrier() to fixup coherency.
> +		 */
> +		if (atomic_read(&engine->irq_count) != irq)
> +			break;
> +

Looks good but now need to wait for patch to introduce
irq_counts to materialize.

-Mika

>  		if (signal_pending_state(state, current))
>  			break;
>  
> -- 
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx