Re: [Intel-gfx] [RFC v2 3/3] drm/i915/pmu: deny perf driver level sampling of i915 PMU

[Peter Zijlstra]

On Wed, Aug 23, 2017 at 11:43:13PM +, Rogozhkin, Dmitry V wrote:
> Hi Chris,
> 
> Why we had event->hw->hrtimer in i915 PMU? Was there any particular
> reason? You had some use case which did not work?

Some uncore drivers have a hrtimer to poll the counter to deal with
counter overflow. This is needed when the hardware has 'short' counters.

For instance, the regular uncore stuff has 48 bit counters, so we need a
timer to occasionally read them to fold the delta into our 64bit counter
value.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC v2 3/3] drm/i915/pmu: deny perf driver level sampling of i915 PMU

[Rogozhkin, Dmitry V]

Returning mailing list back.

Just tried ./intel-gpu-overlay. From what I see it wonderfully works even when 
I wiped out event->sampling_period from i915 RFC PMU and prohibited it. Mind 
that I did not remove sampling inside i915 RFC PMU: I removed only timer staff 
which is exposed to the perf core. Sampling itself is still there and works as 
far as I see.

Thus, the question: why event->hw->hrtimer staff was introduced in i915 RFC 
PMU? Right now it does not make sense to me.

Dmitry.

-Original Message-
From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] 
Sent: Friday, August 25, 2017 11:19 AM
To: Rogozhkin, Dmitry V ; Wilson, Chris 

Subject: RE: [RFC v2 3/3] drm/i915/pmu: deny perf driver level sampling of i915 
PMU

Quoting Rogozhkin, Dmitry V (2017-08-25 19:06:13)
> Hi Chris, not sure you saw my mail. So, I try to remind.

It's integral to all the sampling counters we use; which are the majority of 
the events exposed and used by intel-gpu-overlay.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC v2 3/3] drm/i915/pmu: deny perf driver level sampling of i915 PMU

[Rogozhkin, Dmitry V]

Hi Chris,

Why we had event->hw->hrtimer in i915 PMU? Was there any particular
reason? You had some use case which did not work?

According to Peter we should not expose the timer out of our pmu, and I
do not see the reason why we need it at the first place. So, I went
forward and wiped it out and prohibited events to be intialized with the
sampling_period. I don't see what will be broken. From my perspective
nothing because internal sampling timer still remains.

Could you, please, comment?

Dmitry.

