On Tue, 2014-07-01 at 09:18 +0530, Deepak S wrote:
> On Friday 13 June 2014 05:24 PM, Imre Deak wrote:
> > This functionality will be also needed by an upcoming patch, so factor
> > it out. As a bonus this also makes things a bit more uniform across
> > platforms. Note that this also changes the register read-modify-write
> > to a simple write during disabling. This is what we do during enabling
> > anyway and according to the spec all the relevant bits are reserved-MBZ
> > or reserved with a 0 default value.
> >
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 2 ++
> > drivers/gpu/drm/i915/intel_pm.c | 75 ++++++++++++++++++++---------------------
> > 2 files changed, 39 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f36d9eb..211a173 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2590,6 +2590,8 @@ extern void gen6_set_rps(struct drm_device *dev, u8 val);
> > extern void valleyview_set_rps(struct drm_device *dev, u8 val);
> > extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv);
> > extern int valleyview_rps_min_freq(struct drm_i915_private *dev_priv);
> > +extern void intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
> > + bool enable);
> > extern void intel_detect_pch(struct drm_device *dev);
> > extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
> > extern int intel_enable_rc6(const struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 0b088fe..e55622e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -789,12 +789,33 @@ static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
> > return NULL;
> > }
> >
> > -static void pineview_disable_cxsr(struct drm_device *dev)
> > +void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct drm_device *dev = dev_priv->dev;
> > + u32 val;
> >
> > - /* deactivate cxsr */
> > - I915_WRITE(DSPFW3, I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN);
> > + if (IS_VALLEYVIEW(dev)) {
> > + I915_WRITE(FW_BLC_SELF_VLV, enable ? FW_CSPWRDWNEN : 0);
> > + } else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
> > + I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
> > + } else if (IS_PINEVIEW(dev)) {
> > + val = I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN;
> > + val |= enable ? PINEVIEW_SELF_REFRESH_EN : 0;
> > + I915_WRITE(DSPFW3, val);
> > + } else if (IS_I945G(dev) || IS_I945GM(dev)) {
> > + val = enable ? _MASKED_BIT_ENABLE(FW_BLC_SELF_EN) :
> > + _MASKED_BIT_DISABLE(FW_BLC_SELF_EN);
> > + I915_WRITE(FW_BLC_SELF, val);
> > + } else if (IS_I915GM(dev)) {
> > + val = enable ? _MASKED_BIT_ENABLE(INSTPM_SELF_EN) :
> > + _MASKED_BIT_DISABLE(INSTPM_SELF_EN);
> > + I915_WRITE(INSTPM, val);
> > + } else {
> > + return;
> > + }
> > +
> > + DRM_DEBUG_KMS("memory self-refresh is %s\n",
> > + enable ? "enabled" : "disabled");
> > }
> >
> > /*
> > @@ -1033,7 +1054,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
> > dev_priv->fsb_freq, dev_priv->mem_freq);
> > if (!latency) {
> > DRM_DEBUG_KMS("Unknown FSB/MEM found, disable CxSR\n");
> > - pineview_disable_cxsr(dev);
> > + intel_set_memory_cxsr(dev_priv, false);
> > return;
> > }
> >
> > @@ -1085,12 +1106,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
> > DRM_DEBUG_KMS("DSPFW3 register is %x\n", reg);
> >
> > /* activate cxsr */
> > - I915_WRITE(DSPFW3,
> > - I915_READ(DSPFW3) | PINEVIEW_SELF_REFRESH_EN);
> > - DRM_DEBUG_KMS("Self-refresh is enabled\n");
> > - } else {
> > - pineview_disable_cxsr(dev);
> > - DRM_DEBUG_KMS("Self-refresh is disabled\n");
> > + intel_set_memory_cxsr(dev_priv, true);
>
> I think we need to pass "false" here to disable cxsr right?
Yes, in the else branch, which I left out for some reason.. Thanks a lot
for spotting it, I'll send an updated patch.
--Imre
>
> Apart for this everything else looks good.
>
> Reviewed-by: Deepak S<deepak.s@xxxxxxxxxxxxxxx>
>
> > }
> > }
> >
> > @@ -1342,10 +1358,9 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
> > &valleyview_wm_info,
> > &valleyview_cursor_wm_info,
> > &ignore_plane_sr, &cursor_sr)) {
> > - I915_WRITE(FW_BLC_SELF_VLV, FW_CSPWRDWNEN);
> > + intel_set_memory_cxsr(dev_priv, true);
> > } else {
> > - I915_WRITE(FW_BLC_SELF_VLV,
> > - I915_READ(FW_BLC_SELF_VLV) & ~FW_CSPWRDWNEN);
> > + intel_set_memory_cxsr(dev_priv, false);
> > plane_sr = cursor_sr = 0;
> > }
> >
> > @@ -1394,10 +1409,9 @@ static void g4x_update_wm(struct drm_crtc *crtc)
> > &g4x_wm_info,
> > &g4x_cursor_wm_info,
> > &plane_sr, &cursor_sr)) {
> > - I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
> > + intel_set_memory_cxsr(dev_priv, true);
> > } else {
> > - I915_WRITE(FW_BLC_SELF,
> > - I915_READ(FW_BLC_SELF) & ~FW_BLC_SELF_EN);
> > + intel_set_memory_cxsr(dev_priv, false);
> > plane_sr = cursor_sr = 0;
> > }
> >
> > @@ -1468,13 +1482,10 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
> > DRM_DEBUG_KMS("self-refresh watermark: display plane %d "
> > "cursor %d\n", srwm, cursor_sr);
> >
> > - if (IS_CRESTLINE(dev))
> > - I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
> > + intel_set_memory_cxsr(dev_priv, true);
> > } else {
> > /* Turn off self refresh if both pipes are enabled */
> > - if (IS_CRESTLINE(dev))
> > - I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
> > - & ~FW_BLC_SELF_EN);
> > + intel_set_memory_cxsr(dev_priv, false);
> > }
> >
> > DRM_DEBUG_KMS("Setting FIFO watermarks - A: 8, B: 8, C: 8, SR %d\n",
> > @@ -1560,10 +1571,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> > cwm = 2;
> >
> > /* Play safe and disable self-refresh before adjusting watermarks. */
> > - if (IS_I945G(dev) || IS_I945GM(dev))
> > - I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN_MASK | 0);
> > - else if (IS_I915GM(dev))
> > - I915_WRITE(INSTPM, _MASKED_BIT_DISABLE(INSTPM_SELF_EN));
> > + intel_set_memory_cxsr(dev_priv, false);
> >
> > /* Calc sr entries for one plane configs */
> > if (HAS_FW_BLC(dev) && enabled) {
> > @@ -1609,17 +1617,8 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> > I915_WRITE(FW_BLC, fwater_lo);
> > I915_WRITE(FW_BLC2, fwater_hi);
> >
> > - if (HAS_FW_BLC(dev)) {
> > - if (enabled) {
> > - if (IS_I945G(dev) || IS_I945GM(dev))
> > - I915_WRITE(FW_BLC_SELF,
> > - FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
> > - else if (IS_I915GM(dev))
> > - I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_SELF_EN));
> > - DRM_DEBUG_KMS("memory self refresh enabled\n");
> > - } else
> > - DRM_DEBUG_KMS("memory self refresh disabled\n");
> > - }
> > + if (enabled)
> > + intel_set_memory_cxsr(dev_priv, true);
> > }
> >
> > static void i845_update_wm(struct drm_crtc *unused_crtc)
> > @@ -6662,7 +6661,7 @@ void intel_init_pm(struct drm_device *dev)
> > (dev_priv->is_ddr3 == 1) ? "3" : "2",
> > dev_priv->fsb_freq, dev_priv->mem_freq);
> > /* Disable CxSR and never update its watermark again */
> > - pineview_disable_cxsr(dev);
> > + intel_set_memory_cxsr(dev_priv, false);
> > dev_priv->display.update_wm = NULL;
> > } else
> > dev_priv->display.update_wm = pineview_update_wm;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
