| Message ID | 1412604925-11290-10-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 06, 2014 at 03:15:13PM +0100, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> To thin commit message. Also I wonder whethere we should track the olr state more explicitly in the request structure instead of jumping through all these hoops. And explicit olr state for a request might also help to clarify control flow and checks in a bunch of places. > For: VIZ-4377 > Signed-off-by: John.C.Harrison@Intel.com > --- > drivers/gpu/drm/i915/i915_drv.h | 17 ++++++++++++++++- > drivers/gpu/drm/i915/i915_gem.c | 25 ++++++++++--------------- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 3 files changed, 27 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 1401266..9504206 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2483,7 +2483,7 @@ bool i915_gem_retire_requests(struct drm_device *dev); > void i915_gem_retire_requests_ring(struct intel_engine_cs *ring); > int __must_check i915_gem_check_wedge(struct i915_gpu_error *error, > bool interruptible); > -int __must_check i915_gem_check_olr(struct intel_engine_cs *ring, u32 seqno); > +int __must_check i915_gem_check_olr(struct drm_i915_gem_request *req); > > static inline bool i915_reset_in_progress(struct i915_gpu_error *error) > { > @@ -3020,4 +3020,19 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) > } > } > > +/************************* Deprecated *************************/ There's a gcc flag for functions if you want to mark this as depracated, otherwise I'd just go with a "XXX: This temporary function will disappear at the end of the patch series" somewhere. > +static inline int __must_check i915_gem_check_ols(struct intel_engine_cs *ring, u32 seqno) > +{ > + int ret; > + > + BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex)); WARN_ON instead of BUG_ON for locking checks. > + > + ret = 0; > + if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request)) > + ret = i915_add_request(ring, NULL); > + > + return ret; > +} > +/************************* Deprecated *************************/ > + > #endif > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 0d0eb26..2c7deca 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1097,19 +1097,18 @@ i915_gem_check_wedge(struct i915_gpu_error *error, > } > > /* > - * Compare seqno against outstanding lazy request. Emit a request if they are > - * equal. > + * Compare arbitrary request against outstanding lazy request. Emit on match. > */ > int > -i915_gem_check_olr(struct intel_engine_cs *ring, u32 seqno) > +i915_gem_check_olr(struct drm_i915_gem_request *req) > { > int ret; > > - BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex)); > + BUG_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); Same here. Plus please a stern warning in the commit message and cc: to whomever who managed to sneak this in (and failed to catch it in review). > > ret = 0; > - if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request)) > - ret = i915_add_request(ring, NULL); > + if (req == req->ring->outstanding_lazy_request) > + ret = i915_add_request(req->ring, NULL); > > return ret; > } > @@ -1271,7 +1270,7 @@ i915_wait_seqno(struct intel_engine_cs *ring, uint32_t seqno) > if (ret) > return ret; > > - ret = i915_gem_check_olr(ring, seqno); > + ret = i915_gem_check_ols(ring, seqno); > if (ret) > return ret; > > @@ -1338,7 +1337,6 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj, > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_engine_cs *ring = obj->ring; > unsigned reset_counter; > - u32 seqno; > int ret; > > BUG_ON(!mutex_is_locked(&dev->struct_mutex)); > @@ -1348,21 +1346,18 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj, > if (!req) > return 0; > > - seqno = i915_gem_request_get_seqno(req); > - BUG_ON(seqno == 0); > - > ret = i915_gem_check_wedge(&dev_priv->gpu_error, true); > if (ret) > return ret; > > - ret = i915_gem_check_olr(ring, seqno); > + ret = i915_gem_check_olr(req); > if (ret) > return ret; > > reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter); > i915_gem_request_reference(req); > mutex_unlock(&dev->struct_mutex); > - ret = __wait_seqno(ring, seqno, reset_counter, true, NULL, file_priv); > + ret = __wait_seqno(ring, i915_gem_request_get_seqno(req), reset_counter, true, NULL, file_priv); > mutex_lock(&dev->struct_mutex); > i915_gem_request_unreference(req); > if (ret) > @@ -2786,7 +2781,7 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj) > int ret; > > if (obj->active) { > - ret = i915_gem_check_olr(obj->ring, i915_gem_request_get_seqno(obj->last_read_req)); > + ret = i915_gem_check_olr(obj->last_read_req); > if (ret) > return ret; > > @@ -2913,7 +2908,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj, > if (seqno <= from->semaphore.sync_seqno[idx]) /* <--- broken?! needs to use i915_seqno_passed()??? */ > return 0; > > - ret = i915_gem_check_olr(obj->ring, seqno); > + ret = i915_gem_check_olr(obj->last_read_req); > if (ret) > return ret; > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 6da18c5..2af421e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9775,7 +9775,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj) > i915_gem_request_get_seqno(obj->last_write_req))) > return 0; > > - ret = i915_gem_check_olr(ring, i915_gem_request_get_seqno(obj->last_write_req)); > + ret = i915_gem_check_olr(obj->last_write_req); > if (ret) > return ret; > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 19/10/2014 13:55, Daniel Vetter wrote: > On Mon, Oct 06, 2014 at 03:15:13PM +0100, John.C.Harrison@Intel.com wrote: >> From: John Harrison <John.C.Harrison@Intel.com> > To thin commit message. > > Also I wonder whethere we should track the olr state more explicitly in > the request structure instead of jumping through all these hoops. And > explicit olr state for a request might also help to clarify control flow > and checks in a bunch of places. What would be nicer would be to get rid of the OLR completely. If code wants to write to the ring then it should request a request structure at the start, pass that request in to all the ring writing code and submit the same request structure at the end. The whole idea of some random request floating around in the ether scooping up any ring accesses that happen to occur with no idea about ownership, sequence, etc. just seems highly dodgy. Indeed it definitely causes issues with the scheduler with things like page flips being mixed up in completely unrelated batch buffer submissions! I believe that is one of the things Chris did in his re-write the universe patch. >> For: VIZ-4377 >> Signed-off-by: John.C.Harrison@Intel.com >> --- >> drivers/gpu/drm/i915/i915_drv.h | 17 ++++++++++++++++- >> drivers/gpu/drm/i915/i915_gem.c | 25 ++++++++++--------------- >> drivers/gpu/drm/i915/intel_display.c | 2 +- >> 3 files changed, 27 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 1401266..9504206 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2483,7 +2483,7 @@ bool i915_gem_retire_requests(struct drm_device *dev); >> void i915_gem_retire_requests_ring(struct intel_engine_cs *ring); >> int __must_check i915_gem_check_wedge(struct i915_gpu_error *error, >> bool interruptible); >> -int __must_check i915_gem_check_olr(struct intel_engine_cs *ring, u32 seqno); >> +int __must_check i915_gem_check_olr(struct drm_i915_gem_request *req); >> >> static inline bool i915_reset_in_progress(struct i915_gpu_error *error) >> { >> @@ -3020,4 +3020,19 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) >> } >> } >> >> +/************************* Deprecated *************************/ > There's a gcc flag for functions if you want to mark this as depracated, > otherwise I'd just go with a "XXX: This temporary function will disappear > at the end of the patch series" somewhere. > >> +static inline int __must_check i915_gem_check_ols(struct intel_engine_cs *ring, u32 seqno) >> +{ >> + int ret; >> + >> + BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex)); > WARN_ON instead of BUG_ON for locking checks. > >> + >> + ret = 0; >> + if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request)) >> + ret = i915_add_request(ring, NULL); >> + >> + return ret; >> +} >> +/************************* Deprecated *************************/ >> + >> #endif >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index 0d0eb26..2c7deca 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -1097,19 +1097,18 @@ i915_gem_check_wedge(struct i915_gpu_error *error, >> } >> >> /* >> - * Compare seqno against outstanding lazy request. Emit a request if they are >> - * equal. >> + * Compare arbitrary request against outstanding lazy request. Emit on match. >> */ >> int >> -i915_gem_check_olr(struct intel_engine_cs *ring, u32 seqno) >> +i915_gem_check_olr(struct drm_i915_gem_request *req) >> { >> int ret; >> >> - BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex)); >> + BUG_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); > Same here. Plus please a stern warning in the commit message and cc: to > whomever who managed to sneak this in (and failed to catch it in review). >> >> ret = 0; >> - if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request)) >> - ret = i915_add_request(ring, NULL); >> + if (req == req->ring->outstanding_lazy_request) >> + ret = i915_add_request(req->ring, NULL); >> >> return ret; >> } >> @@ -1271,7 +1270,7 @@ i915_wait_seqno(struct intel_engine_cs *ring, uint32_t seqno) >> if (ret) >> return ret; >> >> - ret = i915_gem_check_olr(ring, seqno); >> + ret = i915_gem_check_ols(ring, seqno); >> if (ret) >> return ret; >> >> @@ -1338,7 +1337,6 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj, >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_engine_cs *ring = obj->ring; >> unsigned reset_counter; >> - u32 seqno; >> int ret; >> >> BUG_ON(!mutex_is_locked(&dev->struct_mutex)); >> @@ -1348,21 +1346,18 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj, >> if (!req) >> return 0; >> >> - seqno = i915_gem_request_get_seqno(req); >> - BUG_ON(seqno == 0); >> - >> ret = i915_gem_check_wedge(&dev_priv->gpu_error, true); >> if (ret) >> return ret; >> >> - ret = i915_gem_check_olr(ring, seqno); >> + ret = i915_gem_check_olr(req); >> if (ret) >> return ret; >> >> reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter); >> i915_gem_request_reference(req); >> mutex_unlock(&dev->struct_mutex); >> - ret = __wait_seqno(ring, seqno, reset_counter, true, NULL, file_priv); >> + ret = __wait_seqno(ring, i915_gem_request_get_seqno(req), reset_counter, true, NULL, file_priv); >> mutex_lock(&dev->struct_mutex); >> i915_gem_request_unreference(req); >> if (ret) >> @@ -2786,7 +2781,7 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj) >> int ret; >> >> if (obj->active) { >> - ret = i915_gem_check_olr(obj->ring, i915_gem_request_get_seqno(obj->last_read_req)); >> + ret = i915_gem_check_olr(obj->last_read_req); >> if (ret) >> return ret; >> >> @@ -2913,7 +2908,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj, >> if (seqno <= from->semaphore.sync_seqno[idx]) /* <--- broken?! needs to use i915_seqno_passed()??? */ >> return 0; >> >> - ret = i915_gem_check_olr(obj->ring, seqno); >> + ret = i915_gem_check_olr(obj->last_read_req); >> if (ret) >> return ret; >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 6da18c5..2af421e 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -9775,7 +9775,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj) >> i915_gem_request_get_seqno(obj->last_write_req))) >> return 0; >> >> - ret = i915_gem_check_olr(ring, i915_gem_request_get_seqno(obj->last_write_req)); >> + ret = i915_gem_check_olr(obj->last_write_req); >> if (ret) >> return ret; >> >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Oct 28, 2014 at 02:01:52PM +0000, John Harrison wrote: > On 19/10/2014 13:55, Daniel Vetter wrote: > >On Mon, Oct 06, 2014 at 03:15:13PM +0100, John.C.Harrison@Intel.com wrote: > >>From: John Harrison <John.C.Harrison@Intel.com> > >To thin commit message. > > > >Also I wonder whethere we should track the olr state more explicitly in > >the request structure instead of jumping through all these hoops. And > >explicit olr state for a request might also help to clarify control flow > >and checks in a bunch of places. > > What would be nicer would be to get rid of the OLR completely. If code wants > to write to the ring then it should request a request structure at the > start, pass that request in to all the ring writing code and submit the same > request structure at the end. The whole idea of some random request floating > around in the ether scooping up any ring accesses that happen to occur with > no idea about ownership, sequence, etc. just seems highly dodgy. Indeed it > definitely causes issues with the scheduler with things like page flips > being mixed up in completely unrelated batch buffer submissions! I believe > that is one of the things Chris did in his re-write the universe patch. Oh OLR is highly dodgy ;-) It is unfortunately a bit of work to fix things up, especially without a rewrite-the-universe patch. Perhaps: 1. Explicitly preallocate the request, still store it at the same place. 2. Pass the explicit request to add_request, add any missing add_requests call where we currently relied on the olr to scoop up register writes. 3. Shovel the request pointer from the engine to the ring (since that's where we need it it for execlist), that means we need to allocate the request for a specific engine. Steps 1&3 might be better if switched, dunno. 4. Sprinkle asserts over all ring functions to make sure the request is there already. 5. Throw away olr logic. 6. Optional, refactoring over the next few years: Use requests as the primary cmd submission object everywhere. Comments? -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1401266..9504206 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2483,7 +2483,7 @@ bool i915_gem_retire_requests(struct drm_device *dev); void i915_gem_retire_requests_ring(struct intel_engine_cs *ring); int __must_check i915_gem_check_wedge(struct i915_gpu_error *error, bool interruptible); -int __must_check i915_gem_check_olr(struct intel_engine_cs *ring, u32 seqno); +int __must_check i915_gem_check_olr(struct drm_i915_gem_request *req); static inline bool i915_reset_in_progress(struct i915_gpu_error *error) { @@ -3020,4 +3020,19 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) } } +/************************* Deprecated *************************/ +static inline int __must_check i915_gem_check_ols(struct intel_engine_cs *ring, u32 seqno) +{ + int ret; + + BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex)); + + ret = 0; + if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request)) + ret = i915_add_request(ring, NULL); + + return ret; +} +/************************* Deprecated *************************/ + #endif diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 0d0eb26..2c7deca 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1097,19 +1097,18 @@ i915_gem_check_wedge(struct i915_gpu_error *error, } /* - * Compare seqno against outstanding lazy request. Emit a request if they are - * equal. + * Compare arbitrary request against outstanding lazy request. Emit on match. */ int -i915_gem_check_olr(struct intel_engine_cs *ring, u32 seqno) +i915_gem_check_olr(struct drm_i915_gem_request *req) { int ret; - BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex)); + BUG_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); ret = 0; - if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request)) - ret = i915_add_request(ring, NULL); + if (req == req->ring->outstanding_lazy_request) + ret = i915_add_request(req->ring, NULL); return ret; } @@ -1271,7 +1270,7 @@ i915_wait_seqno(struct intel_engine_cs *ring, uint32_t seqno) if (ret) return ret; - ret = i915_gem_check_olr(ring, seqno); + ret = i915_gem_check_ols(ring, seqno); if (ret) return ret; @@ -1338,7 +1337,6 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj, struct drm_i915_private *dev_priv = dev->dev_private; struct intel_engine_cs *ring = obj->ring; unsigned reset_counter; - u32 seqno; int ret; BUG_ON(!mutex_is_locked(&dev->struct_mutex)); @@ -1348,21 +1346,18 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj, if (!req) return 0; - seqno = i915_gem_request_get_seqno(req); - BUG_ON(seqno == 0); - ret = i915_gem_check_wedge(&dev_priv->gpu_error, true); if (ret) return ret; - ret = i915_gem_check_olr(ring, seqno); + ret = i915_gem_check_olr(req); if (ret) return ret; reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter); i915_gem_request_reference(req); mutex_unlock(&dev->struct_mutex); - ret = __wait_seqno(ring, seqno, reset_counter, true, NULL, file_priv); + ret = __wait_seqno(ring, i915_gem_request_get_seqno(req), reset_counter, true, NULL, file_priv); mutex_lock(&dev->struct_mutex); i915_gem_request_unreference(req); if (ret) @@ -2786,7 +2781,7 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj) int ret; if (obj->active) { - ret = i915_gem_check_olr(obj->ring, i915_gem_request_get_seqno(obj->last_read_req)); + ret = i915_gem_check_olr(obj->last_read_req); if (ret) return ret; @@ -2913,7 +2908,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj, if (seqno <= from->semaphore.sync_seqno[idx]) /* <--- broken?! needs to use i915_seqno_passed()??? */ return 0; - ret = i915_gem_check_olr(obj->ring, seqno); + ret = i915_gem_check_olr(obj->last_read_req); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6da18c5..2af421e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9775,7 +9775,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj) i915_gem_request_get_seqno(obj->last_write_req))) return 0; - ret = i915_gem_check_olr(ring, i915_gem_request_get_seqno(obj->last_write_req)); + ret = i915_gem_check_olr(obj->last_write_req); if (ret) return ret;