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