<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 21, 2016 at 4:18 PM, Robert Bragg <span dir="ltr"><<a href="mailto:robert@sixbynine.org" target="_blank">robert@sixbynine.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Thu, Apr 21, 2016 at 12:09 AM, Chris Wilson <span dir="ltr"><<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On Wed, Apr 20, 2016 at 03:23:10PM +0100, Robert Bragg wrote:<br>
</span><span>> +static void i915_oa_stream_enable(struct i915_perf_stream *stream)<br>
> +{<br>
> + struct drm_i915_private *dev_priv = stream->dev_priv;<br>
> +<br>
> + dev_priv->perf.oa.ops.oa_enable(dev_priv);<br>
> +<br>
> + if (dev_priv->perf.oa.periodic)<br>
> + hrtimer_start(&dev_priv->perf.oa.poll_check_timer,<br>
> + ns_to_ktime(POLL_PERIOD),<br>
> + HRTIMER_MODE_REL_PINNED);<br>
> +}<br>
<br>
</span><span>> +static void i915_oa_stream_disable(struct i915_perf_stream *stream)<br>
> +{<br>
> + struct drm_i915_private *dev_priv = stream->dev_priv;<br>
> +<br>
> + dev_priv->perf.oa.ops.oa_disable(dev_priv);<br>
> +<br>
> + if (dev_priv->perf.oa.periodic)<br>
> + hrtimer_cancel(&dev_priv->perf.oa.poll_check_timer);<br>
> +}<br>
<br>
</span><span>> +static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer)<br>
> +{<br>
> + struct drm_i915_private *dev_priv =<br>
> + container_of(hrtimer, typeof(*dev_priv),<br>
> + perf.oa.poll_check_timer);<br>
> +<br>
> + if (!dev_priv->perf.oa.ops.oa_buffer_is_empty(dev_priv))<br>
> + wake_up(&dev_priv->perf.oa.poll_wq);<br>
> +<br>
> + hrtimer_forward_now(hrtimer, ns_to_ktime(POLL_PERIOD));<br>
> +<br>
> + return HRTIMER_RESTART;<br>
> +}<br>
<br>
</span><span>> @@ -424,8 +1313,37 @@ void i915_perf_init(struct drm_device *dev)<br>
> {<br>
> struct drm_i915_private *dev_priv = to_i915(dev);<br>
><br>
> + if (!IS_HASWELL(dev))<br>
> + return;<br>
> +<br>
> + hrtimer_init(&dev_priv->perf.oa.poll_check_timer,<br>
> + CLOCK_MONOTONIC, HRTIMER_MODE_REL);<br>
> + dev_priv->perf.oa.poll_check_timer.function = oa_poll_check_timer_cb;<br>
> + init_waitqueue_head(&dev_priv->perf.oa.poll_wq);<br>
<br>
</span>This timer only serves to wake up pollers / wait_unlocked, right? So why<br>
is it always running (when the stream is enabled)?<br>
<br></blockquote><div><br></div></div></div><div>Right, it's unecessary. I'll look at limitting it to just while polling or for blocking reads.<br></div></div></div></div></blockquote><div><br></div><div>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.<br></div><br></div><div class="gmail_quote">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.<br><br></div><div class="gmail_quote">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?<br></div><div class="gmail_quote"><br></div><div class="gmail_quote">Regards,<br></div><div class="gmail_quote">- Robert<br></div><br></div></div>