Re: [Intel-gfx] [PATCH] drm/i915/rps: Centralize computation of freq caps
> -Original Message-
> From: Dixit, Ashutosh
> Sent: Thursday, April 7, 2022 12:49 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Gupta, Anshuman
> Subject: [PATCH] drm/i915/rps: Centralize computation of freq caps
>
> Freq caps (i.e. RP0, RP1 and RPn frequencies) are read from HW. However the
> formats (bit positions, widths, registers and units) of these vary for
> different
> generations with even more variations arriving in the future. In order not to
> have to do identical computation for these caps in multiple places, here we
> centralize the computation of these caps. This makes the code cleaner and also
> more extensible for the future.
>
> v2: Clarify that caps are in "hw units" in comments (Lucas De Marchi)
> v3: Minor checkpatch fix
> v4: s/intel_rps_get_freq_caps/gen6_rps_get_freq_caps/ (Badal Nilawar)
> v5: Changes comments to kernel doc (Anshuman Gupta)
Acked-by: Anshuman Gupta
>
> Cc: Anshuman Gupta
> Signed-off-by: Ashutosh Dixit
> Reviewed-by: Badal Nilawar
> ---
> drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 24 +---
> drivers/gpu/drm/i915/gt/intel_rps.c | 108 +++---
> drivers/gpu/drm/i915/gt/intel_rps.h | 2 +-
> drivers/gpu/drm/i915/gt/intel_rps_types.h | 15 +++
> drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 14 +--
> 5 files changed, 91 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> index 003a53c49c86..0c6b9eb724ae 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> @@ -342,17 +342,16 @@ void intel_gt_pm_frequency_dump(struct intel_gt
> *gt, struct drm_printer *p)
> } else if (GRAPHICS_VER(i915) >= 6) {
> u32 rp_state_limits;
> u32 gt_perf_status;
> - u32 rp_state_cap;
> + struct intel_rps_freq_caps caps;
> u32 rpmodectl, rpinclimit, rpdeclimit;
> u32 rpstat, cagf, reqf;
> u32 rpcurupei, rpcurup, rpprevup;
> u32 rpcurdownei, rpcurdown, rpprevdown;
> u32 rpupei, rpupt, rpdownei, rpdownt;
> u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
> - int max_freq;
>
> rp_state_limits = intel_uncore_read(uncore,
> GEN6_RP_STATE_LIMITS);
> - rp_state_cap = intel_rps_read_state_cap(rps);
> + gen6_rps_get_freq_caps(rps, );
> if (IS_GEN9_LP(i915))
> gt_perf_status = intel_uncore_read(uncore,
> BXT_GT_PERF_STATUS);
> else
> @@ -474,25 +473,12 @@ void intel_gt_pm_frequency_dump(struct intel_gt
> *gt, struct drm_printer *p)
> drm_printf(p, "RP DOWN THRESHOLD: %d (%lldns)\n",
> rpdownt, intel_gt_pm_interval_to_ns(gt, rpdownt));
>
> - max_freq = (IS_GEN9_LP(i915) ? rp_state_cap >> 0 :
> - rp_state_cap >> 16) & 0xff;
> - max_freq *= (IS_GEN9_BC(i915) ||
> - GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER :
> 1);
> drm_printf(p, "Lowest (RPN) frequency: %dMHz\n",
> -intel_gpu_freq(rps, max_freq));
> -
> - max_freq = (rp_state_cap & 0xff00) >> 8;
> - max_freq *= (IS_GEN9_BC(i915) ||
> - GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER :
> 1);
> +intel_gpu_freq(rps, caps.min_freq));
> drm_printf(p, "Nominal (RP1) frequency: %dMHz\n",
> -intel_gpu_freq(rps, max_freq));
> -
> - max_freq = (IS_GEN9_LP(i915) ? rp_state_cap >> 16 :
> - rp_state_cap >> 0) & 0xff;
> - max_freq *= (IS_GEN9_BC(i915) ||
> - GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER :
> 1);
> +intel_gpu_freq(rps, caps.rp1_freq));
> drm_printf(p, "Max non-overclocked (RP0) frequency:
> %dMHz\n",
> -intel_gpu_freq(rps, max_freq));
> +intel_gpu_freq(rps, caps.rp0_freq));
> drm_printf(p, "Max overclocked frequency: %dMHz\n",
> intel_gpu_freq(rps, rps->max_freq));
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
> b/drivers/gpu/drm/i915/gt/intel_rps.c
> index 6c9fdf7906c5..3476a11f294c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -1070,24 +1070,67 @@ int intel_rps_set(struct intel_rps *rps, u8 val)
> return 0;
> }
>
> -static void gen6_rps_init(struct intel_rps *rps)
> +static u32 intel_rps_read_state_cap(struct intel_rps *rps)
> {
> struct drm_i915_private *i915 = rps_to_i915(rps);
> - u32 rp_state_cap = intel_rps_read_state_cap(rps);
> + struct intel_uncore *uncore = rps_to_uncore(rps);
>
> - /* All of these values are in units of 50MHz */
> + if
Re: [Intel-gfx] [PATCH] drm/i915/rps: Centralize computation of freq caps
On Wed, 06 Apr 2022 03:09:45 -0700, Anshuman Gupta wrote: > On 2022-03-24 at 01:24:35 +0530, Ashutosh Dixit wrote: > > +/* "Caps" frequencies should be converted to MHz using intel_gpu_freq() */ > IMHO, if this exported function deserves a comment, it should Kernel Doc > comment. > for an example see Doc comment of intel_runtime_pm_get_raw(). > Thanks, > Anshuman Gupta. > > +void gen6_rps_get_freq_caps(struct intel_rps *rps, struct > > intel_rps_freq_caps *caps) I have changed the comments to kernel doc for both gen6_rps_get_freq_caps() and 'struct intel_rps_freq_caps' in v5 (v6 in Patchwork). Please take a look. Thanks.
Re: [Intel-gfx] [PATCH] drm/i915/rps: Centralize computation of freq caps
On 2022-03-24 at 01:24:35 +0530, Ashutosh Dixit wrote:
> Freq caps (i.e. RP0, RP1 and RPn frequencies) are read from HW. However the
> formats (bit positions, widths, registers and units) of these vary for
> different generations with even more variations arriving in the future. In
> order not to have to do identical computation for these caps in multiple
> places, here we centralize the computation of these caps. This makes the
> code cleaner and also more extensible for the future.
>
> v2: Clarify that caps are in "hw units" in comments (Lucas De Marchi)
> v3: Minor checkpatch fix
> v4: s/intel_rps_get_freq_caps/gen6_rps_get_freq_caps/ (Badal Nilawar)
>
> Signed-off-by: Ashutosh Dixit
> ---
> drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 24 +
> drivers/gpu/drm/i915/gt/intel_rps.c | 101 ++
> drivers/gpu/drm/i915/gt/intel_rps.h | 2 +-
> drivers/gpu/drm/i915/gt/intel_rps_types.h | 10 ++
> drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 14 +--
> 5 files changed, 79 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> index 31dbb2b96738..280a27cb9bdf 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> @@ -342,17 +342,16 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
> struct drm_printer *p)
> } else if (GRAPHICS_VER(i915) >= 6) {
> u32 rp_state_limits;
> u32 gt_perf_status;
> - u32 rp_state_cap;
> + struct intel_rps_freq_caps caps;
> u32 rpmodectl, rpinclimit, rpdeclimit;
> u32 rpstat, cagf, reqf;
> u32 rpcurupei, rpcurup, rpprevup;
> u32 rpcurdownei, rpcurdown, rpprevdown;
> u32 rpupei, rpupt, rpdownei, rpdownt;
> u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
> - int max_freq;
>
> rp_state_limits = intel_uncore_read(uncore,
> GEN6_RP_STATE_LIMITS);
> - rp_state_cap = intel_rps_read_state_cap(rps);
> + gen6_rps_get_freq_caps(rps, );
> if (IS_GEN9_LP(i915))
> gt_perf_status = intel_uncore_read(uncore,
> BXT_GT_PERF_STATUS);
> else
> @@ -474,25 +473,12 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
> struct drm_printer *p)
> drm_printf(p, "RP DOWN THRESHOLD: %d (%lldns)\n",
> rpdownt, intel_gt_pm_interval_to_ns(gt, rpdownt));
>
> - max_freq = (IS_GEN9_LP(i915) ? rp_state_cap >> 0 :
> - rp_state_cap >> 16) & 0xff;
> - max_freq *= (IS_GEN9_BC(i915) ||
> - GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER : 1);
> drm_printf(p, "Lowest (RPN) frequency: %dMHz\n",
> -intel_gpu_freq(rps, max_freq));
> -
> - max_freq = (rp_state_cap & 0xff00) >> 8;
> - max_freq *= (IS_GEN9_BC(i915) ||
> - GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER : 1);
> +intel_gpu_freq(rps, caps.min_freq));
> drm_printf(p, "Nominal (RP1) frequency: %dMHz\n",
> -intel_gpu_freq(rps, max_freq));
> -
> - max_freq = (IS_GEN9_LP(i915) ? rp_state_cap >> 16 :
> - rp_state_cap >> 0) & 0xff;
> - max_freq *= (IS_GEN9_BC(i915) ||
> - GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER : 1);
> +intel_gpu_freq(rps, caps.rp1_freq));
> drm_printf(p, "Max non-overclocked (RP0) frequency: %dMHz\n",
> -intel_gpu_freq(rps, max_freq));
> +intel_gpu_freq(rps, caps.rp0_freq));
> drm_printf(p, "Max overclocked frequency: %dMHz\n",
> intel_gpu_freq(rps, rps->max_freq));
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
> b/drivers/gpu/drm/i915/gt/intel_rps.c
> index 6c9fdf7906c5..f21f6e454998 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -1070,23 +1070,59 @@ int intel_rps_set(struct intel_rps *rps, u8 val)
> return 0;
> }
>
> -static void gen6_rps_init(struct intel_rps *rps)
> +static u32 intel_rps_read_state_cap(struct intel_rps *rps)
> {
> struct drm_i915_private *i915 = rps_to_i915(rps);
> - u32 rp_state_cap = intel_rps_read_state_cap(rps);
> + struct intel_uncore *uncore = rps_to_uncore(rps);
>
> - /* All of these values are in units of 50MHz */
> + if (IS_XEHPSDV(i915))
> + return intel_uncore_read(uncore, XEHPSDV_RP_STATE_CAP);
> + else if (IS_GEN9_LP(i915))
> + return intel_uncore_read(uncore, BXT_RP_STATE_CAP);
> + else
> + return intel_uncore_read(uncore, GEN6_RP_STATE_CAP);
> +}
> +
> +/* "Caps" frequencies should
Re: [Intel-gfx] [PATCH] drm/i915/rps: Centralize computation of freq caps
On 24-03-2022 01:24, Ashutosh Dixit wrote:
Freq caps (i.e. RP0, RP1 and RPn frequencies) are read from HW. However the
formats (bit positions, widths, registers and units) of these vary for
different generations with even more variations arriving in the future. In
order not to have to do identical computation for these caps in multiple
places, here we centralize the computation of these caps. This makes the
code cleaner and also more extensible for the future.
v2: Clarify that caps are in "hw units" in comments (Lucas De Marchi)
v3: Minor checkpatch fix
v4: s/intel_rps_get_freq_caps/gen6_rps_get_freq_caps/ (Badal Nilawar)
Signed-off-by: Ashutosh Dixit
This looks ok to me.
Reviewed-by: Badal Nilawar
---
drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 24 +
drivers/gpu/drm/i915/gt/intel_rps.c | 101 ++
drivers/gpu/drm/i915/gt/intel_rps.h | 2 +-
drivers/gpu/drm/i915/gt/intel_rps_types.h | 10 ++
drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 14 +--
5 files changed, 79 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
index 31dbb2b96738..280a27cb9bdf 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
@@ -342,17 +342,16 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
struct drm_printer *p)
} else if (GRAPHICS_VER(i915) >= 6) {
u32 rp_state_limits;
u32 gt_perf_status;
- u32 rp_state_cap;
+ struct intel_rps_freq_caps caps;
u32 rpmodectl, rpinclimit, rpdeclimit;
u32 rpstat, cagf, reqf;
u32 rpcurupei, rpcurup, rpprevup;
u32 rpcurdownei, rpcurdown, rpprevdown;
u32 rpupei, rpupt, rpdownei, rpdownt;
u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
- int max_freq;
rp_state_limits = intel_uncore_read(uncore, GEN6_RP_STATE_LIMITS);
- rp_state_cap = intel_rps_read_state_cap(rps);
+ gen6_rps_get_freq_caps(rps, );
if (IS_GEN9_LP(i915))
gt_perf_status = intel_uncore_read(uncore,
BXT_GT_PERF_STATUS);
else
@@ -474,25 +473,12 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
struct drm_printer *p)
drm_printf(p, "RP DOWN THRESHOLD: %d (%lldns)\n",
rpdownt, intel_gt_pm_interval_to_ns(gt, rpdownt));
- max_freq = (IS_GEN9_LP(i915) ? rp_state_cap >> 0 :
- rp_state_cap >> 16) & 0xff;
- max_freq *= (IS_GEN9_BC(i915) ||
-GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER : 1);
drm_printf(p, "Lowest (RPN) frequency: %dMHz\n",
- intel_gpu_freq(rps, max_freq));
-
- max_freq = (rp_state_cap & 0xff00) >> 8;
- max_freq *= (IS_GEN9_BC(i915) ||
-GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER : 1);
+ intel_gpu_freq(rps, caps.min_freq));
drm_printf(p, "Nominal (RP1) frequency: %dMHz\n",
- intel_gpu_freq(rps, max_freq));
-
- max_freq = (IS_GEN9_LP(i915) ? rp_state_cap >> 16 :
- rp_state_cap >> 0) & 0xff;
- max_freq *= (IS_GEN9_BC(i915) ||
-GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER : 1);
+ intel_gpu_freq(rps, caps.rp1_freq));
drm_printf(p, "Max non-overclocked (RP0) frequency: %dMHz\n",
- intel_gpu_freq(rps, max_freq));
+ intel_gpu_freq(rps, caps.rp0_freq));
drm_printf(p, "Max overclocked frequency: %dMHz\n",
intel_gpu_freq(rps, rps->max_freq));
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index 6c9fdf7906c5..f21f6e454998 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -1070,23 +1070,59 @@ int intel_rps_set(struct intel_rps *rps, u8 val)
return 0;
}
-static void gen6_rps_init(struct intel_rps *rps)
+static u32 intel_rps_read_state_cap(struct intel_rps *rps)
{
struct drm_i915_private *i915 = rps_to_i915(rps);
- u32 rp_state_cap = intel_rps_read_state_cap(rps);
+ struct intel_uncore *uncore = rps_to_uncore(rps);
- /* All of these values are in units of 50MHz */
+ if (IS_XEHPSDV(i915))
+ return intel_uncore_read(uncore, XEHPSDV_RP_STATE_CAP);
+ else if (IS_GEN9_LP(i915))
+ return intel_uncore_read(uncore, BXT_RP_STATE_CAP);
+ else
+ return intel_uncore_read(uncore, GEN6_RP_STATE_CAP);
+}
+
+/* "Caps" frequencies should be converted to MHz using intel_gpu_freq() */
+void
Re: [Intel-gfx] [PATCH] drm/i915/rps: Centralize computation of freq caps
On Wed, 23 Mar 2022 00:03:23 -0700, Nilawar, Badal wrote: > > > +/* "Caps" frequencies should be converted to MHz using intel_gpu_freq() */ > > +void intel_rps_get_freq_caps(struct intel_rps *rps, struct > > intel_rps_freq_caps *capSis) > > Since this function is covering gen6 and above it would be good to rename > it as gen6_rps_get_freq_caps. I've made this change in v4. Thanks.
Re: [Intel-gfx] [PATCH] drm/i915/rps: Centralize computation of freq caps
On 23-03-2022 00:26, Ashutosh Dixit wrote:
Freq caps (i.e. RP0, RP1 and RPn frequencies) are read from HW. However the
formats (bit positions, widths, registers and units) of these vary for
different generations with even more variations arriving in the future. In
order not to have to do identical computation for these caps in multiple
places, here we centralize the computation of these caps. This makes the
code cleaner and also more extensible for the future.
v2: Clarify that caps are in "hw units" in comments (Lucas De Marchi)
Signed-off-by: Ashutosh Dixit
---
drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 24 +
drivers/gpu/drm/i915/gt/intel_rps.c | 101 ++
drivers/gpu/drm/i915/gt/intel_rps.h | 2 +-
drivers/gpu/drm/i915/gt/intel_rps_types.h | 10 ++
drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 14 +--
5 files changed, 79 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
index 31dbb2b96738..f5fbb74ed076 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
@@ -342,17 +342,16 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
struct drm_printer *p)
} else if (GRAPHICS_VER(i915) >= 6) {
u32 rp_state_limits;
u32 gt_perf_status;
- u32 rp_state_cap;
+ struct intel_rps_freq_caps caps;
u32 rpmodectl, rpinclimit, rpdeclimit;
u32 rpstat, cagf, reqf;
u32 rpcurupei, rpcurup, rpprevup;
u32 rpcurdownei, rpcurdown, rpprevdown;
u32 rpupei, rpupt, rpdownei, rpdownt;
u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
- int max_freq;
rp_state_limits = intel_uncore_read(uncore, GEN6_RP_STATE_LIMITS);
- rp_state_cap = intel_rps_read_state_cap(rps);
+ intel_rps_get_freq_caps(rps, );
if (IS_GEN9_LP(i915))
gt_perf_status = intel_uncore_read(uncore,
BXT_GT_PERF_STATUS);
else
@@ -474,25 +473,12 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
struct drm_printer *p)
drm_printf(p, "RP DOWN THRESHOLD: %d (%lldns)\n",
rpdownt, intel_gt_pm_interval_to_ns(gt, rpdownt));
- max_freq = (IS_GEN9_LP(i915) ? rp_state_cap >> 0 :
- rp_state_cap >> 16) & 0xff;
- max_freq *= (IS_GEN9_BC(i915) ||
-GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER : 1);
drm_printf(p, "Lowest (RPN) frequency: %dMHz\n",
- intel_gpu_freq(rps, max_freq));
-
- max_freq = (rp_state_cap & 0xff00) >> 8;
- max_freq *= (IS_GEN9_BC(i915) ||
-GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER : 1);
+ intel_gpu_freq(rps, caps.min_freq));
drm_printf(p, "Nominal (RP1) frequency: %dMHz\n",
- intel_gpu_freq(rps, max_freq));
-
- max_freq = (IS_GEN9_LP(i915) ? rp_state_cap >> 16 :
- rp_state_cap >> 0) & 0xff;
- max_freq *= (IS_GEN9_BC(i915) ||
-GRAPHICS_VER(i915) >= 11 ? GEN9_FREQ_SCALER : 1);
+ intel_gpu_freq(rps, caps.rp1_freq));
drm_printf(p, "Max non-overclocked (RP0) frequency: %dMHz\n",
- intel_gpu_freq(rps, max_freq));
+ intel_gpu_freq(rps, caps.rp0_freq));
drm_printf(p, "Max overclocked frequency: %dMHz\n",
intel_gpu_freq(rps, rps->max_freq));
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index 6c9fdf7906c5..4528da9db590 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -1070,23 +1070,59 @@ int intel_rps_set(struct intel_rps *rps, u8 val)
return 0;
}
-static void gen6_rps_init(struct intel_rps *rps)
+static u32 intel_rps_read_state_cap(struct intel_rps *rps)
{
struct drm_i915_private *i915 = rps_to_i915(rps);
- u32 rp_state_cap = intel_rps_read_state_cap(rps);
+ struct intel_uncore *uncore = rps_to_uncore(rps);
- /* All of these values are in units of 50MHz */
+ if (IS_XEHPSDV(i915))
+ return intel_uncore_read(uncore, XEHPSDV_RP_STATE_CAP);
+ else if (IS_GEN9_LP(i915))
+ return intel_uncore_read(uncore, BXT_RP_STATE_CAP);
+ else
+ return intel_uncore_read(uncore, GEN6_RP_STATE_CAP);
+}
+
+/* "Caps" frequencies should be converted to MHz using intel_gpu_freq() */
+void intel_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps
*capSis)
Since this function is covering gen6 and above it would be good to
rename it as
Re: [Intel-gfx] [PATCH] drm/i915/rps: Centralize computation of freq caps
On Mon, 21 Mar 2022 11:17:46 -0700, Lucas De Marchi wrote:
>
> On Mon, Mar 21, 2022 at 10:56:04AM -0700, Ashutosh Dixit wrote:
> > diff --git a/drivers/gpu/drm/i915/gt/intel_rps_types.h
> > b/drivers/gpu/drm/i915/gt/intel_rps_types.h
> > index 3941d8551f52..5990df35b393 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_rps_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_rps_types.h
> > @@ -37,6 +37,13 @@ enum {
> > INTEL_RPS_TIMER,
> > };
> >
> > +/* Freq caps exposed by HW, in units of 16.67 MHz for recent gens */
>
> "recent gens" is probably too broad. Since we are exporting this struct
> to other parts of the driver and we are already abstracting the
> register location and bit position, maybe we should also already
> abstract the unit in the same place? i.e. just make it always be
> "units of 16.67 MHz", or even just make it a standard Hz unit.
>
> If this would rather make things more complicated for places that expect
> "hw units", then maybe just say in this comment the value is in "HW
> units" and intel_gpu_freq() should be used to convert to hz.
Fixed in v2. In v2 I've changed the comment to say values are in "hw units"
and intel_gpu_freq() should be used to convert to MHz.
I have also added a couple of hopefully clarifying comments to
intel_rps_get_freq_caps() in v2. Some of the history of this code was not
clear to me previously and I had to trace all the way back to cee991cb9323
to figure out what is happening.
Thanks.
--
Ashutosh
> > +struct intel_rps_freq_caps {
> > + u8 rp0_freq;/* Non-overclocked max frequency. */
> > + u8 rp1_freq;/* "less than" RP0 power/freqency */
> > + u8 min_freq;/* AKA RPn. Minimum frequency */
> > +};
> > +
> > struct intel_rps {
> > struct mutex lock; /* protects enabling and the worker */
> >
Re: [Intel-gfx] [PATCH] drm/i915/rps: Centralize computation of freq caps
On Mon, 21 Mar 2022 11:17:46 -0700, Lucas De Marchi wrote:
>
> On Mon, Mar 21, 2022 at 10:56:04AM -0700, Ashutosh Dixit wrote:
> > diff --git a/drivers/gpu/drm/i915/gt/intel_rps_types.h
> > b/drivers/gpu/drm/i915/gt/intel_rps_types.h
> > index 3941d8551f52..5990df35b393 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_rps_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_rps_types.h
> > @@ -37,6 +37,13 @@ enum {
> > INTEL_RPS_TIMER,
> > };
> >
> > +/* Freq caps exposed by HW, in units of 16.67 MHz for recent gens */
>
> "recent gens" is probably too broad. Since we are exporting this struct
> to other parts of the driver and we are already abstracting the
> register location and bit position, maybe we should also already
> abstract the unit in the same place? i.e. just make it always be
> "units of 16.67 MHz", or even just make it a standard Hz unit.
>
> If this would rather make things more complicated for places that expect
> "hw units", then maybe just say in this comment the value is in "HW
> units" and intel_gpu_freq() should be used to convert to hz.
OK, let me investigate some more, see what's possible and then get back
regarding this. Thanks for reviewing.
Re: [Intel-gfx] [PATCH] drm/i915/rps: Centralize computation of freq caps
On Mon, Mar 21, 2022 at 10:56:04AM -0700, Ashutosh Dixit wrote:
diff --git a/drivers/gpu/drm/i915/gt/intel_rps_types.h
b/drivers/gpu/drm/i915/gt/intel_rps_types.h
index 3941d8551f52..5990df35b393 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_rps_types.h
@@ -37,6 +37,13 @@ enum {
INTEL_RPS_TIMER,
};
+/* Freq caps exposed by HW, in units of 16.67 MHz for recent gens */
"recent gens" is probably too broad. Since we are exporting this struct
to other parts of the driver and we are already abstracting the
register location and bit position, maybe we should also already
abstract the unit in the same place? i.e. just make it always be
"units of 16.67 MHz", or even just make it a standard Hz unit.
If this would rather make things more complicated for places that expect
"hw units", then maybe just say in this comment the value is in "HW
units" and intel_gpu_freq() should be used to convert to hz.
thanks
Lucas De Marchi
+struct intel_rps_freq_caps {
+ u8 rp0_freq;/* Non-overclocked max frequency. */
+ u8 rp1_freq;/* "less than" RP0 power/freqency */
+ u8 min_freq;/* AKA RPn. Minimum frequency */
+};
+
struct intel_rps {
struct mutex lock; /* protects enabling and the worker */
