[Intel-gfx] [RFC v5 05/11] drm/i915/pmu: Suspend sampling when GPU is idle

[Tvrtko Ursulin]

On 14/09/2017 20:57, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-09-13 11:34:17)
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> If only a subset of events is enabled we can afford to suspend
>> the sampling timer when the GPU is idle and so save some cycles
>> and power.
>>
>> v2: Rebase and limit timer even more.
>> v3: Rebase.
>> v4: Rebase.
>> v5: Skip action if perf PMU failed to register.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h         |  8 ++++
>>   drivers/gpu/drm/i915/i915_gem.c         |  1 +
>>   drivers/gpu/drm/i915/i915_gem_request.c |  1 +
>>   drivers/gpu/drm/i915/i915_pmu.c         | 70 ++++++++++++++++++++++++++++-----
>>   4 files changed, 70 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 62646b8dfb7a..70be8c5d9a65 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2244,6 +2244,10 @@ struct i915_pmu {
>>           */
>>          unsigned int enable_count[I915_PMU_MASK_BITS];
>>          /**
>> +        * @timer_enabled: Should the internal sampling timer be running.
>> +        */
>> +       bool timer_enabled;
>> +       /**
>>           * @sample: Current counter value for i915 events which need sampling.
>>           *
>>           * These counters are updated from the i915 PMU sampling timer.
>> @@ -3989,9 +3993,13 @@ extern void i915_perf_unregister(struct drm_i915_private *dev_priv);
>>   #ifdef CONFIG_PERF_EVENTS
>>   extern void i915_pmu_register(struct drm_i915_private *i915);
>>   extern void i915_pmu_unregister(struct drm_i915_private *i915);
>> +extern void i915_pmu_gt_idle(struct drm_i915_private *i915);
>> +extern void i915_pmu_gt_active(struct drm_i915_private *i915);
>>   #else
>>   static inline void i915_pmu_register(struct drm_i915_private *i915) {}
>>   static inline void i915_pmu_unregister(struct drm_i915_private *i915) {}
>> +static inline void i915_pmu_gt_idle(struct drm_i915_private *i915) {}
>> +static inline void i915_pmu_gt_active(struct drm_i915_private *i915) {}
>>   #endif
>>   
>>   /* i915_suspend.c */
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index f445587c1a4b..201b09eda93b 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3227,6 +3227,7 @@ i915_gem_idle_work_handler(struct work_struct *work)
>>   
>>          intel_engines_mark_idle(dev_priv);
>>          i915_gem_timelines_mark_idle(dev_priv);
>> +       i915_pmu_gt_idle(dev_priv);
>>   
>>          GEM_BUG_ON(!dev_priv->gt.awake);
>>          dev_priv->gt.awake = false;
>> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
>> index 813a3b546d6e..18a1e379253e 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_request.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
>> @@ -258,6 +258,7 @@ static void mark_busy(struct drm_i915_private *i915)
>>          i915_update_gfx_val(i915);
>>          if (INTEL_GEN(i915) >= 6)
>>                  gen6_rps_busy(i915);
>> +       i915_pmu_gt_active(i915);
>>   
>>          queue_delayed_work(i915->wq,
>>                             &i915->gt.retire_work,
>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>> index e82648e6635b..22246918757c 100644
>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>> @@ -90,6 +90,52 @@ static unsigned int event_enabled_bit(struct perf_event *event)
>>          return config_enabled_bit(event->attr.config);
>>   }
>>   
>> +static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
>> +{
>> +       u64 enable = i915->pmu.enable;
>> +
>> +       enable &= config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY) |
>> +                 config_enabled_mask(I915_PMU_REQUESTED_FREQUENCY) |
>> +                 ENGINE_SAMPLE_MASK;
>> +
>> +       if (!gpu_active)
>> +               enable &= ~ENGINE_SAMPLE_MASK;
> 
> Ok.

I'll add a comment here. It gets even more advanced when busy stats are 
added.

>> +
>> +       return enable;
>> +}
>> +
>> +void i915_pmu_gt_idle(struct drm_i915_private *i915)
>> +{
>> +       if (!i915->pmu.base.event_init)
>> +               return;
>> +
>> +       spin_lock_irq(&i915->pmu.lock);
>> +       /*
>> +        * Signal sampling timer to stop if only engine events are enabled and
>> +        * GPU went idle.
>> +        */
>> +       i915->pmu.timer_enabled = pmu_needs_timer(i915, false);
>> +       spin_unlock_irq(&i915->pmu.lock);
>> +}
>> +
>> +void i915_pmu_gt_active(struct drm_i915_private *i915)
>> +{
>> +       if (!i915->pmu.base.event_init)
>> +               return;
>> +
>> +       spin_lock_irq(&i915->pmu.lock);
>> +       /*
>> +        * Re-enable sampling timer when GPU goes active.
>> +        */
>> +       if (!i915->pmu.timer_enabled && pmu_needs_timer(i915, true)) {
>> +               i915->pmu.timer_enabled = true;
>> +               hrtimer_start_range_ns(&i915->pmu.timer,
>> +                                      ns_to_ktime(PERIOD), 0,
>> +                                      HRTIMER_MODE_REL_PINNED);
>> +       }
> 
> Duplication here, __i915_pmu_gt_active() for the critical section. Then
> also helps allay fears that we hold the spinlock which is not clear from
> the diff context below.

Agreed, good cleanup suggestion.

Regards,

Tvrtko