<div dir="ltr">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 ? 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>