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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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