On Wed, Dec 16, 2015 at 10:44:19AM +0100, Daniel Vetter wrote: > On Fri, Dec 11, 2015 at 11:33:03AM +0000, Chris Wilson wrote: > > As the request is only valid during the same global reset epoch, we can > > record the current reset_counter when constructing the request and reuse > > it when waiting upon that request in future. This removes a very hairy > > atomic check serialised by the struct_mutex at the time of waiting and > > allows us to transfer those waits to a central dispatcher for all > > waiters and all requests. > > > > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk> > > Cc: Daniel Vetter <daniel.vet...@ffwll.ch> > > "global reset epoch" got me thinking about what the implications for TDR > are. Now the current TDR patches iirc are still somewhat tacked on the > side of the existing reset handling, and so don't end up incrementing the > reset counter. > > But I don't agree with that design decision, and I think any reset must > increment the counter (and do the reset-in-progress flag dance), since > that's the only way to guarantee that waiters will get off locks. Even > though we have fewer of those with every kernel release. > > But then there's the problem that at least for some request they'll still > be valid after the reset, and would break with this design here. Which can > easily be fixed by incrementing the reset epoch for every request we > decide should keep running (or which gets re-scheduled/emitted for another > attempt), and doing that explicitly seems like a good idea. > > The only implication I see is that for per-ring reset we might want to go > to a per-engine reset epoch counter.
Yes, I think per-engine epoch's will be required. (I've been trying to choose my words carefully so that it is clear that it only applies to today and not in a TDR future!) > So given all that I think this is a solid idea. But one comment below as a > fallout. > > > --- > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > drivers/gpu/drm/i915/i915_gem.c | 40 > > +++++++++++---------------------- > > drivers/gpu/drm/i915/intel_display.c | 7 +----- > > drivers/gpu/drm/i915/intel_lrc.c | 7 ------ > > drivers/gpu/drm/i915/intel_ringbuffer.c | 6 ----- > > 5 files changed, 15 insertions(+), 47 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 1043ddd670a5..f30c305a6889 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2178,6 +2178,7 @@ struct drm_i915_gem_request { > > /** On Which ring this request was generated */ > > struct drm_i915_private *i915; > > struct intel_engine_cs *ring; > > + unsigned reset_counter; > > > > /** GEM sequence number associated with the previous request, > > * when the HWS breadcrumb is equal to this the GPU is processing > > @@ -3059,7 +3060,6 @@ void __i915_add_request(struct drm_i915_gem_request > > *req, > > #define i915_add_request_no_flush(req) \ > > __i915_add_request(req, NULL, false) > > int __i915_wait_request(struct drm_i915_gem_request *req, > > - unsigned reset_counter, > > bool interruptible, > > s64 *timeout, > > struct intel_rps_client *rps); > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > b/drivers/gpu/drm/i915/i915_gem.c > > index 27e617b76418..b17cc0e42a4f 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -1214,7 +1214,6 @@ static int __i915_spin_request(struct > > drm_i915_gem_request *req, int state) > > /** > > * __i915_wait_request - wait until execution of request has finished > > * @req: duh! > > - * @reset_counter: reset sequence associated with the given request > > * @interruptible: do an interruptible wait (normally yes) > > * @timeout: in - how long to wait (NULL forever); out - how much time > > remaining > > * > > @@ -1229,7 +1228,6 @@ static int __i915_spin_request(struct > > drm_i915_gem_request *req, int state) > > * errno with remaining time filled in timeout argument. > > */ > > int __i915_wait_request(struct drm_i915_gem_request *req, > > - unsigned reset_counter, > > bool interruptible, > > s64 *timeout, > > struct intel_rps_client *rps) > > @@ -1288,7 +1286,7 @@ int __i915_wait_request(struct drm_i915_gem_request > > *req, > > > > /* We need to check whether any gpu reset happened in between > > * the caller grabbing the seqno and now ... */ > > - if (reset_counter != i915_reset_counter(&dev_priv->gpu_error)) { > > + if (req->reset_counter != > > i915_reset_counter(&dev_priv->gpu_error)) { > > READ_ONCE(req->reset) as a future proofing maybe for TDR? Or punt that to > TDR? Could be confusing to READ_ONCE something that's (with the current > code at least) invariant. Right, my plan was to solve this in TDR! At the moment, I am thinking that we regenerate a new request for each one resubmitted (that requires patching the seqno embedded into the ring before restarting the ring), but otherwise we can just duplicate the contents of the request. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx