[Intel-gfx] [PATCH 1/6] drm/i915/pmu: Support PMU for all engines

[Umesh Nerlige Ramappa]

From: Tvrtko Ursulin 

Given how the metrics are already exported, we also need to run sampling
over engines from all GTs.

Problem of GT frequencies is left for later.

Signed-off-by: Tvrtko Ursulin 
Reviewed-by: Umesh Nerlige Ramappa 
Signed-off-by: Umesh Nerlige Ramappa 
---
 drivers/gpu/drm/i915/i915_pmu.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 7ece883a7d95..67fa6cd77529 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -10,6 +10,7 @@
 #include "gt/intel_engine_pm.h"
 #include "gt/intel_engine_regs.h"
 #include "gt/intel_engine_user.h"
+#include "gt/intel_gt.h"
 #include "gt/intel_gt_pm.h"
 #include "gt/intel_gt_regs.h"
 #include "gt/intel_rc6.h"
@@ -414,8 +415,9 @@ static enum hrtimer_restart i915_sample(struct hrtimer 
*hrtimer)
struct drm_i915_private *i915 =
container_of(hrtimer, struct drm_i915_private, pmu.timer);
struct i915_pmu *pmu = >pmu;
-   struct intel_gt *gt = to_gt(i915);
unsigned int period_ns;
+   struct intel_gt *gt;
+   unsigned int i;
ktime_t now;
 
if (!READ_ONCE(pmu->timer_enabled))
@@ -431,8 +433,13 @@ static enum hrtimer_restart i915_sample(struct hrtimer 
*hrtimer)
 * grabbing the forcewake. However the potential error from timer call-
 * back delay greatly dominates this so we keep it simple.
 */
-   engines_sample(gt, period_ns);
-   frequency_sample(gt, period_ns);
+
+   for_each_gt(gt, i915, i) {
+   engines_sample(gt, period_ns);
+
+   if (i == 0) /* FIXME */
+   frequency_sample(gt, period_ns);
+   }
 
hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));
 
-- 
2.36.1

[Intel-gfx] [PATCH 1/6] drm/i915/pmu: Support PMU for all engines

[Umesh Nerlige Ramappa]

From: Tvrtko Ursulin 

Given how the metrics are already exported, we also need to run sampling
over engines from all GTs.

Problem of GT frequencies is left for later.

Signed-off-by: Tvrtko Ursulin 
Reviewed-by: Umesh Nerlige Ramappa 
Signed-off-by: Umesh Nerlige Ramappa 
---
 drivers/gpu/drm/i915/i915_pmu.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 7ece883a7d95..67fa6cd77529 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -10,6 +10,7 @@
 #include "gt/intel_engine_pm.h"
 #include "gt/intel_engine_regs.h"
 #include "gt/intel_engine_user.h"
+#include "gt/intel_gt.h"
 #include "gt/intel_gt_pm.h"
 #include "gt/intel_gt_regs.h"
 #include "gt/intel_rc6.h"
@@ -414,8 +415,9 @@ static enum hrtimer_restart i915_sample(struct hrtimer 
*hrtimer)
struct drm_i915_private *i915 =
container_of(hrtimer, struct drm_i915_private, pmu.timer);
struct i915_pmu *pmu = >pmu;
-   struct intel_gt *gt = to_gt(i915);
unsigned int period_ns;
+   struct intel_gt *gt;
+   unsigned int i;
ktime_t now;
 
if (!READ_ONCE(pmu->timer_enabled))
@@ -431,8 +433,13 @@ static enum hrtimer_restart i915_sample(struct hrtimer 
*hrtimer)
 * grabbing the forcewake. However the potential error from timer call-
 * back delay greatly dominates this so we keep it simple.
 */
-   engines_sample(gt, period_ns);
-   frequency_sample(gt, period_ns);
+
+   for_each_gt(gt, i915, i) {
+   engines_sample(gt, period_ns);
+
+   if (i == 0) /* FIXME */
+   frequency_sample(gt, period_ns);
+   }
 
hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));
 
-- 
2.36.1

Re: [Intel-gfx] [PATCH 1/6] drm/i915/pmu: Support PMU for all engines

[Tvrtko Ursulin]

On 08/05/2023 18:52, Umesh Nerlige Ramappa wrote:

On Fri, May 05, 2023 at 05:58:11PM -0700, Umesh Nerlige Ramappa wrote:

From: Tvrtko Ursulin 

Given how the metrics are already exported, we also need to run sampling
over engines from all GTs.

Problem of GT frequencies is left for later.

Signed-off-by: Tvrtko Ursulin 
Signed-off-by: Umesh Nerlige Ramappa 
---
drivers/gpu/drm/i915/i915_pmu.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c 
b/drivers/gpu/drm/i915/i915_pmu.c

index 7ece883a7d95..67fa6cd77529 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -10,6 +10,7 @@
#include "gt/intel_engine_pm.h"
#include "gt/intel_engine_regs.h"
#include "gt/intel_engine_user.h"
+#include "gt/intel_gt.h"
#include "gt/intel_gt_pm.h"
#include "gt/intel_gt_regs.h"
#include "gt/intel_rc6.h"
@@ -414,8 +415,9 @@ static enum hrtimer_restart i915_sample(struct 
hrtimer *hrtimer)

struct drm_i915_private *i915 =
    container_of(hrtimer, struct drm_i915_private, pmu.timer);
struct i915_pmu *pmu = >pmu;
-    struct intel_gt *gt = to_gt(i915);
unsigned int period_ns;
+    struct intel_gt *gt;
+    unsigned int i;
ktime_t now;

