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

[John Harrison <John.C.Harrison@xxxxxxxxx>]

On 26/11/2014 13:24, Daniel Vetter wrote:
> On Mon, Nov 24, 2014 at 06:49:39PM +0000, John.C.Harrison@xxxxxxxxx wrote:
> > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Reviewed-by: Thomas Daniel <Thomas.Daniel@xxxxxxxxx>
> > ---
> >   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.

> > +}
> > +
> >   #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)))) {
> >   		ring->irq_put(ring);
> > -		ring->trace_irq_seqno = 0;
> > +		i915_gem_request_assign(&ring->trace_irq_req, NULL);
> >   	}
> >   
> >   	while (!list_empty(&ring->delayed_free_list)) {
> > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> > index 2c0327b..2ade958 100644
> > --- a/drivers/gpu/drm/i915/i915_trace.h
> > +++ b/drivers/gpu/drm/i915/i915_trace.h
> > @@ -369,7 +369,7 @@ TRACE_EVENT(i915_gem_ring_dispatch,
> >   			   __entry->ring = ring->id;
> >   			   __entry->seqno = i915_gem_request_get_seqno(req);
> >   			   __entry->flags = flags;
> > -			   i915_trace_irq_get(ring, __entry->seqno);
> > +			   i915_trace_irq_get(ring, req);
> >   			   ),
> >   
> >   	    TP_printk("dev=%u, ring=%u, seqno=%u, flags=%x",
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index cd1a9f9..c8b84de 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -142,7 +142,7 @@ struct  intel_engine_cs {
> >   
> >   	unsigned irq_refcount; /* protected by dev_priv->irq_lock */
> >   	u32		irq_enable_mask;	/* bitmask to enable ring interrupt */
> > -	u32		trace_irq_seqno;
> > +	struct drm_i915_gem_request *trace_irq_req;
> >   	bool __must_check (*irq_get)(struct intel_engine_cs *ring);
> >   	void		(*irq_put)(struct intel_engine_cs *ring);
> >   
> > @@ -444,10 +444,4 @@ intel_ring_get_request(struct intel_engine_cs *ring)
> >   	return ring->outstanding_lazy_request;
> >   }
> >   
> > -static inline void i915_trace_irq_get(struct intel_engine_cs *ring, u32 seqno)
> > -{
> > -	if (ring->trace_irq_seqno == 0 && ring->irq_get(ring))
> > -		ring->trace_irq_seqno = seqno;
> > -}
> > -
> >   #endif /* _INTEL_RINGBUFFER_H_ */
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx