Re: [PATCH 09/10] drm/i915: Only signal from interrupt when requested

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 09/10] drm/i915: Only signal from interrupt when requested
Date: Wed, 17 Jan 2018 12:22:33 +0000	[thread overview]
Message-ID: <a2894bed-9e67-df0e-cded-ba0d1633caf6@linux.intel.com> (raw)
In-Reply-To: <20180115212455.24046-10-chris@chris-wilson.co.uk>

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)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx