| Message ID | 20230506005816.1891043-2-umesh.nerlige.ramappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add MTL PMU support for multi-gt | expand |
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 >
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 <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. 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
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); + } hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));