Re: [PATCH v15 3/6] drm/xe/pmu: Extract xe_pmu_event_update()

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Jan 23, 2025 at 08:59:16AM -0600, Lucas De Marchi wrote:
> On Thu, Jan 23, 2025 at 05:20:09AM -0500, Rodrigo Vivi wrote:
> > On Wed, Jan 22, 2025 at 08:19:20PM -0800, Lucas De Marchi wrote:
> > > Like other pmu drivers, keep the update separate from the read so it can
> > > be called from other methods (like stop()) without side effects.
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_pmu.c | 24 +++++++++++++++---------
> > >  1 file changed, 15 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
> > > index df93ba96bdc1e..33598272db6aa 100644
> > > --- a/drivers/gpu/drm/xe/xe_pmu.c
> > > +++ b/drivers/gpu/drm/xe/xe_pmu.c
> > > @@ -117,18 +117,11 @@ static u64 __xe_pmu_event_read(struct perf_event *event)
> > >  	return val;
> > >  }
> > > 
> > > -static void xe_pmu_event_read(struct perf_event *event)
> > > +static void xe_pmu_event_update(struct perf_event *event)
> > >  {
> > > -	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
> > >  	struct hw_perf_event *hwc = &event->hw;
> > > -	struct xe_pmu *pmu = &xe->pmu;
> > >  	u64 prev, new;
> > > 
> > > -	if (!pmu->registered) {
> > > -		event->hw.state = PERF_HES_STOPPED;
> > > -		return;
> > > -	}
> > > -
> > >  	prev = local64_read(&hwc->prev_count);
> > >  	do {
> > >  		new = __xe_pmu_event_read(event);
> > 
> > I really have the feeling that the names are inverted, and that this
> > function should be the one actually called read, but I double
> > checked and everybody else is doing the same, so I guess I just need
> > more coffee...
> 
> yeah... that's because "read" here is used in 2 different contexts:
> 
> 1) as implementation of the perf_pmu functor. We are implementing the
> .event_read() so we call the function xe_pmu_event_read()
> 2) as the verb implying we are reading the HW since it's commonly used
> in xe, so we call the function __xe_pmu_event_read()
> 
> however the perf_pmu implies that the HW values are updated not only in
> the .read() call, but also in e.g. in the .stop() call. So we have
> 
> 	xe_pmu_event_stop
> 	  xe_pmu_event_update
> 	    __xe_pmu_event_read
> 
> and
> 
> 	xe_pmu_event_read
> 	  xe_pmu_event_update
> 	    __xe_pmu_event_read
> 

This kind of makes sense now, or at least explains my own confusion...

> __xe_pmu_event_read could be called event_hw_readout() or something like
> that if it helps clearing up the confusion. Thoughts?
> 
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> let me know what you think about the rename above and I will keep your
> r-b.

No no, let's continue with the style that is used everywhere...

> 
> thanks
> Lucas De Marchi
> 
> > 
> > > @@ -137,6 +130,19 @@ static void xe_pmu_event_read(struct perf_event *event)
> > >  	local64_add(new - prev, &event->count);
> > >  }
> > > 
> > > +static void xe_pmu_event_read(struct perf_event *event)
> > > +{
> > > +	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
> > > +	struct xe_pmu *pmu = &xe->pmu;
> > > +
> > > +	if (!pmu->registered) {
> > > +		event->hw.state = PERF_HES_STOPPED;
> > > +		return;
> > > +	}
> > > +
> > > +	xe_pmu_event_update(event);
> > > +}
> > > +
> > >  static void xe_pmu_enable(struct perf_event *event)
> > >  {
> > >  	/*
> > > @@ -166,7 +172,7 @@ static void xe_pmu_event_stop(struct perf_event *event, int flags)
> > > 
> > >  	if (pmu->registered)
> > >  		if (flags & PERF_EF_UPDATE)
> > > -			xe_pmu_event_read(event);
> > > +			xe_pmu_event_update(event);
> > > 
> > >  	event->hw.state = PERF_HES_STOPPED;
> > >  }
> > > --
> > > 2.48.0
> > >