[Intel-gfx] [PATCH 09/10] drm/i915: Only signal from interrupt when requested

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-01-17 12:22:33)
> 
> On 15/01/2018 21:24, Chris Wilson wrote:
> > Avoid calling dma_fence_signal() from inside the interrupt if we haven't
> > enabled signaling on the request.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/i915_gem_request.c | 2 +-
> >   drivers/gpu/drm/i915/i915_irq.c         | 3 ++-
> >   drivers/gpu/drm/i915/intel_ringbuffer.h | 5 ++---
> >   3 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> > index 08bbd56277e5..46848aef1648 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_request.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> > @@ -1214,7 +1214,7 @@ long i915_wait_request(struct drm_i915_gem_request *req,
> >       if (flags & I915_WAIT_LOCKED)
> >               add_wait_queue(errq, &reset);
> >   
> > -     intel_wait_init(&wait, req);
> > +     intel_wait_init(&wait);
> >   
> >   restart:
> >       do {
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index e5f76d580010..ea290f102784 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1093,7 +1093,8 @@ static void notify_ring(struct intel_engine_cs *engine)
> >               if (i915_seqno_passed(seqno, wait->seqno)) {
> >                       struct drm_i915_gem_request *waiter = wait->request;
> >   
> > -                     if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > +                     if (waiter &&
> > +                         !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> >                                     &waiter->fence.flags) &&
> >                           intel_wait_check_request(wait, waiter))
> >                               rq = i915_gem_request_get(waiter);
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index f406d0ff4612..cea2092d25d9 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -874,11 +874,10 @@ static inline u32 intel_hws_preempt_done_address(struct intel_engine_cs *engine)
> >   /* intel_breadcrumbs.c -- user interrupt bottom-half for waiters */
> >   int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine);
> >   
> > -static inline void intel_wait_init(struct intel_wait *wait,
> > -                                struct drm_i915_gem_request *rq)
> > +static inline void intel_wait_init(struct intel_wait *wait)
> >   {
> >       wait->tsk = current;
> > -     wait->request = rq;
> > +     wait->request = NULL;
> 
> Unless it is in this patch series, I can't see that this now leaves any 
> code path which sets wait->request?

The signaler is now the only entity that sets up wait.request.
 
> It's strange anyway - is this guarding against null ptr deref or what?

Uninitialized pointer deref in the irq handler, yup.
-Chris