On Wed, 2017-08-23 at 08:26 -0700, Dmitry Rogozhkin wrote:
> This patch should probably be squashed with Tvrtko's PMU enabling patch...
> 
> As per discussion with Peter, i915 PMU is an example of uncore PMU which
> are prohibited to support perf driver level sampling. This patch removes
> hrtimer which we expose to perf core and denies events creation with
> non-zero event->attr.sampling_period.
> 
> Mind that this patch does _not_ remove i915 PMU _internal_ sampling timer.
> So, sampling metrics are still gathered, but can be accessed only by
> explicit request to get metric counter, i.e. by sys_read().
> 
> Change-Id: I33f345f679f0a5a8ecc9867f9e7c1bfb357e708d
> Signed-off-by: Dmitry Rogozhkin 
> Cc: Tvrtko Ursulin 
> Cc: Chris Wilson 
> Cc: Peter Zijlstra 
> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 89 
> ++---
>  1 file changed, 4 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index c551d64..311aeeb 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -239,50 +239,6 @@ static int engine_event_init(struct perf_event *event)
>   return 0;
>  }
>  
> -static DEFINE_PER_CPU(struct pt_regs, i915_pmu_pt_regs);
> -
> -static enum hrtimer_restart hrtimer_sample(struct hrtimer *hrtimer)
> -{
> - struct pt_regs *regs = this_cpu_ptr(_pmu_pt_regs);
> - struct perf_sample_data data;
> - struct perf_event *event;
> - u64 period;
> -
> - event = container_of(hrtimer, struct perf_event, hw.hrtimer);
> - if (event->state != PERF_EVENT_STATE_ACTIVE)
> - return HRTIMER_NORESTART;
> -
> - event->pmu->read(event);
> -
> - perf_sample_data_init(, 0, event->hw.last_period);
> - perf_event_overflow(event, , regs);
> -
> - period = max_t(u64, 1, event->hw.sample_period);
> - hrtimer_forward_now(hrtimer, ns_to_ktime(period));
> - return HRTIMER_RESTART;
> -}
> -
> -static void init_hrtimer(struct perf_event *event)
> -{
> - struct hw_perf_event *hwc = >hw;
> -
> - if (!is_sampling_event(event))
> - return;
> -
> - hrtimer_init(>hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> - hwc->hrtimer.function = hrtimer_sample;
> -
> - if (event->attr.freq) {
> - long freq = event->attr.sample_freq;
> -
> - event->attr.sample_period = NSEC_PER_SEC / freq;
> - hwc->sample_period = event->attr.sample_period;
> - local64_set(>period_left, hwc->sample_period);
> - hwc->last_period = hwc->sample_period;
> - event->attr.freq = 0;
> - }
> -}
> -
>  static int i915_pmu_event_init(struct perf_event *event)
>  {
>   struct drm_i915_private *i915 =
> @@ -293,6 +249,10 @@ static int i915_pmu_event_init(struct perf_event *event)
>   if (event->attr.type != event->pmu->type)
>   return -ENOENT;
>  
> + /* unsupported modes and filters */
> + if (event->attr.sample_period) /* no sampling */
> + return -EINVAL;
> +
>   if (has_branch_stack(event))
>   return -EOPNOTSUPP;
>  
> @@ -328,46 +288,9 @@ static int i915_pmu_event_init(struct perf_event *event)
>   if (!event->parent)
>   event->destroy = i915_pmu_event_destroy;
>  
> - init_hrtimer(event);
> -
>   return 0;
>  }
>  
> -static void i915_pmu_timer_start(struct perf_event *event)
> -{
> - struct hw_perf_event *hwc = >hw;
> - s64 period;
> -
> - if (!is_sampling_event(event))
> - return;
> -
> - period = local64_read(>period_left);
> - if (period) {
> - if (period < 0)
> - period = 1;
> -
> - local64_set(>period_left, 0);
> - } else {
> - period = max_t(u64, 1, hwc->sample_period);
> - }
> -
> - hrtimer_start_range_ns(>hrtimer,
> -ns_to_ktime(period), 0,
> -HRTIMER_MODE_REL_PINNED);
> -}
> -
> -static void i915_pmu_timer_cancel(struct perf_event *event)
> -{
> - struct hw_perf_event *hwc = >hw;
> -
> - if (!is_sampling_event(event))
> - return;
> -
> - local64_set(>period_left,
> - ktime_to_ns(hrtimer_get_remaining(>hrtimer)));
> - hrtimer_cancel(>hrtimer);
> -}
> -
>  static bool 

[Intel-gfx] [RFC v2 3/3] drm/i915/pmu: deny perf driver level sampling of i915 PMU

[Dmitry Rogozhkin]

This patch should probably be squashed with Tvrtko's PMU enabling patch...

As per discussion with Peter, i915 PMU is an example of uncore PMU which
are prohibited to support perf driver level sampling. This patch removes
hrtimer which we expose to perf core and denies events creation with
non-zero event->attr.sampling_period.

Mind that this patch does _not_ remove i915 PMU _internal_ sampling timer.
So, sampling metrics are still gathered, but can be accessed only by
explicit request to get metric counter, i.e. by sys_read().

Change-Id: I33f345f679f0a5a8ecc9867f9e7c1bfb357e708d
Signed-off-by: Dmitry Rogozhkin 
Cc: Tvrtko Ursulin 
Cc: Chris Wilson 
Cc: Peter Zijlstra 
---
 drivers/gpu/drm/i915/i915_pmu.c | 89 ++---
 1 file changed, 4 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index c551d64..311aeeb 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -239,50 +239,6 @@ static int engine_event_init(struct perf_event *event)
return 0;
 }
 
