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@xxxxxxxxx> > > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
