Re: [Intel-gfx] [PATCH 9/9] drm/i915/pmu: Enable legacy PMU events for MTL

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Fri, Mar 31, 2023 at 02:02:40PM +0100, Tvrtko Ursulin wrote:
>
>On 30/03/2023 19:31, Umesh Nerlige Ramappa wrote:
>>+ Joonas for comments on this
>>
>>On Thu, Mar 30, 2023 at 02:38:03PM +0100, Tvrtko Ursulin wrote:
>>>
>>>On 30/03/2023 01:41, Umesh Nerlige Ramappa wrote:
>>>>MTL introduces separate GTs for render and media. This complicates the
>>>>definition of frequency and rc6 counters for the GPU as a whole since
>>>>each GT has an independent counter. The best way to support this change
>>>>is to deprecate the GPU-specific counters and create GT-specific
>>>>counters, however that just breaks ABI. Since perf tools and scripts may
>>>>be decentralized with probably many users, it's hard to deprecate the
>>>>legacy counters and have all the users on board with that.
>>>>
>>>>Re-introduce the legacy counters and support them as min/max of
>>>>GT-specific counters as necessary to ensure backwards compatibility.
>>>>
>>>>I915_PMU_ACTUAL_FREQUENCY - will show max of GT-specific counters
>>>>I915_PMU_REQUESTED_FREQUENCY - will show max of GT-specific counters
>>>>I915_PMU_INTERRUPTS - no changes since it is GPU specific on all 
>>>>platforms
>>>>I915_PMU_RC6_RESIDENCY - will show min of GT-specific counters
>>>>I915_PMU_SOFTWARE_GT_AWAKE_TIME - will show max of GT-specific counters
>>>
>>>IMO max/min games are _very_ low value and probably just confusing.
>>
>>By value, do you mean ROI or actually that the values would be incorrect?
>
>Both really.
>
>>>I am not convinced we need to burden the kernel with this. New 
>>>platform, new counters.. userspace can just deal with it.
>>
>>I agree and would prefer to drop this patch. There are some counter 
>>arguments, I have added Joonas here for comments.
>>
>>1) an app/script hard-coded with the legacy events would be used on 
>>a new platform and fail and we should maintain backwards 
>>compatibility.
>
>I thought we pretty much agreed multiple times in the past (on 
>different topics) that a new platform can require new userspace.
>
>PMU is probably even a more clear cut case since it is exposing 
>hardware counters (or close) so sometimes it is not even theoretically 
>possible to preserve "backward" compatibility.
>
>(I double quote backward because I think real backward compatibility 
>does not apply on a new platform. And MTL is under force probe still.)
>
>So for me it all comes under the "would be nice" category. But since 
>we need to add kernel code to do it, code which asy intel_gpu_top 
>could run in userspace, I am not at all convinced it wouldn't be a bad 
>idea.
>
>The aggregated counters wouldn't even be giving the full picture.
>
>So I'd simply add tiles/gt support to intel_gpu_top. Same as it 
>currently can do "-p" on the command line, or '1' in the interactive 
>mode, to aggregate the engine classes into one line item, I'd extend 
>that concept into frequencies and RC6.
>
>By default we start with normalized values and in physical mode we 
>show separate counters per tile/gt.
>
>Someone running old intel_gpu_top on MTL gets to see nothing since the 
>counter names are different. Which is IMO fine - better than showing 
>tile 0 data, or some minimums/maximums from one tile only.
>
>>2) the sysfs attributes for rc6/frequency have already adopted an 
>>aggregate vs gt0/gt1 approach to address that and pmu should have a 
>>similar solution (or rather, PMU and the sysfs approaches should 
>>match based on whatever is the approach)
>
>Yeah I disagreed with min/max reads in sysfs too and am pretty sure I 
>expressed that at the time. :shrug:
>
>But I don't think there is a strong argument that PMU needs to follow.
>
>Only impact is to people who access perf_event_open directly so yeah, 
>if there are such users, they will need to add multi-tile support.

I discussed with Joonas offline and I guess I had a wrong idea regarding 
ABI. Looks like ABI is only broken if we remove something that existed 
for a platform, so this does not break ABI for MTL. The motivation was 
to have per-platform differences smaller for applications/UMD.

The other aspect he pointed out was that we should not push anything 
that does not have an IGT. For aggregate events, we do not have any 
plans to support IGTs (large effort as well as no clear way to support 
such aggregation).

In short, I will drop this patch and post what you originally had for 
multi-gt PMU support.

Thanks,
Umesh
>
>Regards,
>
>Tvrtko