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@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxx>
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
