[22/22] semaphore-no-stats

Commit Message

Chris Wilson Feb. 4, 2019, 1:22 p.m. UTC

SW PMU reports semaphore time as busy, HW PMU reports semaphore time as
idle. Who is correct?
---
 drivers/gpu/drm/i915/intel_lrc.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Tvrtko Ursulin Feb. 5, 2019, 10:03 a.m. UTC | #1

On 04/02/2019 13:22, Chris Wilson wrote:
> SW PMU reports semaphore time as busy, HW PMU reports semaphore time as
> idle. Who is correct?

[It's not really HW PMU, it's a different implementation of the SW PMU. :)]

As an additional data point, HW tracking of accumulated total context 
runtime as stored in the PPHWSP also reports semaphore spin time 
(polling mode) as context running.

So overall from the point of view of busy being opposite of idle, it is 
kind of correct. Regardless of whether engine is doing something useful 
or not. It is unavailable for other contexts due some action of the 
currently executing context.

In this light we could view busy as aggregate of busy and semaphore 
want. (MI_WAIT_EVENT is an open.) But there is indeed an inconsistency 
on platforms which cannot do context tracking.

Therefore solution a) add semaphore wait time to busy when reporting 
busy on those platforms.

Advantage - PMU sampling timer is already running on these platform so 
additional cost is small.

 From the point of view of wanting to make busy mean "useful" work, that 
seems much harder.

Option b) could be subtract semaphore wait time from busy, on the other 
set of platforms.

Disadvantage - this would mean running the PMU sampling timer when it 
today doesn't need to.

So I am leaning towards option a). Engine busy time semantics would 
therefore be defined as engine not being idle = occupied by a context 
doing something.

Regards,

Tvrtko

> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index ae90ce034252..d00b268ed6ee 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2308,7 +2308,6 @@ void intel_execlists_set_default_submission(struct intel_engine_cs *engine)
>   	engine->unpark = NULL;
>   
>   	engine->flags |= I915_ENGINE_HAS_SEMAPHORES;
> -	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
>   	if (engine->i915->preempt_context)
>   		engine->flags |= I915_ENGINE_HAS_PREEMPTION;
>   }
>

Chris Wilson Feb. 5, 2019, 10:07 a.m. UTC | #2

Quoting Tvrtko Ursulin (2019-02-05 10:03:04)
> 
> On 04/02/2019 13:22, Chris Wilson wrote:
> > SW PMU reports semaphore time as busy, HW PMU reports semaphore time as
> > idle. Who is correct?
> 
> [It's not really HW PMU, it's a different implementation of the SW PMU. :)]
> 
> As an additional data point, HW tracking of accumulated total context 
> runtime as stored in the PPHWSP also reports semaphore spin time 
> (polling mode) as context running.
> 
> So overall from the point of view of busy being opposite of idle, it is 
> kind of correct. Regardless of whether engine is doing something useful 
> or not. It is unavailable for other contexts due some action of the 
> currently executing context.
> 
> In this light we could view busy as aggregate of busy and semaphore 
> want. (MI_WAIT_EVENT is an open.) But there is indeed an inconsistency 
> on platforms which cannot do context tracking.
> 
> Therefore solution a) add semaphore wait time to busy when reporting 
> busy on those platforms.
> 
> Advantage - PMU sampling timer is already running on these platform so 
> additional cost is small.
> 
>  From the point of view of wanting to make busy mean "useful" work, that 
> seems much harder.
> 
> Option b) could be subtract semaphore wait time from busy, on the other 
> set of platforms.
> 
> Disadvantage - this would mean running the PMU sampling timer when it 
> today doesn't need to.
> 
> So I am leaning towards option a). Engine busy time semantics would 
> therefore be defined as engine not being idle = occupied by a context 
> doing something.

(a) is fine by me. The disadvantage is that if clients care about the
spinning they need to account for it themselves... Or they opt out of
semaphores (but they really want a global switch rather than per-context
for accurate system tracking). Is this the compelling reason to have a
context param?
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ae90ce034252..d00b268ed6ee 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2308,7 +2308,6 @@  void intel_execlists_set_default_submission(struct intel_engine_cs *engine)
 	engine->unpark = NULL;
 
 	engine->flags |= I915_ENGINE_HAS_SEMAPHORES;
-	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
 	if (engine->i915->preempt_context)
 		engine->flags |= I915_ENGINE_HAS_PREEMPTION;
 }