-static DEFINE_PER_CPU(struct pt_regs, i915_pmu_pt_regs);
-
-static enum hrtimer_restart hrtimer_sample(struct hrtimer *hrtimer)
-{
-   struct pt_regs *regs = this_cpu_ptr(_pmu_pt_regs);
-   struct perf_sample_data data;
-   struct perf_event *event;
-   u64 period;
-
-   event = container_of(hrtimer, struct perf_event, hw.hrtimer);
-   if (event->state != PERF_EVENT_STATE_ACTIVE)
-   return HRTIMER_NORESTART;
-
-   event->pmu->read(event);
-
-   perf_sample_data_init(, 0, event->hw.last_period);
-   perf_event_overflow(event, , regs);
-
-   period = max_t(u64, 1, event->hw.sample_period);
-   hrtimer_forward_now(hrtimer, ns_to_ktime(period));
-   return HRTIMER_RESTART;
-}
-
-static void init_hrtimer(struct perf_event *event)
-{
-   struct hw_perf_event *hwc = >hw;
-
-   if (!is_sampling_event(event))
-   return;
-
-   hrtimer_init(>hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-   hwc->hrtimer.function = hrtimer_sample;
-
-   if (event->attr.freq) {
-   long freq = event->attr.sample_freq;
-
-   event->attr.sample_period = NSEC_PER_SEC / freq;
-   hwc->sample_period = event->attr.sample_period;
-   local64_set(>period_left, hwc->sample_period);
-   hwc->last_period = hwc->sample_period;
-   event->attr.freq = 0;
-   }
-}
-
 static int i915_pmu_event_init(struct perf_event *event)
 {
struct drm_i915_private *i915 =
@@ -293,6 +249,10 @@ static int i915_pmu_event_init(struct perf_event *event)
if (event->attr.type != event->pmu->type)
return -ENOENT;
 
+   /* unsupported modes and filters */
+   if (event->attr.sample_period) /* no sampling */
+   return -EINVAL;
+
if (has_branch_stack(event))
return -EOPNOTSUPP;
 
@@ -328,46 +288,9 @@ static int i915_pmu_event_init(struct perf_event *event)
if (!event->parent)
event->destroy = i915_pmu_event_destroy;
 
-   init_hrtimer(event);
-
return 0;
 }
 
-static void i915_pmu_timer_start(struct perf_event *event)
-{
-   struct hw_perf_event *hwc = >hw;
-   s64 period;
-
-   if (!is_sampling_event(event))
-   return;
-
-   period = local64_read(>period_left);
-   if (period) {
-   if (period < 0)
-   period = 1;
-
-   local64_set(>period_left, 0);
-   } else {
-   period = max_t(u64, 1, hwc->sample_period);
-   }
-
-   hrtimer_start_range_ns(>hrtimer,
-  ns_to_ktime(period), 0,
-  HRTIMER_MODE_REL_PINNED);
-}
-
-static void i915_pmu_timer_cancel(struct perf_event *event)
-{
-   struct hw_perf_event *hwc = >hw;
-
-   if (!is_sampling_event(event))
-   return;
-
-   local64_set(>period_left,
-   ktime_to_ns(hrtimer_get_remaining(>hrtimer)));
-   hrtimer_cancel(>hrtimer);
-}
-
 static bool engine_needs_busy_stats(struct intel_engine_cs *engine)
 {
return supports_busy_stats() &&
@@ -493,8 +416,6 @@ static void i915_pmu_enable(struct perf_event *event)
}
 
spin_unlock_irqrestore(>pmu.lock, flags);
-
-   i915_pmu_timer_start(event);
 }
 
 static void i915_pmu_disable(struct perf_event *event)
@@ -534,8 +455,6 @@ static void i915_pmu_disable(struct perf_event *event)
i915->pmu.timer_enabled &= pmu_needs_timer(i915, true);
 
spin_unlock_irqrestore(>pmu.lock, flags);
-
-   i915_pmu_timer_cancel(event);
 }
 
 static void i915_pmu_event_start(struct perf_event *event, int flags)
-- 
1.8.3.1

___
Intel-gfx mailing list