[Intel-gfx] [PATCH 05/16] drm/i915: Fix race on unreferencing the wrong mmio-flip-request

[Daniel Vetter]

On Mon, Apr 27, 2015 at 01:41:16PM +0100, Chris Wilson wrote:
> As we perform the mmio-flip without any locking and then try to acquire
> the struct_mutex prior to dereferencing the request, it is possible for
> userspace to queue a new pageflip before the worker can finish clearing
> the old state - and then it will clear the new flip request. The result
> is that the new flip could be completed before the GPU has finished
> rendering.
> 
> The bugs stems from removing the seqno checking in
> commit 536f5b5e86b225dab94c7ff8061ae482b6077387
> Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> Date:   Thu Nov 6 11:03:40 2014 +0200
> 
>     drm/i915: Make mmio flip wait for seqno in the work function
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>

I think I grumbled about this before, but the rq vs. req distinction
elludes me. rq = runqueue in my reading ... What do we need to use "req"
for that we're forced to have such an ambigious name for requests?

Anyway, queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
>  drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
>  3 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c9161c3d1e34..0a412eaf7a31 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2136,10 +2136,12 @@ i915_gem_request_get_ring(struct drm_i915_gem_request *req)
>  	return req ? req->ring : NULL;
>  }
>  
> -static inline void
> +static inline struct drm_i915_gem_request *
>  i915_gem_request_reference(struct drm_i915_gem_request *req)
>  {
> -	kref_get(&req->ref);
> +	if (req)
> +		kref_get(&req->ref);
> +	return req;
>  }
>  
>  static inline void
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e3399af8e0cd..2ca71eeaf2d7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10651,22 +10651,18 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
>  
>  static void intel_mmio_flip_work_func(struct work_struct *work)
>  {
> -	struct intel_crtc *crtc =
> -		container_of(work, struct intel_crtc, mmio_flip.work);
> -	struct intel_mmio_flip *mmio_flip;
> +	struct intel_mmio_flip *mmio_flip =
> +		container_of(work, struct intel_mmio_flip, work);
>  
> -	mmio_flip = &crtc->mmio_flip;
> -	if (mmio_flip->req)
> -		WARN_ON(__i915_wait_request(mmio_flip->req,
> -					    crtc->reset_counter,
> -					    false, NULL, NULL) != 0);
> +	if (mmio_flip->rq)
> +		WARN_ON(__i915_wait_request(mmio_flip->rq,
> +					    mmio_flip->crtc->reset_counter,
> +					    false, NULL, NULL));
>  
> -	intel_do_mmio_flip(crtc);
> -	if (mmio_flip->req) {
> -		mutex_lock(&crtc->base.dev->struct_mutex);
> -		i915_gem_request_assign(&mmio_flip->req, NULL);
> -		mutex_unlock(&crtc->base.dev->struct_mutex);
> -	}
> +	intel_do_mmio_flip(mmio_flip->crtc);
> +
> +	i915_gem_request_unreference__unlocked(mmio_flip->rq);
> +	kfree(mmio_flip);
>  }
>  
>  static int intel_queue_mmio_flip(struct drm_device *dev,
> @@ -10676,12 +10672,17 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
>  				 struct intel_engine_cs *ring,
>  				 uint32_t flags)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_mmio_flip *mmio_flip;
> +
> +	mmio_flip = kmalloc(sizeof(*mmio_flip), GFP_KERNEL);
> +	if (mmio_flip == NULL)
> +		return -ENOMEM;
>  
> -	i915_gem_request_assign(&intel_crtc->mmio_flip.req,
> -				obj->last_write_req);
> +	mmio_flip->rq = i915_gem_request_reference(obj->last_write_req);
> +	mmio_flip->crtc = to_intel_crtc(crtc);
>  
> -	schedule_work(&intel_crtc->mmio_flip.work);
> +	INIT_WORK(&mmio_flip->work, intel_mmio_flip_work_func);
> +	schedule_work(&mmio_flip->work);
>  
>  	return 0;
>  }
> @@ -13669,8 +13670,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
>  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>  
> -	INIT_WORK(&intel_crtc->mmio_flip.work, intel_mmio_flip_work_func);
> -
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  
>  	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 23a42a40abae..7f8cce797ba2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -468,8 +468,9 @@ struct intel_pipe_wm {
>  };
>  
>  struct intel_mmio_flip {
> -	struct drm_i915_gem_request *req;
>  	struct work_struct work;
> +	struct drm_i915_gem_request *rq;
> +	struct intel_crtc *crtc;
>  };
>  
>  struct skl_pipe_wm {
> @@ -554,7 +555,6 @@ struct intel_crtc {
>  	} wm;
>  
>  	int scanline_offset;
> -	struct intel_mmio_flip mmio_flip;
>  
>  	struct intel_crtc_atomic_commit atomic;
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch