[Intel-gfx] [PATCH 0/2] drm/i915/pmu: couple of cleanups

[Intel-gfx] [PATCH 0/2] drm/i915/pmu: couple of cleanups

[Ashutosh Dixit]

Ashutosh Dixit (2):
  drm/i915/pmu: Turn off the timer to sample frequencies when GT is
    parked
  drm/i915/pmu: Make PMU sample array two-dimensional

 drivers/gpu/drm/i915/i915_pmu.c | 72 +++++++++++----------------------
 drivers/gpu/drm/i915/i915_pmu.h |  2 +-
 2 files changed, 24 insertions(+), 50 deletions(-)

-- 
2.38.0

[Intel-gfx] [PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

[Ashutosh Dixit]

pmu_needs_timer() keeps the timer running even when GT is parked,
ostensibly to sample requested/actual frequencies. However
frequency_sample() has the following:

	/* Report 0/0 (actual/requested) frequency while parked. */
	if (!intel_gt_pm_get_if_awake(gt))
		return;

The above code prevents frequencies to be sampled while the GT is
parked. So we might as well turn off the sampling timer itself in this
case and save CPU cycles/power.

v2: Instead of turning freq bits off, return false, since no counters will
    run after this change when GT is parked (Tvrtko)

Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index a814583e19fd7..b47d890d4ada1 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -144,6 +144,10 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
 	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
 	u32 enable;
 
+	/* When GPU is idle, at present no counters need to run */
+	if (!gpu_active)
+		return false;
+
 	/*
 	 * Only some counters need the sampling timer.
 	 *
@@ -157,17 +161,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
 	 */
 	enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
 
-	/*
-	 * When the GPU is idle per-engine counters do not need to be
-	 * running so clear those bits out.
-	 */
-	if (!gpu_active)
-		enable &= ~ENGINE_SAMPLE_MASK;
 	/*
 	 * Also there is software busyness tracking available we do not
 	 * need the timer for I915_SAMPLE_BUSY counter.
 	 */
-	else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
+	if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
 		enable &= ~BIT(I915_SAMPLE_BUSY);
 
 	/*
-- 
2.38.0

Re: [Intel-gfx] [PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

[Andrzej Hajda]

On 23.05.2023 17:19, Ashutosh Dixit wrote:
> pmu_needs_timer() keeps the timer running even when GT is parked,
> ostensibly to sample requested/actual frequencies. However
> frequency_sample() has the following:
> 
> 	/* Report 0/0 (actual/requested) frequency while parked. */
> 	if (!intel_gt_pm_get_if_awake(gt))
> 		return;
> 
> The above code prevents frequencies to be sampled while the GT is
> parked. So we might as well turn off the sampling timer itself in this
> case and save CPU cycles/power.
> 
> v2: Instead of turning freq bits off, return false, since no counters will
>      run after this change when GT is parked (Tvrtko)
> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_pmu.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index a814583e19fd7..b47d890d4ada1 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -144,6 +144,10 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
>   	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
>   	u32 enable;
>   
> +	/* When GPU is idle, at present no counters need to run */
> +	if (!gpu_active)
> +		return false;
> +

What is then purpose of calling pmu_needs_timer with 2nd arg false?
Why not just replace all occurrences of pmu_needs_timer(.., false) with 
false? And remove the 2nd argument.

Regards
Andrzej



>   	/*
>   	 * Only some counters need the sampling timer.
>   	 *
> @@ -157,17 +161,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
>   	 */
>   	enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
>   
> -	/*
> -	 * When the GPU is idle per-engine counters do not need to be
> -	 * running so clear those bits out.
> -	 */
> -	if (!gpu_active)
> -		enable &= ~ENGINE_SAMPLE_MASK;
>   	/*
>   	 * Also there is software busyness tracking available we do not
>   	 * need the timer for I915_SAMPLE_BUSY counter.
>   	 */
> -	else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
> +	if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
>   		enable &= ~BIT(I915_SAMPLE_BUSY);
>   
>   	/*

Re: [Intel-gfx] [PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

[Dixit, Ashutosh]

On Wed, 24 May 2023 02:12:31 -0700, Andrzej Hajda wrote:
>

Hi Andrzej,

> On 23.05.2023 17:19, Ashutosh Dixit wrote:
> > pmu_needs_timer() keeps the timer running even when GT is parked,
> > ostensibly to sample requested/actual frequencies. However
> > frequency_sample() has the following:
> >
> >	/* Report 0/0 (actual/requested) frequency while parked. */
> >	if (!intel_gt_pm_get_if_awake(gt))
> >		return;
> >
> > The above code prevents frequencies to be sampled while the GT is
> > parked. So we might as well turn off the sampling timer itself in this
> > case and save CPU cycles/power.
> >
> > v2: Instead of turning freq bits off, return false, since no counters will
> >      run after this change when GT is parked (Tvrtko)
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_pmu.c | 12 +++++-------
> >   1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index a814583e19fd7..b47d890d4ada1 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -144,6 +144,10 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
> >	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
> >	u32 enable;
> >   +	/* When GPU is idle, at present no counters need to run */
> > +	if (!gpu_active)
> > +		return false;
> > +
>
> What is then purpose of calling pmu_needs_timer with 2nd arg false?
> Why not just replace all occurrences of pmu_needs_timer(.., false) with
> false? And remove the 2nd argument.

OK, this didn't seem unreasonable so I went ahead and made this change in
Patch v3. Copying Tvrtko too in case he prefers v2 for any reason. Please
review.

Thanks.
--
Ashutosh


>
>
>
> >	/*
> >	 * Only some counters need the sampling timer.
> >	 *
> > @@ -157,17 +161,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
> >	 */
> >	enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
> >   -	/*
> > -	 * When the GPU is idle per-engine counters do not need to be
> > -	 * running so clear those bits out.
> > -	 */
> > -	if (!gpu_active)
> > -		enable &= ~ENGINE_SAMPLE_MASK;
> >	/*
> >	 * Also there is software busyness tracking available we do not
> >	 * need the timer for I915_SAMPLE_BUSY counter.
> >	 */
> > -	else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
> > +	if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
> >		enable &= ~BIT(I915_SAMPLE_BUSY);
> >		/*
>

Re: [Intel-gfx] [PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

[Tvrtko Ursulin]

On 24/05/2023 22:46, Dixit, Ashutosh wrote:
> On Wed, 24 May 2023 02:12:31 -0700, Andrzej Hajda wrote:
>>
> 
> Hi Andrzej,
> 
>> On 23.05.2023 17:19, Ashutosh Dixit wrote:
>>> pmu_needs_timer() keeps the timer running even when GT is parked,
>>> ostensibly to sample requested/actual frequencies. However
>>> frequency_sample() has the following:
>>>
>>> 	/* Report 0/0 (actual/requested) frequency while parked. */
>>> 	if (!intel_gt_pm_get_if_awake(gt))
>>> 		return;
>>>
>>> The above code prevents frequencies to be sampled while the GT is
>>> parked. So we might as well turn off the sampling timer itself in this
>>> case and save CPU cycles/power.
>>>
>>> v2: Instead of turning freq bits off, return false, since no counters will
>>>       run after this change when GT is parked (Tvrtko)
>>>
>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_pmu.c | 12 +++++-------
>>>    1 file changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>>> index a814583e19fd7..b47d890d4ada1 100644
>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>> @@ -144,6 +144,10 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
>>> 	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
>>> 	u32 enable;
>>>    +	/* When GPU is idle, at present no counters need to run */
>>> +	if (!gpu_active)
>>> +		return false;
>>> +
>>
>> What is then purpose of calling pmu_needs_timer with 2nd arg false?
>> Why not just replace all occurrences of pmu_needs_timer(.., false) with
>> false? And remove the 2nd argument.
> 
> OK, this didn't seem unreasonable so I went ahead and made this change in
> Patch v3. Copying Tvrtko too in case he prefers v2 for any reason. Please
> review.

It is all fine by me (the latest version and all)!

Regards,

Tvrtko

[Intel-gfx] [PATCH 2/2] drm/i915/pmu: Make PMU sample array two-dimensional

[Ashutosh Dixit]

No functional changes but we can remove some unsightly index computation
and read/write functions if we convert the PMU sample array from a
one-dimensional to a two-dimensional array.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 60 ++++++++++-----------------------
 drivers/gpu/drm/i915/i915_pmu.h |  2 +-
 2 files changed, 19 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index b47d890d4ada1..137e0df9573ee 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -195,33 +195,6 @@ static inline s64 ktime_since_raw(const ktime_t kt)
 	return ktime_to_ns(ktime_sub(ktime_get_raw(), kt));
 }
 
