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