[PATCH] platform/x86/intel-uncore-freq: Do not present separate package-die domain

[PATCH] platform/x86/intel-uncore-freq: Do not present separate package-die domain

[Srinivas Pandruvada]

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

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();
-- 
2.39.3

Re: [PATCH] platform/x86/intel-uncore-freq: Do not present separate package-die domain

[Ilpo Järvinen]

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.

-- 
 i.


> 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(-)
> 
> 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();
> 

Re: [PATCH] platform/x86/intel-uncore-freq: Do not present separate package-die domain

[srinivas pandruvada]

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

Re: [PATCH] platform/x86/intel-uncore-freq: Do not present separate package-die domain

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2638 bytes --]

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

-- 
 i.

Re: [PATCH] platform/x86/intel-uncore-freq: Do not present separate package-die domain

[srinivas pandruvada]

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

>