-static unsigned int
-__sample_idx(struct i915_pmu *pmu, unsigned int gt_id, int sample)
-{
-	unsigned int idx = gt_id * __I915_NUM_PMU_SAMPLERS + sample;
-
-	GEM_BUG_ON(idx >= ARRAY_SIZE(pmu->sample));
-
-	return idx;
-}
-
-static u64 read_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample)
-{
-	return pmu->sample[__sample_idx(pmu, gt_id, sample)].cur;
-}
-
-static void
-store_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample, u64 val)
-{
-	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur = val;
-}
-
-static void
-add_sample_mult(struct i915_pmu *pmu, unsigned int gt_id, int sample, u32 val, u32 mul)
-{
-	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur += mul_u32_u32(val, mul);
-}
-
 static u64 get_rc6(struct intel_gt *gt)
 {
 	struct drm_i915_private *i915 = gt->i915;
@@ -240,7 +213,7 @@ static u64 get_rc6(struct intel_gt *gt)
 	spin_lock_irqsave(&pmu->lock, flags);
 
 	if (awake) {
-		store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
+		pmu->sample[gt_id][__I915_SAMPLE_RC6].cur = val;
 	} else {
 		/*
 		 * We think we are runtime suspended.
@@ -250,13 +223,13 @@ static u64 get_rc6(struct intel_gt *gt)
 		 * counter value.
 		 */
 		val = ktime_since_raw(pmu->sleep_last[gt_id]);
-		val += read_sample(pmu, gt_id, __I915_SAMPLE_RC6);
+		val += pmu->sample[gt_id][__I915_SAMPLE_RC6].cur;
 	}
 
-	if (val < read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED))
-		val = read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED);
+	if (val < pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur)
+		val = pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur;
 	else
-		store_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED, val);
+		pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
 
 	spin_unlock_irqrestore(&pmu->lock, flags);
 
@@ -275,9 +248,8 @@ static void init_rc6(struct i915_pmu *pmu)
 		with_intel_runtime_pm(gt->uncore->rpm, wakeref) {
 			u64 val = __get_rc6(gt);
 
-			store_sample(pmu, i, __I915_SAMPLE_RC6, val);
-			store_sample(pmu, i, __I915_SAMPLE_RC6_LAST_REPORTED,
-				     val);
+			pmu->sample[i][__I915_SAMPLE_RC6].cur = val;
+			pmu->sample[i][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
 			pmu->sleep_last[i] = ktime_get_raw();
 		}
 	}
@@ -287,7 +259,7 @@ static void park_rc6(struct intel_gt *gt)
 {
 	struct i915_pmu *pmu = &gt->i915->pmu;
 
-	store_sample(pmu, gt->info.id, __I915_SAMPLE_RC6, __get_rc6(gt));
+	pmu->sample[gt->info.id][__I915_SAMPLE_RC6].cur = __get_rc6(gt);
 	pmu->sleep_last[gt->info.id] = ktime_get_raw();
 }
 
@@ -428,6 +400,12 @@ engines_sample(struct intel_gt *gt, unsigned int period_ns)
 	}
 }
 
+static void
+add_sample_mult(struct i915_pmu_sample *sample, u32 val, u32 mul)
+{
+	sample->cur += mul_u32_u32(val, mul);
+}
+
 static bool
 frequency_sampling_enabled(struct i915_pmu *pmu, unsigned int gt)
 {
@@ -467,12 +445,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
 		if (!val)
 			val = intel_gpu_freq(rps, rps->cur_freq);
 
-		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_ACT,
+		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT],
 				val, period_ns / 1000);
 	}
 
 	if (pmu->enable & config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt_id))) {
-		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_REQ,
+		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ],
 				intel_rps_get_requested_frequency(rps),
 				period_ns / 1000);
 	}
@@ -673,14 +651,12 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
 		switch (config) {
 		case I915_PMU_ACTUAL_FREQUENCY:
 			val =
-			   div_u64(read_sample(pmu, gt_id,
-					       __I915_SAMPLE_FREQ_ACT),
+			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT].cur,
 				   USEC_PER_SEC /* to MHz */);
 			break;
 		case I915_PMU_REQUESTED_FREQUENCY:
 			val =
-			   div_u64(read_sample(pmu, gt_id,
-					       __I915_SAMPLE_FREQ_REQ),
+			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ].cur,
 				   USEC_PER_SEC /* to MHz */);
 			break;
 		case I915_PMU_INTERRUPTS:
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 33d80fbaab8bc..d20592e7db999 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -127,7 +127,7 @@ struct i915_pmu {
 	 * Only global counters are held here, while the per-engine ones are in
 	 * struct intel_engine_cs.
 	 */
-	struct i915_pmu_sample sample[I915_PMU_MAX_GTS * __I915_NUM_PMU_SAMPLERS];
+	struct i915_pmu_sample sample[I915_PMU_MAX_GTS][__I915_NUM_PMU_SAMPLERS];
 	/**
 	 * @sleep_last: Last time GT parked for RC6 estimation.
 	 */
-- 
2.38.0

Re: [Intel-gfx] [PATCH 2/2] drm/i915/pmu: Make PMU sample array two-dimensional

[Andrzej Hajda]

On 23.05.2023 17:19, Ashutosh Dixit wrote:
> No functional changes but we can remove some unsightly index computation
> and read/write functions if we convert the PMU sample array from a
> one-dimensional to a two-dimensional array.
> 
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

> ---
>   drivers/gpu/drm/i915/i915_pmu.c | 60 ++++++++++-----------------------
>   drivers/gpu/drm/i915/i915_pmu.h |  2 +-
>   2 files changed, 19 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index b47d890d4ada1..137e0df9573ee 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -195,33 +195,6 @@ static inline s64 ktime_since_raw(const ktime_t kt)
>   	return ktime_to_ns(ktime_sub(ktime_get_raw(), kt));
>   }
>   
> -static unsigned int
> -__sample_idx(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> -{
> -	unsigned int idx = gt_id * __I915_NUM_PMU_SAMPLERS + sample;
> -
> -	GEM_BUG_ON(idx >= ARRAY_SIZE(pmu->sample));
> -
> -	return idx;
> -}
> -
> -static u64 read_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> -{
> -	return pmu->sample[__sample_idx(pmu, gt_id, sample)].cur;
> -}
> -
> -static void
> -store_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample, u64 val)
> -{
> -	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur = val;
> -}
> -
> -static void
> -add_sample_mult(struct i915_pmu *pmu, unsigned int gt_id, int sample, u32 val, u32 mul)
> -{
> -	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur += mul_u32_u32(val, mul);
> -}
> -
>   static u64 get_rc6(struct intel_gt *gt)
>   {
>   	struct drm_i915_private *i915 = gt->i915;
> @@ -240,7 +213,7 @@ static u64 get_rc6(struct intel_gt *gt)
>   	spin_lock_irqsave(&pmu->lock, flags);
>   
>   	if (awake) {
> -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
> +		pmu->sample[gt_id][__I915_SAMPLE_RC6].cur = val;
>   	} else {
>   		/*
>   		 * We think we are runtime suspended.
> @@ -250,13 +223,13 @@ static u64 get_rc6(struct intel_gt *gt)
>   		 * counter value.
>   		 */
>   		val = ktime_since_raw(pmu->sleep_last[gt_id]);
> -		val += read_sample(pmu, gt_id, __I915_SAMPLE_RC6);
> +		val += pmu->sample[gt_id][__I915_SAMPLE_RC6].cur;
>   	}
>   
> -	if (val < read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED))
> -		val = read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED);
> +	if (val < pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur)
> +		val = pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur;
>   	else
> -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED, val);
> +		pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
>   
>   	spin_unlock_irqrestore(&pmu->lock, flags);
>   
> @@ -275,9 +248,8 @@ static void init_rc6(struct i915_pmu *pmu)
>   		with_intel_runtime_pm(gt->uncore->rpm, wakeref) {
>   			u64 val = __get_rc6(gt);
>   
> -			store_sample(pmu, i, __I915_SAMPLE_RC6, val);
> -			store_sample(pmu, i, __I915_SAMPLE_RC6_LAST_REPORTED,
> -				     val);
> +			pmu->sample[i][__I915_SAMPLE_RC6].cur = val;
> +			pmu->sample[i][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
>   			pmu->sleep_last[i] = ktime_get_raw();
>   		}
>   	}
> @@ -287,7 +259,7 @@ static void park_rc6(struct intel_gt *gt)
>   {
>   	struct i915_pmu *pmu = &gt->i915->pmu;
>   
> -	store_sample(pmu, gt->info.id, __I915_SAMPLE_RC6, __get_rc6(gt));
> +	pmu->sample[gt->info.id][__I915_SAMPLE_RC6].cur = __get_rc6(gt);
>   	pmu->sleep_last[gt->info.id] = ktime_get_raw();
>   }
>   
> @@ -428,6 +400,12 @@ engines_sample(struct intel_gt *gt, unsigned int period_ns)
>   	}
>   }
>   
> +static void
> +add_sample_mult(struct i915_pmu_sample *sample, u32 val, u32 mul)
> +{
> +	sample->cur += mul_u32_u32(val, mul);
> +}
> +
>   static bool
>   frequency_sampling_enabled(struct i915_pmu *pmu, unsigned int gt)
>   {
> @@ -467,12 +445,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
>   		if (!val)
>   			val = intel_gpu_freq(rps, rps->cur_freq);
>   
> -		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_ACT,
> +		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT],
>   				val, period_ns / 1000);
>   	}
>   
>   	if (pmu->enable & config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt_id))) {
> -		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_REQ,
> +		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ],
>   				intel_rps_get_requested_frequency(rps),
>   				period_ns / 1000);
>   	}
> @@ -673,14 +651,12 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
>   		switch (config) {
>   		case I915_PMU_ACTUAL_FREQUENCY:
>   			val =
> -			   div_u64(read_sample(pmu, gt_id,
> -					       __I915_SAMPLE_FREQ_ACT),
> +			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT].cur,
>   				   USEC_PER_SEC /* to MHz */);
>   			break;
>   		case I915_PMU_REQUESTED_FREQUENCY:
>   			val =
> -			   div_u64(read_sample(pmu, gt_id,
> -					       __I915_SAMPLE_FREQ_REQ),
> +			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ].cur,
>   				   USEC_PER_SEC /* to MHz */);
>   			break;
>   		case I915_PMU_INTERRUPTS:
> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> index 33d80fbaab8bc..d20592e7db999 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.h
> +++ b/drivers/gpu/drm/i915/i915_pmu.h
> @@ -127,7 +127,7 @@ struct i915_pmu {
>   	 * Only global counters are held here, while the per-engine ones are in
>   	 * struct intel_engine_cs.
>   	 */
> -	struct i915_pmu_sample sample[I915_PMU_MAX_GTS * __I915_NUM_PMU_SAMPLERS];
> +	struct i915_pmu_sample sample[I915_PMU_MAX_GTS][__I915_NUM_PMU_SAMPLERS];
>   	/**
>   	 * @sleep_last: Last time GT parked for RC6 estimation.
>   	 */

Re: [Intel-gfx] [PATCH 2/2] drm/i915/pmu: Make PMU sample array two-dimensional

[Tvrtko Ursulin]

On 23/05/2023 16:19, Ashutosh Dixit wrote:
> No functional changes but we can remove some unsightly index computation
> and read/write functions if we convert the PMU sample array from a
> one-dimensional to a two-dimensional array.
> 
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_pmu.c | 60 ++++++++++-----------------------
>   drivers/gpu/drm/i915/i915_pmu.h |  2 +-
>   2 files changed, 19 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index b47d890d4ada1..137e0df9573ee 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -195,33 +195,6 @@ static inline s64 ktime_since_raw(const ktime_t kt)
>   	return ktime_to_ns(ktime_sub(ktime_get_raw(), kt));
>   }
>   
> -static unsigned int
> -__sample_idx(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> -{
> -	unsigned int idx = gt_id * __I915_NUM_PMU_SAMPLERS + sample;
> -
> -	GEM_BUG_ON(idx >= ARRAY_SIZE(pmu->sample));
> -
> -	return idx;
> -}
> -
> -static u64 read_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> -{
> -	return pmu->sample[__sample_idx(pmu, gt_id, sample)].cur;
> -}
> -
> -static void
> -store_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample, u64 val)
> -{
> -	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur = val;
> -}
> -
> -static void
> -add_sample_mult(struct i915_pmu *pmu, unsigned int gt_id, int sample, u32 val, u32 mul)
> -{
> -	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur += mul_u32_u32(val, mul);
> -}

