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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Fri, May 05, 2023 at 05:58:11PM -0700, Umesh Nerlige Ramappa wrote:
>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
>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 <tvrtko.ursulin@intel.com>
>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>---
> 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 = &i915->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 <umesh.nerlige.ramappa@intel.com>

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