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. Matt > > --Imre > > > > mutex_lock(&dev->struct_mutex); > > drm_atomic_helper_cleanup_planes(dev, state); > > mutex_unlock(&dev->struct_mutex); -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