IMO read and store helpers could have stayed and just changed the 
implementation. Like add_sample_mult which you just moved. I would have 
been a smaller patch. So dunno.. a bit of a reluctant r-b.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> -
>   static u64 get_rc6(struct intel_gt *gt)
>   {
>   	struct drm_i915_private *i915 = gt->i915;
> @@ -240,7 +213,7 @@ static u64 get_rc6(struct intel_gt *gt)
>   	spin_lock_irqsave(&pmu->lock, flags);
>   
>   	if (awake) {
> -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
> +		pmu->sample[gt_id][__I915_SAMPLE_RC6].cur = val;
>   	} else {
>   		/*
>   		 * We think we are runtime suspended.
> @@ -250,13 +223,13 @@ static u64 get_rc6(struct intel_gt *gt)
>   		 * counter value.
>   		 */
>   		val = ktime_since_raw(pmu->sleep_last[gt_id]);
> -		val += read_sample(pmu, gt_id, __I915_SAMPLE_RC6);
> +		val += pmu->sample[gt_id][__I915_SAMPLE_RC6].cur;
>   	}
>   
> -	if (val < read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED))
> -		val = read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED);
> +	if (val < pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur)
> +		val = pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur;
>   	else
> -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED, val);
> +		pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
>   
>   	spin_unlock_irqrestore(&pmu->lock, flags);
>   
> @@ -275,9 +248,8 @@ static void init_rc6(struct i915_pmu *pmu)
>   		with_intel_runtime_pm(gt->uncore->rpm, wakeref) {
>   			u64 val = __get_rc6(gt);
>   
> -			store_sample(pmu, i, __I915_SAMPLE_RC6, val);
> -			store_sample(pmu, i, __I915_SAMPLE_RC6_LAST_REPORTED,
> -				     val);
> +			pmu->sample[i][__I915_SAMPLE_RC6].cur = val;
> +			pmu->sample[i][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
>   			pmu->sleep_last[i] = ktime_get_raw();
>   		}
>   	}
> @@ -287,7 +259,7 @@ static void park_rc6(struct intel_gt *gt)
>   {
>   	struct i915_pmu *pmu = &gt->i915->pmu;
>   
> -	store_sample(pmu, gt->info.id, __I915_SAMPLE_RC6, __get_rc6(gt));
> +	pmu->sample[gt->info.id][__I915_SAMPLE_RC6].cur = __get_rc6(gt);
>   	pmu->sleep_last[gt->info.id] = ktime_get_raw();
>   }
>   
> @@ -428,6 +400,12 @@ engines_sample(struct intel_gt *gt, unsigned int period_ns)
>   	}
>   }
>   
> +static void
> +add_sample_mult(struct i915_pmu_sample *sample, u32 val, u32 mul)
> +{
> +	sample->cur += mul_u32_u32(val, mul);
> +}
> +
>   static bool
>   frequency_sampling_enabled(struct i915_pmu *pmu, unsigned int gt)
>   {
> @@ -467,12 +445,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
>   		if (!val)
>   			val = intel_gpu_freq(rps, rps->cur_freq);
>   
> -		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_ACT,
> +		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT],
>   				val, period_ns / 1000);
>   	}
>   
>   	if (pmu->enable & config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt_id))) {
> -		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_REQ,
> +		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ],
>   				intel_rps_get_requested_frequency(rps),
>   				period_ns / 1000);
>   	}
> @@ -673,14 +651,12 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
>   		switch (config) {
>   		case I915_PMU_ACTUAL_FREQUENCY:
>   			val =
> -			   div_u64(read_sample(pmu, gt_id,
> -					       __I915_SAMPLE_FREQ_ACT),
> +			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT].cur,
>   				   USEC_PER_SEC /* to MHz */);
>   			break;
>   		case I915_PMU_REQUESTED_FREQUENCY:
>   			val =
> -			   div_u64(read_sample(pmu, gt_id,
> -					       __I915_SAMPLE_FREQ_REQ),
> +			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ].cur,
>   				   USEC_PER_SEC /* to MHz */);
>   			break;
>   		case I915_PMU_INTERRUPTS:
> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> index 33d80fbaab8bc..d20592e7db999 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.h
> +++ b/drivers/gpu/drm/i915/i915_pmu.h
> @@ -127,7 +127,7 @@ struct i915_pmu {
>   	 * Only global counters are held here, while the per-engine ones are in
>   	 * struct intel_engine_cs.
>   	 */
> -	struct i915_pmu_sample sample[I915_PMU_MAX_GTS * __I915_NUM_PMU_SAMPLERS];
> +	struct i915_pmu_sample sample[I915_PMU_MAX_GTS][__I915_NUM_PMU_SAMPLERS];
>   	/**
>   	 * @sleep_last: Last time GT parked for RC6 estimation.
>   	 */

Re: [Intel-gfx] [PATCH 2/2] drm/i915/pmu: Make PMU sample array two-dimensional

[Dixit, Ashutosh]

On Wed, 24 May 2023 04:38:18 -0700, Tvrtko Ursulin wrote:
>

Hi Tvrtko,

> On 23/05/2023 16:19, Ashutosh Dixit wrote:
> > No functional changes but we can remove some unsightly index computation
> > and read/write functions if we convert the PMU sample array from a
> > one-dimensional to a two-dimensional array.
> >
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_pmu.c | 60 ++++++++++-----------------------
> >   drivers/gpu/drm/i915/i915_pmu.h |  2 +-
> >   2 files changed, 19 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index b47d890d4ada1..137e0df9573ee 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -195,33 +195,6 @@ static inline s64 ktime_since_raw(const ktime_t kt)
> >	return ktime_to_ns(ktime_sub(ktime_get_raw(), kt));
> >   }
> >   -static unsigned int
> > -__sample_idx(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> > -{
> > -	unsigned int idx = gt_id * __I915_NUM_PMU_SAMPLERS + sample;
> > -
> > -	GEM_BUG_ON(idx >= ARRAY_SIZE(pmu->sample));
> > -
> > -	return idx;
> > -}
> > -
> > -static u64 read_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> > -{
> > -	return pmu->sample[__sample_idx(pmu, gt_id, sample)].cur;
> > -}
> > -
> > -static void
> > -store_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample, u64 val)
> > -{
> > -	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur = val;
> > -}
> > -
> > -static void
> > -add_sample_mult(struct i915_pmu *pmu, unsigned int gt_id, int sample, u32 val, u32 mul)
> > -{
> > -	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur += mul_u32_u32(val, mul);
> > -}
>
> IMO read and store helpers could have stayed and just changed the
> implementation. Like add_sample_mult which you just moved. I would have
> been a smaller patch. So dunno.. a bit of a reluctant r-b.

