On Tue, Apr 07, 2015 at 04:20:30PM +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>
Usual question: Does this go boom/do we have an igt?
> ---
> 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 31011988d153..0bc913934d3f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2140,10 +2140,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;
Neat pattern but since it's different than all the other reference
counting functions I don't think it's a good idea ...
> }
>
> static inline void
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0415e40cef6e..94c09bf0047d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10105,22 +10105,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,
> @@ -10130,12 +10126,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;
> }
> @@ -13059,8 +13060,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 686014bd5ec0..0bcc5f36a810 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -403,8 +403,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;
I haven't really followed the discussion with Tvrkto, but what exactly is
the distinction between req and rq again? At least to me rq sounds more
like a short-form for runqueue than request, so why not just leave it at
req?
Besides these two nitpick patch looks good.
-Daniel
> + struct intel_crtc *crtc;
> };
>
> struct skl_pipe_wm {
> @@ -489,7 +490,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
