2013/5/5 Chris Wilson <chris at chris-wilson.co.uk>: > On Fri, May 03, 2013 at 05:23:38PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni at intel.com> >> >> The spec says the linetime watermarks must be programmed before >> enabling any display low power watermarks, but we're currently >> updating the linetime watermarks after we call intel_update_watermarks >> (and only at crtc_mode_set, not at crtc_{enable,disable}). So IMHO the >> best way guarantee the linetime watermarks will be updated before the >> low power watermarks is inside the update_wm function, because it's >> the function that enables low power watermarks. And since Haswell is >> the only platform that has linetime watermarks, let's completely kill >> the "intel_update_linetime_watermarks" abstraction and just use the >> intel_update_watermarks abstraction by creating haswell_update_wm. > >> +static void haswell_update_wm(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct drm_crtc *crtc; >> + enum pipe pipe; >> + >> + for_each_pipe(pipe) { >> + crtc = dev_priv->pipe_to_crtc_mapping[pipe]; >> + haswell_update_linetime_wm(dev, crtc); >> + } > > The LP watermarks are still enabled at this point. Did you mean "they're not disabled yet"? Well, at least now we're programming the linetime registers _before_ we update the LP registers, and every time. If we want to be 100% spec-compliant without any intermediate steps then I guess we can discard this linetime series and merge it all inside the single patch that will fully implement haswell_update_wm. But it will be one single big patch. Just tell me which one you prefer. >> + >> + sandybridge_update_wm(dev); >> +} > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre -- Paulo Zanoni