On Mon, Feb 20, 2017 at 10:04:06AM +0100, Maarten Lankhorst wrote:
> Op 17-02-17 om 16:01 schreef ville.syrj...@linux.intel.com:
> > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> >
> > In order to make cursor updates actually safe wrt. watermark programming
> > we have to clear the legacy_cursor_update flag in the atomic state. That
> > will cause the regular atomic update path to do the necessary vblank
> > wait after the plane update if needed, otherwise the vblank wait would
> > be skipped and we'd feed the optimal watermarks to the hardware before
> > the plane update has actually happened.
> >
> > To make the slow vs. fast path determination in
> > intel_legacy_cursor_update() a little simpler we can ignore the actual
> > visibility of the plane (which can only get computed once we've already
> > chosen out path) and instead we simply check whether the fb is being
> > set or cleared by the user. This means a fully clipped but logically
> > visible cursor will be considered visible as far as watermark
> > programming is concerned. We can do that for the cursor since it's a
> > fixed size plane and the clipped size doesn't play a role in the
> > watermark computation.
> >
> > This should fix underruns that can occur when the cursor gets
> > enable/disabled or the size gets changed. Hopefully it's good enough
> > that only pure cursor movement and flips go through unthrottled.
> >
> > Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > Cc: Uwe Kleine-König <u...@kleine-koenig.org>
> > Reported-by: Uwe Kleine-König <u...@kleine-koenig.org>
> > Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting
> > legacy page flip to atomic, v3.")
> > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++-----------
> > drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++--------
> > 2 files changed, 30 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index b05d9c85384b..356ac04093e8 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct drm_device
> > *dev,
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > int ret = 0;
> >
> > + /*
> > + * The intel_legacy_cursor_update() fast path takes care
> > + * of avoiding the vblank waits for simple cursor
> > + * movement and flips. For cursor on/off and size changes,
> > + * we want to perform the vblank waits so that watermark
> > + * updates happen during the correct frames. Gen9+ have
> > + * double buffered watermarks and so shouldn't need this.
> > + */
> > + if (INTEL_GEN(dev_priv) < 9)
> > + state->legacy_cursor_update = false;
> Could we perhaps add a check in ilk_compute_pipe_wm which unsets the
> legacy_cursor_update flag so we keep things unsynced as much as possible?
I'd have to sprinkle that stuff everywhere but the SKL code
eventually. Seems a little pointless when I can just plop it
there.
> > ret = drm_atomic_helper_setup_commit(state, nonblock);
> > if (ret)
> > return ret;
> > @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > old_plane_state->src_h != src_h ||
> > old_plane_state->crtc_w != crtc_w ||
> > old_plane_state->crtc_h != crtc_h ||
> > - !old_plane_state->visible ||
> > - old_plane_state->fb->modifier != fb->modifier)
> > + !old_plane_state->fb != !fb)
> > goto slow;
> >
> > new_plane_state = intel_plane_duplicate_state(plane);
> > @@ -13479,10 +13489,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > if (ret)
> > goto out_free;
> >
> > - /* Visibility changed, must take slowpath. */
> > - if (!new_plane_state->visible)
> > - goto slow_free;
> > -
> > ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> > if (ret)
> > goto out_free;
> Those 2 hunks are needed. If you move the cursor out of the visible area or
> back you need to update.
No. I changed the wm code to consider a non-visible but logicall active
cursor as needing proper watermarks. That avoids needing this fallback
path here.
> Easiest way to trigger a bug is load a 256 width cursor out of the visible
> area, move it back in and you get
> a fifo underrun.
>
> Why is switching fb's synced?
It is not.
> Identical sized fb should be unsynced if possible.
>
> > @@ -13522,9 +13528,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > new_plane_state->fb = old_fb;
> > to_intel_plane_state(new_plane_state)->vma = old_vma;
> >
> > - intel_plane->update_plane(plane,
> > - to_intel_crtc_state(crtc->state),
> > - to_intel_plane_state(plane->state));
> > + if (plane->state->visible)
> > + intel_plane->update_plane(plane,
> > + to_intel_crtc_state(crtc->state),
> > + to_intel_plane_state(plane->state));
> > + else
> > + intel_plane->disable_plane(plane, crtc);
> >
> > intel_cleanup_plane_fb(plane, new_plane_state);
> >
> > @@ -13534,8 +13543,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > intel_plane_destroy_state(plane, new_plane_state);
> > return ret;
> >
> > -slow_free:
> > - intel_plane_destroy_state(plane, new_plane_state);
> > slow:
> > return drm_atomic_helper_update_plane(plane, crtc, fb,
> > crtc_x, crtc_y, crtc_w, crtc_h,
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index fe243c65de1a..4de8c40acc7e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1831,20 +1831,24 @@ static uint32_t ilk_compute_cur_wm(const struct
> > intel_crtc_state *cstate,
> > const struct intel_plane_state *pstate,
> > uint32_t mem_value)
> > {
> > + int cpp;
> > +
> > /*
> > - * We treat the cursor plane as always-on for the purposes of watermark
> > - * calculation. Until we have two-stage watermark programming merged,
> > - * this is necessary to avoid flickering.
> > + * Treat cursor with fb as always visible since cursor updates
> > + * can happen faster than the vrefresh rate, and the current
> > + * watermark code doesn't handle that correctly. Cursor updates
> > + * which set/clear the fb or change the cursor size are going
> > + * to get throttled by intel_legacy_cursor_update() to work
> > + * around this problem with the watermark code.
> > */
> > - int cpp = 4;
> > - int width = pstate->base.visible ? pstate->base.crtc_w : 64;
> > -
> > - if (!cstate->base.active)
> > + if (!cstate->base.active || !pstate->base.fb)
> > return 0;
> >
> > + cpp = pstate->base.fb->format->cpp[0];
> > +
> > return ilk_wm_method2(cstate->pixel_rate,
> > cstate->base.adjusted_mode.crtc_htotal,
> > - width, cpp, mem_value);
> > + pstate->base.crtc_w, cpp, mem_value);
> > }
> >
> > /* Only for WM_LP. */
>
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
