[Intel-gfx] [PATCH 5/9] drm/i915: Enable i915 perf stream for Haswell OA unit

[Robert Bragg]

On Thu, Apr 21, 2016 at 4:18 PM, Robert Bragg <robert at sixbynine.org> wrote:

>
>
> On Thu, Apr 21, 2016 at 12:09 AM, Chris Wilson <chris at chris-wilson.co.uk>
> wrote:
>
>> On Wed, Apr 20, 2016 at 03:23:10PM +0100, Robert Bragg wrote:
>> > +static void i915_oa_stream_enable(struct i915_perf_stream *stream)
>> > +{
>> > +     struct drm_i915_private *dev_priv = stream->dev_priv;
>> > +
>> > +     dev_priv->perf.oa.ops.oa_enable(dev_priv);
>> > +
>> > +     if (dev_priv->perf.oa.periodic)
>> > +             hrtimer_start(&dev_priv->perf.oa.poll_check_timer,
>> > +                           ns_to_ktime(POLL_PERIOD),
>> > +                           HRTIMER_MODE_REL_PINNED);
>> > +}
>>
>> > +static void i915_oa_stream_disable(struct i915_perf_stream *stream)
>> > +{
>> > +     struct drm_i915_private *dev_priv = stream->dev_priv;
>> > +
>> > +     dev_priv->perf.oa.ops.oa_disable(dev_priv);
>> > +
>> > +     if (dev_priv->perf.oa.periodic)
>> > +             hrtimer_cancel(&dev_priv->perf.oa.poll_check_timer);
>> > +}
>>
>> > +static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer
>> *hrtimer)
>> > +{
>> > +     struct drm_i915_private *dev_priv =
>> > +             container_of(hrtimer, typeof(*dev_priv),
>> > +                          perf.oa.poll_check_timer);
>> > +
>> > +     if (!dev_priv->perf.oa.ops.oa_buffer_is_empty(dev_priv))
>> > +             wake_up(&dev_priv->perf.oa.poll_wq);
>> > +
>> > +     hrtimer_forward_now(hrtimer, ns_to_ktime(POLL_PERIOD));
>> > +
>> > +     return HRTIMER_RESTART;
>> > +}
>>
>> > @@ -424,8 +1313,37 @@ void i915_perf_init(struct drm_device *dev)
>> >  {
>> >       struct drm_i915_private *dev_priv = to_i915(dev);
>> >
>> > +     if (!IS_HASWELL(dev))
>> > +             return;
>> > +
>> > +     hrtimer_init(&dev_priv->perf.oa.poll_check_timer,
>> > +                  CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>> > +     dev_priv->perf.oa.poll_check_timer.function =
>> oa_poll_check_timer_cb;
>> > +     init_waitqueue_head(&dev_priv->perf.oa.poll_wq);
>>
>> This timer only serves to wake up pollers / wait_unlocked, right? So why
>> is it always running (when the stream is enabled)?
>>
>>
> Right, it's unecessary. I'll look at limitting it to just while polling or
> for blocking reads.
>

Actually, looking at this, I couldn't see a clean way to synchronized with
do_sys_poll() returning to be able to cancel the hrtimer. The .poll fop is
only responsible for registering a wait queue via poll_wait() and checking
if there are already events pending before any wait.

The current hrtimer frequency was picked as a reasonable default to ensure
we pick up samples before an overflow with high frequency OA sampling but
also avoids latency in picking up samples written at a lower frequency.
Something I've considered a few times before is that we might want to add a
property that can influence the latency userspace is happy with, which
might also alieviate some of this concern that the hrtimer runs all the
time the stream is enabled when it could often be fine to run at a much
lower frequency than the current 200Hz.

For now, maybe it's ok to stick with the fixed frequency, and I'll plan to
experiment with a property for influencing the maximum latency?

Regards,
- Robert
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20160422/9a0f26f5/attachment.html>