[Intel-gfx] [PATCH 2/3] drm/i915/pmu: add event_to_pmu() helper

[Andi Shyti]

On Thu, Oct 26, 2023 at 11:51:02AM +0100, Tvrtko Ursulin wrote:
> On 26/10/2023 11:36, Andi Shyti wrote:
> > > On 26/10/2023 11:22, Jani Nikula wrote:
> > > > On Wed, 25 Oct 2023, Andi Shyti <andi.shyti at linux.intel.com> wrote:
> > > > > On Wed, Oct 25, 2023 at 11:20:25AM +0100, Tvrtko Ursulin wrote:
> > > > > > 
> > > > > > On 24/10/2023 13:42, Jani Nikula wrote:
> > > > > > > On Tue, 24 Oct 2023, Andi Shyti <andi.shyti at linux.intel.com> wrote:
> > > > > > > > Hi Jani,
> > > > > > > > 
> > > > > > > > On Mon, Oct 23, 2023 at 06:02:55PM +0300, Jani Nikula wrote:
> > > > > > > > > It's tedious to duplicate the container_of() everywhere. Add a helper.
> > > > > > > > > 
> > > > > > > > > Also do the logical steps of first getting from struct perf_event to
> > > > > > > > > struct i915_pmu, and then from struct i915_pmu to struct
> > > > > > > > > drm_i915_private if needed, instead of perf_event->i915->pmu. Not all
> > > > > > > > > places even need the i915 pointer.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> > > > > > > > > ---
> > > > > > > > >     drivers/gpu/drm/i915/i915_pmu.c | 45 +++++++++++++++------------------
> > > > > > > > >     1 file changed, 20 insertions(+), 25 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > > > > > > > > index dcae2fcd8d36..d45b40ec6d47 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > > > > > > > > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > > > > > > > > @@ -31,6 +31,11 @@
> > > > > > > > >     static cpumask_t i915_pmu_cpumask;
> > > > > > > > >     static unsigned int i915_pmu_target_cpu = -1;
> > > > > > > > > +static struct i915_pmu *event_to_pmu(struct perf_event *event)
> > > > > > > > 
> > > > > > > > I would call it perfevent (or perf_event), event is too generic.
> > > > > > > > We have other kind of events, too.
> > > > > > > 
> > > > > > > Fair enough.
> > > > > > 
> > > > > > Counter argument is that i915_pmu.c consistently names this event (which is
> > > > > > likely lifted from other PMU drivers) so is the proposal to churn it all, or
> > > > > > create an inconsistency?
> > > > > 
> > > > > The first that comes to my mind is that the debugger is also
> > > > > using the term "event"... on the other hand there is no debugger
> > > > > in i915.
> > > > 
> > > > Have you settled on this? I don't care either way, could apply either
> > > > patch.
> > 
> > no... unfortunately not...
> 
> :(
> 
> $ grep "struct perf_event \*event" . -r | wc -l
> 1912
> $ grep "struct perf_event \*perf_event" . -r | wc -l
> 5
> 
> ;)

with "I haven't settled on this", I meant that the debugger has
not been posted upstream for i915 and it won't be. It's going to
go in the XE driver.

> Now seriously, I don't mind perf_event, as long as _whole_ i915_pmu.c is
> switched over. At which point I questioned would the churn be worth it.

I like Jani's patch, of course your grep search concludes the
the discussion, so that I'm not going to argue agains "event"
as name :-)

Acked-by: Andi Shyti <andi.shyti at linux.intel.com>

Andi