* [Intel-gfx] [PATCH v1] drm/i915/gt: Add sysfs RAPL PL1 interface
@ 2022-11-03 12:37 Sujaritha Sundaresan
2022-11-03 19:17 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork
2022-11-09 1:07 ` [Intel-gfx] [PATCH v1] " Dixit, Ashutosh
0 siblings, 2 replies; 4+ messages in thread
From: Sujaritha Sundaresan @ 2022-11-03 12:37 UTC (permalink / raw)
To: intel-gfx
Adding the rapl_pl1_freq_mhz sysfs attribute.
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 20 ++++++++++
drivers/gpu/drm/i915/gt/intel_rps.c | 44 +++++++++++++++++++++
drivers/gpu/drm/i915/gt/intel_rps.h | 3 ++
drivers/gpu/drm/i915/i915_reg.h | 4 ++
4 files changed, 71 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
index 904160952369..e7f00ec252f8 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
@@ -496,6 +496,17 @@ static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
static const struct attribute * const gen6_rps_attrs[] = GEN6_RPS_ATTR;
static const struct attribute * const gen6_gt_attrs[] = GEN6_GT_ATTR;
+static ssize_t rapl_pl1_freq_mhz_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buff)
+{
+ struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
+ u32 rapl_pl1 = intel_rps_read_rapl_pl1_frequency(>->rps);
+
+ return sysfs_emit(buff, "%u\n", rapl_pl1);
+}
+
+
static ssize_t punit_req_freq_mhz_show(struct device *dev,
struct device_attribute *attr,
char *buff)
@@ -534,6 +545,7 @@ struct intel_gt_bool_throttle_attr attr_##sysfs_func__ = { \
.mask = mask__, \
}
+static DEVICE_ATTR_RO(rapl_pl1_freq_mhz);
static DEVICE_ATTR_RO(punit_req_freq_mhz);
static INTEL_GT_RPS_BOOL_ATTR_RO(throttle_reason_status, GT0_PERF_LIMIT_REASONS_MASK);
static INTEL_GT_RPS_BOOL_ATTR_RO(throttle_reason_pl1, POWER_LIMIT_1_MASK);
@@ -790,12 +802,20 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
if (!is_object_gt(kobj))
return;
+ ret = sysfs_create_file(kobj, &dev_attr_rapl_pl1_freq_mhz.attr);
+ if (ret)
+ drm_warn(>->i915->drm,
+ "failed to create gt%u rapl_pl1_freq_mhz sysfs(%pe)",
+ gt->info.id, ERR_PTR(ret));
+
+
ret = sysfs_create_file(kobj, &dev_attr_punit_req_freq_mhz.attr);
if (ret)
drm_warn(>->i915->drm,
"failed to create gt%u punit_req_freq_mhz sysfs (%pe)",
gt->info.id, ERR_PTR(ret));
+
if (i915_mmio_reg_valid(intel_gt_perf_limit_reasons_reg(gt))) {
ret = sysfs_create_files(kobj, throttle_reason_attrs);
if (ret)
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index 17b40b625e31..0e89b941e3be 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -9,6 +9,7 @@
#include "i915_drv.h"
#include "i915_irq.h"
+#include "i915_reg.h"
#include "intel_breadcrumbs.h"
#include "intel_gt.h"
#include "intel_gt_clock_utils.h"
@@ -2422,6 +2423,49 @@ bool rps_read_mask_mmio(struct intel_rps *rps,
return rps_read_mmio(rps, reg32) & mask;
}
+u32 intel_rps_read_rapl_pl1(struct intel_rps *rps)
+{
+ struct drm_i915_private *i915 = rps_to_i915(rps);
+ i915_reg_t rgadr;
+ u32 rapl_pl1;
+
+ if (IS_METEORLAKE(i915)) {
+ rgadr = MTL_RAPL_PL1_FREQ_LIMIT;
+ } else if (IS_XEHPSDV(i915)) {
+ rgadr = XEHPSDV_RAPL_PL1_FREQ_LIMIT;
+ } else {
+ MISSING_CASE(GRAPHICS_VER(i915));
+ rgadr = INVALID_MMIO_REG;
+ }
+
+ if (!i915_mmio_reg_valid(rgadr))
+ rapl_pl1 = 0;
+ else
+ rapl_pl1 = rps_read_mmio(rps, rgadr);
+
+ return rapl_pl1;
+}
+
+u32 intel_rps_get_rapl(struct intel_rps *rps, u32 rapl_pl1)
+{
+ struct drm_i915_private *i915 = rps_to_i915(rps);
+ u32 rapl = 0;
+
+ if (IS_METEORLAKE(i915) || IS_XEHPSDV(i915))
+ rapl = rapl_pl1 & RAPL_PL1_FREQ_LIMIT_MASK;
+ else
+ MISSING_CASE(GRAPHICS_VER(i915));
+
+ return rapl;
+}
+
+u32 intel_rps_read_rapl_pl1_frequency(struct intel_rps *rps)
+{
+ u32 rapl_freq = intel_rps_get_rapl(rps, intel_rps_read_rapl_pl1(rps));
+
+ return (rapl_freq >> 8) * GT_FREQUENCY_MULTIPLIER;
+}
+
/* External interface for intel_ips.ko */
static struct drm_i915_private __rcu *ips_mchdev;
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h
index 4509dfdc52e0..4adc6aaedba0 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.h
+++ b/drivers/gpu/drm/i915/gt/intel_rps.h
@@ -34,6 +34,7 @@ void intel_rps_mark_interactive(struct intel_rps *rps, bool interactive);
int intel_gpu_freq(struct intel_rps *rps, int val);
int intel_freq_opcode(struct intel_rps *rps, int val);
u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat1);
+u32 intel_rps_get_rapl(struct intel_rps *rps, u32 rapl_pl1);
u32 intel_rps_read_actual_frequency(struct intel_rps *rps);
u32 intel_rps_get_requested_frequency(struct intel_rps *rps);
u32 intel_rps_get_min_frequency(struct intel_rps *rps);
@@ -47,6 +48,8 @@ u32 intel_rps_get_rp1_frequency(struct intel_rps *rps);
u32 intel_rps_get_rpn_frequency(struct intel_rps *rps);
u32 intel_rps_read_punit_req(struct intel_rps *rps);
u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps);
+u32 intel_rps_read_rapl_pl1(struct intel_rps *rps);
+u32 intel_rps_read_rapl_pl1_frequency(struct intel_rps *rps);
void gen6_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps *caps);
void intel_rps_raise_unslice(struct intel_rps *rps);
void intel_rps_lower_unslice(struct intel_rps *rps);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5003a5ffbc6a..68ff98e27b8d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1818,6 +1818,10 @@
#define GT0_PERF_LIMIT_REASONS_LOG_MASK REG_GENMASK(31, 16)
#define MTL_MEDIA_PERF_LIMIT_REASONS _MMIO(0x138030)
+#define XEHPSDV_RAPL_PL1_FREQ_LIMIT _MMIO(0x250070)
+#define MTL_RAPL_PL1_FREQ_LIMIT _MMIO(0x281070)
+#define RAPL_PL1_FREQ_LIMIT_MASK 0xffff
+
#define CHV_CLK_CTL1 _MMIO(0x101100)
#define VLV_CLK_CTL2 _MMIO(0x101104)
#define CLK_CTL2_CZCOUNT_30NS_SHIFT 28
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/gt: Add sysfs RAPL PL1 interface
2022-11-03 12:37 [Intel-gfx] [PATCH v1] drm/i915/gt: Add sysfs RAPL PL1 interface Sujaritha Sundaresan
@ 2022-11-03 19:17 ` Patchwork
2022-11-09 1:07 ` [Intel-gfx] [PATCH v1] " Dixit, Ashutosh
1 sibling, 0 replies; 4+ messages in thread
From: Patchwork @ 2022-11-03 19:17 UTC (permalink / raw)
To: Sujaritha Sundaresan; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/gt: Add sysfs RAPL PL1 interface
URL : https://patchwork.freedesktop.org/series/110476/
State : failure
== Summary ==
Error: patch https://patchwork.freedesktop.org/api/1.0/series/110476/revisions/1/mbox/ not applied
Applying: drm/i915/gt: Add sysfs RAPL PL1 interface
Using index info to reconstruct a base tree...
M drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
M drivers/gpu/drm/i915/gt/intel_rps.c
M drivers/gpu/drm/i915/gt/intel_rps.h
M drivers/gpu/drm/i915/i915_reg.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_reg.h
Auto-merging drivers/gpu/drm/i915/gt/intel_rps.h
Auto-merging drivers/gpu/drm/i915/gt/intel_rps.c
Auto-merging drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 drm/i915/gt: Add sysfs RAPL PL1 interface
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915/gt: Add sysfs RAPL PL1 interface
2022-11-03 12:37 [Intel-gfx] [PATCH v1] drm/i915/gt: Add sysfs RAPL PL1 interface Sujaritha Sundaresan
2022-11-03 19:17 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork
@ 2022-11-09 1:07 ` Dixit, Ashutosh
2022-11-11 3:32 ` Sundaresan, Sujaritha
1 sibling, 1 reply; 4+ messages in thread
From: Dixit, Ashutosh @ 2022-11-09 1:07 UTC (permalink / raw)
To: Sujaritha Sundaresan; +Cc: intel-gfx
On Thu, 03 Nov 2022 05:37:23 -0700, Sujaritha Sundaresan wrote:
>
Hi Suja,
> Adding the rapl_pl1_freq_mhz sysfs attribute.
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 20 ++++++++++
> drivers/gpu/drm/i915/gt/intel_rps.c | 44 +++++++++++++++++++++
> drivers/gpu/drm/i915/gt/intel_rps.h | 3 ++
> drivers/gpu/drm/i915/i915_reg.h | 4 ++
> 4 files changed, 71 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> index 904160952369..e7f00ec252f8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> @@ -496,6 +496,17 @@ static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
> static const struct attribute * const gen6_rps_attrs[] = GEN6_RPS_ATTR;
> static const struct attribute * const gen6_gt_attrs[] = GEN6_GT_ATTR;
>
> +static ssize_t rapl_pl1_freq_mhz_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buff)
> +{
> + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> + u32 rapl_pl1 = intel_rps_read_rapl_pl1_frequency(>->rps);
> +
> + return sysfs_emit(buff, "%u\n", rapl_pl1);
> +}
> +
> +
> static ssize_t punit_req_freq_mhz_show(struct device *dev,
> struct device_attribute *attr,
> char *buff)
> @@ -534,6 +545,7 @@ struct intel_gt_bool_throttle_attr attr_##sysfs_func__ = { \
> .mask = mask__, \
> }
>
> +static DEVICE_ATTR_RO(rapl_pl1_freq_mhz);
> static DEVICE_ATTR_RO(punit_req_freq_mhz);
Is this patch against old code? Since this is now INTEL_GT_ATTR_RO. Yes the
build failed. So rapl_pl1_freq_mhz will need to follow punit_req_freq_mhz.
> static INTEL_GT_RPS_BOOL_ATTR_RO(throttle_reason_status, GT0_PERF_LIMIT_REASONS_MASK);
> static INTEL_GT_RPS_BOOL_ATTR_RO(throttle_reason_pl1, POWER_LIMIT_1_MASK);
> @@ -790,12 +802,20 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
> if (!is_object_gt(kobj))
> return;
>
> + ret = sysfs_create_file(kobj, &dev_attr_rapl_pl1_freq_mhz.attr);
The convention here is to create sysfs files only for platforms on which a
feature (in this case RAPL PL1 freq) is supported.
Also are we sure this is only available on MTL and XEHPSDV and not on DG2?
Since generally a feature appears first on a platform and then is available
for all successive products. If it's available on DG2 too then we can use
something like:
if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))
See GRAPHICS_VER_FULL for various platforms in i915_pci.c.
> + if (ret)
> + drm_warn(>->i915->drm,
> + "failed to create gt%u rapl_pl1_freq_mhz sysfs(%pe)",
> + gt->info.id, ERR_PTR(ret));
> +
> +
> ret = sysfs_create_file(kobj, &dev_attr_punit_req_freq_mhz.attr);
> if (ret)
> drm_warn(>->i915->drm,
> "failed to create gt%u punit_req_freq_mhz sysfs (%pe)",
> gt->info.id, ERR_PTR(ret));
>
> +
Remove empty line.
> if (i915_mmio_reg_valid(intel_gt_perf_limit_reasons_reg(gt))) {
> ret = sysfs_create_files(kobj, throttle_reason_attrs);
> if (ret)
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
> index 17b40b625e31..0e89b941e3be 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -9,6 +9,7 @@
>
> #include "i915_drv.h"
> #include "i915_irq.h"
> +#include "i915_reg.h"
Not needed, see below.
> #include "intel_breadcrumbs.h"
> #include "intel_gt.h"
> #include "intel_gt_clock_utils.h"
> @@ -2422,6 +2423,49 @@ bool rps_read_mask_mmio(struct intel_rps *rps,
> return rps_read_mmio(rps, reg32) & mask;
> }
>
> +u32 intel_rps_read_rapl_pl1(struct intel_rps *rps)
> +{
> + struct drm_i915_private *i915 = rps_to_i915(rps);
> + i915_reg_t rgadr;
> + u32 rapl_pl1;
> +
> + if (IS_METEORLAKE(i915)) {
> + rgadr = MTL_RAPL_PL1_FREQ_LIMIT;
> + } else if (IS_XEHPSDV(i915)) {
> + rgadr = XEHPSDV_RAPL_PL1_FREQ_LIMIT;
> + } else {
> + MISSING_CASE(GRAPHICS_VER(i915));
> + rgadr = INVALID_MMIO_REG;
No need for this, the sysfs file will only be visible for platforms on
which this is supported so this will never be hit.
> + }
> +
> + if (!i915_mmio_reg_valid(rgadr))
> + rapl_pl1 = 0;
No need for this either.
> + else
> + rapl_pl1 = rps_read_mmio(rps, rgadr);
> +
> + return rapl_pl1;
> +}
> +
> +u32 intel_rps_get_rapl(struct intel_rps *rps, u32 rapl_pl1)
> +{
> + struct drm_i915_private *i915 = rps_to_i915(rps);
> + u32 rapl = 0;
> +
> + if (IS_METEORLAKE(i915) || IS_XEHPSDV(i915))
> + rapl = rapl_pl1 & RAPL_PL1_FREQ_LIMIT_MASK;
> + else
> + MISSING_CASE(GRAPHICS_VER(i915));
No need for this either.
> +
> + return rapl;
> +}
> +
> +u32 intel_rps_read_rapl_pl1_frequency(struct intel_rps *rps)
> +{
> + u32 rapl_freq = intel_rps_get_rapl(rps, intel_rps_read_rapl_pl1(rps));
> +
> + return (rapl_freq >> 8) * GT_FREQUENCY_MULTIPLIER;
Use REG_FIELD_GET to extract the freq.
There doesn't seem any need to have 3 functions here, let's combine them
into a single function and use REG_FIELD_GET.
> +}
> +
> /* External interface for intel_ips.ko */
>
> static struct drm_i915_private __rcu *ips_mchdev;
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h
> index 4509dfdc52e0..4adc6aaedba0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.h
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.h
> @@ -34,6 +34,7 @@ void intel_rps_mark_interactive(struct intel_rps *rps, bool interactive);
> int intel_gpu_freq(struct intel_rps *rps, int val);
> int intel_freq_opcode(struct intel_rps *rps, int val);
> u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat1);
> +u32 intel_rps_get_rapl(struct intel_rps *rps, u32 rapl_pl1);
> u32 intel_rps_read_actual_frequency(struct intel_rps *rps);
> u32 intel_rps_get_requested_frequency(struct intel_rps *rps);
> u32 intel_rps_get_min_frequency(struct intel_rps *rps);
> @@ -47,6 +48,8 @@ u32 intel_rps_get_rp1_frequency(struct intel_rps *rps);
> u32 intel_rps_get_rpn_frequency(struct intel_rps *rps);
> u32 intel_rps_read_punit_req(struct intel_rps *rps);
> u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps);
> +u32 intel_rps_read_rapl_pl1(struct intel_rps *rps);
> +u32 intel_rps_read_rapl_pl1_frequency(struct intel_rps *rps);
These should be static and not in the .h file. Anyway I think as pointed
out above these 3 functions should probably be combined into just one.
> void gen6_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps *caps);
> void intel_rps_raise_unslice(struct intel_rps *rps);
> void intel_rps_lower_unslice(struct intel_rps *rps);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5003a5ffbc6a..68ff98e27b8d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1818,6 +1818,10 @@
> #define GT0_PERF_LIMIT_REASONS_LOG_MASK REG_GENMASK(31, 16)
> #define MTL_MEDIA_PERF_LIMIT_REASONS _MMIO(0x138030)
>
> +#define XEHPSDV_RAPL_PL1_FREQ_LIMIT _MMIO(0x250070)
> +#define MTL_RAPL_PL1_FREQ_LIMIT _MMIO(0x281070)
> +#define RAPL_PL1_FREQ_LIMIT_MASK 0xffff
Use REG_GENMASK(15, 0) here.
Also:
* The registers now need to be sorted by offset. Let's move these #defines
to gt/intel_gt_regs.h
* RAPL_PL1_FREQ_LIMIT_MASK will need to be indented a couple of spaces as
done for other registers
> #define CHV_CLK_CTL1 _MMIO(0x101100)
> #define VLV_CLK_CTL2 _MMIO(0x101104)
> #define CLK_CTL2_CZCOUNT_30NS_SHIFT 28
> --
> 2.34.1
>
Thanks.
--
Ashutosh
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915/gt: Add sysfs RAPL PL1 interface
2022-11-09 1:07 ` [Intel-gfx] [PATCH v1] " Dixit, Ashutosh
@ 2022-11-11 3:32 ` Sundaresan, Sujaritha
0 siblings, 0 replies; 4+ messages in thread
From: Sundaresan, Sujaritha @ 2022-11-11 3:32 UTC (permalink / raw)
To: Dixit, Ashutosh; +Cc: intel-gfx
On 11/9/2022 6:37 AM, Dixit, Ashutosh wrote:
> On Thu, 03 Nov 2022 05:37:23 -0700, Sujaritha Sundaresan wrote:
> Hi Suja,
>
>> Adding the rapl_pl1_freq_mhz sysfs attribute.
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 20 ++++++++++
>> drivers/gpu/drm/i915/gt/intel_rps.c | 44 +++++++++++++++++++++
>> drivers/gpu/drm/i915/gt/intel_rps.h | 3 ++
>> drivers/gpu/drm/i915/i915_reg.h | 4 ++
>> 4 files changed, 71 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
>> index 904160952369..e7f00ec252f8 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
>> @@ -496,6 +496,17 @@ static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
>> static const struct attribute * const gen6_rps_attrs[] = GEN6_RPS_ATTR;
>> static const struct attribute * const gen6_gt_attrs[] = GEN6_GT_ATTR;
>>
>> +static ssize_t rapl_pl1_freq_mhz_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buff)
>> +{
>> + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
>> + u32 rapl_pl1 = intel_rps_read_rapl_pl1_frequency(>->rps);
>> +
>> + return sysfs_emit(buff, "%u\n", rapl_pl1);
>> +}
>> +
>> +
>> static ssize_t punit_req_freq_mhz_show(struct device *dev,
>> struct device_attribute *attr,
>> char *buff)
>> @@ -534,6 +545,7 @@ struct intel_gt_bool_throttle_attr attr_##sysfs_func__ = { \
>> .mask = mask__, \
>> }
>>
>> +static DEVICE_ATTR_RO(rapl_pl1_freq_mhz);
>> static DEVICE_ATTR_RO(punit_req_freq_mhz);
> Is this patch against old code? Since this is now INTEL_GT_ATTR_RO. Yes the
> build failed. So rapl_pl1_freq_mhz will need to follow punit_req_freq_mhz.
Okay yes looks like I might not have grabbed the latest tree.
>
>> static INTEL_GT_RPS_BOOL_ATTR_RO(throttle_reason_status, GT0_PERF_LIMIT_REASONS_MASK);
>> static INTEL_GT_RPS_BOOL_ATTR_RO(throttle_reason_pl1, POWER_LIMIT_1_MASK);
>> @@ -790,12 +802,20 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
>> if (!is_object_gt(kobj))
>> return;
>>
>> + ret = sysfs_create_file(kobj, &dev_attr_rapl_pl1_freq_mhz.attr);
> The convention here is to create sysfs files only for platforms on which a
> feature (in this case RAPL PL1 freq) is supported.
>
> Also are we sure this is only available on MTL and XEHPSDV and not on DG2?
> Since generally a feature appears first on a platform and then is available
> for all successive products. If it's available on DG2 too then we can use
> something like:
>
> if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))
>
> See GRAPHICS_VER_FULL for various platforms in i915_pci.c.
I will check again for DG2.
>
>> + if (ret)
>> + drm_warn(>->i915->drm,
>> + "failed to create gt%u rapl_pl1_freq_mhz sysfs(%pe)",
>> + gt->info.id, ERR_PTR(ret));
>> +
>> +
>> ret = sysfs_create_file(kobj, &dev_attr_punit_req_freq_mhz.attr);
>> if (ret)
>> drm_warn(>->i915->drm,
>> "failed to create gt%u punit_req_freq_mhz sysfs (%pe)",
>> gt->info.id, ERR_PTR(ret));
>>
>> +
> Remove empty line.
>
>> if (i915_mmio_reg_valid(intel_gt_perf_limit_reasons_reg(gt))) {
>> ret = sysfs_create_files(kobj, throttle_reason_attrs);
>> if (ret)
>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
>> index 17b40b625e31..0e89b941e3be 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
>> @@ -9,6 +9,7 @@
>>
>> #include "i915_drv.h"
>> #include "i915_irq.h"
>> +#include "i915_reg.h"
> Not needed, see below.
>
>> #include "intel_breadcrumbs.h"
>> #include "intel_gt.h"
>> #include "intel_gt_clock_utils.h"
>> @@ -2422,6 +2423,49 @@ bool rps_read_mask_mmio(struct intel_rps *rps,
>> return rps_read_mmio(rps, reg32) & mask;
>> }
>>
>> +u32 intel_rps_read_rapl_pl1(struct intel_rps *rps)
>> +{
>> + struct drm_i915_private *i915 = rps_to_i915(rps);
>> + i915_reg_t rgadr;
>> + u32 rapl_pl1;
>> +
>> + if (IS_METEORLAKE(i915)) {
>> + rgadr = MTL_RAPL_PL1_FREQ_LIMIT;
>> + } else if (IS_XEHPSDV(i915)) {
>> + rgadr = XEHPSDV_RAPL_PL1_FREQ_LIMIT;
>> + } else {
>> + MISSING_CASE(GRAPHICS_VER(i915));
>> + rgadr = INVALID_MMIO_REG;
> No need for this, the sysfs file will only be visible for platforms on
> which this is supported so this will never be hit.
>
>> + }
>> +
>> + if (!i915_mmio_reg_valid(rgadr))
>> + rapl_pl1 = 0;
> No need for this either.
>
>> + else
>> + rapl_pl1 = rps_read_mmio(rps, rgadr);
>> +
>> + return rapl_pl1;
>> +}
>> +
>> +u32 intel_rps_get_rapl(struct intel_rps *rps, u32 rapl_pl1)
>> +{
>> + struct drm_i915_private *i915 = rps_to_i915(rps);
>> + u32 rapl = 0;
>> +
>> + if (IS_METEORLAKE(i915) || IS_XEHPSDV(i915))
>> + rapl = rapl_pl1 & RAPL_PL1_FREQ_LIMIT_MASK;
>> + else
>> + MISSING_CASE(GRAPHICS_VER(i915));
> No need for this either.
>
>> +
>> + return rapl;
>> +}
>> +
>> +u32 intel_rps_read_rapl_pl1_frequency(struct intel_rps *rps)
>> +{
>> + u32 rapl_freq = intel_rps_get_rapl(rps, intel_rps_read_rapl_pl1(rps));
>> +
>> + return (rapl_freq >> 8) * GT_FREQUENCY_MULTIPLIER;
> Use REG_FIELD_GET to extract the freq.
>
> There doesn't seem any need to have 3 functions here, let's combine them
> into a single function and use REG_FIELD_GET.
Got it. Will fix this
>
>> +}
>> +
>> /* External interface for intel_ips.ko */
>>
>> static struct drm_i915_private __rcu *ips_mchdev;
>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h
>> index 4509dfdc52e0..4adc6aaedba0 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_rps.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.h
>> @@ -34,6 +34,7 @@ void intel_rps_mark_interactive(struct intel_rps *rps, bool interactive);
>> int intel_gpu_freq(struct intel_rps *rps, int val);
>> int intel_freq_opcode(struct intel_rps *rps, int val);
>> u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat1);
>> +u32 intel_rps_get_rapl(struct intel_rps *rps, u32 rapl_pl1);
>> u32 intel_rps_read_actual_frequency(struct intel_rps *rps);
>> u32 intel_rps_get_requested_frequency(struct intel_rps *rps);
>> u32 intel_rps_get_min_frequency(struct intel_rps *rps);
>> @@ -47,6 +48,8 @@ u32 intel_rps_get_rp1_frequency(struct intel_rps *rps);
>> u32 intel_rps_get_rpn_frequency(struct intel_rps *rps);
>> u32 intel_rps_read_punit_req(struct intel_rps *rps);
>> u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps);
>> +u32 intel_rps_read_rapl_pl1(struct intel_rps *rps);
>> +u32 intel_rps_read_rapl_pl1_frequency(struct intel_rps *rps);
> These should be static and not in the .h file. Anyway I think as pointed
> out above these 3 functions should probably be combined into just one.
>
>> void gen6_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps *caps);
>> void intel_rps_raise_unslice(struct intel_rps *rps);
>> void intel_rps_lower_unslice(struct intel_rps *rps);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 5003a5ffbc6a..68ff98e27b8d 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1818,6 +1818,10 @@
>> #define GT0_PERF_LIMIT_REASONS_LOG_MASK REG_GENMASK(31, 16)
>> #define MTL_MEDIA_PERF_LIMIT_REASONS _MMIO(0x138030)
>>
>> +#define XEHPSDV_RAPL_PL1_FREQ_LIMIT _MMIO(0x250070)
>> +#define MTL_RAPL_PL1_FREQ_LIMIT _MMIO(0x281070)
>> +#define RAPL_PL1_FREQ_LIMIT_MASK 0xffff
> Use REG_GENMASK(15, 0) here.
>
> Also:
> * The registers now need to be sorted by offset. Let's move these #defines
> to gt/intel_gt_regs.h
> * RAPL_PL1_FREQ_LIMIT_MASK will need to be indented a couple of spaces as
> done for other registers
Sure will move it to that file.
>> #define CHV_CLK_CTL1 _MMIO(0x101100)
>> #define VLV_CLK_CTL2 _MMIO(0x101104)
>> #define CLK_CTL2_CZCOUNT_30NS_SHIFT 28
>> --
>> 2.34.1
>>
> Thanks.
> --
> Ashutosh
Thank you for the review
-Suja
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-11 3:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-03 12:37 [Intel-gfx] [PATCH v1] drm/i915/gt: Add sysfs RAPL PL1 interface Sujaritha Sundaresan
2022-11-03 19:17 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork
2022-11-09 1:07 ` [Intel-gfx] [PATCH v1] " Dixit, Ashutosh
2022-11-11 3:32 ` Sundaresan, Sujaritha
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.