| Message ID | 20240731185756.1853197-1-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86/intel-uncore-freq: Do not present separate package-die domain | expand |
On Wed, 31 Jul 2024, Srinivas Pandruvada wrote: > The scope of uncore control is per power domain in a package and die. > A package-die can have multiple power domains on some processors. In this > case package-die domain (root domain) aggregates all information from > power domains in it. > > On some processors, CPUID enumerates the die number same as power domain > ID. In this case there is one to one relationship between package-die and > power domain ID. There is no use of aggregating information from all > power domain IDs as the information will be duplicate and confusing. In > this case do not create separate package-die domain. Hi Srinivas, I got confused by this changelog because its order is quite illogical. First paragraph talks about case A. When you say "all information" is "aggregated", I immediately make the assumption that the aggregated information is what is wanted because, well, you normally want "all information" and nothing else is being told here. Second paragraph starts to talk about case B and then suddenly switches to talk what should have been done in case A (that aggregated information is useless/confusing). So I think some reorganization of the sentences would be useful to not jump between cases mid-paragraph without any hints. (I hope my explanation is enough to highlight why it was hard to follow). When I finally understood what the changelog was saying, I found out the code change is fine too but it first looked like it was doing exactly the opposite to my expectations/reasonale given in the changelog.
Hi Ilpo, On Mon, 2024-08-12 at 14:16 +0300, Ilpo Järvinen wrote: > On Wed, 31 Jul 2024, Srinivas Pandruvada wrote: > > > The scope of uncore control is per power domain in a package and > > die. > > A package-die can have multiple power domains on some processors. > > In this > > case package-die domain (root domain) aggregates all information > > from > > power domains in it. > > > > On some processors, CPUID enumerates the die number same as power > > domain > > ID. In this case there is one to one relationship between package- > > die and > > power domain ID. There is no use of aggregating information from > > all > > power domain IDs as the information will be duplicate and > > confusing. In > > this case do not create separate package-die domain. > > Hi Srinivas, > > I got confused by this changelog because its order is quite > illogical. > > First paragraph talks about case A. When you say "all information" > is "aggregated", I immediately make the assumption that the > aggregated > information is what is wanted because, well, you normally want "all > information" and nothing else is being told here. > > Second paragraph starts to talk about case B and then suddenly > switches to > talk what should have been done in case A (that aggregated > information is > useless/confusing). > Is this any better: " The scope of uncore control is per power domain in a package and die with TPMI. There are two types of processor configurations possible: 1. A compute die is not enumerated in CPUID. In this case there is only one die in a package. In this case there will be multiple power domains in a single die. 2. A power domain in a package is enumerated as a compute die in CPUID. So there is one to one relationship between a die and power domain. To allow die level controls, the current implementation creates a root domain and aggregates all information from power domains in it. This is well suited for configuration 1 above. But when newer processors use configuration 2 above, this will present redundant information, So no use of aggregating. In this case do not create separate root domain. " Thanks, Srinivas > So I think some reorganization of the sentences would be useful to > not > jump between cases mid-paragraph without any hints. > > (I hope my explanation is enough to highlight why it was hard to > follow). > > When I finally understood what the changelog was saying, I found out > the > code change is fine too but it first looked like it was doing exactly > the > opposite to my expectations/reasonale given in the changelog. >
On Mon, 12 Aug 2024, srinivas pandruvada wrote: > On Mon, 2024-08-12 at 14:16 +0300, Ilpo Järvinen wrote: > > On Wed, 31 Jul 2024, Srinivas Pandruvada wrote: > > > > > The scope of uncore control is per power domain in a package and > > > die. > > > A package-die can have multiple power domains on some processors. > > > In this > > > case package-die domain (root domain) aggregates all information > > > from > > > power domains in it. > > > > > > On some processors, CPUID enumerates the die number same as power > > > domain > > > ID. In this case there is one to one relationship between package- > > > die and > > > power domain ID. There is no use of aggregating information from > > > all > > > power domain IDs as the information will be duplicate and > > > confusing. In > > > this case do not create separate package-die domain. > > > > Hi Srinivas, > > > > I got confused by this changelog because its order is quite > > illogical. > > > > First paragraph talks about case A. When you say "all information" > > is "aggregated", I immediately make the assumption that the > > aggregated > > information is what is wanted because, well, you normally want "all > > information" and nothing else is being told here. > > > > Second paragraph starts to talk about case B and then suddenly > > switches to > > talk what should have been done in case A (that aggregated > > information is > > useless/confusing). > > > Is this any better: > > " > The scope of uncore control is per power domain in a package and die > with TPMI. > > There are two types of processor configurations possible: > 1. A compute die is not enumerated in CPUID. In this case there is only > one die in a package. In this case there will be multiple power domains > in a single die. > 2. A power domain in a package is enumerated as a compute die in CPUID. > So there is one to one relationship between a die and power domain. So there are multiple dies in a package and one to one relationship between a die and power domain. ? > > To allow die level controls, the current implementation creates a root > domain and aggregates all information from power domains in it. This > is well suited for configuration 1 above. > > But when newer processors use configuration 2 above, this will present > redundant information, So no use of aggregating. In this case do not > create separate root domain. > " Yes, it is now clearer. A minor suggestion above to better map with the code (explicitly stating the condition that matches to the check done by the code).
On Tue, 2024-08-13 at 10:51 +0300, Ilpo Järvinen wrote: > On Mon, 12 Aug 2024, srinivas pandruvada wrote: > > On Mon, 2024-08-12 at 14:16 +0300, Ilpo Järvinen wrote: > > > On Wed, 31 Jul 2024, Srinivas Pandruvada wrote: > > > > > > > The scope of uncore control is per power domain in a package > > > > and > > > > die. > > > > A package-die can have multiple power domains on some > > > > processors. > > > > In this > > > > case package-die domain (root domain) aggregates all > > > > information > > > > from > > > > power domains in it. > > > > > > > > On some processors, CPUID enumerates the die number same as > > > > power > > > > domain > > > > ID. In this case there is one to one relationship between > > > > package- > > > > die and > > > > power domain ID. There is no use of aggregating information > > > > from > > > > all > > > > power domain IDs as the information will be duplicate and > > > > confusing. In > > > > this case do not create separate package-die domain. > > > > > > Hi Srinivas, > > > > > > I got confused by this changelog because its order is quite > > > illogical. > > > > > > First paragraph talks about case A. When you say "all > > > information" > > > is "aggregated", I immediately make the assumption that the > > > aggregated > > > information is what is wanted because, well, you normally want > > > "all > > > information" and nothing else is being told here. > > > > > > Second paragraph starts to talk about case B and then suddenly > > > switches to > > > talk what should have been done in case A (that aggregated > > > information is > > > useless/confusing). > > > > > Is this any better: > > > > " > > The scope of uncore control is per power domain in a package and > > die > > with TPMI. > > > > There are two types of processor configurations possible: > > 1. A compute die is not enumerated in CPUID. In this case there is > > only > > one die in a package. In this case there will be multiple power > > domains > > in a single die. > > 2. A power domain in a package is enumerated as a compute die in > > CPUID. > > So there is one to one relationship between a die and power domain. > > So there are multiple dies in a package and one to one relationship > between a die and power domain. In case 2, yes. > > > > > To allow die level controls, the current implementation creates a > > root > > domain and aggregates all information from power domains in it. > > This > > is well suited for configuration 1 above. > > > > But when newer processors use configuration 2 above, this will > > present > > redundant information, So no use of aggregating. In this case do > > not > > create separate root domain. > > " > > Yes, it is now clearer. A minor suggestion above to better map with > the > code (explicitly stating the condition that matches to the check done > by the code). OK. Thanks, Srinivas >
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c index 9fa3037c03d1..6c2e607968f2 100644 --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c @@ -427,6 +427,9 @@ static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_ auxiliary_set_drvdata(auxdev, tpmi_uncore); + if (topology_max_dies_per_package() > 1) + return 0; + tpmi_uncore->root_cluster.root_domain = true; tpmi_uncore->root_cluster.uncore_root = tpmi_uncore; @@ -450,7 +453,9 @@ static void uncore_remove(struct auxiliary_device *auxdev) { struct tpmi_uncore_struct *tpmi_uncore = auxiliary_get_drvdata(auxdev); - uncore_freq_remove_die_entry(&tpmi_uncore->root_cluster.uncore_data); + if (tpmi_uncore->root_cluster.root_domain) + uncore_freq_remove_die_entry(&tpmi_uncore->root_cluster.uncore_data); + remove_cluster_entries(tpmi_uncore); uncore_freq_common_exit();
The scope of uncore control is per power domain in a package and die. A package-die can have multiple power domains on some processors. In this case package-die domain (root domain) aggregates all information from power domains in it. On some processors, CPUID enumerates the die number same as power domain ID. In this case there is one to one relationship between package-die and power domain ID. There is no use of aggregating information from all power domain IDs as the information will be duplicate and confusing. In this case do not create separate package-die domain. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- .../x86/intel/uncore-frequency/uncore-frequency-tpmi.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)