<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Oct 26, 2016 at 4:03 PM, Robert Bragg <<a href="mailto:robert.bragg@gmail.com" target="_blank">robert.bragg@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="h5">On 26 Oct 2016 11:08 a.m., "Matthew Auld" <<a href="mailto:matthew.william.auld@gmail.com" target="_blank">matthew.william.auld@gmail.com</a>> wrote: > > On 26 October 2016 at 00:51, Robert Bragg <<a href="mailto:robert@sixbynine.org" target="_blank">robert@sixbynine.org</a>> wrote: > > > > > > On Tue, Oct 25, 2016 at 10:35 PM, Matthew Auld > > <<a href="mailto:matthew.william.auld@gmail.com" target="_blank">matthew.william.auld@gmail.com</a>> wrote: > >> > >> On 25 October 2016 at 00:19, Robert Bragg <<a href="mailto:robert@sixbynine.org" target="_blank">robert@sixbynine.org</a>> wrote: > > > > > >> > >> > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h > >> > b/drivers/gpu/drm/i915/i915_drv.h > >> > index 3448d05..ea24814 100644 > >> > --- a/drivers/gpu/drm/i915/i915_drv.h > >> > +++ b/drivers/gpu/drm/i915/i915_drv.h > >> > @@ -1764,6 +1764,11 @@ struct intel_wm_config { > >> > >> > > >> > struct drm_i915_private { > >> > @@ -2149,16 +2164,46 @@ struct drm_i915_private { > >> > > >> > struct { > >> > bool initialized; > >> > + > >> > struct mutex lock; > >> > struct list_head streams; > >> > > >> > + spinlock_t hook_lock; > >> > + > >> > struct { > >> > - u32 metrics_set; > >> > + struct i915_perf_stream *exclusive_stream; > >> > + > >> > + u32 specific_ctx_id; > >> Can we just get rid of this, now that the vma remains pinned we can > >> simply get the ggtt address at the time of configuring the OA_CONTROL > >> register ? > > > > > > 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. > > > >> > >> > >> > + > >> > + struct hrtimer poll_check_timer; > >> > + wait_queue_head_t poll_wq; > >> > + atomic_t pollin; > >> > + > >> > > > >> > >> > +/* The maximum exponent the hardware accepts is 63 (essentially it > >> > selects one > >> > + * of the 64bit timestamp bits to trigger reports from) but there's > >> > currently > >> > + * no known use case for sampling as infrequently as once per 47 > >> > thousand years. > >> > + * > >> > + * Since the timestamps included in OA reports are only 32bits it seems > >> > + * reasonable to limit the OA exponent where it's still possible to > >> > account for > >> > + * overflow in OA report timestamps. > >> > + */ > >> > +#define OA_EXPONENT_MAX 31 > >> > + > >> > +#define INVALID_CTX_ID 0xffffffff > >> We shouldn't need this anymore. > > > > > > 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. > Can we not make the specific_ctx_id per-stream, the gem context > already is, then we don't need to be concerned with resetting it ? </div></div>Hmm, I'm not sure about that, conceptually to me it's global OA unit state. Currently the driver only supports a single exclusive stream, while Sourab later relaxes that to a per-engine stream and that could be relaxed further with non-oa metric stream types. With multiple streams we'll still only be able to programmer a single ctx id in oacontol. Conceptually to me, other stream types could be associated with different contexts (if they don't depend on the OA unit) so to me stream->ctx isn't necessarily OA unit state. It probably could be played around with, but right now we don't track OA specific state in the stream. For the ID it's just semantics to say it's OA state, and we could consider that it's maybe generally useful to track the ID, even for future non-oa streams. That might mean potentially redundantly pinning state for the sake of tracking the ID for streams that don't end up needing it. </div></blockquote><div> I started to try out moving the specific_ctx_id and vma pointer (new) to the stream, and also looked at initializing them together with the stream->ctx reference, but I'm not really happy with how it's looking. </div><div>The specific_ctx_id and pinning are only for the render context, since the OA unit is only well integrated with the render engine, which makes me more inclined to consider them OA stream specific, not something we want/need for all streams (considering that Sourab enables multiple streams in his series). </div><div>Btw, for reference, my patches for gen8+ can also end up making use of the INVALID_CTX_ID define (when overwriting the undefined ctx_id field in HW reports when the report's ctx-id is flagged as invalid by the OA unit.) so we maybe don't want to worry to much about removing the need for it here. </div><div>- Robert </div></div> </div></div>