| Message ID | 1400856685-3158-2-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 23, 2014 at 4:51 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Now that we moved all the register writing to haswell_crtc_enable, we > can also move the PLL enabling there. This is the last step to do > before allowing runtime PM on DPMS. > > Daniel has some patches to do this while also moving all the HSW PLL > code to the shared DPLL structs, but while we still discuss the color > of those patches, we can apply this quick one and at least get runtime > PM on DPMS running in the meantime. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Nope. If you want to do this you need 2 refcounts per shared dpll: - One that tracks whether a crtc has a mode set that needs a given shared dpll, whether the crtc is actually enabled at the hw level or no. You need to increment that refcount in ->crtc_mode_set and decrement it in ->crtc_off or ->crtc_mode_set. - A 2nd refcount which tracks active usage and gets incremented in ->crtc_enable and decremented in ->crtc_disable. This need is the entire point of the shared dpll infrastructure. I haven't tested you patch on actual hw but from reading through it you still keep that single refcount and increment it in crtc_mode_set and decrement it in crtc_disable. That means: - Multiple dpms off/on cycles will underrun the refcount since it's unbalanced. - Sharing is broken since if you dpms off a crtc its pll can be stolen and reused, which means upon dpms on you get a completely wrong pll setting. I think the kms_flip/*dpms* and kms_flip/2x*dpms* tests should be able to hit this. Wrt testing please also note that even for just testing runtime pm running only pm_rpm isn't good enough - some of the new rpm tests I've added are subtests of kms_flip (since I wanted to test vblanks and similar stuff). So you really need to use piglits filtering to get all the rpm tests. Cheers, Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3da73ef..1ea4fbe 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4052,6 +4052,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) > if (intel_crtc->active) > return; > > + intel_ddi_pll_enable(intel_crtc); > + > if (intel_crtc->config.has_dp_encoder) > intel_dp_set_m_n(intel_crtc); > > @@ -4243,6 +4245,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) > intel_update_fbc(dev); > intel_edp_psr_update(dev); > mutex_unlock(&dev->struct_mutex); > + > + intel_ddi_put_crtc_pll(crtc); > } > > static void ironlake_crtc_off(struct drm_crtc *crtc) > @@ -4253,7 +4257,6 @@ static void ironlake_crtc_off(struct drm_crtc *crtc) > > static void haswell_crtc_off(struct drm_crtc *crtc) > { > - intel_ddi_put_crtc_pll(crtc); > } > > static void i9xx_pfit_enable(struct intel_crtc *crtc) > @@ -7471,7 +7474,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, > > if (!intel_ddi_pll_select(intel_crtc)) > return -EINVAL; > - intel_ddi_pll_enable(intel_crtc); > > intel_crtc->lowfreq_avail = false; > > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3da73ef..1ea4fbe 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4052,6 +4052,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) if (intel_crtc->active) return; + intel_ddi_pll_enable(intel_crtc); + if (intel_crtc->config.has_dp_encoder) intel_dp_set_m_n(intel_crtc); @@ -4243,6 +4245,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) intel_update_fbc(dev); intel_edp_psr_update(dev); mutex_unlock(&dev->struct_mutex); + + intel_ddi_put_crtc_pll(crtc); } static void ironlake_crtc_off(struct drm_crtc *crtc) @@ -4253,7 +4257,6 @@ static void ironlake_crtc_off(struct drm_crtc *crtc) static void haswell_crtc_off(struct drm_crtc *crtc) { - intel_ddi_put_crtc_pll(crtc); } static void i9xx_pfit_enable(struct intel_crtc *crtc) @@ -7471,7 +7474,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, if (!intel_ddi_pll_select(intel_crtc)) return -EINVAL; - intel_ddi_pll_enable(intel_crtc); intel_crtc->lowfreq_avail = false;