Are you referring just to add_sample_mult or to all the other functions
too? add_sample_mult I moved it to where it was before bc4be0a38b63
("drm/i915/pmu: Prepare for multi-tile non-engine counters"), could have
left it here I guess.

The other read and store helpers are not needed with the 2-d array at all
since the compiler itself will do that, so I thought it was better to get
rid of them completely.

Let me know if you want any changes, otherwise I will leave as is.

> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks for the review. Thanks Andrzej too :)
--
Ashutosh

> > -
> >   static u64 get_rc6(struct intel_gt *gt)
> >   {
> >	struct drm_i915_private *i915 = gt->i915;
> > @@ -240,7 +213,7 @@ static u64 get_rc6(struct intel_gt *gt)
> >	spin_lock_irqsave(&pmu->lock, flags);
> >		if (awake) {
> > -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
> > +		pmu->sample[gt_id][__I915_SAMPLE_RC6].cur = val;
> >	} else {
> >		/*
> >		 * We think we are runtime suspended.
> > @@ -250,13 +223,13 @@ static u64 get_rc6(struct intel_gt *gt)
> >		 * counter value.
> >		 */
> >		val = ktime_since_raw(pmu->sleep_last[gt_id]);
> > -		val += read_sample(pmu, gt_id, __I915_SAMPLE_RC6);
> > +		val += pmu->sample[gt_id][__I915_SAMPLE_RC6].cur;
> >	}
> >   -	if (val < read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED))
> > -		val = read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED);
> > +	if (val < pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur)
> > +		val = pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur;
> >	else
> > -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED, val);
> > +		pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
> >		spin_unlock_irqrestore(&pmu->lock, flags);
> >   @@ -275,9 +248,8 @@ static void init_rc6(struct i915_pmu *pmu)
> >		with_intel_runtime_pm(gt->uncore->rpm, wakeref) {
> >			u64 val = __get_rc6(gt);
> >   -			store_sample(pmu, i, __I915_SAMPLE_RC6, val);
> > -			store_sample(pmu, i, __I915_SAMPLE_RC6_LAST_REPORTED,
> > -				     val);
> > +			pmu->sample[i][__I915_SAMPLE_RC6].cur = val;
> > +			pmu->sample[i][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
> >			pmu->sleep_last[i] = ktime_get_raw();
> >		}
> >	}
> > @@ -287,7 +259,7 @@ static void park_rc6(struct intel_gt *gt)
> >   {
> >	struct i915_pmu *pmu = &gt->i915->pmu;
> >   -	store_sample(pmu, gt->info.id, __I915_SAMPLE_RC6, __get_rc6(gt));
> > +	pmu->sample[gt->info.id][__I915_SAMPLE_RC6].cur = __get_rc6(gt);
> >	pmu->sleep_last[gt->info.id] = ktime_get_raw();
> >   }
> >   @@ -428,6 +400,12 @@ engines_sample(struct intel_gt *gt, unsigned int
> > period_ns)
> >	}
> >   }
> >   +static void
> > +add_sample_mult(struct i915_pmu_sample *sample, u32 val, u32 mul)
> > +{
> > +	sample->cur += mul_u32_u32(val, mul);
> > +}
> > +
> >   static bool
> >   frequency_sampling_enabled(struct i915_pmu *pmu, unsigned int gt)
> >   {
> > @@ -467,12 +445,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
> >		if (!val)
> >			val = intel_gpu_freq(rps, rps->cur_freq);
> >   -		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_ACT,
> > +		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT],
> >				val, period_ns / 1000);
> >	}
> >		if (pmu->enable &
> > config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt_id))) {
> > -		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_REQ,
> > +		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ],
> >				intel_rps_get_requested_frequency(rps),
> >				period_ns / 1000);
> >	}
> > @@ -673,14 +651,12 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
> >		switch (config) {
> >		case I915_PMU_ACTUAL_FREQUENCY:
> >			val =
> > -			   div_u64(read_sample(pmu, gt_id,
> > -					       __I915_SAMPLE_FREQ_ACT),
> > +			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT].cur,
> >				   USEC_PER_SEC /* to MHz */);
> >			break;
> >		case I915_PMU_REQUESTED_FREQUENCY:
> >			val =
> > -			   div_u64(read_sample(pmu, gt_id,
> > -					       __I915_SAMPLE_FREQ_REQ),
> > +			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ].cur,
> >				   USEC_PER_SEC /* to MHz */);
> >			break;
> >		case I915_PMU_INTERRUPTS:
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> > index 33d80fbaab8bc..d20592e7db999 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.h
> > +++ b/drivers/gpu/drm/i915/i915_pmu.h
> > @@ -127,7 +127,7 @@ struct i915_pmu {
> >	 * Only global counters are held here, while the per-engine ones are in
> >	 * struct intel_engine_cs.
> >	 */
> > -	struct i915_pmu_sample sample[I915_PMU_MAX_GTS * __I915_NUM_PMU_SAMPLERS];
> > +	struct i915_pmu_sample sample[I915_PMU_MAX_GTS][__I915_NUM_PMU_SAMPLERS];
> >	/**
> >	 * @sleep_last: Last time GT parked for RC6 estimation.
> >	 */

Re: [Intel-gfx] [PATCH 2/2] drm/i915/pmu: Make PMU sample array two-dimensional

[Tvrtko Ursulin]

On 24/05/2023 18:38, Dixit, Ashutosh wrote:
> On Wed, 24 May 2023 04:38:18 -0700, Tvrtko Ursulin wrote:
>>
> 
> Hi Tvrtko,
> 
>> On 23/05/2023 16:19, Ashutosh Dixit wrote:
>>> No functional changes but we can remove some unsightly index computation
>>> and read/write functions if we convert the PMU sample array from a
>>> one-dimensional to a two-dimensional array.
>>>
>>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_pmu.c | 60 ++++++++++-----------------------
>>>    drivers/gpu/drm/i915/i915_pmu.h |  2 +-
>>>    2 files changed, 19 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>>> index b47d890d4ada1..137e0df9573ee 100644
>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>> @@ -195,33 +195,6 @@ static inline s64 ktime_since_raw(const ktime_t kt)
>>> 	return ktime_to_ns(ktime_sub(ktime_get_raw(), kt));
>>>    }
>>>    -static unsigned int
>>> -__sample_idx(struct i915_pmu *pmu, unsigned int gt_id, int sample)
>>> -{
>>> -	unsigned int idx = gt_id * __I915_NUM_PMU_SAMPLERS + sample;
>>> -
>>> -	GEM_BUG_ON(idx >= ARRAY_SIZE(pmu->sample));
>>> -
>>> -	return idx;
>>> -}
>>> -
>>> -static u64 read_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample)
>>> -{
>>> -	return pmu->sample[__sample_idx(pmu, gt_id, sample)].cur;
>>> -}
>>> -
>>> -static void
>>> -store_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample, u64 val)
>>> -{
>>> -	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur = val;
>>> -}
>>> -
>>> -static void
>>> -add_sample_mult(struct i915_pmu *pmu, unsigned int gt_id, int sample, u32 val, u32 mul)
>>> -{
>>> -	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur += mul_u32_u32(val, mul);
>>> -}
>>
>> IMO read and store helpers could have stayed and just changed the
>> implementation. Like add_sample_mult which you just moved. I would have
>> been a smaller patch. So dunno.. a bit of a reluctant r-b.
> 
> Are you referring just to add_sample_mult or to all the other functions
> too? add_sample_mult I moved it to where it was before bc4be0a38b63

