[Intel-gfx] [PATCH 3/8] drm/i915: Framework for capturing command stream based OA reports

[Chris Wilson]

On Thu, Mar 16, 2017 at 06:42:21PM +0530, sourab gupta wrote:
> On Thu, 2017-03-16 at 03:09 -0700, Chris Wilson wrote:
> > On Thu, Mar 16, 2017 at 03:22:03PM +0530, sourab gupta wrote:
> > > On Thu, 2017-03-16 at 02:03 -0700, Chris Wilson wrote:
> > > > On Thu, Mar 16, 2017 at 02:24:55PM +0530, sourab gupta wrote:
> > > > > On Thu, 2017-03-16 at 01:10 -0700, Chris Wilson wrote:
> > > > > > On Thu, Mar 16, 2017 at 11:44:10AM +0530, sourab.gupta at intel.com wrote:
> > > > > > > +void i915_perf_command_stream_hook(struct drm_i915_gem_request *request)
> > > > > > > +{
> > > > > > > +	struct intel_engine_cs *engine = request->engine;
> > > > > > > +	struct drm_i915_private *dev_priv = engine->i915;
> > > > > > > +	struct i915_perf_stream *stream;
> > > > > > > +
> > > > > > > +	if (!dev_priv->perf.initialized)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	mutex_lock(&dev_priv->perf.streams_lock);
> > > > > > 
> > > > > > Justify a new global lock very, very carefully on execbuf.
> > > > > 
> > > > > The lock introduced here is to protect the the perf.streams list against
> > > > > addition/deletion while we're processing the list during execbuf call.
> > > > > The other places where the mutex is taken is when a new stream is being
> > > > > created (using perf_open ioctl) or a stream is being destroyed
> > > > > (perf_release ioctl), which just protect the list_add and list_del into
> > > > > the perf.streams list.
> > > > > As such, there should not be much impact on execbuf path.
> > > > > Does this seem reasonable?
> > > > 
> > > > It doesn't sound like it needs a mutex, which will simplify the other
> > > > users as well (if switched to, for example, an RCU protected list).
> > > 
> > > Ok. Sounds reasonable, I'll switch to using an RCU protected list here.
> > 
> > (I may be overthinking this, but it still seems overkill and made the
> > timer callback uglier than expected.)
> > 
> Are you suggesting that using RCU here is overkill, and mutex would do?

No, I still think it can be done without a mutex, just slightly more
ugly due to the potential sleep here (perhaps srcu?). It would just be a
shame when we get parallel submission of execbuf sorted for it all to be
serialised by walking a simple list.

> > I was mostly thrown by the idea that you were reassigning requests,
> > which history has shown to be a bad idea (hence i915_gem_active).
> > However, stream->last_request should be a reservation_object.
> > 
> If I understand correctly, I need to have last_request of type
> reservation object to hold all fences corresponding to the requests
> pertaining to the stream. So, instead of i915_gem_active_set, I need to
> do something like this:
> 	reservation_object_add_excl_fence(obj->resv, &rq->fence); /* Or any
> other api to be used here ? */

reservation_object_lock();
if (reservation_object_reserve_shared() == 0)
	reservation_object_add_shared_fence();
reservation_object_unlock();

> And, when we want to wait for requests submitted on the stream to
> complete, we have to wait for fences associated with the 'last_request'
> reservation_object, for e.g. as done in
> 'i915_gem_object_wait_reservation' function.
> Is this all to it, or am I missing some piece of action with this
> reservation_object?

reservation_object_test_signaled_rcu(.test_all = true)
reservation_object_wait_timeout_rcu(.wait_all = true)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre