<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 25, 2016 at 10:35 PM, Matthew Auld <span dir="ltr"><<a href="mailto:matthew.william.auld@gmail.com" target="_blank">matthew.william.auld@gmail.com</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 25 October 2016 at 00:19, Robert Bragg <<a href="mailto:robert@sixbynine.org">robert@sixbynine.org</a>> wrote:</span></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><div><div class="h5">
> diff --git a/drivers/gpu/drm/i915/i915_<wbr>drv.h b/drivers/gpu/drm/i915/i915_<wbr>drv.h<br>
> index 3448d05..ea24814 100644<br>
> --- a/drivers/gpu/drm/i915/i915_<wbr>drv.h<br>
> +++ b/drivers/gpu/drm/i915/i915_<wbr>drv.h<br>
> @@ -1764,6 +1764,11 @@ struct intel_wm_config {<br><br>
><br>
> struct drm_i915_private {<br>
> @@ -2149,16 +2164,46 @@ struct drm_i915_private {<br>
><br>
> struct {<br>
> bool initialized;<br>
> +<br>
> struct mutex lock;<br>
> struct list_head streams;<br>
><br>
> + spinlock_t hook_lock;<br>
> +<br>
> struct {<br>
> - u32 metrics_set;<br>
> + struct i915_perf_stream *exclusive_stream;<br>
> +<br>
> + u32 specific_ctx_id;<br>
</div></div>Can we just get rid of this, now that the vma remains pinned we can<br>
simply get the ggtt address at the time of configuring the OA_CONTROL<br>
register ?<br></blockquote><div><br></div><div>I considered that, but would ideally prefer to keep it considering the gen8+ patches to come. For gen8+ (with execlists) the context ID isn't a gtt offset.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +<br>
> + struct hrtimer poll_check_timer;<br>
> + wait_queue_head_t poll_wq;<br>
> + atomic_t pollin;<br>
> +<br><br></div></div></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> +/* The maximum exponent the hardware accepts is 63 (essentially it selects one<br>
> + * of the 64bit timestamp bits to trigger reports from) but there's currently<br>
> + * no known use case for sampling as infrequently as once per 47 thousand years.<br>
> + *<br>
> + * Since the timestamps included in OA reports are only 32bits it seems<br>
> + * reasonable to limit the OA exponent where it's still possible to account for<br>
> + * overflow in OA report timestamps.<br>
> + */<br>
> +#define OA_EXPONENT_MAX 31<br>
> +<br>
> +#define INVALID_CTX_ID 0xffffffff<br>
</div></div>We shouldn't need this anymore.<br></blockquote><div><br></div><div>yeah I removed it and then added it back, just for the sake of explicitly setting the specific_ctx_id to an invalid ID when closing the exclusive stream - though resetting the value isn't strictly necessary.<br><br></div><div>also maybe your comment is assuming specific_ctx_id can be removed, while I'd prefer to keep it.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><div><div class="h5">
> +<br>
> +static int claim_specific_ctx(struct i915_perf_stream *stream)<br>
> +{<br>
</div></div>pin_oa_specific_ctx, or something? Also would it not make more sense<br>
to operate on the context, not the stream.<br></blockquote><div><br></div><div>Yeah, I avoided a name like that mainly because it's also initializing specific_ctx_id, which seemed to me like it would become an unexpected side effect with that more specific name.<br><br></div><div>The other consideration is that in my gen8+ patches the pinning code is conditional depending on whether execlists are enabled, while the function still initializes specific_ctx_id.<br></div><div><br></div><div>Certainly not attached to the names though.<br></div><div><br></div><div>Chris has some feedback with the code, so maybe that will affect this too.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> + struct drm_i915_private *dev_priv = stream->dev_priv;<br>
> + struct i915_vma *vma;<br>
> + int ret;<br>
> +<br>
> + ret = i915_mutex_lock_interruptible(<wbr>&dev_priv->drm);<br>
> + if (ret)<br>
> + return ret;<br>
> +<br>
> + /* So that we don't have to worry about updating the context ID<br>
> + * in OACONTOL on the fly we make sure to pin the context<br>
> + * upfront for the lifetime of the stream...<br>
> + */<br>
> + vma = stream->ctx->engine[RCS].<wbr>state;<br>
> + ret = i915_vma_pin(vma, 0, stream->ctx->ggtt_alignment,<br>
> + PIN_GLOBAL | PIN_HIGH);<br>
> + if (ret)<br>
> + return ret;<br>
> +<br>
> + dev_priv->perf.oa.specific_<wbr>ctx_id = i915_ggtt_offset(vma);<br>
> +<br>
> + mutex_unlock(&dev_priv->drm.<wbr>struct_mutex);<br>
> +<br>
> + return 0;<br>
> +}<br></div></div></blockquote><div><br><br></div><div>I'll also follow up on the other notes; thanks!<br><br></div><div>- Robert <br></div></div><br></div></div>