Read and store helpers.

> ("drm/i915/pmu: Prepare for multi-tile non-engine counters"), could have
> left it here I guess.
> 
> The other read and store helpers are not needed with the 2-d array at all
> since the compiler itself will do that, so I thought it was better to get
> rid of them completely.

Yes I get it, just that I didn't see the benefit of removing them.

For example:

  -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
  +		pmu->sample[gt_id][__I915_SAMPLE_RC6].cur = val;

It's a meh for me. Either flavour looks fine to me so I would have erred 
on the side of keeping the patch small. If anything I probably slightly 
prefer that the struct pmu_sample implementation was able to be changed 
with less churn before. For example. But a very minor argument really.

Or maybe next step is get rid of the struct i915_pmu_sample. It is a 
struct because originally previous value was tracked too. Then I removed 
that and it was easier to keep the struct. I guess it can go now and 
then the removal of helpers here will look somewhat nicer without the 
trailing .cur on every affected line.

> Let me know if you want any changes, otherwise I will leave as is.

You can leave it as is, I dont' mind much.

Regards,

Tvrtko

>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Thanks for the review. Thanks Andrzej too :)
> --
> Ashutosh
> 
>>> -
>>>    static u64 get_rc6(struct intel_gt *gt)
>>>    {
>>> 	struct drm_i915_private *i915 = gt->i915;
>>> @@ -240,7 +213,7 @@ static u64 get_rc6(struct intel_gt *gt)
>>> 	spin_lock_irqsave(&pmu->lock, flags);
>>> 		if (awake) {
>>> -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
>>> +		pmu->sample[gt_id][__I915_SAMPLE_RC6].cur = val;
>>> 	} else {
>>> 		/*
>>> 		 * We think we are runtime suspended.
>>> @@ -250,13 +223,13 @@ static u64 get_rc6(struct intel_gt *gt)
>>> 		 * counter value.
>>> 		 */
>>> 		val = ktime_since_raw(pmu->sleep_last[gt_id]);
>>> -		val += read_sample(pmu, gt_id, __I915_SAMPLE_RC6);
>>> +		val += pmu->sample[gt_id][__I915_SAMPLE_RC6].cur;
>>> 	}
>>>    -	if (val < read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED))
>>> -		val = read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED);
>>> +	if (val < pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur)
>>> +		val = pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur;
>>> 	else
>>> -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED, val);
>>> +		pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
>>> 		spin_unlock_irqrestore(&pmu->lock, flags);
>>>    @@ -275,9 +248,8 @@ static void init_rc6(struct i915_pmu *pmu)
>>> 		with_intel_runtime_pm(gt->uncore->rpm, wakeref) {
>>> 			u64 val = __get_rc6(gt);
>>>    -			store_sample(pmu, i, __I915_SAMPLE_RC6, val);
>>> -			store_sample(pmu, i, __I915_SAMPLE_RC6_LAST_REPORTED,
>>> -				     val);
>>> +			pmu->sample[i][__I915_SAMPLE_RC6].cur = val;
>>> +			pmu->sample[i][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
>>> 			pmu->sleep_last[i] = ktime_get_raw();
>>> 		}
>>> 	}
>>> @@ -287,7 +259,7 @@ static void park_rc6(struct intel_gt *gt)
>>>    {
>>> 	struct i915_pmu *pmu = &gt->i915->pmu;
>>>    -	store_sample(pmu, gt->info.id, __I915_SAMPLE_RC6, __get_rc6(gt));
>>> +	pmu->sample[gt->info.id][__I915_SAMPLE_RC6].cur = __get_rc6(gt);
>>> 	pmu->sleep_last[gt->info.id] = ktime_get_raw();
>>>    }
>>>    @@ -428,6 +400,12 @@ engines_sample(struct intel_gt *gt, unsigned int
>>> period_ns)
>>> 	}
>>>    }
>>>    +static void
>>> +add_sample_mult(struct i915_pmu_sample *sample, u32 val, u32 mul)
>>> +{
>>> +	sample->cur += mul_u32_u32(val, mul);
>>> +}
>>> +
>>>    static bool
>>>    frequency_sampling_enabled(struct i915_pmu *pmu, unsigned int gt)
>>>    {
>>> @@ -467,12 +445,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
>>> 		if (!val)
>>> 			val = intel_gpu_freq(rps, rps->cur_freq);
>>>    -		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_ACT,
>>> +		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT],
>>> 				val, period_ns / 1000);
>>> 	}
>>> 		if (pmu->enable &
>>> config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt_id))) {
>>> -		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_REQ,
>>> +		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ],
>>> 				intel_rps_get_requested_frequency(rps),
>>> 				period_ns / 1000);
>>> 	}
>>> @@ -673,14 +651,12 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
>>> 		switch (config) {
>>> 		case I915_PMU_ACTUAL_FREQUENCY:
>>> 			val =
>>> -			   div_u64(read_sample(pmu, gt_id,
>>> -					       __I915_SAMPLE_FREQ_ACT),
>>> +			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT].cur,
>>> 				   USEC_PER_SEC /* to MHz */);
>>> 			break;
>>> 		case I915_PMU_REQUESTED_FREQUENCY:
>>> 			val =
>>> -			   div_u64(read_sample(pmu, gt_id,
>>> -					       __I915_SAMPLE_FREQ_REQ),
>>> +			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ].cur,
>>> 				   USEC_PER_SEC /* to MHz */);
>>> 			break;
>>> 		case I915_PMU_INTERRUPTS:
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
>>> index 33d80fbaab8bc..d20592e7db999 100644
>>> --- a/drivers/gpu/drm/i915/i915_pmu.h
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.h
>>> @@ -127,7 +127,7 @@ struct i915_pmu {
>>> 	 * Only global counters are held here, while the per-engine ones are in
>>> 	 * struct intel_engine_cs.
>>> 	 */
>>> -	struct i915_pmu_sample sample[I915_PMU_MAX_GTS * __I915_NUM_PMU_SAMPLERS];
>>> +	struct i915_pmu_sample sample[I915_PMU_MAX_GTS][__I915_NUM_PMU_SAMPLERS];
>>> 	/**
>>> 	 * @sleep_last: Last time GT parked for RC6 estimation.
>>> 	 */

Re: [Intel-gfx] [PATCH 2/2] drm/i915/pmu: Make PMU sample array two-dimensional

[Dixit, Ashutosh]

On Wed, 24 May 2023 10:53:20 -0700, Tvrtko Ursulin wrote:
>

Hi Tvrtko,

> On 24/05/2023 18:38, Dixit, Ashutosh wrote:
> > On Wed, 24 May 2023 04:38:18 -0700, Tvrtko Ursulin wrote:
> >> On 23/05/2023 16:19, Ashutosh Dixit wrote:
> >>> No functional changes but we can remove some unsightly index computation
> >>> and read/write functions if we convert the PMU sample array from a
> >>> one-dimensional to a two-dimensional array.
> >>>
> >>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_pmu.c | 60 ++++++++++-----------------------
> >>>    drivers/gpu/drm/i915/i915_pmu.h |  2 +-
> >>>    2 files changed, 19 insertions(+), 43 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> >>> index b47d890d4ada1..137e0df9573ee 100644
> >>> --- a/drivers/gpu/drm/i915/i915_pmu.c
> >>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> >>> @@ -195,33 +195,6 @@ static inline s64 ktime_since_raw(const ktime_t kt)
> >>>	return ktime_to_ns(ktime_sub(ktime_get_raw(), kt));
> >>>    }
> >>>    -static unsigned int
> >>> -__sample_idx(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> >>> -{
> >>> -	unsigned int idx = gt_id * __I915_NUM_PMU_SAMPLERS + sample;
> >>> -
> >>> -	GEM_BUG_ON(idx >= ARRAY_SIZE(pmu->sample));
> >>> -
> >>> -	return idx;
> >>> -}
> >>> -
> >>> -static u64 read_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample)
> >>> -{
> >>> -	return pmu->sample[__sample_idx(pmu, gt_id, sample)].cur;
> >>> -}
> >>> -
> >>> -static void
> >>> -store_sample(struct i915_pmu *pmu, unsigned int gt_id, int sample, u64 val)
> >>> -{
> >>> -	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur = val;
> >>> -}
> >>> -
> >>> -static void
> >>> -add_sample_mult(struct i915_pmu *pmu, unsigned int gt_id, int sample, u32 val, u32 mul)
> >>> -{
> >>> -	pmu->sample[__sample_idx(pmu, gt_id, sample)].cur += mul_u32_u32(val, mul);
> >>> -}
> >>
> >> IMO read and store helpers could have stayed and just changed the
> >> implementation. Like add_sample_mult which you just moved. I would have
> >> been a smaller patch. So dunno.. a bit of a reluctant r-b.
> >
> > Are you referring just to add_sample_mult or to all the other functions
> > too? add_sample_mult I moved it to where it was before bc4be0a38b63
>
> Read and store helpers.
>
> > ("drm/i915/pmu: Prepare for multi-tile non-engine counters"), could have
> > left it here I guess.
> >
> > The other read and store helpers are not needed with the 2-d array at all
> > since the compiler itself will do that, so I thought it was better to get
> > rid of them completely.
>
> Yes I get it, just that I didn't see the benefit of removing them.
>
> For example:
>
>  -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
>  +		pmu->sample[gt_id][__I915_SAMPLE_RC6].cur = val;
>
> It's a meh for me. Either flavour looks fine to me so I would have erred on
> the side of keeping the patch small. If anything I probably slightly prefer
> that the struct pmu_sample implementation was able to be changed with less
> churn before. For example. But a very minor argument really.