if (!READ_ONCE(pmu->timer_enabled))
@@ -431,8 +433,13 @@ static enum hrtimer_restart i915_sample(struct 
hrtimer *hrtimer)
 * grabbing the forcewake. However the potential error from timer 
call-

 * back delay greatly dominates this so we keep it simple.
 */
-    engines_sample(gt, period_ns);
-    frequency_sample(gt, period_ns);
+
+    for_each_gt(gt, i915, i) {
+    engines_sample(gt, period_ns);
+
+    if (i == 0) /* FIXME */
+    frequency_sample(gt, period_ns);


If the current series is already handling the FIXME at a later patch, I 
would just change this to a comment - /* Support gt0 for now */


With or without that, this is

Reviewed-by: Umesh Nerlige Ramappa 

@Tvrtko, Note that I am only transporting the patches (unmodified) from 
internal to upstream, so assuming I am still a valid reviewer. If not, 
let me know.


I think that is okay.

More of a problem is when you make comments like the above "I would just 
change" - the question then is are you expecting me to make that change? 
;) I think it would be best if you handled such tweaks in the series. In 
this particular patch it is probably not really required since it gets 
overwritten later as you say. It's probably just a left-over untidiness 
from "back in the day".


Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 1/6] drm/i915/pmu: Support PMU for all engines

[Umesh Nerlige Ramappa]

On Fri, May 05, 2023 at 05:58:11PM -0700, Umesh Nerlige Ramappa wrote:

From: Tvrtko Ursulin 

Given how the metrics are already exported, we also need to run sampling
over engines from all GTs.

Problem of GT frequencies is left for later.

Signed-off-by: Tvrtko Ursulin 
Signed-off-by: Umesh Nerlige Ramappa 
---
drivers/gpu/drm/i915/i915_pmu.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 7ece883a7d95..67fa6cd77529 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -10,6 +10,7 @@
#include "gt/intel_engine_pm.h"
#include "gt/intel_engine_regs.h"
#include "gt/intel_engine_user.h"
+#include "gt/intel_gt.h"
#include "gt/intel_gt_pm.h"
#include "gt/intel_gt_regs.h"
#include "gt/intel_rc6.h"
@@ -414,8 +415,9 @@ static enum hrtimer_restart i915_sample(struct hrtimer 
*hrtimer)
struct drm_i915_private *i915 =
container_of(hrtimer, struct drm_i915_private, pmu.timer);
struct i915_pmu *pmu = >pmu;
-   struct intel_gt *gt = to_gt(i915);
unsigned int period_ns;
+   struct intel_gt *gt;
+   unsigned int i;
ktime_t now;

if (!READ_ONCE(pmu->timer_enabled))
@@ -431,8 +433,13 @@ static enum hrtimer_restart i915_sample(struct hrtimer 
*hrtimer)
 * grabbing the forcewake. However the potential error from timer call-
 * back delay greatly dominates this so we keep it simple.
 */
-   engines_sample(gt, period_ns);
-   frequency_sample(gt, period_ns);
+
+   for_each_gt(gt, i915, i) {
+   engines_sample(gt, period_ns);
+
+   if (i == 0) /* FIXME */
+   frequency_sample(gt, period_ns);


If the current series is already handling the FIXME at a later patch, I 
would just change this to a comment - /* Support gt0 for now */


With or without that, this is

Reviewed-by: Umesh Nerlige Ramappa 

@Tvrtko, Note that I am only transporting the patches (unmodified) from 
internal to upstream, so assuming I am still a valid reviewer. If not, 
let me know.


Thanks,
Umesh


+   }

hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));

--
2.36.1

[Intel-gfx] [PATCH 1/6] drm/i915/pmu: Support PMU for all engines

[Umesh Nerlige Ramappa]

From: Tvrtko Ursulin 

Given how the metrics are already exported, we also need to run sampling
over engines from all GTs.

Problem of GT frequencies is left for later.

Signed-off-by: Tvrtko Ursulin 
Signed-off-by: Umesh Nerlige Ramappa 
---
 drivers/gpu/drm/i915/i915_pmu.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 7ece883a7d95..67fa6cd77529 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -10,6 +10,7 @@
 #include "gt/intel_engine_pm.h"
 #include "gt/intel_engine_regs.h"
 #include "gt/intel_engine_user.h"
+#include "gt/intel_gt.h"
 #include "gt/intel_gt_pm.h"
 #include "gt/intel_gt_regs.h"
 #include "gt/intel_rc6.h"
@@ -414,8 +415,9 @@ static enum hrtimer_restart i915_sample(struct hrtimer 
*hrtimer)
struct drm_i915_private *i915 =
container_of(hrtimer, struct drm_i915_private, pmu.timer);
struct i915_pmu *pmu = >pmu;
-   struct intel_gt *gt = to_gt(i915);
unsigned int period_ns;
+   struct intel_gt *gt;
+   unsigned int i;
ktime_t now;
 
if (!READ_ONCE(pmu->timer_enabled))
@@ -431,8 +433,13 @@ static enum hrtimer_restart i915_sample(struct hrtimer 
*hrtimer)
 * grabbing the forcewake. However the potential error from timer call-
 * back delay greatly dominates this so we keep it simple.
 */
-   engines_sample(gt, period_ns);
-   frequency_sample(gt, period_ns);
+
+   for_each_gt(gt, i915, i) {
+   engines_sample(gt, period_ns);
+
+   if (i == 0) /* FIXME */
+   frequency_sample(gt, period_ns);
+   }
 
hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));
 
-- 
2.36.1