Re: [PATCH V2 1/6] perf/x86/intel: Add Grand Ridge and Sierra Forest

[Liang, Kan]

On 2023-06-06 9:24 a.m., Peter Zijlstra wrote:
> On Tue, Jun 06, 2023 at 08:42:42AM -0400, Liang, Kan wrote:
>> Hi Peter,
>>
>> On 2023-05-22 7:30 a.m., kan.liang@xxxxxxxxxxxxxxx wrote:
>>> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>>
>>> The Grand Ridge and Sierra Forest are successors to Snow Ridge. They
>>> both have Crestmont core. From the core PMU's perspective, they are
>>> similar to the e-core of MTL. The only difference is the LBR event
>>> logging feature, which will be implemented in the following patches.
>>>
>>> Create a non-hybrid PMU setup for Grand Ridge and Sierra Forest.
>>>
>>> Reviewed-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>> ---
>>>
>>
>>
>> Gentle ping.
>>
>> Do you have any comments for the patch set?
>>
>> The patch set based on the perf/core branch which doesn't
>> include the latest fix, 90befef5a9e8 ("perf/x86: Fix missing sample size
>> update on AMD BRS").
>> https://lore.kernel.org/lkml/2f09023a-cccb-35df-da0a-d245ee5238be@xxxxxxxxxxxxxxx/
>>
>> Should I rebase it on the perf/urgent and send the V3?
>>
> 
> I can pull urgent into perf/core, but:

Thanks.

> 
>>> +	case INTEL_FAM6_GRANDRIDGE:
>>> +	case INTEL_FAM6_SIERRAFOREST_X:
>                         ^^^^^^^^^^^^^^^
> 
> Those are just plain wrong; please fix up the intel-family.h thing like
> suggested earlier in this thread.
>> And Tony, please no more of that platform name nonsense.. we want uarch
> names for a reason, so that enums like the above become something
> sensible like:
> 
> 	case INTEL_FAM6_ATOM_CRESTMONT:
> 	case INTEL_FAM6_ATOM_CRESTMONT_X:
> 
> and now it's super obvious why they're grouped.
> 
>>> +		pr_cont("Crestmont events, ");

The Sierra Forest should not be a platform name. I think it's the code
name of the processor.

The problem is that the uarch name doesn't work for the hybrid, since it
has different uarchs in the same processors. To make the naming rules
consistent among big core, atom, and hybrid, maybe we should use the
code name of the processor in intel-family.h.

I will propose a patch to update the rules of using the processor name.
I think we may want to have further discussion there.

Thanks,
Kan