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

[Daniel Vetter]

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. 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