[Intel-gfx] [PATCH v3 17/28] drm/i915: Convert 'trace_irq' to use requests rather than seqnos

[Chris Wilson]

On Wed, Nov 26, 2014 at 02:12:53PM +0000, John Harrison wrote:
> On 26/11/2014 13:24, Daniel Vetter wrote:
> >On Mon, Nov 24, 2014 at 06:49:39PM +0000, John.C.Harrison at Intel.com wrote:
> >>From: John Harrison <John.C.Harrison at Intel.com>
> >>
> >>Updated the trace_irq code to use requests instead of seqnos. This includes
> >>reference counting the request object to ensure it sticks around when required.
> >>Note that getting access to the reference counting functions means moving the
> >>inline i915_trace_irq_get() function from intel_ringbuffer.h to i915_drv.h.
> >>
> >>For: VIZ-4377
> >>Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> >>Reviewed-by: Thomas Daniel <Thomas.Daniel at intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.h         |    7 +++++++
> >>  drivers/gpu/drm/i915/i915_gem.c         |    7 ++++---
> >>  drivers/gpu/drm/i915/i915_trace.h       |    2 +-
> >>  drivers/gpu/drm/i915/intel_ringbuffer.h |    8 +-------
> >>  4 files changed, 13 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index 8bfdac6..831fae2 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -3130,4 +3130,11 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
> >>  	}
> >>  }
> >>+static inline void i915_trace_irq_get(struct intel_engine_cs *ring,
> >>+				      struct drm_i915_gem_request *req)
> >>+{
> >>+	if (ring->trace_irq_req == NULL && ring->irq_get(ring))
> >>+		i915_gem_request_assign(&ring->trace_irq_req, req);
> >This looks a bit suspiciuos. Thus far ring->trace_irq_req was essentially
> >ot protected at all by anything. But that was nothing troublesome since we
> >didn't hang a real resource of it.
> >
> >But now there's a refcounted request in that pointer, which means if we
> >race we leak. I'll skip this patch for now.
> >-Daniel
> 
> Race how? The assignment only ever occurs from inside execbuffer
> submission at which point the driver mutex lock is held. Therefore
> it is very definitely protected. Not doing the reference count means
> that there is now the possibility of a dangling pointer and thus the
> possibility of going bang with a kernel oops.

It's suspicious because the code is broken and you didn't read my patch.
 
> >
> >>+}
> >>+
> >>  #endif
> >>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>index 69c3e50..69bddcb 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>@@ -2810,10 +2810,11 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
> >>  		i915_gem_free_request(request);
> >>  	}
> >>-	if (unlikely(ring->trace_irq_seqno &&
> >>-		     i915_seqno_passed(seqno, ring->trace_irq_seqno))) {
> >>+	if (unlikely(ring->trace_irq_req &&
> >>+		     i915_seqno_passed(seqno,
> >>+			 i915_gem_request_get_seqno(ring->trace_irq_req)))) {

It simply does not need transitioning to requests.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre