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

[Lucas De Marchi]

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 at 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

__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 at intel.com>

let me know what you think about the rename above and I will keep your
r-b.

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
>>