| Message ID | 1424366285-29232-53-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/02/2015 17:18, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > The outstanding_lazy_request is no longer used anywhere in the driver. > Everything that was looking at it now has a request explicitly passed in from on > high. Everything that was relying upon behind the scenes is now explicitly > creating/passing/submitting it's own private request. Thus the OLR can be > removed. > "Everything that was relying upon behind the scenes is now explicitly creating/passing/submitting it's ..." ---> "Everything that was relying on it behind the scenes is now explicitly creating/passing/submitting its ..." ? > For: VIZ-5115 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 16 +--------------- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +--- > drivers/gpu/drm/i915/intel_lrc.c | 6 ++---- > drivers/gpu/drm/i915/intel_ringbuffer.c | 17 ++--------------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 4 ---- > 5 files changed, 6 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 60f6671..8e7418b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1156,15 +1156,9 @@ i915_gem_check_wedge(struct i915_gpu_error *error, > int > i915_gem_check_olr(struct drm_i915_gem_request *req) > { > - int ret; > - > WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); > > - ret = 0; > - if (req == req->ring->outstanding_lazy_request) > - ret = i915_add_request(req); > - > - return ret; > + return 0; > } > > static void fake_irq(unsigned long data) > @@ -2424,8 +2418,6 @@ int __i915_add_request(struct drm_i915_gem_request *request, > dev_priv = ring->dev->dev_private; > ringbuf = request->ringbuf; > > - WARN_ON(request != ring->outstanding_lazy_request); > - > request_start = intel_ring_get_tail(ringbuf); > /* > * Emit any outstanding flushes - execbuf can fail to emit the flush > @@ -2486,7 +2478,6 @@ int __i915_add_request(struct drm_i915_gem_request *request, > } > > trace_i915_gem_request_add(request); > - ring->outstanding_lazy_request = NULL; > > i915_queue_hangcheck(ring->dev); > > @@ -2672,9 +2663,6 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, > > i915_gem_free_request(request); > } > - > - /* This may not have been flushed before the reset, so clean it now */ > - i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); > } > > void i915_gem_restore_fences(struct drm_device *dev) > @@ -3124,8 +3112,6 @@ int i915_gpu_idle(struct drm_device *dev) > } > } > > - WARN_ON(ring->outstanding_lazy_request); > - > ret = intel_ring_idle(ring); > if (ret) > return ret; > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 6a703e6..0eae592 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1571,10 +1571,8 @@ err: > * must be freed again. If it was submitted then it is being tracked > * on the active request list and no clean up is required here. > */ > - if (ret && params->request) { > + if (ret && params->request) > i915_gem_request_unreference(params->request); > - ring->outstanding_lazy_request = NULL; > - } > > mutex_unlock(&dev->struct_mutex); > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 2911cf6..db63ea0 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1032,8 +1032,7 @@ int intel_logical_ring_alloc_request(struct intel_engine_cs *ring, > if (!req_out) > return -EINVAL; > > - if ((*req_out = ring->outstanding_lazy_request) != NULL) > - return 0; > + *req_out = NULL; > > request = kzalloc(sizeof(*request), GFP_KERNEL); > if (request == NULL) > @@ -1067,7 +1066,7 @@ int intel_logical_ring_alloc_request(struct intel_engine_cs *ring, > i915_gem_context_reference(request->ctx); > request->ringbuf = ctx->engine[ring->id].ringbuf; > > - *req_out = ring->outstanding_lazy_request = request; > + *req_out = request; > return 0; > } > > @@ -1393,7 +1392,6 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring) > > intel_logical_ring_stop(ring); > WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0); > - i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); > > if (ring->cleanup) > ring->cleanup(ring); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 5eef02e..85daa18 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -2034,7 +2034,6 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) > > intel_unpin_ringbuffer_obj(ringbuf); > intel_destroy_ringbuffer_obj(ringbuf); > - i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); > > if (ring->cleanup) > ring->cleanup(ring); > @@ -2153,15 +2152,6 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring) > int intel_ring_idle(struct intel_engine_cs *ring) > { > struct drm_i915_gem_request *req; > - int ret; > - > - /* We need to add any requests required to flush the objects and ring */ > - WARN_ON(ring->outstanding_lazy_request); > - if (ring->outstanding_lazy_request) { > - ret = i915_add_request(ring->outstanding_lazy_request); > - if (ret) > - return ret; > - } > > /* Wait upon the last request to be completed */ > if (list_empty(&ring->request_list)) > @@ -2186,8 +2176,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring, > if (!req_out) > return -EINVAL; > > - if ((*req_out = ring->outstanding_lazy_request) != NULL) > - return 0; > + *req_out = NULL; > > request = kzalloc(sizeof(*request), GFP_KERNEL); > if (request == NULL) > @@ -2206,7 +2195,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring, > return ret; > } > > - *req_out = ring->outstanding_lazy_request = request; > + *req_out = request; > return 0; > } > > @@ -2281,8 +2270,6 @@ void intel_ring_init_seqno(struct intel_engine_cs *ring, u32 seqno) > struct drm_device *dev = ring->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > > - BUG_ON(ring->outstanding_lazy_request); > - > if (INTEL_INFO(dev)->gen == 6 || INTEL_INFO(dev)->gen == 7) { > I915_WRITE(RING_SYNC_0(ring->mmio_base), 0); > I915_WRITE(RING_SYNC_1(ring->mmio_base), 0); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 3b0261f..d2c6427 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -258,10 +258,6 @@ struct intel_engine_cs { > */ > struct list_head request_list; > > - /** > - * Do we have some not yet emitted requests outstanding? > - */ > - struct drm_i915_gem_request *outstanding_lazy_request; > bool gpu_caches_dirty; > bool fbc_dirty; > > Since we're removing the i915_add_request from i915_check_olr and thereby removing the OLR emission the following comment at i915_gem_object_flush_active is no longer valid: /** * Ensures that an object will eventually get non-busy by flushing any required * write domains, emitting any outstanding lazy request and retiring and * completed requests. */ static int i915_gem_object_flush_active(struct drm_i915_gem_object *obj) { struct intel_engine_cs *ring; int ret; if (obj->active) { ring = i915_gem_request_get_ring(obj->last_read_req); ret = i915_gem_check_olr(obj->last_read_req); Thanks, Tomas
On Mon, Mar 09, 2015 at 11:51:26PM +0000, Tomas Elf wrote: > On 19/02/2015 17:18, John.C.Harrison@Intel.com wrote: > >From: John Harrison <John.C.Harrison@Intel.com> > > > >The outstanding_lazy_request is no longer used anywhere in the driver. > >Everything that was looking at it now has a request explicitly passed in from on > >high. Everything that was relying upon behind the scenes is now explicitly > >creating/passing/submitting it's own private request. Thus the OLR can be > >removed. > > > > "Everything that was relying upon behind the scenes is now explicitly > creating/passing/submitting it's ..." > ---> > "Everything that was relying on it behind the scenes is now explicitly > creating/passing/submitting its ..." ? > > > >For: VIZ-5115 > >Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > >--- > > drivers/gpu/drm/i915/i915_gem.c | 16 +--------------- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +--- > > drivers/gpu/drm/i915/intel_lrc.c | 6 ++---- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 17 ++--------------- > > drivers/gpu/drm/i915/intel_ringbuffer.h | 4 ---- > > 5 files changed, 6 insertions(+), 41 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >index 60f6671..8e7418b 100644 > >--- a/drivers/gpu/drm/i915/i915_gem.c > >+++ b/drivers/gpu/drm/i915/i915_gem.c > >@@ -1156,15 +1156,9 @@ i915_gem_check_wedge(struct i915_gpu_error *error, > > int > > i915_gem_check_olr(struct drm_i915_gem_request *req) > > { > >- int ret; > >- > > WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); > > > >- ret = 0; > >- if (req == req->ring->outstanding_lazy_request) > >- ret = i915_add_request(req); > >- > >- return ret; > >+ return 0; > > } > > > > static void fake_irq(unsigned long data) > >@@ -2424,8 +2418,6 @@ int __i915_add_request(struct drm_i915_gem_request *request, > > dev_priv = ring->dev->dev_private; > > ringbuf = request->ringbuf; > > > >- WARN_ON(request != ring->outstanding_lazy_request); > >- > > request_start = intel_ring_get_tail(ringbuf); > > /* > > * Emit any outstanding flushes - execbuf can fail to emit the flush > >@@ -2486,7 +2478,6 @@ int __i915_add_request(struct drm_i915_gem_request *request, > > } > > > > trace_i915_gem_request_add(request); > >- ring->outstanding_lazy_request = NULL; > > > > i915_queue_hangcheck(ring->dev); > > > >@@ -2672,9 +2663,6 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, > > > > i915_gem_free_request(request); > > } > >- > >- /* This may not have been flushed before the reset, so clean it now */ > >- i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); > > } > > > > void i915_gem_restore_fences(struct drm_device *dev) > >@@ -3124,8 +3112,6 @@ int i915_gpu_idle(struct drm_device *dev) > > } > > } > > > >- WARN_ON(ring->outstanding_lazy_request); > >- > > ret = intel_ring_idle(ring); > > if (ret) > > return ret; > >diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >index 6a703e6..0eae592 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >@@ -1571,10 +1571,8 @@ err: > > * must be freed again. If it was submitted then it is being tracked > > * on the active request list and no clean up is required here. > > */ > >- if (ret && params->request) { > >+ if (ret && params->request) > > i915_gem_request_unreference(params->request); > >- ring->outstanding_lazy_request = NULL; > >- } > > > > mutex_unlock(&dev->struct_mutex); > > > >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >index 2911cf6..db63ea0 100644 > >--- a/drivers/gpu/drm/i915/intel_lrc.c > >+++ b/drivers/gpu/drm/i915/intel_lrc.c > >@@ -1032,8 +1032,7 @@ int intel_logical_ring_alloc_request(struct intel_engine_cs *ring, > > if (!req_out) > > return -EINVAL; > > > >- if ((*req_out = ring->outstanding_lazy_request) != NULL) > >- return 0; > >+ *req_out = NULL; > > > > request = kzalloc(sizeof(*request), GFP_KERNEL); > > if (request == NULL) > >@@ -1067,7 +1066,7 @@ int intel_logical_ring_alloc_request(struct intel_engine_cs *ring, > > i915_gem_context_reference(request->ctx); > > request->ringbuf = ctx->engine[ring->id].ringbuf; > > > >- *req_out = ring->outstanding_lazy_request = request; > >+ *req_out = request; > > return 0; > > } > > > >@@ -1393,7 +1392,6 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring) > > > > intel_logical_ring_stop(ring); > > WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0); > >- i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); > > > > if (ring->cleanup) > > ring->cleanup(ring); > >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > >index 5eef02e..85daa18 100644 > >--- a/drivers/gpu/drm/i915/intel_ringbuffer.c > >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > >@@ -2034,7 +2034,6 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) > > > > intel_unpin_ringbuffer_obj(ringbuf); > > intel_destroy_ringbuffer_obj(ringbuf); > >- i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); > > > > if (ring->cleanup) > > ring->cleanup(ring); > >@@ -2153,15 +2152,6 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring) > > int intel_ring_idle(struct intel_engine_cs *ring) > > { > > struct drm_i915_gem_request *req; > >- int ret; > >- > >- /* We need to add any requests required to flush the objects and ring */ > >- WARN_ON(ring->outstanding_lazy_request); > >- if (ring->outstanding_lazy_request) { > >- ret = i915_add_request(ring->outstanding_lazy_request); > >- if (ret) > >- return ret; > >- } > > > > /* Wait upon the last request to be completed */ > > if (list_empty(&ring->request_list)) > >@@ -2186,8 +2176,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring, > > if (!req_out) > > return -EINVAL; > > > >- if ((*req_out = ring->outstanding_lazy_request) != NULL) > >- return 0; > >+ *req_out = NULL; > > > > request = kzalloc(sizeof(*request), GFP_KERNEL); > > if (request == NULL) > >@@ -2206,7 +2195,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring, > > return ret; > > } > > > >- *req_out = ring->outstanding_lazy_request = request; > >+ *req_out = request; > > return 0; > > } > > > >@@ -2281,8 +2270,6 @@ void intel_ring_init_seqno(struct intel_engine_cs *ring, u32 seqno) > > struct drm_device *dev = ring->dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > > >- BUG_ON(ring->outstanding_lazy_request); > >- > > if (INTEL_INFO(dev)->gen == 6 || INTEL_INFO(dev)->gen == 7) { > > I915_WRITE(RING_SYNC_0(ring->mmio_base), 0); > > I915_WRITE(RING_SYNC_1(ring->mmio_base), 0); > >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > >index 3b0261f..d2c6427 100644 > >--- a/drivers/gpu/drm/i915/intel_ringbuffer.h > >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > >@@ -258,10 +258,6 @@ struct intel_engine_cs { > > */ > > struct list_head request_list; > > > >- /** > >- * Do we have some not yet emitted requests outstanding? > >- */ > >- struct drm_i915_gem_request *outstanding_lazy_request; > > bool gpu_caches_dirty; > > bool fbc_dirty; > > > > > > Since we're removing the i915_add_request from i915_check_olr and thereby > removing the OLR emission the following comment at > i915_gem_object_flush_active is no longer valid: > > /** > * Ensures that an object will eventually get non-busy by flushing > any required > * write domains, emitting any outstanding lazy request and retiring > and > * completed requests. > */ > static int > i915_gem_object_flush_active(struct drm_i915_gem_object *obj) > { > struct intel_engine_cs *ring; > int ret; > > if (obj->active) { > ring = > i915_gem_request_get_ring(obj->last_read_req); > ret = i915_gem_check_olr(obj->last_read_req); Nice catch! On top of that the int return value is no longer needed, so a follow-up patch should simplify it to void. -Daniel
On 10/03/2015 10:18, Daniel Vetter wrote: > On Mon, Mar 09, 2015 at 11:51:26PM +0000, Tomas Elf wrote: >> On 19/02/2015 17:18, John.C.Harrison@Intel.com wrote: >>> From: John Harrison <John.C.Harrison@Intel.com> >>> >>> The outstanding_lazy_request is no longer used anywhere in the driver. >>> Everything that was looking at it now has a request explicitly passed in from on >>> high. Everything that was relying upon behind the scenes is now explicitly >>> creating/passing/submitting it's own private request. Thus the OLR can be >>> removed. >>> >> "Everything that was relying upon behind the scenes is now explicitly >> creating/passing/submitting it's ..." >> ---> >> "Everything that was relying on it behind the scenes is now explicitly >> creating/passing/submitting its ..." ? >> >> >>> For: VIZ-5115 >>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_gem.c | 16 +--------------- >>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +--- >>> drivers/gpu/drm/i915/intel_lrc.c | 6 ++---- >>> drivers/gpu/drm/i915/intel_ringbuffer.c | 17 ++--------------- >>> drivers/gpu/drm/i915/intel_ringbuffer.h | 4 ---- >>> 5 files changed, 6 insertions(+), 41 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >>> index 60f6671..8e7418b 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem.c >>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>> @@ -1156,15 +1156,9 @@ i915_gem_check_wedge(struct i915_gpu_error *error, >>> int >>> i915_gem_check_olr(struct drm_i915_gem_request *req) >>> { >>> - int ret; >>> - >>> WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); >>> >>> - ret = 0; >>> - if (req == req->ring->outstanding_lazy_request) >>> - ret = i915_add_request(req); >>> - >>> - return ret; >>> + return 0; >>> } >>> >>> static void fake_irq(unsigned long data) >>> @@ -2424,8 +2418,6 @@ int __i915_add_request(struct drm_i915_gem_request *request, >>> dev_priv = ring->dev->dev_private; >>> ringbuf = request->ringbuf; >>> >>> - WARN_ON(request != ring->outstanding_lazy_request); >>> - >>> request_start = intel_ring_get_tail(ringbuf); >>> /* >>> * Emit any outstanding flushes - execbuf can fail to emit the flush >>> @@ -2486,7 +2478,6 @@ int __i915_add_request(struct drm_i915_gem_request *request, >>> } >>> >>> trace_i915_gem_request_add(request); >>> - ring->outstanding_lazy_request = NULL; >>> >>> i915_queue_hangcheck(ring->dev); >>> >>> @@ -2672,9 +2663,6 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, >>> >>> i915_gem_free_request(request); >>> } >>> - >>> - /* This may not have been flushed before the reset, so clean it now */ >>> - i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); >>> } >>> >>> void i915_gem_restore_fences(struct drm_device *dev) >>> @@ -3124,8 +3112,6 @@ int i915_gpu_idle(struct drm_device *dev) >>> } >>> } >>> >>> - WARN_ON(ring->outstanding_lazy_request); >>> - >>> ret = intel_ring_idle(ring); >>> if (ret) >>> return ret; >>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >>> index 6a703e6..0eae592 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >>> @@ -1571,10 +1571,8 @@ err: >>> * must be freed again. If it was submitted then it is being tracked >>> * on the active request list and no clean up is required here. >>> */ >>> - if (ret && params->request) { >>> + if (ret && params->request) >>> i915_gem_request_unreference(params->request); >>> - ring->outstanding_lazy_request = NULL; >>> - } >>> >>> mutex_unlock(&dev->struct_mutex); >>> >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>> index 2911cf6..db63ea0 100644 >>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>> @@ -1032,8 +1032,7 @@ int intel_logical_ring_alloc_request(struct intel_engine_cs *ring, >>> if (!req_out) >>> return -EINVAL; >>> >>> - if ((*req_out = ring->outstanding_lazy_request) != NULL) >>> - return 0; >>> + *req_out = NULL; >>> >>> request = kzalloc(sizeof(*request), GFP_KERNEL); >>> if (request == NULL) >>> @@ -1067,7 +1066,7 @@ int intel_logical_ring_alloc_request(struct intel_engine_cs *ring, >>> i915_gem_context_reference(request->ctx); >>> request->ringbuf = ctx->engine[ring->id].ringbuf; >>> >>> - *req_out = ring->outstanding_lazy_request = request; >>> + *req_out = request; >>> return 0; >>> } >>> >>> @@ -1393,7 +1392,6 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring) >>> >>> intel_logical_ring_stop(ring); >>> WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0); >>> - i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); >>> >>> if (ring->cleanup) >>> ring->cleanup(ring); >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c >>> index 5eef02e..85daa18 100644 >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >>> @@ -2034,7 +2034,6 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) >>> >>> intel_unpin_ringbuffer_obj(ringbuf); >>> intel_destroy_ringbuffer_obj(ringbuf); >>> - i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); >>> >>> if (ring->cleanup) >>> ring->cleanup(ring); >>> @@ -2153,15 +2152,6 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring) >>> int intel_ring_idle(struct intel_engine_cs *ring) >>> { >>> struct drm_i915_gem_request *req; >>> - int ret; >>> - >>> - /* We need to add any requests required to flush the objects and ring */ >>> - WARN_ON(ring->outstanding_lazy_request); >>> - if (ring->outstanding_lazy_request) { >>> - ret = i915_add_request(ring->outstanding_lazy_request); >>> - if (ret) >>> - return ret; >>> - } >>> >>> /* Wait upon the last request to be completed */ >>> if (list_empty(&ring->request_list)) >>> @@ -2186,8 +2176,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring, >>> if (!req_out) >>> return -EINVAL; >>> >>> - if ((*req_out = ring->outstanding_lazy_request) != NULL) >>> - return 0; >>> + *req_out = NULL; >>> >>> request = kzalloc(sizeof(*request), GFP_KERNEL); >>> if (request == NULL) >>> @@ -2206,7 +2195,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring, >>> return ret; >>> } >>> >>> - *req_out = ring->outstanding_lazy_request = request; >>> + *req_out = request; >>> return 0; >>> } >>> >>> @@ -2281,8 +2270,6 @@ void intel_ring_init_seqno(struct intel_engine_cs *ring, u32 seqno) >>> struct drm_device *dev = ring->dev; >>> struct drm_i915_private *dev_priv = dev->dev_private; >>> >>> - BUG_ON(ring->outstanding_lazy_request); >>> - >>> if (INTEL_INFO(dev)->gen == 6 || INTEL_INFO(dev)->gen == 7) { >>> I915_WRITE(RING_SYNC_0(ring->mmio_base), 0); >>> I915_WRITE(RING_SYNC_1(ring->mmio_base), 0); >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h >>> index 3b0261f..d2c6427 100644 >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >>> @@ -258,10 +258,6 @@ struct intel_engine_cs { >>> */ >>> struct list_head request_list; >>> >>> - /** >>> - * Do we have some not yet emitted requests outstanding? >>> - */ >>> - struct drm_i915_gem_request *outstanding_lazy_request; >>> bool gpu_caches_dirty; >>> bool fbc_dirty; >>> >>> >> Since we're removing the i915_add_request from i915_check_olr and thereby >> removing the OLR emission the following comment at >> i915_gem_object_flush_active is no longer valid: >> >> /** >> * Ensures that an object will eventually get non-busy by flushing >> any required >> * write domains, emitting any outstanding lazy request and retiring >> and >> * completed requests. >> */ >> static int >> i915_gem_object_flush_active(struct drm_i915_gem_object *obj) >> { >> struct intel_engine_cs *ring; >> int ret; >> >> if (obj->active) { >> ring = >> i915_gem_request_get_ring(obj->last_read_req); >> ret = i915_gem_check_olr(obj->last_read_req); > Nice catch! On top of that the int return value is no longer needed, so a > follow-up patch should simplify it to void. > -Daniel The comment also talks about flushing write domains but there is no obvious flush call. All it does is the check_olr (which is now a no-op) and the retire (which won't do anything unless the request has already completed). So is there any need for this function at all anymore? Or should it just be removed and replaced with a call to retire instead?
On Fri, Mar 13, 2015 at 01:32:38PM +0000, John Harrison wrote: > On 10/03/2015 10:18, Daniel Vetter wrote: > >On Mon, Mar 09, 2015 at 11:51:26PM +0000, Tomas Elf wrote: > >>On 19/02/2015 17:18, John.C.Harrison@Intel.com wrote: > >>>diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > >>>index 3b0261f..d2c6427 100644 > >>>--- a/drivers/gpu/drm/i915/intel_ringbuffer.h > >>>+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > >>>@@ -258,10 +258,6 @@ struct intel_engine_cs { > >>> */ > >>> struct list_head request_list; > >>> > >>>- /** > >>>- * Do we have some not yet emitted requests outstanding? > >>>- */ > >>>- struct drm_i915_gem_request *outstanding_lazy_request; > >>> bool gpu_caches_dirty; > >>> bool fbc_dirty; > >>> > >>> > >>Since we're removing the i915_add_request from i915_check_olr and thereby > >>removing the OLR emission the following comment at > >>i915_gem_object_flush_active is no longer valid: > >> > >> /** > >> * Ensures that an object will eventually get non-busy by flushing > >>any required > >> * write domains, emitting any outstanding lazy request and retiring > >>and > >> * completed requests. > >> */ > >> static int > >> i915_gem_object_flush_active(struct drm_i915_gem_object *obj) > >> { > >> struct intel_engine_cs *ring; > >> int ret; > >> > >> if (obj->active) { > >> ring = > >>i915_gem_request_get_ring(obj->last_read_req); > >> ret = i915_gem_check_olr(obj->last_read_req); > >Nice catch! On top of that the int return value is no longer needed, so a > >follow-up patch should simplify it to void. > >-Daniel > > The comment also talks about flushing write domains but there is no obvious > flush call. All it does is the check_olr (which is now a no-op) and the > retire (which won't do anything unless the request has already completed). > So is there any need for this function at all anymore? Or should it just be > removed and replaced with a call to retire instead? That's historical baggage - way back we've done lazy cache-flushing, which mean on readback you might have needed to flush caches first. Massive case of premature optimization, the comment is just plainly outdated. See commit 65ce3027415d4dc9ee18ef0a135214b4fb76730b Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Fri Jul 20 12:41:02 2012 +0100 drm/i915: Remove the defunct flushing list and commit cc889e0f6ce6a63c62db17d702ecfed86d58083f Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Wed Jun 13 20:45:19 2012 +0200 drm/i915: disable flushing_list/gpu_write_list You can savely remove the comment (maybe with the above commits referenced in the commit message). -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 60f6671..8e7418b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1156,15 +1156,9 @@ i915_gem_check_wedge(struct i915_gpu_error *error, int i915_gem_check_olr(struct drm_i915_gem_request *req) { - int ret; - WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); - ret = 0; - if (req == req->ring->outstanding_lazy_request) - ret = i915_add_request(req); - - return ret; + return 0; } static void fake_irq(unsigned long data) @@ -2424,8 +2418,6 @@ int __i915_add_request(struct drm_i915_gem_request *request, dev_priv = ring->dev->dev_private; ringbuf = request->ringbuf; - WARN_ON(request != ring->outstanding_lazy_request); - request_start = intel_ring_get_tail(ringbuf); /* * Emit any outstanding flushes - execbuf can fail to emit the flush @@ -2486,7 +2478,6 @@ int __i915_add_request(struct drm_i915_gem_request *request, } trace_i915_gem_request_add(request); - ring->outstanding_lazy_request = NULL; i915_queue_hangcheck(ring->dev); @@ -2672,9 +2663,6 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, i915_gem_free_request(request); } - - /* This may not have been flushed before the reset, so clean it now */ - i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); } void i915_gem_restore_fences(struct drm_device *dev) @@ -3124,8 +3112,6 @@ int i915_gpu_idle(struct drm_device *dev) } } - WARN_ON(ring->outstanding_lazy_request); - ret = intel_ring_idle(ring); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 6a703e6..0eae592 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1571,10 +1571,8 @@ err: * must be freed again. If it was submitted then it is being tracked * on the active request list and no clean up is required here. */ - if (ret && params->request) { + if (ret && params->request) i915_gem_request_unreference(params->request); - ring->outstanding_lazy_request = NULL; - } mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 2911cf6..db63ea0 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1032,8 +1032,7 @@ int intel_logical_ring_alloc_request(struct intel_engine_cs *ring, if (!req_out) return -EINVAL; - if ((*req_out = ring->outstanding_lazy_request) != NULL) - return 0; + *req_out = NULL; request = kzalloc(sizeof(*request), GFP_KERNEL); if (request == NULL) @@ -1067,7 +1066,7 @@ int intel_logical_ring_alloc_request(struct intel_engine_cs *ring, i915_gem_context_reference(request->ctx); request->ringbuf = ctx->engine[ring->id].ringbuf; - *req_out = ring->outstanding_lazy_request = request; + *req_out = request; return 0; } @@ -1393,7 +1392,6 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring) intel_logical_ring_stop(ring); WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0); - i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); if (ring->cleanup) ring->cleanup(ring); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 5eef02e..85daa18 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2034,7 +2034,6 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) intel_unpin_ringbuffer_obj(ringbuf); intel_destroy_ringbuffer_obj(ringbuf); - i915_gem_request_assign(&ring->outstanding_lazy_request, NULL); if (ring->cleanup) ring->cleanup(ring); @@ -2153,15 +2152,6 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring) int intel_ring_idle(struct intel_engine_cs *ring) { struct drm_i915_gem_request *req; - int ret; - - /* We need to add any requests required to flush the objects and ring */ - WARN_ON(ring->outstanding_lazy_request); - if (ring->outstanding_lazy_request) { - ret = i915_add_request(ring->outstanding_lazy_request); - if (ret) - return ret; - } /* Wait upon the last request to be completed */ if (list_empty(&ring->request_list)) @@ -2186,8 +2176,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring, if (!req_out) return -EINVAL; - if ((*req_out = ring->outstanding_lazy_request) != NULL) - return 0; + *req_out = NULL; request = kzalloc(sizeof(*request), GFP_KERNEL); if (request == NULL) @@ -2206,7 +2195,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring, return ret; } - *req_out = ring->outstanding_lazy_request = request; + *req_out = request; return 0; } @@ -2281,8 +2270,6 @@ void intel_ring_init_seqno(struct intel_engine_cs *ring, u32 seqno) struct drm_device *dev = ring->dev; struct drm_i915_private *dev_priv = dev->dev_private; - BUG_ON(ring->outstanding_lazy_request); - if (INTEL_INFO(dev)->gen == 6 || INTEL_INFO(dev)->gen == 7) { I915_WRITE(RING_SYNC_0(ring->mmio_base), 0); I915_WRITE(RING_SYNC_1(ring->mmio_base), 0); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 3b0261f..d2c6427 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -258,10 +258,6 @@ struct intel_engine_cs { */ struct list_head request_list; - /** - * Do we have some not yet emitted requests outstanding? - */ - struct drm_i915_gem_request *outstanding_lazy_request; bool gpu_caches_dirty; bool fbc_dirty;