On ma, 2016-03-07 at 08:10 -0800, Matt Roper wrote: > On Sat, Mar 05, 2016 at 03:25:05AM +0200, Imre Deak wrote: > > Hi Matt, > > > > On Fri, 2016-03-04 at 15:59 -0800, Matt Roper wrote: > > > At the end of an atomic commit, we currently wait for vblanks to > > > complete, call put() on the various runtime PM references, and then > > > try > > > to optimize our watermarks (on platforms that need two-step watermark > > > programming). This can lead to watermark registers being programmed > > > while the power well is powered down. We need to wait until after > > > watermark optimization is complete before dropping our runtime power > > > references. > > > > > > Note that in the future the watermark optimization is probably going > > > to > > > move to an asynchronous workqueue task that happens at some arbitrary > > > point after vblank. When we make that change, we'll no longer > > > necessarily be operating under the power reference held here, so > > > we'll > > > need to wrap the watermark register programmin in a call to > > > intel_runtime_pm_get_if_in_use() or similar. > > > > > > Cc: arun.siluvery@xxxxxxxxxxxxxxx > > > Cc: ville.syrjala@xxxxxxxxxxxxxxx > > > Cc: maarten.lankhorst@xxxxxxxxxxxxxxx > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94349 > > > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark > > > programming (v11)") > > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++---------- > > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 62d36a7..0af08d7 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -13789,16 +13789,6 @@ static int intel_atomic_commit(struct > > > drm_device *dev, > > > if (!state->legacy_cursor_update) > > > intel_atomic_wait_for_vblanks(dev, dev_priv, > > > crtc_vblank_mask); > > > > > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > > > - intel_post_plane_update(to_intel_crtc(crtc)); > > > - > > > - if (put_domains[i]) > > > - modeset_put_power_domains(dev_priv, > > > put_domains[i]); > > > - } > > > - > > > - if (intel_state->modeset) > > > - intel_display_power_put(dev_priv, > > > POWER_DOMAIN_MODESET); > > > - > > > /* > > > * Now that the vblank has passed, we can go ahead and > > > program the > > > * optimal watermarks on platforms that need two-step > > > watermark > > > @@ -13813,6 +13803,16 @@ static int intel_atomic_commit(struct > > > drm_device *dev, > > > dev_priv- > > > > display.optimize_watermarks(intel_cstate); > > > } > > > > > > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > > > + intel_post_plane_update(to_intel_crtc(crtc)); > > > + > > > + if (put_domains[i]) > > > + modeset_put_power_domains(dev_priv, > > > put_domains[i]); > > > + } > > > + > > > + if (intel_state->modeset) > > > +> intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > > > + > > > > Since the error was caused by writing some WM register while the HW is > > suspended, I don't see what would be the point of programming it if the > > register loses its value anyway after dropping the power reference. > > What would make sense to me is to avoid programming the WM registers if > > all the outputs are disabled (like they were when the bug triggered) > > and program them next time around when any output gets enabled. > > Yeah, that was actually the first approach I went with: > > https://patchwork.freedesktop.org/patch/75772/ > > Ville felt like it was more of a workaround than a fix though, so I > posted this alternative instead. Ok, I haven't noticed that patch. One other thing that I realized only after sending my email is that - on some old platforms at least - it would still make sense to program the WM state even with all outputs disabled: the description of FW_BLC_Self[15] on GEN3 for example could mean that memory self refresh is disabled whenever we set this bit to 0, independently of the display output state, although this description is somewhat vague. --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
