Re: [PATCH v2] drm/i915/slpc: Use platform limits for min/max frequency

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10/13/2022 3:28 PM, Dixit, Ashutosh wrote:
On Thu, 13 Oct 2022 08:55:24 -0700, Vinay Belgaumkar wrote:
Hi Vinay,

GuC will set the min/max frequencies to theoretical max on
ATS-M. This will break kernel ABI, so limit min/max frequency
to RP0(platform max) instead.
Isn't what we are calling "theoretical max" or "RPmax" really just -1U
(0xFFFFFFFF)? Though I have heard this is not a max value but -1U indicates
FW default values unmodified by host SW, which would mean frequencies are
fully controlled by FW (min == max == -1U). But if this were the case I
don't know why this would be the case only for server, why doesn't FW set
these for clients too to indicate it is fully in control?
FW sets max to -1U for client products(we already pull it down to RP0). It additionally makes min=max for server parts.

So the question what does -1U actually represent? Is it the RPmax value or
does -1U represent "FW defaults"?

Also this concept of using -1U as "FW defaults" is present in Level0/OneAPI
(and likely in firmware) but we seem to have blocked in the i915 ABI.

I understand we may not be able to make such changes at present but this
provides some context for the review comments below.

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
index fdd895f73f9f..11613d373a49 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
@@ -263,6 +263,7 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc)

	slpc->max_freq_softlimit = 0;
	slpc->min_freq_softlimit = 0;
+	slpc->min_is_rpmax = false;

	slpc->boost_freq = 0;
	atomic_set(&slpc->num_waiters, 0);
@@ -588,6 +589,31 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc)
	return 0;
  }

+static bool is_slpc_min_freq_rpmax(struct intel_guc_slpc *slpc)
+{
+	int slpc_min_freq;
+
+	if (intel_guc_slpc_get_min_freq(slpc, &slpc_min_freq))
+		return false;
+
+	if (slpc_min_freq > slpc->rp0_freq)
or >=.
If what we are calling "rpmax" really -1U then why don't we just check for
-1U here?

	u32 slpc_min_freq;

	if (slpc_min_freq == -1U)
That'll work similarly too. Only time slpc_min_freq is greater than rp0 is for a server part.

+		return true;
+	else
+		return false;
+}
+
+static void update_server_min_softlimit(struct intel_guc_slpc *slpc)
+{
+	/* For server parts, SLPC min will be at RPMax.
+	 * Use min softlimit to clamp it to RP0 instead.
+	 */
+	if (is_slpc_min_freq_rpmax(slpc) &&
+	    !slpc->min_freq_softlimit) {
+		slpc->min_is_rpmax = true;
+		slpc->min_freq_softlimit = slpc->rp0_freq;
Isn't it safer to use a platform check such as IS_ATSM or IS_XEHPSDV (or
even #define IS_SERVER()) to set min freq to RP0 rather than this -1U value
from FW? What if -1U means "FW defaults" and FW starts setting this on
client products tomorrow?

We are not checking for -1 specifically, but only if FW has set min > RP0 as an indicator. Also, might be worth having IS_SERVER at some point if there are other places we need this info as well.


Also, we need to set gt->defaults.min_freq here.

yes, need to add that.

Thanks,

Vinay.


Thanks.
--
Ashutosh


+	}
+}
+
  static int slpc_use_fused_rp0(struct intel_guc_slpc *slpc)
  {
	/* Force SLPC to used platform rp0 */
@@ -647,6 +673,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)

	slpc_get_rp_values(slpc);

+	/* Handle the case where min=max=RPmax */
+	update_server_min_softlimit(slpc);
+
	/* Set SLPC max limit to RP0 */
	ret = slpc_use_fused_rp0(slpc);
	if (unlikely(ret)) {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
index 73d208123528..a6ef53b04e04 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
@@ -19,6 +19,9 @@ struct intel_guc_slpc {
	bool supported;
	bool selected;

+	/* Indicates this is a server part */
+	bool min_is_rpmax;
+
	/* platform frequency limits */
	u32 min_freq;
	u32 rp0_freq;
--
2.35.1




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux