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

[Tvrtko Ursulin]

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 at least it would 
have the benefit of avoiding the header ordering problems. I think at 
least, will see.

Will go with engine->flags, NEEDS_CMD_PARSER and SUPPORTS_STATS.

Regards,

Tvrtko