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

["Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>]

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 <dmitry.v.rogozhkin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  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(&i915_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(&data, 0, event->hw.last_period);
> -	perf_event_overflow(event, &data, regs);
> -
> -	period = max_t(u64, 10000, 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 = &event->hw;
> -
> -	if (!is_sampling_event(event))
> -		return;
> -
> -	hrtimer_init(&hwc->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(&hwc->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 = &event->hw;
> -	s64 period;
> -
> -	if (!is_sampling_event(event))
> -		return;
> -
> -	period = local64_read(&hwc->period_left);
> -	if (period) {
> -		if (period < 0)
> -			period = 10000;
> -
> -		local64_set(&hwc->period_left, 0);
> -	} else {
> -		period = max_t(u64, 10000, hwc->sample_period);
> -	}
> -
> -	hrtimer_start_range_ns(&hwc->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 = &event->hw;
> -
> -	if (!is_sampling_event(event))
> -		return;
> -
> -	local64_set(&hwc->period_left,
> -		    ktime_to_ns(hrtimer_get_remaining(&hwc->hrtimer)));
> -	hrtimer_cancel(&hwc->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(&i915->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(&i915->pmu.lock, flags);
> -
> -	i915_pmu_timer_cancel(event);
>  }
>  
>  static void i915_pmu_event_start(struct perf_event *event, int flags)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx