Re: [Intel-gfx] [PATCH] drm/i915: Don't lock panel registers when downclocking

2012-02-13 Thread Daniel Vetter 
On Mon, Feb 13, 2012 at 10:13:02AM -0500, Sean Paul wrote:
 On Mon, Feb 13, 2012 at 5:08 AM, Daniel Vetter dan...@ffwll.ch wrote:
  On Wed, Feb 01, 2012 at 05:31:30PM -0500, Sean Paul wrote:
  This patch removes the locking from the downclock routines since we are no
  longer locking the registers at all. See ed10fca9 for the original commit
  changing this philosophy.
 
  Signed-off-by: Sean Paul seanp...@chromium.org
 
  I've thought this was due to paranoia because we don't trust our own code
  and because we don't trust the bios to randomly lock this again. Without
  any reasons to the contrary, I'll prefer to keep this.
 
 Thanks for the explanation, Daniel, however I'd ask that you
 reconsider this patch.
 
 The state coming into the downclock functions is unlocked and without
 this patch, the state coming out is locked. This causes at least one
 warning in the code from assert_panel_unlocked.

Ah, that's a pretty important thing missing from the commit message. I've
checked the code and we have indeed an issue there. Can you please:
- Extend your commit message to mention that you're actually hitting the
  assert_panel_unlocked assert (and how this happens). Maybe explain that
  you need lvds downclocking, which is disabled by default.
- Add an assert_panel_unlocked call instead of unlocking the panel in your
  patch (we do need to be paranoid about these things).

Then I think your patch is good to go into -next.

Yours, Daniel
-- 
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Don't lock panel registers when downclocking

2012-02-13 Thread Sean Paul 
This patch replaces the locking from the downclock routines with an assert
to ensure the registers are indeed unlocked. Without this patch, pre-SNB
devices would lock the registers when downclocking which would cause a
WARNING on suspend/resume with downclocking enabled.

Note: To hit this bug, you need to have lvds downclocking enabled.

Signed-off-by: Sean Paul seanp...@chromium.org
---
 drivers/gpu/drm/i915/intel_display.c |   14 ++
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index ebe71ed..33ef4f3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6968,9 +6968,7 @@ static void intel_increase_pllclock(struct drm_crtc *crtc)
if (!HAS_PIPE_CXSR(dev)  (dpll  DISPLAY_RATE_SELECT_FPA1)) {
DRM_DEBUG_DRIVER(upclocking LVDS\n);
 
-   /* Unlock panel regs */
-   I915_WRITE(PP_CONTROL,
-  I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS);
+   assert_panel_unlocked(dev_priv, pipe);
 
dpll = ~DISPLAY_RATE_SELECT_FPA1;
I915_WRITE(dpll_reg, dpll);
@@ -6979,9 +6977,6 @@ static void intel_increase_pllclock(struct drm_crtc *crtc)
dpll = I915_READ(dpll_reg);
if (dpll  DISPLAY_RATE_SELECT_FPA1)
DRM_DEBUG_DRIVER(failed to upclock LVDS!\n);
-
-   /* ...and lock them again */
-   I915_WRITE(PP_CONTROL, I915_READ(PP_CONTROL)  0x3);
}
 
/* Schedule downclock */
@@ -7011,9 +7006,7 @@ static void intel_decrease_pllclock(struct drm_crtc *crtc)
if (!HAS_PIPE_CXSR(dev)  intel_crtc-lowfreq_avail) {
DRM_DEBUG_DRIVER(downclocking LVDS\n);
 
-   /* Unlock panel regs */
-   I915_WRITE(PP_CONTROL, I915_READ(PP_CONTROL) |
-  PANEL_UNLOCK_REGS);
+   assert_panel_unlocked(dev_priv, pipe);
 
dpll |= DISPLAY_RATE_SELECT_FPA1;
I915_WRITE(dpll_reg, dpll);
@@ -7021,9 +7014,6 @@ static void intel_decrease_pllclock(struct drm_crtc *crtc)
dpll = I915_READ(dpll_reg);
if (!(dpll  DISPLAY_RATE_SELECT_FPA1))
DRM_DEBUG_DRIVER(failed to downclock LVDS!\n);
-
-   /* ...and lock them again */
-   I915_WRITE(PP_CONTROL, I915_READ(PP_CONTROL)  0x3);
}
 
 }
-- 
1.7.7.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't lock panel registers when downclocking

2012-02-13 Thread Jesse Barnes 
On Mon, 13 Feb 2012 13:14:51 -0500
Sean Paul seanp...@chromium.org wrote:

 This patch replaces the locking from the downclock routines with an assert
 to ensure the registers are indeed unlocked. Without this patch, pre-SNB
 devices would lock the registers when downclocking which would cause a
 WARNING on suspend/resume with downclocking enabled.
 
 Note: To hit this bug, you need to have lvds downclocking enabled.
 
 Signed-off-by: Sean Paul seanp...@chromium.org
 ---
  drivers/gpu/drm/i915/intel_display.c |   14 ++
  1 files changed, 2 insertions(+), 12 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index ebe71ed..33ef4f3 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -6968,9 +6968,7 @@ static void intel_increase_pllclock(struct drm_crtc 
 *crtc)
   if (!HAS_PIPE_CXSR(dev)  (dpll  DISPLAY_RATE_SELECT_FPA1)) {
   DRM_DEBUG_DRIVER(upclocking LVDS\n);
  
 - /* Unlock panel regs */
 - I915_WRITE(PP_CONTROL,
 -I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS);
 + assert_panel_unlocked(dev_priv, pipe);
  
   dpll = ~DISPLAY_RATE_SELECT_FPA1;
   I915_WRITE(dpll_reg, dpll);
 @@ -6979,9 +6977,6 @@ static void intel_increase_pllclock(struct drm_crtc 
 *crtc)
   dpll = I915_READ(dpll_reg);
   if (dpll  DISPLAY_RATE_SELECT_FPA1)
   DRM_DEBUG_DRIVER(failed to upclock LVDS!\n);
 -
 - /* ...and lock them again */
 - I915_WRITE(PP_CONTROL, I915_READ(PP_CONTROL)  0x3);
   }
  
   /* Schedule downclock */
 @@ -7011,9 +7006,7 @@ static void intel_decrease_pllclock(struct drm_crtc 
 *crtc)
   if (!HAS_PIPE_CXSR(dev)  intel_crtc-lowfreq_avail) {
   DRM_DEBUG_DRIVER(downclocking LVDS\n);
  
 - /* Unlock panel regs */
 - I915_WRITE(PP_CONTROL, I915_READ(PP_CONTROL) |
 -PANEL_UNLOCK_REGS);
 + assert_panel_unlocked(dev_priv, pipe);
  
   dpll |= DISPLAY_RATE_SELECT_FPA1;
   I915_WRITE(dpll_reg, dpll);
 @@ -7021,9 +7014,6 @@ static void intel_decrease_pllclock(struct drm_crtc 
 *crtc)
   dpll = I915_READ(dpll_reg);
   if (!(dpll  DISPLAY_RATE_SELECT_FPA1))
   DRM_DEBUG_DRIVER(failed to downclock LVDS!\n);
 -
 - /* ...and lock them again */
 - I915_WRITE(PP_CONTROL, I915_READ(PP_CONTROL)  0x3);
   }
  
  }

Yeah, looks good.

Acked-by: Jesse Barnes jbar...@virtuousgeek.org

-- 
Jesse Barnes, Intel Open Source Technology Center


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx