[Intel-gfx] [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Ville Syrjälä]

On Thu, Mar 05, 2015 at 01:20:17PM +0100, Daniel Vetter wrote:
> On Wed, Mar 04, 2015 at 08:21:16PM +0200, Ville Syrjälä wrote:
> > On Wed, Mar 04, 2015 at 05:13:07PM +0100, Daniel Vetter wrote:
> > > On Wed, Mar 04, 2015 at 11:45:42AM +0200, Ville Syrjälä wrote:
> > > > On Tue, Mar 03, 2015 at 06:15:12PM -0800, Matt Roper wrote:
> > > > > Universal planes allow us to have an active CRTC without a primary plane
> > > > > framebuffer bound.  Drop the test for primary->fb from
> > > > > intel_crtc_active() since we can clearly have active CRTC's without a
> > > > > framebuffer, and this check is now interfering with watermark
> > > > > calculations when we try to re-enable the primary plane.
> > > > > 
> > > > > Note that commit
> > > > > 
> > > > >         commit 0fda65680e92545caea5be7805a7f0a617fb6c20
> > > > >         Author: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > > >         Date:   Fri Feb 27 15:12:35 2015 +0000
> > > > > 
> > > > >             drm/i915/skl: Update watermarks for Y tiling
> > > > > 
> > > > > adds a test for primary plane enable/disable to trigger a watermark
> > > > > update (previously we ignored updates to primary planes, which wasn't
> > > > > really correct, but we got lucky since we always pretended the primary
> > > > > plane was on).  Tvrtko's patch tries to update watermarks when we
> > > > > re-enable the primary plane, but that watermark computation gets aborted
> > > > > early because intel_crtc_active() always returns false when the primary
> > > > > plane is disabled; this leads to watermark underruns (at least on
> > > > > platforms with ILK-style watermark code).
> > > > 
> > > > I think this will make a bunch of the old platforms blow up. Well, I
> > > > think they might already blow up since they now look at crtc->state->fb
> > > > based on the result of intel_crtc_active(). So I believe more fixes are
> > > > needed all over the wm code at least.
> > > > 
> > > > In fact looking at the code, I think most (or all?) of the call sites in
> > > > the in the ilk+ wm code should just be using crtc->active. So for some
> > > > extra safety it might make sense to do leave intel_crtc_active() alone
> > > > for now, or in fact we should maybe change it to also look at
> > > > primary->state->fb instead. That should more or less keep the old
> > > > platforms in the same state as they were before, while letting new
> > > > platforms handle each plane properly.
> > > 
> > > intel_crtc->active shouldn't be used anywhere in state computation code.
> > > The only thing you're allowed to look at is crtc_state->enable in the
> > > atomic world.
> > 
> > We'll want crtc->active in a bunch of places in the ilk+ wm code
> > since it's actually intersted in the current state, not the future
> > state.
> > 
> > The way it should work (after getting my remaining ilk+ wm patches
> > reworked+merged):
> > 
> > 1. Compute new watermarks for this specific pipe using the future plane/crtc state.
> >    We don't consider at all how many pipes will be active or are active currently,
> >    as those are not variables in our actual watermark equations.
> > 2. Fail the operation if LP0 comes out invalid. LP0 limits do not depend
> >    on the number of active pipes, so a failure to meet the LP0 limits is
> >    always fatal. LP1+ limits we can ignore at this stage since LP1+ are
> >    optional.
> > 3. Merge the watermarks computed at step 1 with the current watermarks
> >    on the pipe and store the result as the current watermarks on this pipe.
> >    These are what I called "intermediate watermarks" and are needed due
> >    to lack of double buffered watermark registers.
> > 4. Merge the current watermarks from all currently active pipes, applying
> >    whatever extra limits are imposed by the number of active pipes (eg.
> >    disallow LP1+ on ilk/snb/ivb with multiple pipes) and write the result
> >    to the hardware registers
> > 5. Commit the plane/crtc state
> > 6. Wait until the hardware plane/crtc state has changed (ie. until the
> >    next vblank)
> > 7. Store the watermarks computed at step 1 as the current watermarks
> >    of the pipe, replacing the "intermediate watermarks" from step 3
> > 8. <same as step 4>
> > 
> > So step 1 is pretty much the only place where we should consider the
> > future state as opposed to the current state of the hardware.
> 
> Yeah that's a valid use-case, but you instead want to look at
> crtc_state->active. intel_crtc->active will just be a consistency check in
> the end really. And since crtc_state->active is set correctly even in the
> check phase of the modeset we could compute the final watermarks right
> away.

During crtc enable/disable we may need to do a watermark updates like so:

enable:
1. active=true
2. update watermarks
3. enable pipe

disable:
1. disable pipe
2. active=false
3. update watermarks

So if crtc_state->active works for that then we can use it.

But I think we still want to keep doing what I listed since we don't
want to go recomputing the watermarks for every pipe when only one pipe
needs updating. For one, that would involve taking locks on all the
crtcs/planes which sounds like something to avoid. There are also other
factors besides the number of active pipes affecting how we merge the
watermarks so I can't accept any "but all locks are held at modeset"
excuse here :)

If we recompute the watermarks only for the specific pipe we're changing
we already have the required locks. Any shared wm state will be protected
by dev_priv->wm.mutex.

> intel_crtc->active is only set while committing the state to hw.
> 
> This is important from a locking pov since state objects must be invariant
> once committed to foo->state pointers. So you'd end up with some
> interesting book-keeping problems I expect. We'll see.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC