* [PATCH] drm/i915: Don't write DSPSURF for old chips
@ 2012-02-27 11:39 Takashi Iwai
2012-02-27 12:17 ` Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2012-02-27 11:39 UTC (permalink / raw)
To: intel-gfx
It seems that writing DSPSURF in intel_flush_display_plane() causes
the blank screen on some old laptops like Dell D630 with 965GM.
Since this operation is needed only for ILK+, make it conditional.
Cc: <stable@kernel.org> [v3.1+]
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
drivers/gpu/drm/i915/intel_display.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1379,7 +1379,8 @@ static void intel_flush_display_plane(st
enum plane plane)
{
I915_WRITE(DSPADDR(plane), I915_READ(DSPADDR(plane)));
- I915_WRITE(DSPSURF(plane), I915_READ(DSPSURF(plane)));
+ if (dev_priv->info->gen >= 5)
+ I915_WRITE(DSPSURF(plane), I915_READ(DSPSURF(plane)));
}
/**
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915: Don't write DSPSURF for old chips 2012-02-27 11:39 [PATCH] drm/i915: Don't write DSPSURF for old chips Takashi Iwai @ 2012-02-27 12:17 ` Chris Wilson 2012-02-27 13:08 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Chris Wilson @ 2012-02-27 12:17 UTC (permalink / raw) To: Takashi Iwai, intel-gfx On Mon, 27 Feb 2012 12:39:24 +0100, Takashi Iwai <tiwai@suse.de> wrote: > It seems that writing DSPSURF in intel_flush_display_plane() causes > the blank screen on some old laptops like Dell D630 with 965GM. > Since this operation is needed only for ILK+, make it conditional. The specs say that DSPASURF is the latch register for updates of the DSPA registers on gen4 (including 965gm) as well. Presumably the bug is that we only partially update the DSPA registers prior to the first call to intel_flush_display_plane() which this papers over by disabling the update until a valid address is written to DSPASURF. And there is such a spurious call to intel_enable_plane() prior to us setting a valid scanout surface: diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_d index 66b19d3..ea8e4a1 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5453,7 +5453,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, I915_WRITE(DSPCNTR(plane), dspcntr); POSTING_READ(DSPCNTR(plane)); - intel_enable_plane(dev_priv, plane, pipe); ret = intel_pipe_set_base(crtc, x, y, old_fb); -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't write DSPSURF for old chips 2012-02-27 12:17 ` Chris Wilson @ 2012-02-27 13:08 ` Takashi Iwai 2012-02-28 6:43 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2012-02-27 13:08 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx At Mon, 27 Feb 2012 12:17:57 +0000, Chris Wilson wrote: > > On Mon, 27 Feb 2012 12:39:24 +0100, Takashi Iwai <tiwai@suse.de> wrote: > > It seems that writing DSPSURF in intel_flush_display_plane() causes > > the blank screen on some old laptops like Dell D630 with 965GM. > > Since this operation is needed only for ILK+, make it conditional. > > The specs say that DSPASURF is the latch register for updates of the DSPA > registers on gen4 (including 965gm) as well. Presumably the bug is that > we only partially update the DSPA registers prior to the first call to > intel_flush_display_plane() which this papers over by disabling the > update until a valid address is written to DSPASURF. And there is such a > spurious call to intel_enable_plane() prior to us setting a valid > scanout surface: Sounds reasonable. FWIW, the change was first introduced in commit [b24e7179: drm/i915: add pipe/plane enable/disable functions], then in commit [efc2924e: drm/i915: Call intel_enable_plane from i9xx_crtc_mode_set (again)], it's placed into i9xx_crtc_mode_set(). This explains the fact that it was discovered only on old machines as i9xx_crtc_mode_set() is the only crtc_mode_set op calling intel_enable_plane(). BTW, the bisection leaded to a merge commit, so the bug is really depending on the activation path or timing. I'll ask a tester to try your patch. thanks, Takashi > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_d > index 66b19d3..ea8e4a1 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5453,7 +5453,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc > *crtc, > > I915_WRITE(DSPCNTR(plane), dspcntr); > POSTING_READ(DSPCNTR(plane)); > - intel_enable_plane(dev_priv, plane, pipe); > > ret = intel_pipe_set_base(crtc, x, y, old_fb); > > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't write DSPSURF for old chips 2012-02-27 13:08 ` Takashi Iwai @ 2012-02-28 6:43 ` Takashi Iwai 2012-02-29 23:54 ` Chris Wilson 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2012-02-28 6:43 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx At Mon, 27 Feb 2012 14:08:28 +0100, Takashi Iwai wrote: > > At Mon, 27 Feb 2012 12:17:57 +0000, > Chris Wilson wrote: > > > > On Mon, 27 Feb 2012 12:39:24 +0100, Takashi Iwai <tiwai@suse.de> wrote: > > > It seems that writing DSPSURF in intel_flush_display_plane() causes > > > the blank screen on some old laptops like Dell D630 with 965GM. > > > Since this operation is needed only for ILK+, make it conditional. > > > > The specs say that DSPASURF is the latch register for updates of the DSPA > > registers on gen4 (including 965gm) as well. Presumably the bug is that > > we only partially update the DSPA registers prior to the first call to > > intel_flush_display_plane() which this papers over by disabling the > > update until a valid address is written to DSPASURF. And there is such a > > spurious call to intel_enable_plane() prior to us setting a valid > > scanout surface: > > Sounds reasonable. FWIW, the change was first introduced in commit > [b24e7179: drm/i915: add pipe/plane enable/disable functions], > then in commit [efc2924e: drm/i915: Call intel_enable_plane from > i9xx_crtc_mode_set (again)], it's placed into i9xx_crtc_mode_set(). > > This explains the fact that it was discovered only on old machines > as i9xx_crtc_mode_set() is the only crtc_mode_set op calling > intel_enable_plane(). > > BTW, the bisection leaded to a merge commit, so the bug is really > depending on the activation path or timing. > > I'll ask a tester to try your patch. He reported back that it reduces the failure rate but doesn't fix completely. Still get a blank screen in 20% rate. Any other clue? thanks, Takashi > > > thanks, > > Takashi > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_d > > index 66b19d3..ea8e4a1 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5453,7 +5453,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc > > *crtc, > > > > I915_WRITE(DSPCNTR(plane), dspcntr); > > POSTING_READ(DSPCNTR(plane)); > > - intel_enable_plane(dev_priv, plane, pipe); > > > > ret = intel_pipe_set_base(crtc, x, y, old_fb); > > > > -Chris > > > > -- > > Chris Wilson, Intel Open Source Technology Centre > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't write DSPSURF for old chips 2012-02-28 6:43 ` Takashi Iwai @ 2012-02-29 23:54 ` Chris Wilson 2012-03-01 6:37 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Chris Wilson @ 2012-02-29 23:54 UTC (permalink / raw) To: Takashi Iwai; +Cc: intel-gfx On Tue, 28 Feb 2012 07:43:55 +0100, Takashi Iwai <tiwai@suse.de> wrote: > At Mon, 27 Feb 2012 14:08:28 +0100, > Takashi Iwai wrote: > > > > At Mon, 27 Feb 2012 12:17:57 +0000, > > Chris Wilson wrote: > > > > > > On Mon, 27 Feb 2012 12:39:24 +0100, Takashi Iwai <tiwai@suse.de> wrote: > > > > It seems that writing DSPSURF in intel_flush_display_plane() causes > > > > the blank screen on some old laptops like Dell D630 with 965GM. > > > > Since this operation is needed only for ILK+, make it conditional. > > > > > > The specs say that DSPASURF is the latch register for updates of the DSPA > > > registers on gen4 (including 965gm) as well. Presumably the bug is that > > > we only partially update the DSPA registers prior to the first call to > > > intel_flush_display_plane() which this papers over by disabling the > > > update until a valid address is written to DSPASURF. And there is such a > > > spurious call to intel_enable_plane() prior to us setting a valid > > > scanout surface: > > > > Sounds reasonable. FWIW, the change was first introduced in commit > > [b24e7179: drm/i915: add pipe/plane enable/disable functions], > > then in commit [efc2924e: drm/i915: Call intel_enable_plane from > > i9xx_crtc_mode_set (again)], it's placed into i9xx_crtc_mode_set(). > > > > This explains the fact that it was discovered only on old machines > > as i9xx_crtc_mode_set() is the only crtc_mode_set op calling > > intel_enable_plane(). > > > > BTW, the bisection leaded to a merge commit, so the bug is really > > depending on the activation path or timing. > > > > I'll ask a tester to try your patch. > > He reported back that it reduces the failure rate but doesn't fix > completely. Still get a blank screen in 20% rate. > > Any other clue? I haven't yet found anything else in the same vein as this, but the 965gm does ring a warning bell for this recent regression: commit 5ca0c34ae28344b6b4ca3036bc82f89c8db16a59 Author: Dave Airlie <airlied@redhat.com> Date: Thu Feb 23 15:33:40 2012 +0000 drm/i915: fix mode set on load pipe. (v2) Booted my i965 machine and it started printing the unsupported pixel format of 0 message (once I added content to it). Oh looksie here, we pass 0. fix. v2: compile it. Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45966 Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Dave Airlie <airlied@redhat.com> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> which is currently sitting in airlied/drm-fixes. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't write DSPSURF for old chips 2012-02-29 23:54 ` Chris Wilson @ 2012-03-01 6:37 ` Takashi Iwai 0 siblings, 0 replies; 6+ messages in thread From: Takashi Iwai @ 2012-03-01 6:37 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx At Wed, 29 Feb 2012 23:54:46 +0000, Chris Wilson wrote: > > On Tue, 28 Feb 2012 07:43:55 +0100, Takashi Iwai <tiwai@suse.de> wrote: > > At Mon, 27 Feb 2012 14:08:28 +0100, > > Takashi Iwai wrote: > > > > > > At Mon, 27 Feb 2012 12:17:57 +0000, > > > Chris Wilson wrote: > > > > > > > > On Mon, 27 Feb 2012 12:39:24 +0100, Takashi Iwai <tiwai@suse.de> wrote: > > > > > It seems that writing DSPSURF in intel_flush_display_plane() causes > > > > > the blank screen on some old laptops like Dell D630 with 965GM. > > > > > Since this operation is needed only for ILK+, make it conditional. > > > > > > > > The specs say that DSPASURF is the latch register for updates of the DSPA > > > > registers on gen4 (including 965gm) as well. Presumably the bug is that > > > > we only partially update the DSPA registers prior to the first call to > > > > intel_flush_display_plane() which this papers over by disabling the > > > > update until a valid address is written to DSPASURF. And there is such a > > > > spurious call to intel_enable_plane() prior to us setting a valid > > > > scanout surface: > > > > > > Sounds reasonable. FWIW, the change was first introduced in commit > > > [b24e7179: drm/i915: add pipe/plane enable/disable functions], > > > then in commit [efc2924e: drm/i915: Call intel_enable_plane from > > > i9xx_crtc_mode_set (again)], it's placed into i9xx_crtc_mode_set(). > > > > > > This explains the fact that it was discovered only on old machines > > > as i9xx_crtc_mode_set() is the only crtc_mode_set op calling > > > intel_enable_plane(). > > > > > > BTW, the bisection leaded to a merge commit, so the bug is really > > > depending on the activation path or timing. > > > > > > I'll ask a tester to try your patch. > > > > He reported back that it reduces the failure rate but doesn't fix > > completely. Still get a blank screen in 20% rate. > > > > Any other clue? > > I haven't yet found anything else in the same vein as this, but the > 965gm does ring a warning bell for this recent regression: > > commit 5ca0c34ae28344b6b4ca3036bc82f89c8db16a59 > Author: Dave Airlie <airlied@redhat.com> > Date: Thu Feb 23 15:33:40 2012 +0000 > > drm/i915: fix mode set on load pipe. (v2) > > Booted my i965 machine and it started printing the unsupported pixel > format of 0 message (once I added content to it). > > Oh looksie here, we pass 0. fix. > > v2: compile it. > > Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45966 > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Dave Airlie <airlied@redhat.com> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > which is currently sitting in airlied/drm-fixes. Hm, this must be irrelevant (inapplicable) because the tests were done with 3.1, 3.2 (and SLE11-SP2 kernels which has backports from 3.3-rc2 but without these fixes). The bug seems to have been introduced between 3.0 and 3.1. thanks, Takashi ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-01 6:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-02-27 11:39 [PATCH] drm/i915: Don't write DSPSURF for old chips Takashi Iwai 2012-02-27 12:17 ` Chris Wilson 2012-02-27 13:08 ` Takashi Iwai 2012-02-28 6:43 ` Takashi Iwai 2012-02-29 23:54 ` Chris Wilson 2012-03-01 6:37 ` Takashi Iwai
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.