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.gu...@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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx