<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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 class="">On Wed, Apr 20, 2016 at 03:23:10PM +0100, Robert Bragg wrote:<br>
</span><span class="">> +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 class="">> +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 class="">> +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 class="">> @@ -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>
What happens to poll / wait_unlocked if oa.periodic is not set? It seems<br>
like those functions would block indefinitely.<br></blockquote><div><br></div><div>Right, it's unecessary. I'll look at limitting it to just while polling or for blocking reads.<br><br></div><div>Good point about the blocking case too.<br><br></div><div>I  just started testing that scenario yesterday, writting an MI_RPC unit test which opens a stream without requesting periodic sampling, but didn't poll or read in that case so far so didn't hit this yet.<br><br></div><div>At least for the read() this is partially considered by returning -EIO if attempting a blocking read while the stream is disabled, but it doesn't consider the case that the stream is enabled but periodic sampling isn't enabled.</div><div><br></div><div>Regards,<br></div><div>- Robert<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">-Chris<br>
<br>
--<br>
Chris Wilson, Intel Open Source Technology Centre<br>
</div></div></blockquote></div><br></div></div>