| Message ID | 20180115212455.24046-10-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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? It's strange anyway - is this guarding against null ptr deref or what? Regards, Tvrtko > } > > static inline void intel_wait_init_for_seqno(struct intel_wait *wait, u32 seqno) >
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@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
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; } static inline void intel_wait_init_for_seqno(struct intel_wait *wait, u32 seqno)
Avoid calling dma_fence_signal() from inside the interrupt if we haven't enabled signaling on the request. Signed-off-by: Chris Wilson <chris@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(-)