Re: [PATCH] drm/i915: Redo WMs when cursor size changes

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Feb 26, 2015 at 05:47:35PM -0800, Matt Roper wrote:
> On Thu, Feb 26, 2015 at 02:48:44PM -0800, Joe Konno wrote:
> > From: Joe Konno <joe.konno@intel.com>
> > 
> > In instances where cursor sizes change, as in Chromium Ozone/Freon,
> > watermarks should be recomputed. There should be no hard-coded
> > assumptions about cursor widths. This was corrected originally here:
> > 
> >     commit 64f962e3e38bf6f40bbd2462f8380dee0369e1bf
> >     Author: Chris Wilson <chris@chris-wilson.co.uk>
> >     Date:   Wed Mar 26 12:38:15 2014 +0000
> > 
> >         drm/i915: Recompute WM when the cursor size changes
> > 
> > However, it seems the recompute logic got lost in refactoring.
> > Re-introduce the relevant WM re-compute code from the original patch.
> 
> I don't believe the recompute logic got lost, it just got moved around a
> bit so that it wouldn't happen during our time-sensitive vblank evasion
> while interrupts are disabled.  The old_width != intel_crtc->cursor_width
> check that you're re-adding below already happens in
> intel_check_cursor_plane():
> 
>    finish: 
>            if (intel_crtc->active) {
>                    if (intel_crtc->cursor_width != state->base.crtc_w)
>                            intel_crtc->atomic.update_wm = true;
> 
> We just set a flag so that we know whether we'll need to update
> watermarks later or not.  Then when we get to the commit phase, in
> intel_begin_crtc_commit(), we do the following before entering vblank
> evasion:
> 
>         if (intel_crtc->atomic.update_wm)
>                 intel_update_watermarks(crtc);
> 
> Some of the watermark code we have today sleeps, so we can't call it
> under vblank evasion (i.e., during the commit phase while interrupts are
> disabled).
> 
> Our high-level atomic workflow (which legacy plane ioctls also flow
> through internally) looks something like this:
> 
>  - Some userspace ioctl (SetCursor, SetPlane, etc.)
>       - intel_check_TYPE_plane()
>             - check validity of request, bail if invalid
>             - set flags in intel_crtc->atomic for various stuff we will
>               need to do before/after vblank evasion (e.g., wm's)
>       - intel_begin_crtc_commit()
>             - Perform sleepable operations noted in intel_crtc->atomic
>             - intel_pipe_update_start (interrupts are now disabled!)
>       - intel_commit_TYPE_plane()
>       - intel_finish_crtc_commit()
>             - intel_pipe_update_end (interrupts are now reenabled)
>             - Perform sleepable operations noted in intel_crtc->atomic
> 
> So your patch below could result in sleeps happening while vblanks are
> disabled, which is bad (IIRC, most of those sleeps are in the SKL
> codepath right now, but I think there's a workaround-related wait for
> IVB as well).
> 
> Our watermark code needs a lot of work to beat it into proper shape for
> atomic and that's what I'm working on at the moment.

Alternative issue is that wm recompute happens before we update all the
legacy state. Tvrkto just stumbled over that for some of his skl watermark
code, he had to switch a few places in the wm code from looking at
plane->fb to look at plane->state->fb.

Similar changes might be needed for the cursor wm code -
cursor_width/height is kinda redundant since universal planes support.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx