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

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-11-26 18:58:20)
> 
> 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.

So my confusion boils down to 'enabled' in config_enabled_bit.
To me that makes it seem like it is a state, as opposed to the bit to be
used to track that state. (I feel enabled <-> tracked is quite clunky.)

config_enable_bit() is better, but I think you can just drop the
'enabled' altogether to leave

static unsigned int config_bit(u64 config)
{
	if (is_engine_config(config))
		return engine_config_bit(config);

	if (is_tracked_config(config))
		return tracked_config_bit(config);

	return -1;
}

static u64 config_mask(u64 config)
{
        return BIT_ULL(config_bit(config));
}

static unsigned int event_bit(struct perf_event *event)
{
        return config_bit(event->attr.config);
}

unsigned int bit = event_bit(event);

Or if that is too much,
	config_sample_bit()
	config_sample_mask()
	event_sample_bit()
?
-Chris