On Tue, Apr 19, 2016 at 09:52:27AM +0200, Maarten Lankhorst wrote:
> With intel_pipe_update begin/end we ensure that the mmio updates
> don't run during vblank interrupt, using the hw counter we can
> be sure that when current vblank count != vblank count at the time
> of pipe_update_end the mmio update is complete.
Still seems too racy for my taste.
What should be done is:
1. evade vblank
2. write regs
3. sample vblank counter and allow the irq handler to complete the flip
when passing the target vblank count
4. check if we just missed the vblank irq, and if so complete the flip immediately
Also I would have started by ridding us of the flip done interrupt
first, because that means every platform would then follow the same code
path leading to better testing coverage (and hopefully less bugs). And
it would allow us to remove the extra vblank wait hacks on BDW.
> This allows us to use mmio updates on all platforms, using the
> update_plane call.
>
> With Chris Wilson's patch to skip waiting for vblanks for
> legacy_cursor_update this potentially leaves a small race
> condition, in which update_plane can be called with a freed
> crtc_state. Because of this commit acf4e84d61673
> ("drm/i915: Avoid stalling on pending flips for legacy cursor updates")
> is temporarily reverted.
>
> Changes since v1:
> - Split out the flip_work rename.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 105 ++++-------------------------------
> drivers/gpu/drm/i915/intel_drv.h | 1 -
> 2 files changed, 12 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8b61a07c4c52..d1181089512a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11324,9 +11324,6 @@ static bool use_mmio_flip(struct intel_engine_cs *engine,
> if (engine == NULL)
> return true;
>
> - if (INTEL_INFO(engine->dev)->gen < 5)
> - return false;
> -
> if (i915.use_mmio_flip < 0)
> return false;
> else if (i915.use_mmio_flip > 0)
> @@ -11341,92 +11338,15 @@ static bool use_mmio_flip(struct intel_engine_cs *engine,
> return engine != i915_gem_request_get_engine(obj->last_write_req);
> }
>
> -static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
> - unsigned int rotation,
> - struct intel_flip_work *work)
> -{
> - struct drm_device *dev = intel_crtc->base.dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
> - const enum pipe pipe = intel_crtc->pipe;
> - u32 ctl, stride, tile_height;
> -
> - ctl = I915_READ(PLANE_CTL(pipe, 0));
> - ctl &= ~PLANE_CTL_TILED_MASK;
> - switch (fb->modifier[0]) {
> - case DRM_FORMAT_MOD_NONE:
> - break;
> - case I915_FORMAT_MOD_X_TILED:
> - ctl |= PLANE_CTL_TILED_X;
> - break;
> - case I915_FORMAT_MOD_Y_TILED:
> - ctl |= PLANE_CTL_TILED_Y;
> - break;
> - case I915_FORMAT_MOD_Yf_TILED:
> - ctl |= PLANE_CTL_TILED_YF;
> - break;
> - default:
> - MISSING_CASE(fb->modifier[0]);
> - }
> -
> - /*
> - * The stride is either expressed as a multiple of 64 bytes chunks for
> - * linear buffers or in number of tiles for tiled buffers.
> - */
> - if (intel_rotation_90_or_270(rotation)) {
> - /* stride = Surface height in tiles */
> - tile_height = intel_tile_height(dev_priv, fb->modifier[0], 0);
> - stride = DIV_ROUND_UP(fb->height, tile_height);
> - } else {
> - stride = fb->pitches[0] /
> - intel_fb_stride_alignment(dev_priv, fb->modifier[0],
> - fb->pixel_format);
> - }
> -
> - /*
> - * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
> - * PLANE_SURF updates, the update is then guaranteed to be atomic.
> - */
> - I915_WRITE(PLANE_CTL(pipe, 0), ctl);
> - I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> -
> - I915_WRITE(PLANE_SURF(pipe, 0), work->gtt_offset);
> - POSTING_READ(PLANE_SURF(pipe, 0));
> -}
> -
> -static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc,
> - struct intel_flip_work *work)
> -{
> - struct drm_device *dev = intel_crtc->base.dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_framebuffer *intel_fb =
> - to_intel_framebuffer(intel_crtc->base.primary->fb);
> - struct drm_i915_gem_object *obj = intel_fb->obj;
> - i915_reg_t reg = DSPCNTR(intel_crtc->plane);
> - u32 dspcntr;
> -
> - dspcntr = I915_READ(reg);
> -
> - if (obj->tiling_mode != I915_TILING_NONE)
> - dspcntr |= DISPPLANE_TILED;
> - else
> - dspcntr &= ~DISPPLANE_TILED;
> -
> - I915_WRITE(reg, dspcntr);
> -
> - I915_WRITE(DSPSURF(intel_crtc->plane), work->gtt_offset);
> - POSTING_READ(DSPSURF(intel_crtc->plane));
> -}
> -
> static void intel_mmio_flip_work_func(struct work_struct *w)
> {
> struct intel_flip_work *work =
> container_of(w, struct intel_flip_work, mmio_work);
> struct intel_crtc *crtc = to_intel_crtc(work->crtc);
> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> - struct intel_framebuffer *intel_fb =
> - to_intel_framebuffer(crtc->base.primary->fb);
> - struct drm_i915_gem_object *obj = intel_fb->obj;
> + struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_plane *primary = to_intel_plane(crtc->base.primary);
> + struct drm_i915_gem_object *obj = intel_fb_obj(primary->base.state->fb);
>
> if (work->flip_queued_req)
> WARN_ON(__i915_wait_request(work->flip_queued_req,
> @@ -11440,13 +11360,9 @@ static void intel_mmio_flip_work_func(struct work_struct *w)
> MAX_SCHEDULE_TIMEOUT) < 0);
>
> intel_pipe_update_start(crtc);
> -
> - if (INTEL_GEN(dev_priv) >= 9)
> - skl_do_mmio_flip(crtc, work->rotation, work);
> - else
> - /* use_mmio_flip() retricts MMIO flips to ilk+ */
> - ilk_do_mmio_flip(crtc, work);
> -
> + primary->update_plane(&primary->base,
> + crtc->config,
> + to_intel_plane_state(primary->base.state));
> intel_pipe_update_end(crtc, work);
> }
>
> @@ -11479,6 +11395,10 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
> smp_mb__after_atomic();
> vblank = intel_crtc_get_vblank_counter(intel_crtc);
>
> + if (is_mmio_work(work))
> + /* MMIO work completes when vblank is different from flip_queued_vblank. */
> + return vblank != work->flip_queued_vblank;
> +
> if (work->flip_ready_vblank == 0) {
> if (work->flip_queued_req &&
> !i915_gem_request_completed(work->flip_queued_req, true))
> @@ -11519,7 +11439,7 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
> spin_lock(&dev->event_lock);
> work = intel_crtc->flip_work;
> if (work != NULL && __intel_pageflip_stall_check(dev, crtc)) {
> - WARN_ONCE(1,
> + WARN_ONCE(!is_mmio_work(work),
> "Kicking stuck page flip: queued at %d, now %d\n",
> work->flip_queued_vblank, intel_crtc_get_vblank_counter(intel_crtc));
> page_flip_completed(intel_crtc);
> @@ -11675,7 +11595,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> work->gtt_offset = intel_plane_obj_offset(to_intel_plane(primary),
> obj, 0);
> work->gtt_offset += intel_crtc->dspaddr_offset;
> - work->rotation = crtc->primary->state->rotation;
>
> if (mmio_flip) {
> INIT_WORK(&work->mmio_work, intel_mmio_flip_work_func);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e10e6959fd43..bc310513adb8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -950,7 +950,6 @@ struct intel_flip_work {
> struct drm_i915_gem_request *flip_queued_req;
> u32 flip_queued_vblank;
> u32 flip_ready_vblank;
> - unsigned int rotation;
> };
>
> struct intel_load_detect_pipe {
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
