Re: [PATCH 52/53] drm/i915: Remove the now obsolete 'outstanding_lazy_request'

Daniel Vetter
2015-03-10T10:16:26Z

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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch