On Mon, Oct 06, 2014 at 03:15:25PM +0100, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@xxxxxxxxx
I think this should be split up into the different parts:
- s/obj->ring/obj->last_read_req->ring/ for all the cases that just want
the current ring.
- s/obj->ring/obj->last_read_req/ I think in a bunch of places the code
would actually be more readable if we'd check for obj->active instead.
- All the oddball special cases probably deserve their own commit + nice
explanation in the commit message about why the change is correct.
Cheers, Daniel
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 9 +++++----
> drivers/gpu/drm/i915/i915_drv.h | 2 --
> drivers/gpu/drm/i915/i915_gem.c | 32 +++++++++++++++++++------------
> drivers/gpu/drm/i915/i915_gem_context.c | 12 +++++++++++-
> drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
> 6 files changed, 46 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index df53515..b1d989f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -168,8 +168,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
> *t = '\0';
> seq_printf(m, " (%s mappable)", s);
> }
> - if (obj->ring != NULL)
> - seq_printf(m, " (%s)", obj->ring->name);
> + if (obj->last_read_req != NULL)
> + seq_printf(m, " (%s)",
> + i915_gem_request_get_ring(obj->last_read_req)->name);
> if (obj->frontbuffer_bits)
> seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits);
> }
> @@ -336,7 +337,7 @@ static int per_file_stats(int id, void *ptr, void *data)
> if (ppgtt->file_priv != stats->file_priv)
> continue;
>
> - if (obj->ring) /* XXX per-vma statistic */
> + if (obj->last_read_req) /* XXX per-vma statistic */
> stats->active += obj->base.size;
> else
> stats->inactive += obj->base.size;
> @@ -346,7 +347,7 @@ static int per_file_stats(int id, void *ptr, void *data)
> } else {
> if (i915_gem_obj_ggtt_bound(obj)) {
> stats->global += obj->base.size;
> - if (obj->ring)
> + if (obj->last_read_req)
> stats->active += obj->base.size;
> else
> stats->inactive += obj->base.size;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0790593..cdbbdeb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1859,8 +1859,6 @@ struct drm_i915_gem_object {
> void *dma_buf_vmapping;
> int vmapping_count;
>
> - struct intel_engine_cs *ring;
> -
> /** Breadcrumb of last rendering to the buffer. */
> struct drm_i915_gem_request *last_read_req;
> struct drm_i915_gem_request *last_write_req;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d40dad7..561fb81 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2184,14 +2184,18 @@ static void
> i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> struct intel_engine_cs *ring)
> {
> - struct drm_i915_gem_request *req = intel_ring_get_request(ring);
> + struct drm_i915_gem_request *req;
> + struct intel_engine_cs *old_ring;
>
> BUG_ON(ring == NULL);
> - if (obj->ring != ring && obj->last_write_req) {
> +
> + req = intel_ring_get_request(ring);
> + old_ring = i915_gem_request_get_ring(obj->last_read_req);
> +
> + if (old_ring != ring && obj->last_write_req) {
> /* Keep the request relative to the current ring */
> obj->last_write_req = req;
> }
> - obj->ring = ring;
>
> /* Add a reference if we're newly entering the active list. */
> if (!obj->active) {
> @@ -2230,7 +2234,6 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
> intel_fb_obj_flush(obj, true);
>
> list_del_init(&obj->ring_list);
> - obj->ring = NULL;
>
> obj->last_read_req = NULL;
> obj->last_write_req = NULL;
> @@ -2247,9 +2250,7 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
> static void
> i915_gem_object_retire(struct drm_i915_gem_object *obj)
> {
> - struct intel_engine_cs *ring = obj->ring;
> -
> - if (ring == NULL)
> + if (obj->last_read_req == NULL)
> return;
>
> if (i915_gem_request_completed(obj->last_read_req, true))
> @@ -2769,14 +2770,17 @@ i915_gem_idle_work_handler(struct work_struct *work)
> 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);
> if (ret)
> return ret;
>
> - i915_gem_retire_requests_ring(obj->ring);
> + i915_gem_retire_requests_ring(ring);
> }
>
> return 0;
> @@ -2876,9 +2880,11 @@ int
> i915_gem_object_sync(struct drm_i915_gem_object *obj,
> struct intel_engine_cs *to)
> {
> - struct intel_engine_cs *from = obj->ring;
> + struct intel_engine_cs *from;
> int ret, idx;
>
> + from = i915_gem_request_get_ring(obj->last_read_req);
> +
> if (from == NULL || to == from)
> return 0;
>
> @@ -3889,7 +3895,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> bool was_pin_display;
> int ret;
>
> - if (pipelined != obj->ring) {
> + if (pipelined != i915_gem_request_get_ring(obj->last_read_req)) {
> ret = i915_gem_object_sync(obj, pipelined);
> if (ret)
> return ret;
> @@ -4303,9 +4309,11 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
> ret = i915_gem_object_flush_active(obj);
>
> args->busy = obj->active;
> - if (obj->ring) {
> + if (obj->last_read_req) {
> + struct intel_engine_cs *ring;
> BUILD_BUG_ON(I915_NUM_RINGS > 16);
> - args->busy |= intel_ring_flag(obj->ring) << 16;
> + ring = i915_gem_request_get_ring(obj->last_read_req);
> + args->busy |= intel_ring_flag(ring) << 16;
> }
>
> drm_gem_object_unreference(&obj->base);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a5221d8..8f24831 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -613,7 +613,17 @@ static int do_switch(struct intel_engine_cs *ring,
> * swapped, but there is no way to do that yet.
> */
> from->legacy_hw_ctx.rcs_state->dirty = 1;
> - BUG_ON(from->legacy_hw_ctx.rcs_state->ring != ring);
> +
> + /* BUG_ON(i915_gem_request_get_ring(
> + from->legacy_hw_ctx.rcs_state->last_read_req) != ring); */
> + /* NB: last_read_req has already been updated to the current
> + * request however, that request has not yet been submitted.
> + * Thus last_read_req->ring is guaranteed to be null!
> + * NB2: Doing the check before the update of last_read_req
> + * (which happens in i915_vma_move_to_active() just above),
> + * also fails because last_read_req is almost always null on
> + * entry!
> + */
>
> /* obj is kept alive until the next request by its active ref */
> i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state);
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 9545d96..b9ecbd9 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -681,7 +681,8 @@ static void capture_bo(struct drm_i915_error_buffer *err,
> err->dirty = obj->dirty;
> err->purgeable = obj->madv != I915_MADV_WILLNEED;
> err->userptr = obj->userptr.mm != NULL;
> - err->ring = obj->ring ? obj->ring->id : -1;
> + err->ring = obj->last_read_req ?
> + i915_gem_request_get_ring(obj->last_read_req)->id : -1;
> err->cache_level = obj->cache_level;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9ca8f94..8238aac 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9728,7 +9728,7 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
> else if (i915.enable_execlists)
> return true;
> else
> - return ring != obj->ring;
> + return ring != i915_gem_request_get_ring(obj->last_read_req);
> }
>
> static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
> @@ -9769,8 +9769,6 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
> if (!obj->last_write_req)
> return 0;
>
> - ring = obj->ring;
> -
> if (i915_gem_request_completed(obj->last_write_req, true))
> return 0;
>
> @@ -9778,6 +9776,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
> if (ret)
> return ret;
>
> + ring = i915_gem_request_get_ring(obj->last_write_req);
> if (WARN_ON(!ring->irq_get(ring)))
> return 0;
>
> @@ -9837,14 +9836,15 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
> spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
> intel_crtc->mmio_flip.req = obj->last_write_req;
> i915_gem_request_reference(intel_crtc->mmio_flip.req);
> - intel_crtc->mmio_flip.ring_id = obj->ring->id;
> + BUG_ON(ring != i915_gem_request_get_ring(intel_crtc->mmio_flip.req));
> + intel_crtc->mmio_flip.ring_id = ring->id;
> spin_unlock_irqrestore(&dev_priv->mmio_flip_lock, irq_flags);
>
> /*
> * Double check to catch cases where irq fired before
> * mmio flip data was ready
> */
> - intel_notify_mmio_flip(obj->ring);
> + intel_notify_mmio_flip(ring);
> return 0;
> }
>
> @@ -10022,7 +10022,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> } else if (IS_IVYBRIDGE(dev)) {
> ring = &dev_priv->ring[BCS];
> } else if (INTEL_INFO(dev)->gen >= 7) {
> - ring = obj->ring;
> + ring = i915_gem_request_get_ring(obj->last_read_req);
> if (ring == NULL || ring->id != RCS)
> ring = &dev_priv->ring[BCS];
> } else {
> @@ -10043,7 +10043,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> goto cleanup_unpin;
>
> work->flip_queued_req = obj->last_write_req;
> - work->flip_queued_ring = obj->ring;
> + work->flip_queued_ring =
> + i915_gem_request_get_ring(obj->last_write_req);
> } else {
> ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring,
> page_flip_flags);
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