OK, I finally understood and have made this change in Patch v2. Please take
a look.

>
> Or maybe next step is get rid of the struct i915_pmu_sample. It is a struct
> because originally previous value was tracked too. Then I removed that and
> it was easier to keep the struct. I guess it can go now and then the
> removal of helpers here will look somewhat nicer without the trailing .cur
> on every affected line.

I have left this as is for now in case the i915_pmu_sample need to be
expanded again. Should be ok with the read/store helpers.

>
> > Let me know if you want any changes, otherwise I will leave as is.
>
> You can leave it as is, I dont' mind much.

I went ahead and changed it anyway since you seemed to want it.

Thanks.
--
Ashutosh

>
> >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> > Thanks for the review. Thanks Andrzej too :)
> > --
> > Ashutosh
> >
> >>> -
> >>>    static u64 get_rc6(struct intel_gt *gt)
> >>>    {
> >>>	struct drm_i915_private *i915 = gt->i915;
> >>> @@ -240,7 +213,7 @@ static u64 get_rc6(struct intel_gt *gt)
> >>>	spin_lock_irqsave(&pmu->lock, flags);
> >>>		if (awake) {
> >>> -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6, val);
> >>> +		pmu->sample[gt_id][__I915_SAMPLE_RC6].cur = val;
> >>>	} else {
> >>>		/*
> >>>		 * We think we are runtime suspended.
> >>> @@ -250,13 +223,13 @@ static u64 get_rc6(struct intel_gt *gt)
> >>>		 * counter value.
> >>>		 */
> >>>		val = ktime_since_raw(pmu->sleep_last[gt_id]);
> >>> -		val += read_sample(pmu, gt_id, __I915_SAMPLE_RC6);
> >>> +		val += pmu->sample[gt_id][__I915_SAMPLE_RC6].cur;
> >>>	}
> >>>    -	if (val < read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED))
> >>> -		val = read_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED);
> >>> +	if (val < pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur)
> >>> +		val = pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur;
> >>>	else
> >>> -		store_sample(pmu, gt_id, __I915_SAMPLE_RC6_LAST_REPORTED, val);
> >>> +		pmu->sample[gt_id][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
> >>>		spin_unlock_irqrestore(&pmu->lock, flags);
> >>>    @@ -275,9 +248,8 @@ static void init_rc6(struct i915_pmu *pmu)
> >>>		with_intel_runtime_pm(gt->uncore->rpm, wakeref) {
> >>>			u64 val = __get_rc6(gt);
> >>>    -			store_sample(pmu, i, __I915_SAMPLE_RC6, val);
> >>> -			store_sample(pmu, i, __I915_SAMPLE_RC6_LAST_REPORTED,
> >>> -				     val);
> >>> +			pmu->sample[i][__I915_SAMPLE_RC6].cur = val;
> >>> +			pmu->sample[i][__I915_SAMPLE_RC6_LAST_REPORTED].cur = val;
> >>>			pmu->sleep_last[i] = ktime_get_raw();
> >>>		}
> >>>	}
> >>> @@ -287,7 +259,7 @@ static void park_rc6(struct intel_gt *gt)
> >>>    {
> >>>	struct i915_pmu *pmu = &gt->i915->pmu;
> >>>    -	store_sample(pmu, gt->info.id, __I915_SAMPLE_RC6, __get_rc6(gt));
> >>> +	pmu->sample[gt->info.id][__I915_SAMPLE_RC6].cur = __get_rc6(gt);
> >>>	pmu->sleep_last[gt->info.id] = ktime_get_raw();
> >>>    }
> >>>    @@ -428,6 +400,12 @@ engines_sample(struct intel_gt *gt, unsigned int
> >>> period_ns)
> >>>	}
> >>>    }
> >>>    +static void
> >>> +add_sample_mult(struct i915_pmu_sample *sample, u32 val, u32 mul)
> >>> +{
> >>> +	sample->cur += mul_u32_u32(val, mul);
> >>> +}
> >>> +
> >>>    static bool
> >>>    frequency_sampling_enabled(struct i915_pmu *pmu, unsigned int gt)
> >>>    {
> >>> @@ -467,12 +445,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
> >>>		if (!val)
> >>>			val = intel_gpu_freq(rps, rps->cur_freq);
> >>>    -		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_ACT,
> >>> +		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT],
> >>>				val, period_ns / 1000);
> >>>	}
> >>>		if (pmu->enable &
> >>> config_mask(__I915_PMU_REQUESTED_FREQUENCY(gt_id))) {
> >>> -		add_sample_mult(pmu, gt_id, __I915_SAMPLE_FREQ_REQ,
> >>> +		add_sample_mult(&pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ],
> >>>				intel_rps_get_requested_frequency(rps),
> >>>				period_ns / 1000);
> >>>	}
> >>> @@ -673,14 +651,12 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
> >>>		switch (config) {
> >>>		case I915_PMU_ACTUAL_FREQUENCY:
> >>>			val =
> >>> -			   div_u64(read_sample(pmu, gt_id,
> >>> -					       __I915_SAMPLE_FREQ_ACT),
> >>> +			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_ACT].cur,
> >>>				   USEC_PER_SEC /* to MHz */);
> >>>			break;
> >>>		case I915_PMU_REQUESTED_FREQUENCY:
> >>>			val =
> >>> -			   div_u64(read_sample(pmu, gt_id,
> >>> -					       __I915_SAMPLE_FREQ_REQ),
> >>> +			   div_u64(pmu->sample[gt_id][__I915_SAMPLE_FREQ_REQ].cur,
> >>>				   USEC_PER_SEC /* to MHz */);
> >>>			break;
> >>>		case I915_PMU_INTERRUPTS:
> >>> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> >>> index 33d80fbaab8bc..d20592e7db999 100644
> >>> --- a/drivers/gpu/drm/i915/i915_pmu.h
> >>> +++ b/drivers/gpu/drm/i915/i915_pmu.h
> >>> @@ -127,7 +127,7 @@ struct i915_pmu {
> >>>	 * Only global counters are held here, while the per-engine ones are in
> >>>	 * struct intel_engine_cs.
> >>>	 */
> >>> -	struct i915_pmu_sample sample[I915_PMU_MAX_GTS * __I915_NUM_PMU_SAMPLERS];
> >>> +	struct i915_pmu_sample sample[I915_PMU_MAX_GTS][__I915_NUM_PMU_SAMPLERS];
> >>>	/**
> >>>	 * @sleep_last: Last time GT parked for RC6 estimation.
> >>>	 */

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/pmu: couple of cleanups

[Patchwork]

== Series Details ==

Series: drm/i915/pmu: couple of cleanups
URL   : https://patchwork.freedesktop.org/series/118225/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/pmu: couple of cleanups

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5762 bytes --]

== Series Details ==

Series: drm/i915/pmu: couple of cleanups
URL   : https://patchwork.freedesktop.org/series/118225/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13177 -> Patchwork_118225v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/index.html

Participating hosts (39 -> 37)
------------------------------

  Additional (1): fi-kbl-soraka 
  Missing    (3): bat-adlm-1 fi-snb-2520m bat-mtlp-6 

Known issues
------------

  Here are the changes found in Patchwork_118225v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][1] ([fdo#109271] / [i915#2190])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][2] ([fdo#109271] / [i915#4613]) +3 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][3] ([i915#1886] / [i915#7913])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@i915_selftest@live@mman:
    - bat-rpls-2:         [PASS][4] -> [TIMEOUT][5] ([i915#6794] / [i915#7392])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/bat-rpls-2/igt@i915_selftest@live@mman.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/bat-rpls-2/igt@i915_selftest@live@mman.html

  * igt@i915_suspend@basic-s2idle-without-i915:
    - bat-rpls-2:         NOTRUN -> [ABORT][6] ([i915#6687])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/bat-rpls-2/igt@i915_suspend@basic-s2idle-without-i915.html

  * igt@i915_suspend@basic-s3-without-i915:
    - bat-rpls-1:         NOTRUN -> [ABORT][7] ([i915#6687] / [i915#7978])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/bat-rpls-1/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_chamelium_frames@hdmi-crc-fast:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][8] ([fdo#109271]) +14 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/fi-kbl-soraka/igt@kms_chamelium_frames@hdmi-crc-fast.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-1:
    - bat-dg2-8:          [PASS][9] -> [FAIL][10] ([i915#7932])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-1.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-1.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][11] ([fdo#109271] / [i915#4579])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/fi-kbl-soraka/igt@kms_setmode@basic-clone-single-crtc.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@reset:
    - bat-rpls-1:         [ABORT][12] ([i915#4983] / [i915#7461] / [i915#8347] / [i915#8384]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/bat-rpls-1/igt@i915_selftest@live@reset.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/bat-rpls-1/igt@i915_selftest@live@reset.html

  * igt@i915_selftest@live@slpc:
    - {bat-mtlp-8}:       [DMESG-WARN][14] ([i915#6367]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/bat-mtlp-8/igt@i915_selftest@live@slpc.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/bat-mtlp-8/igt@i915_selftest@live@slpc.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#6794]: https://gitlab.freedesktop.org/drm/intel/issues/6794
  [i915#7392]: https://gitlab.freedesktop.org/drm/intel/issues/7392
  [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978
  [i915#8347]: https://gitlab.freedesktop.org/drm/intel/issues/8347
  [i915#8384]: https://gitlab.freedesktop.org/drm/intel/issues/8384


Build changes
-------------

  * Linux: CI_DRM_13177 -> Patchwork_118225v1

  CI-20190529: 20190529
  CI_DRM_13177: 073f9eab1a2a1e53428907d3b26503ed17516be5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7301: 4b388fa87e1281587e723ef864e466fe396c3381 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_118225v1: 073f9eab1a2a1e53428907d3b26503ed17516be5 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

07cfff7dc878 drm/i915/pmu: Make PMU sample array two-dimensional
5857f6877384 drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/index.html

[-- Attachment #2: Type: text/html, Size: 6906 bytes --]

[Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/pmu: couple of cleanups

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 13873 bytes --]

== Series Details ==

Series: drm/i915/pmu: couple of cleanups
URL   : https://patchwork.freedesktop.org/series/118225/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13177_full -> Patchwork_118225v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts

Known issues
------------

  Here are the changes found in Patchwork_118225v1_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@drm_mm@all-tests:
    - shard-glk:          NOTRUN -> [SKIP][1] ([fdo#109271] / [i915#4579])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk9/igt@drm_mm@all-tests.html

  * igt@gem_exec_fair@basic-pace@rcs0:
    - shard-glk:          [PASS][2] -> [FAIL][3] ([i915#2842]) +1 similar issue
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-glk8/igt@gem_exec_fair@basic-pace@rcs0.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk1/igt@gem_exec_fair@basic-pace@rcs0.html

  * igt@gen9_exec_parse@allowed-single:
    - shard-glk:          [PASS][4] -> [ABORT][5] ([i915#5566])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-glk2/igt@gen9_exec_parse@allowed-single.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk9/igt@gen9_exec_parse@allowed-single.html

  * igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a:
    - shard-glk:          NOTRUN -> [SKIP][6] ([fdo#109271] / [i915#1937] / [i915#4579])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk9/igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a.html

  * igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-hflip:
    - shard-glk:          NOTRUN -> [SKIP][7] ([fdo#109271]) +14 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk9/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-hflip.html

  * igt@kms_ccs@pipe-c-ccs-on-another-bo-y_tiled_gen12_rc_ccs_cc:
    - shard-glk:          NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#3886])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk9/igt@kms_ccs@pipe-c-ccs-on-another-bo-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_color@ctm-red-to-blue@pipe-b-vga-1:
    - shard-snb:          NOTRUN -> [SKIP][9] ([fdo#109271] / [i915#4579]) +13 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-snb4/igt@kms_color@ctm-red-to-blue@pipe-b-vga-1.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
    - shard-apl:          [PASS][10] -> [FAIL][11] ([IGT#6] / [i915#2346])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-apl3/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-apl3/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html

  * igt@kms_flip@flip-vs-expired-vblank@c-hdmi-a1:
    - shard-glk:          [PASS][12] -> [FAIL][13] ([i915#2122])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-glk7/igt@kms_flip@flip-vs-expired-vblank@c-hdmi-a1.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk7/igt@kms_flip@flip-vs-expired-vblank@c-hdmi-a1.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-scndscrn-spr-indfb-draw-mmap-wc:
    - shard-glk:          NOTRUN -> [SKIP][14] ([IGT#6] / [fdo#109271]) +12 similar issues
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk9/igt@kms_frontbuffer_tracking@fbcpsr-2p-scndscrn-spr-indfb-draw-mmap-wc.html

  * igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-hdmi-a-1:
    - shard-snb:          NOTRUN -> [SKIP][15] ([fdo#109271]) +16 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-snb1/igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-hdmi-a-1.html

  * igt@kms_psr2_sf@overlay-plane-update-continuous-sf:
    - shard-glk:          NOTRUN -> [SKIP][16] ([fdo#109271] / [i915#658])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk9/igt@kms_psr2_sf@overlay-plane-update-continuous-sf.html

  
#### Possible fixes ####

  * igt@drm_fdinfo@most-busy-check-all@rcs0:
    - {shard-rkl}:        [FAIL][17] ([i915#7742]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-rkl-6/igt@drm_fdinfo@most-busy-check-all@rcs0.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-rkl-1/igt@drm_fdinfo@most-busy-check-all@rcs0.html

  * igt@gem_barrier_race@remote-request@rcs0:
    - shard-glk:          [ABORT][19] ([i915#7461] / [i915#8211]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-glk7/igt@gem_barrier_race@remote-request@rcs0.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk9/igt@gem_barrier_race@remote-request@rcs0.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-glk:          [FAIL][21] ([i915#2842]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-glk7/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk8/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_suspend@basic-s4-devices@lmem0:
    - {shard-dg1}:        [ABORT][23] ([i915#7975] / [i915#8213]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-dg1-14/igt@gem_exec_suspend@basic-s4-devices@lmem0.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-dg1-15/igt@gem_exec_suspend@basic-s4-devices@lmem0.html

  * igt@gem_ppgtt@blt-vs-render-ctx0:
    - shard-snb:          [FAIL][25] ([i915#8295]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-snb6/igt@gem_ppgtt@blt-vs-render-ctx0.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-snb5/igt@gem_ppgtt@blt-vs-render-ctx0.html

  * igt@i915_pm_rc6_residency@rc6-idle@bcs0:
    - {shard-dg1}:        [FAIL][27] ([i915#3591]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-dg1-13/igt@i915_pm_rc6_residency@rc6-idle@bcs0.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-dg1-15/igt@i915_pm_rc6_residency@rc6-idle@bcs0.html

  * igt@i915_pm_rpm@dpms-non-lpsp:
    - {shard-rkl}:        [SKIP][29] ([i915#1397]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-rkl-7/igt@i915_pm_rpm@dpms-non-lpsp.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-rkl-3/igt@i915_pm_rpm@dpms-non-lpsp.html

  * igt@i915_pm_rps@engine-order:
    - shard-apl:          [FAIL][31] ([i915#6537]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-apl4/igt@i915_pm_rps@engine-order.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-apl1/igt@i915_pm_rps@engine-order.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-apl:          [FAIL][33] ([IGT#6] / [i915#2346]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-apl6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-apl2/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_cursor_legacy@forked-bo@pipe-b:
    - {shard-rkl}:        [INCOMPLETE][35] ([i915#8011]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-rkl-7/igt@kms_cursor_legacy@forked-bo@pipe-b.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-rkl-3/igt@kms_cursor_legacy@forked-bo@pipe-b.html

  * igt@kms_flip@2x-flip-vs-expired-vblank@ac-hdmi-a1-hdmi-a2:
    - shard-glk:          [FAIL][37] ([i915#79]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13177/shard-glk8/igt@kms_flip@2x-flip-vs-expired-vblank@ac-hdmi-a1-hdmi-a2.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/shard-glk7/igt@kms_flip@2x-flip-vs-expired-vblank@ac-hdmi-a1-hdmi-a2.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [IGT#2]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/2
  [IGT#6]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/6
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1937]: https://gitlab.freedesktop.org/drm/intel/issues/1937
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5566]: https://gitlab.freedesktop.org/drm/intel/issues/5566
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
  [i915#6493]: https://gitlab.freedesktop.org/drm/intel/issues/6493
  [i915#6537]: https://gitlab.freedesktop.org/drm/intel/issues/6537
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#7975]: https://gitlab.freedesktop.org/drm/intel/issues/7975
  [i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011
  [i915#8211]: https://gitlab.freedesktop.org/drm/intel/issues/8211
  [i915#8213]: https://gitlab.freedesktop.org/drm/intel/issues/8213
  [i915#8292]: https://gitlab.freedesktop.org/drm/intel/issues/8292
  [i915#8295]: https://gitlab.freedesktop.org/drm/intel/issues/8295
  [i915#8411]: https://gitlab.freedesktop.org/drm/intel/issues/8411
  [i915#8414]: https://gitlab.freedesktop.org/drm/intel/issues/8414


Build changes
-------------

  * Linux: CI_DRM_13177 -> Patchwork_118225v1
  * Piglit: None -> piglit_4509

  CI-20190529: 20190529
  CI_DRM_13177: 073f9eab1a2a1e53428907d3b26503ed17516be5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7301: 4b388fa87e1281587e723ef864e466fe396c3381 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_118225v1: 073f9eab1a2a1e53428907d3b26503ed17516be5 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118225v1/index.html

[-- Attachment #2: Type: text/html, Size: 12657 bytes --]

[Intel-gfx] [PATCH v2 0/2] drm/i915/pmu: couple of cleanups

[Ashutosh Dixit]

Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>

Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

Ashutosh Dixit (2):
  drm/i915/pmu: Turn off the timer to sample frequencies when GT is
    parked
  drm/i915/pmu: Make PMU sample array two-dimensional

 drivers/gpu/drm/i915/i915_pmu.c | 32 ++++++++------------------------
 drivers/gpu/drm/i915/i915_pmu.h |  2 +-
 2 files changed, 9 insertions(+), 25 deletions(-)

-- 
2.38.0

[Intel-gfx] [PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

[Ashutosh Dixit]

pmu_needs_timer() keeps the timer running even when GT is parked,
ostensibly to sample requested/actual frequencies. However
frequency_sample() has the following:

	/* Report 0/0 (actual/requested) frequency while parked. */
	if (!intel_gt_pm_get_if_awake(gt))
		return;

The above code prevents frequencies to be sampled while the GT is
parked. So we might as well turn off the sampling timer itself in this
case and save CPU cycles/power.

v2: Instead of turning freq bits off, return false, since no counters will
    run after this change when GT is parked (Tvrtko)
v3: Remove gpu_active argument of pmu_needs_timer (Andrzej)

Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index a814583e19fd7..09313cf9316b4 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -139,7 +139,7 @@ static u32 frequency_enabled_mask(void)
 	return mask;
 }
 
-static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
+static bool pmu_needs_timer(struct i915_pmu *pmu)
 {
 	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
 	u32 enable;
@@ -157,17 +157,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
 	 */
 	enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
 
-	/*
-	 * When the GPU is idle per-engine counters do not need to be
-	 * running so clear those bits out.
-	 */
-	if (!gpu_active)
-		enable &= ~ENGINE_SAMPLE_MASK;
 	/*
 	 * Also there is software busyness tracking available we do not
 	 * need the timer for I915_SAMPLE_BUSY counter.
 	 */
-	else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
+	if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
 		enable &= ~BIT(I915_SAMPLE_BUSY);
 
 	/*
@@ -295,7 +289,7 @@ static void park_rc6(struct intel_gt *gt)
 
 static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
 {
-	if (!pmu->timer_enabled && pmu_needs_timer(pmu, true)) {
+	if (!pmu->timer_enabled && pmu_needs_timer(pmu)) {
 		pmu->timer_enabled = true;
 		pmu->timer_last = ktime_get();
 		hrtimer_start_range_ns(&pmu->timer,
@@ -321,7 +315,7 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
 	 */
 	pmu->unparked &= ~BIT(gt->info.id);
 	if (pmu->unparked == 0)
-		pmu->timer_enabled = pmu_needs_timer(pmu, false);
+		pmu->timer_enabled = false;
 
 	spin_unlock_irq(&pmu->lock);
 }
@@ -827,7 +821,7 @@ static void i915_pmu_disable(struct perf_event *event)
 	 */
 	if (--pmu->enable_count[bit] == 0) {
 		pmu->enable &= ~BIT(bit);
-		pmu->timer_enabled &= pmu_needs_timer(pmu, true);
+		pmu->timer_enabled &= pmu_needs_timer(pmu);
 	}
 
 	spin_unlock_irqrestore(&pmu->lock, flags);
-- 
2.38.0

Re: [Intel-gfx] [PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

[Andrzej Hajda]

On 24.05.2023 23:56, Ashutosh Dixit wrote:
> pmu_needs_timer() keeps the timer running even when GT is parked,
> ostensibly to sample requested/actual frequencies. However
> frequency_sample() has the following:
>
> 	/* Report 0/0 (actual/requested) frequency while parked. */
> 	if (!intel_gt_pm_get_if_awake(gt))
> 		return;
>
> The above code prevents frequencies to be sampled while the GT is
> parked. So we might as well turn off the sampling timer itself in this
> case and save CPU cycles/power.
>
> v2: Instead of turning freq bits off, return false, since no counters will
>      run after this change when GT is parked (Tvrtko)
> v3: Remove gpu_active argument of pmu_needs_timer (Andrzej)
>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej
> ---
>   drivers/gpu/drm/i915/i915_pmu.c | 16 +++++-----------
>   1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index a814583e19fd7..09313cf9316b4 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -139,7 +139,7 @@ static u32 frequency_enabled_mask(void)
>   	return mask;
>   }
>   
> -static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
> +static bool pmu_needs_timer(struct i915_pmu *pmu)
>   {
>   	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
>   	u32 enable;
> @@ -157,17 +157,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
>   	 */
>   	enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
>   
> -	/*
> -	 * When the GPU is idle per-engine counters do not need to be
> -	 * running so clear those bits out.
> -	 */
> -	if (!gpu_active)
> -		enable &= ~ENGINE_SAMPLE_MASK;
>   	/*
>   	 * Also there is software busyness tracking available we do not
>   	 * need the timer for I915_SAMPLE_BUSY counter.
>   	 */
> -	else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
> +	if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
>   		enable &= ~BIT(I915_SAMPLE_BUSY);
>   
>   	/*
> @@ -295,7 +289,7 @@ static void park_rc6(struct intel_gt *gt)
>   
>   static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
>   {
> -	if (!pmu->timer_enabled && pmu_needs_timer(pmu, true)) {
> +	if (!pmu->timer_enabled && pmu_needs_timer(pmu)) {
>   		pmu->timer_enabled = true;
>   		pmu->timer_last = ktime_get();
>   		hrtimer_start_range_ns(&pmu->timer,
> @@ -321,7 +315,7 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
>   	 */
>   	pmu->unparked &= ~BIT(gt->info.id);
>   	if (pmu->unparked == 0)
> -		pmu->timer_enabled = pmu_needs_timer(pmu, false);
> +		pmu->timer_enabled = false;
>   
>   	spin_unlock_irq(&pmu->lock);
>   }
> @@ -827,7 +821,7 @@ static void i915_pmu_disable(struct perf_event *event)
>   	 */
>   	if (--pmu->enable_count[bit] == 0) {
>   		pmu->enable &= ~BIT(bit);
> -		pmu->timer_enabled &= pmu_needs_timer(pmu, true);
> +		pmu->timer_enabled &= pmu_needs_timer(pmu);
>   	}
>   
>   	spin_unlock_irqrestore(&pmu->lock, flags);