[Intel-gfx] [PATCH] drm/i915/pmu: Deprecate I915_PMU_LAST and optimize state tracking

[Tvrtko Ursulin]

On 26/11/2020 16:56, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-11-26 16:47:03)
>> -static unsigned int config_enabled_bit(u64 config)
>> +static unsigned int is_tracked_config(const u64 config)
>>   {
>> -       if (is_engine_config(config))
>> +       unsigned int val;
> 
>> +/**
>> + * Non-engine events that we need to track enabled-disabled transition and
>> + * current state.
>> + */
> 
> I'm not understanding what is_tracked_config() actually means and how
> that becomes config_enabled_bit().
> 
> These look like the non-engine ones where we interact with HW during the
> sample.
> 
> How do the events we define a bit for here differ from the "untracked"
> events?

Tracked means i915 pmu needs to track enabled/disabled transitions and 
state.

So far frequency and rc6 needs that, due sampling timer decisions and 
park/unpark handling respectively.

Interrupts on the contrary don't need to do any of that. We can just 
read the count at any given time.

Is_tracked_config() for convenience returns a "bit + 1", so 0 means 
untracked.

Every tracked event needs a slot in pmu->enabled and pmu->enable_count. 
The rest don't. Before this patch I had too many bits/array elements 
reserved there.

Old state in terms of bit/slot allocation was:

  0 - 15 engine samplers. (Only 3 used, 12 wasted bits/counts).
16 - 63 other counters. (Interrupts was using a slot for no purpose.)

New state:

  0 -  2 engine samplers.
  3 - 31 other counters. (Only 3 used so far, interrupts has no slot.)

It was a handy 1:1 mapping between non-engine events and bits/slots. 
Apart from wasting bits/slots, if I want to partition the u64 config 
space a bit more then 1:1 becomes a problem.

Note that engine bits/counts in top-level struct pmu are for all/any 
engine - just because a sampling timer decision is easy like that. (Set 
bit for any engine having an active sampler of a type, and respective 
enable_count incremented by each engine instance.)

Regards,

Tvrtko

>> +
>> +       switch (config) {
>> +       case I915_PMU_ACTUAL_FREQUENCY:
>> +               val =  __I915_PMU_ACTUAL_FREQUENCY_ENABLED;
>> +               break;
>> +       case I915_PMU_REQUESTED_FREQUENCY:
>> +               val = __I915_PMU_REQUESTED_FREQUENCY_ENABLED;
>> +               break;
>> +       case I915_PMU_RC6_RESIDENCY:
>> +               val = __I915_PMU_RC6_RESIDENCY_ENABLED;
>> +               break;
>> +       default:
>> +               return 0;
>> +       }
>> +
>> +       return val + 1;
>> +}
>> +
>> +static unsigned int config_enabled_bit(const u64 config)
>> +{
>> +       if (is_engine_config(config)) {
>>                  return engine_config_sample(config);
>> -       else
>> -               return ENGINE_SAMPLE_BITS + (config - __I915_PMU_OTHER(0));
>> +       } else {
>> +               unsigned int bit = is_tracked_config(config);
>> +
>> +               if (bit)
>> +                       return I915_ENGINE_SAMPLE_COUNT + bit - 1;
>> +               else
>> +                       return -1;
>> +       }
>>   }