* [PATCH] drm/i915: Don't lock panel registers when downclocking
@ 2012-02-01 22:31 Sean Paul
2012-02-13 10:08 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Sean Paul @ 2012-02-01 22:31 UTC (permalink / raw)
To: intel-gfx; +Cc: jglasgow, seanpaul
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 <seanpaul@chromium.org>
---
drivers/gpu/drm/i915/intel_display.c | 14 --------------
1 files changed, 0 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ebe71ed..fe4787a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6968,10 +6968,6 @@ 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);
-
dpll &= ~DISPLAY_RATE_SELECT_FPA1;
I915_WRITE(dpll_reg, dpll);
intel_wait_for_vblank(dev, pipe);
@@ -6979,9 +6975,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,19 +7004,12 @@ 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);
-
dpll |= DISPLAY_RATE_SELECT_FPA1;
I915_WRITE(dpll_reg, dpll);
intel_wait_for_vblank(dev, pipe);
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't lock panel registers when downclocking
2012-02-01 22:31 [PATCH] drm/i915: Don't lock panel registers when downclocking Sean Paul
@ 2012-02-13 10:08 ` Daniel Vetter
2012-02-13 15:13 ` Sean Paul
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2012-02-13 10:08 UTC (permalink / raw)
To: Sean Paul; +Cc: intel-gfx, jglasgow, seanpaul
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 <seanpaul@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.
Yours, Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't lock panel registers when downclocking
2012-02-13 10:08 ` Daniel Vetter
@ 2012-02-13 15:13 ` Sean Paul
2012-02-13 16:39 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Sean Paul @ 2012-02-13 15:13 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, jglasgow
On Mon, Feb 13, 2012 at 5:08 AM, Daniel Vetter <daniel@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 <seanpaul@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.
Sean
>
> Yours, Daniel
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't lock panel registers when downclocking
2012-02-13 15:13 ` Sean Paul
@ 2012-02-13 16:39 ` Daniel Vetter
2012-02-13 18:14 ` Sean Paul
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2012-02-13 16:39 UTC (permalink / raw)
To: Sean Paul; +Cc: intel-gfx, jglasgow
On Mon, Feb 13, 2012 at 10:13:02AM -0500, Sean Paul wrote:
> On Mon, Feb 13, 2012 at 5:08 AM, Daniel Vetter <daniel@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 <seanpaul@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: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] drm/i915: Don't lock panel registers when downclocking
2012-02-13 16:39 ` Daniel Vetter
@ 2012-02-13 18:14 ` Sean Paul
2012-02-13 19:02 ` Jesse Barnes
0 siblings, 1 reply; 6+ messages in thread
From: Sean Paul @ 2012-02-13 18:14 UTC (permalink / raw)
To: intel-gfx; +Cc: jglasgow
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 <seanpaul@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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't lock panel registers when downclocking
2012-02-13 18:14 ` Sean Paul
@ 2012-02-13 19:02 ` Jesse Barnes
0 siblings, 0 replies; 6+ messages in thread
From: Jesse Barnes @ 2012-02-13 19:02 UTC (permalink / raw)
To: Sean Paul; +Cc: intel-gfx, jglasgow
[-- Attachment #1.1: Type: text/plain, Size: 2539 bytes --]
On Mon, 13 Feb 2012 13:14:51 -0500
Sean Paul <seanpaul@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 <seanpaul@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 <jbarnes@virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-02-13 19:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-01 22:31 [PATCH] drm/i915: Don't lock panel registers when downclocking Sean Paul
2012-02-13 10:08 ` Daniel Vetter
2012-02-13 15:13 ` Sean Paul
2012-02-13 16:39 ` Daniel Vetter
2012-02-13 18:14 ` Sean Paul
2012-02-13 19:02 ` Jesse Barnes
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.