[Intel-gfx] [PATCH] drm/i915: Consolidate checks for engine stats availability

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-24 10:08:07)
> 
> On 22/11/2017 12:37, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-11-22 12:05:18)
> >> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >> Sagar noticed the check can be consolidated between the engine stats
> >> implementation and the PMU.
> >>
> >> My first choice was a static inline helper but that got into include
> >> ordering mess quickly fast so I went with a macro instead. At some point
> >> we should perhaps looking into taking out the non-ringubffer bits from
> >> intel_ringbuffer.h into a new intel_engine.h or something.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >> Suggested-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> >> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_pmu.c         | 9 ++-------
> >>   drivers/gpu/drm/i915/intel_engine_cs.c  | 4 ++--
> >>   drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++
> >>   3 files changed, 6 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> >> index 1071935bfa67..112243720ff3 100644
> >> --- a/drivers/gpu/drm/i915/i915_pmu.c
> >> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> >> @@ -90,11 +90,6 @@ static unsigned int event_enabled_bit(struct perf_event *event)
> >>          return config_enabled_bit(event->attr.config);
> >>   }
> >>   
> >> -static bool supports_busy_stats(struct drm_i915_private *i915)
> >> -{
> >> -       return INTEL_GEN(i915) >= 8;
> >> -}
> >> -
> >>   static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
> >>   {
> >>          u64 enable;
> >> @@ -124,7 +119,7 @@ static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
> >>           * Also there is software busyness tracking available we do not
> >>           * need the timer for I915_SAMPLE_BUSY counter.
> >>           */
> >> -       else if (supports_busy_stats(i915))
> >> +       else if (intel_supports_engine_stats(i915))
> >>                  enable &= ~BIT(I915_SAMPLE_BUSY);
> >>   
> >>          /*
> >> @@ -463,7 +458,7 @@ static void i915_pmu_event_read(struct perf_event *event)
> >>   
> >>   static bool engine_needs_busy_stats(struct intel_engine_cs *engine)
> >>   {
> >> -       return supports_busy_stats(engine->i915) &&
> >> +       return intel_supports_engine_stats(engine->i915) &&
> >>                 (engine->pmu.enable & BIT(I915_SAMPLE_BUSY));
> >>   }
> >>   
> >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> >> index fede62daf3e1..d53680c08cb0 100644
> >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> >> @@ -1863,7 +1863,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
> >>   {
> >>          unsigned long flags;
> >>   
> >> -       if (INTEL_GEN(engine->i915) < 8)
> >> +       if (!intel_supports_engine_stats(engine->i915))
> >>                  return -ENODEV;
> >>   
> >>          spin_lock_irqsave(&engine->stats.lock, flags);
> >> @@ -1924,7 +1924,7 @@ void intel_disable_engine_stats(struct intel_engine_cs *engine)
> >>   {
> >>          unsigned long flags;
> >>   
> >> -       if (INTEL_GEN(engine->i915) < 8)
> >> +       if (!intel_supports_engine_stats(engine->i915))
> >>                  return;
> >>   
> >>          spin_lock_irqsave(&engine->stats.lock, flags);
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> index 3bd30d011866..37a389ff031e 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> @@ -1054,6 +1054,8 @@ static inline void intel_engine_context_out(struct intel_engine_cs *engine)
> >>          spin_unlock_irqrestore(&engine->stats.lock, flags);
> >>   }
> >>   
> >> +#define intel_supports_engine_stats(i915) (INTEL_GEN(i915) >= 8)
> > 
> > Hmm, do we have an engine->flags for caps? Just thinking that would be
> > more accurate than a gen test stuffed away in a header. So at present we
> > have no engine->flags, but we do already have one bool needs_cmd_parser
> > that could be pulled into an engine->flags. (Not sure if engine->caps is
> > a good name if we are adding HW caps at a later point.)
> 
> I can do that sure, I mean I can't imagine the setting being actually 
> per-engine (cmd parser also really isn't, no?),

But it technically it could be! ;)

> but at least it would 
> have the benefit of avoiding the header ordering problems. I think at 
> least, will see.

Pretty much why it is under engine right now, so that the information is
encapsulated where it wants to be used.

Spend some time looking at gma500 and pondering how to fit its mm and
execution into i915 as simply a different type of engine+gtt. It is
definitely possible :)
-Chris