| Message ID | 1317638779.1997.18.camel@jmp |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 03, 2011 at 11:46:19AM +0100, Jose Plans wrote: > SRU Justification: > > Impact: > Every Lenovo x420s with Samsung panels will present a corrupted > display when booting / mode setting. The systems are not usable. > > Fixes: > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/812638 > > Fix: > Backported patch commit from upstream: > b24e71798871089da1a4ab049db2800afc1aac0c drm/i915: add pipe/plane > enable/disable functions by Jesse Barnes. > > Test case: > The reproducer is to simply boot the system with any kernels prior to > Oneiric and using Lenovo x420 and it's Intel graphical chipset. > > Hardware tested was: > - Lenovo x301 and on Lenovo x420s with Samsung panel. > > Note: > This a very intrusive patch, I have spent some time studying it > and I came to the conclusion it would be safer and easier to maintain if > this patch was include as-is rather than providing a forked patch. I > would like the list opinion on this. Well its a vast and epic patch. I am left wondering whether this is a backport or a cherry-pick in this context, below its reported as a backport which implies it was modified, but you say here "as-is" implying the opposite. > There are three major changes: > (1) Addition of functions allowing to cleanly add or remove pipes / > planes. > (2) Addition of assertion functions to alert help prevent scenarios > where pipes or planes are not present and they should be. For example a > plane is being activated but there was no pipe active to contain it. > (3) During crtc mode setting, now the pipe and planes are allocated > later in the process conforming to intel specifications. Seen at the > changes made in: intel_crtc_mode_set() only affecting ironlake > +/sandybridge. > > (3) is the real fix for this panel issue, however (1) is necessary for > it as well as (2) helps preventing further regressions. > > drivers/gpu/drm/i915/i915_reg.h | 5 +- > drivers/gpu/drm/i915/intel_display.c | 308 > +++++++++++++++++++++++---------- > 2 files changed, 217 insertions(+), 96 deletions(-) > > Jose > > From: Jesse Barnes <jbarnes@virtuousgeek.org> > Date: Tue, 4 Jan 2011 15:09:30 -0800 > Subject: [PATCH] drm/i915: add pipe/plane enable/disable functions > > Add plane enable/disable functions to prevent duplicated code and allow > us to easily check for plane enable/disable requirements (such as pipe > enable, plane status, pll status etc). It is clear that this description is inadequate if we are actually changing pipe init order. (Not that that is anything you should change as it _is_ the upstream commit description.) Bad Jesse. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > (Backport to 2.6.38.y) BugLink: http://bugs.launchpad.net/bugs/812638 (backported from commit b24e71798871089da1a4ab049db2800afc1aac0c) "backported from" if its modified, "cherry-picked from" if its not ... ? > Signed-off-by: Robert Hooker <robert.hooker@canonical.com> > Signed-off-by: Jose Plans <jose.plans@canonical.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 5 +- > drivers/gpu/drm/i915/intel_display.c | 308 +++++++++++++++++++++++---------- > 2 files changed, 217 insertions(+), 96 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 12c547a..d59df08 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -2537,9 +2537,10 @@ > #define DISPPLANE_32BPP_30BIT_NO_ALPHA (0xa<<26) > #define DISPPLANE_STEREO_ENABLE (1<<25) > #define DISPPLANE_STEREO_DISABLE 0 > -#define DISPPLANE_SEL_PIPE_MASK (1<<24) > +#define DISPPLANE_SEL_PIPE_SHIFT 24 > +#define DISPPLANE_SEL_PIPE_MASK (3<<DISPPLANE_SEL_PIPE_SHIFT) > #define DISPPLANE_SEL_PIPE_A 0 > -#define DISPPLANE_SEL_PIPE_B (1<<24) > +#define DISPPLANE_SEL_PIPE_B (1<<DISPPLANE_SEL_PIPE_SHIFT) > #define DISPPLANE_SRC_KEY_ENABLE (1<<22) > #define DISPPLANE_SRC_KEY_DISABLE 0 > #define DISPPLANE_LINE_DOUBLE (1<<20) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 841558b..dc51881 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1058,6 +1058,203 @@ void intel_wait_for_pipe_off(struct drm_device *dev, int pipe) > } > } > > +static const char *state_string(bool enabled) > +{ > + return enabled ? "on" : "off"; > +} > + > +/* Only for pre-ILK configs */ > +static void assert_pll(struct drm_i915_private *dev_priv, > + enum pipe pipe, bool state) > +{ > + int reg; > + u32 val; > + bool cur_state; > + > + reg = DPLL(pipe); > + val = I915_READ(reg); > + cur_state = !!(val & DPLL_VCO_ENABLE); > + WARN(cur_state != state, > + "PLL state assertion failure (expected %s, current %s)\n", > + state_string(state), state_string(cur_state)); > +} > +#define assert_pll_enabled(d, p) assert_pll(d, p, true) > +#define assert_pll_disabled(d, p) assert_pll(d, p, false) > + > +static void assert_pipe_enabled(struct drm_i915_private *dev_priv, > + enum pipe pipe) > +{ > + int reg; > + u32 val; > + > + reg = PIPECONF(pipe); > + val = I915_READ(reg); > + WARN(!(val & PIPECONF_ENABLE), > + "pipe %c assertion failure, should be active but is disabled\n", > + pipe ? 'B' : 'A'); > +} > + > +static void assert_plane_enabled(struct drm_i915_private *dev_priv, > + enum plane plane) > +{ > + int reg; > + u32 val; > + > + reg = DSPCNTR(plane); > + val = I915_READ(reg); > + WARN(!(val & DISPLAY_PLANE_ENABLE), > + "plane %c assertion failure, should be active but is disabled\n", > + plane ? 'B' : 'A'); > +} > + > +static void assert_planes_disabled(struct drm_i915_private *dev_priv, > + enum pipe pipe) > +{ > + int reg, i; > + u32 val; > + int cur_pipe; > + > + /* Need to check both planes against the pipe */ > + for (i = 0; i < 2; i++) { > + reg = DSPCNTR(i); > + val = I915_READ(reg); > + cur_pipe = (val & DISPPLANE_SEL_PIPE_MASK) >> > + DISPPLANE_SEL_PIPE_SHIFT; > + WARN((val & DISPLAY_PLANE_ENABLE) && pipe == cur_pipe, > + "plane %d assertion failure, should be off on pipe %c but is still active\n", > + i, pipe ? 'B' : 'A'); > + } > +} > + > +/** > + * intel_enable_pipe - enable a pipe, assertiing requirements > + * @dev_priv: i915 private structure > + * @pipe: pipe to enable > + * > + * Enable @pipe, making sure that various hardware specific requirements > + * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc. > + * > + * @pipe should be %PIPE_A or %PIPE_B. > + * > + * Will wait until the pipe is actually running (i.e. first vblank) before > + * returning. > + */ > +static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe) > +{ > + int reg; > + u32 val; > + > + /* > + * A pipe without a PLL won't actually be able to drive bits from > + * a plane. On ILK+ the pipe PLLs are integrated, so we don't > + * need the check. > + */ > + if (!HAS_PCH_SPLIT(dev_priv->dev)) > + assert_pll_enabled(dev_priv, pipe); > + > + reg = PIPECONF(pipe); > + val = I915_READ(reg); > + val |= PIPECONF_ENABLE; > + I915_WRITE(reg, val); > + POSTING_READ(reg); > + intel_wait_for_vblank(dev_priv->dev, pipe); > +} > + > +/** > + * intel_disable_pipe - disable a pipe, assertiing requirements > + * @dev_priv: i915 private structure > + * @pipe: pipe to disable > + * > + * Disable @pipe, making sure that various hardware specific requirements > + * are met, if applicable, e.g. plane disabled, panel fitter off, etc. > + * > + * @pipe should be %PIPE_A or %PIPE_B. > + * > + * Will wait until the pipe has shut down before returning. > + */ > +static void intel_disable_pipe(struct drm_i915_private *dev_priv, > + enum pipe pipe) > +{ > + int reg; > + u32 val; > + > + /* > + * Make sure planes won't keep trying to pump pixels to us, > + * or we might hang the display. > + */ > + assert_planes_disabled(dev_priv, pipe); > + > + /* Don't disable pipe A or pipe A PLLs if needed */ > + if (pipe == PIPE_A && (dev_priv->quirks & QUIRK_PIPEA_FORCE)) > + return; > + > + reg = PIPECONF(pipe); > + val = I915_READ(reg); > + val &= ~PIPECONF_ENABLE; > + I915_WRITE(reg, val); > + POSTING_READ(reg); > + intel_wait_for_pipe_off(dev_priv->dev, pipe); > +} > + > +/** > + * intel_enable_plane - enable a display plane on a given pipe > + * @dev_priv: i915 private structure > + * @plane: plane to enable > + * @pipe: pipe being fed > + * > + * Enable @plane on @pipe, making sure that @pipe is running first. > + */ > +static void intel_enable_plane(struct drm_i915_private *dev_priv, > + enum plane plane, enum pipe pipe) > +{ > + int reg; > + u32 val; > + > + /* If the pipe isn't enabled, we can't pump pixels and may hang */ > + assert_pipe_enabled(dev_priv, pipe); > + > + reg = DSPCNTR(plane); > + val = I915_READ(reg); > + val |= DISPLAY_PLANE_ENABLE; > + I915_WRITE(reg, val); > + POSTING_READ(reg); > + intel_wait_for_vblank(dev_priv->dev, pipe); > +} > + > +/* > + * Plane regs are double buffered, going from enabled->disabled needs a > + * trigger in order to latch. The display address reg provides this. > + */ > +static void intel_flush_display_plane(struct drm_i915_private *dev_priv, > + enum plane plane) > +{ > + u32 reg = DSPADDR(plane); > + I915_WRITE(reg, I915_READ(reg)); > +} > + > +/** > + * intel_disable_plane - disable a display plane > + * @dev_priv: i915 private structure > + * @plane: plane to disable > + * @pipe: pipe consuming the data > + * > + * Disable @plane; should be an independent operation. > + */ > +static void intel_disable_plane(struct drm_i915_private *dev_priv, > + enum plane plane, enum pipe pipe) > +{ > + int reg; > + u32 val; > + > + reg = DSPCNTR(plane); > + val = I915_READ(reg); > + val &= ~DISPLAY_PLANE_ENABLE; > + I915_WRITE(reg, val); > + POSTING_READ(reg); > + intel_flush_display_plane(dev_priv, plane); > + intel_wait_for_vblank(dev_priv->dev, pipe); > +} > + > static void i8xx_enable_fbc(struct drm_crtc *crtc, unsigned long interval) > { > struct drm_device *dev = crtc->dev; > @@ -2003,14 +2200,6 @@ static void ironlake_fdi_enable(struct drm_crtc *crtc) > } > } > > -static void intel_flush_display_plane(struct drm_device *dev, > - int plane) > -{ > - struct drm_i915_private *dev_priv = dev->dev_private; > - u32 reg = DSPADDR(plane); > - I915_WRITE(reg, I915_READ(reg)); > -} > - > /* > * When we disable a pipe, we need to clear any pending scanline wait events > * to avoid hanging the ring, which we assume we are waiting on. > @@ -2158,22 +2347,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > dev_priv->pch_pf_size); > } > > - /* Enable CPU pipe */ > - reg = PIPECONF(pipe); > - temp = I915_READ(reg); > - if ((temp & PIPECONF_ENABLE) == 0) { > - I915_WRITE(reg, temp | PIPECONF_ENABLE); > - POSTING_READ(reg); > - intel_wait_for_vblank(dev, intel_crtc->pipe); > - } > - > - /* configure and enable CPU plane */ > - reg = DSPCNTR(plane); > - temp = I915_READ(reg); > - if ((temp & DISPLAY_PLANE_ENABLE) == 0) { > - I915_WRITE(reg, temp | DISPLAY_PLANE_ENABLE); > - intel_flush_display_plane(dev, plane); > - } > + intel_enable_pipe(dev_priv, pipe); > + intel_enable_plane(dev_priv, plane, pipe); > > /* Skip the PCH stuff if possible */ > if (!is_pch_port) > @@ -2285,27 +2460,13 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > drm_vblank_off(dev, pipe); > intel_crtc_update_cursor(crtc, false); > > - /* Disable display plane */ > - reg = DSPCNTR(plane); > - temp = I915_READ(reg); > - if (temp & DISPLAY_PLANE_ENABLE) { > - I915_WRITE(reg, temp & ~DISPLAY_PLANE_ENABLE); > - intel_flush_display_plane(dev, plane); > - } > + intel_disable_plane(dev_priv, plane, pipe); > > if (dev_priv->cfb_plane == plane && > dev_priv->display.disable_fbc) > dev_priv->display.disable_fbc(dev); > > - /* disable cpu pipe, disable after all planes disabled */ > - reg = PIPECONF(pipe); > - temp = I915_READ(reg); > - if (temp & PIPECONF_ENABLE) { > - I915_WRITE(reg, temp & ~PIPECONF_ENABLE); > - POSTING_READ(reg); > - /* wait for cpu pipe off, pipe state */ > - intel_wait_for_pipe_off(dev, intel_crtc->pipe); > - } > + intel_disable_pipe(dev_priv, pipe); > > /* Disable PF */ > I915_WRITE(pipe ? PFB_CTL_1 : PFA_CTL_1, 0); > @@ -2500,19 +2661,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) > udelay(150); > } > > - /* Enable the pipe */ > - reg = PIPECONF(pipe); > - temp = I915_READ(reg); > - if ((temp & PIPECONF_ENABLE) == 0) > - I915_WRITE(reg, temp | PIPECONF_ENABLE); > - > - /* Enable the plane */ > - reg = DSPCNTR(plane); > - temp = I915_READ(reg); > - if ((temp & DISPLAY_PLANE_ENABLE) == 0) { > - I915_WRITE(reg, temp | DISPLAY_PLANE_ENABLE); > - intel_flush_display_plane(dev, plane); > - } > + intel_enable_pipe(dev_priv, pipe); > + intel_enable_plane(dev_priv, plane, pipe); > > intel_crtc_load_lut(crtc); > intel_update_fbc(dev); > @@ -2544,33 +2694,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > dev_priv->display.disable_fbc) > dev_priv->display.disable_fbc(dev); > > - /* Disable display plane */ > - reg = DSPCNTR(plane); > - temp = I915_READ(reg); > - if (temp & DISPLAY_PLANE_ENABLE) { > - I915_WRITE(reg, temp & ~DISPLAY_PLANE_ENABLE); > - /* Flush the plane changes */ > - intel_flush_display_plane(dev, plane); > - > - /* Wait for vblank for the disable to take effect */ > - if (IS_GEN2(dev)) > - intel_wait_for_vblank(dev, pipe); > - } > + intel_disable_plane(dev_priv, plane, pipe); > > /* Don't disable pipe A or pipe A PLLs if needed */ > if (pipe == 0 && (dev_priv->quirks & QUIRK_PIPEA_FORCE)) > goto done; > > - /* Next, disable display pipes */ > - reg = PIPECONF(pipe); > - temp = I915_READ(reg); > - if (temp & PIPECONF_ENABLE) { > - I915_WRITE(reg, temp & ~PIPECONF_ENABLE); > - > - /* Wait for the pipe to turn off */ > - POSTING_READ(reg); > - intel_wait_for_pipe_off(dev, pipe); > - } > + intel_disable_pipe(dev_priv, pipe); > > reg = DPLL(pipe); > temp = I915_READ(reg); > @@ -4322,9 +4452,8 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, > pipeconf &= ~PIPECONF_DOUBLE_WIDE; > } > > - dspcntr |= DISPLAY_PLANE_ENABLE; > - pipeconf |= PIPECONF_ENABLE; > - dpll |= DPLL_VCO_ENABLE; > + if (!HAS_PCH_SPLIT(dev)) > + dpll |= DPLL_VCO_ENABLE; I am struggling to understand whether just the following would work just as well without the other changes? That seems to be the main thrust of the change here. Did we try something like that as well? - dspcntr |= DISPLAY_PLANE_ENABLE; - pipeconf |= PIPECONF_ENABLE; - dpll |= DPLL_VCO_ENABLE; + if (!HAS_PCH_SPLIT(dev)) { + dspcntr |= DISPLAY_PLANE_ENABLE; + pipeconf |= PIPECONF_ENABLE; + dpll |= DPLL_VCO_ENABLE; + } The worry I do have about this change is that the changes below actually considerably change initialisation for all other display types, as we now write the configuration without the enable, add the enable bit after, _and_ then wait for vblank which currently we do not do. Has this been tested on anything other than the affected hardware, and more importantly something for which the below is false: #define HAS_PCH_SPLIT(dev) (IS_GEN5(dev) || IS_GEN6(dev) || IS_IVYBRIDGE(dev)) > > DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe == 0 ? 'A' : 'B'); > drm_mode_debug_printmodeline(mode); > @@ -4533,6 +4662,8 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, > > I915_WRITE(PIPECONF(pipe), pipeconf); > POSTING_READ(PIPECONF(pipe)); > + if (!HAS_PCH_SPLIT(dev)) > + intel_enable_pipe(dev_priv, pipe); > > intel_wait_for_vblank(dev, pipe); > > @@ -4543,6 +4674,9 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, > } > > I915_WRITE(DSPCNTR(plane), dspcntr); > + POSTING_READ(DSPCNTR(plane)); > + if (!HAS_PCH_SPLIT(dev)) > + intel_enable_plane(dev_priv, plane, pipe); > > ret = intel_pipe_set_base(crtc, x, y, old_fb); > > @@ -5662,22 +5796,8 @@ static void intel_sanitize_modesetting(struct drm_device *dev, > pipe = !pipe; > > /* Disable the plane and wait for it to stop reading from the pipe. */ > - I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE); > - intel_flush_display_plane(dev, plane); > - > - if (IS_GEN2(dev)) > - intel_wait_for_vblank(dev, pipe); > - > - if (pipe == 0 && (dev_priv->quirks & QUIRK_PIPEA_FORCE)) > - return; > - > - /* Switch off the pipe. */ > - reg = PIPECONF(pipe); > - val = I915_READ(reg); > - if (val & PIPECONF_ENABLE) { > - I915_WRITE(reg, val & ~PIPECONF_ENABLE); > - intel_wait_for_pipe_off(dev, pipe); > - } > + intel_disable_plane(dev_priv, plane, pipe); > + intel_disable_pipe(dev_priv, pipe); > } > > static void intel_crtc_reset(struct drm_crtc *crtc) -apw
On Mon, Oct 03, 2011 at 01:19:55PM +0100, Andy Whitcroft wrote: > Well its a vast and epic patch. I am left wondering whether this is > a backport or a cherry-pick in this context, below its reported as a > backport which implies it was modified, but you say here "as-is" implying > the opposite. [...] > > Subject: [PATCH] drm/i915: add pipe/plane enable/disable functions > > > > Add plane enable/disable functions to prevent duplicated code and allow > > us to easily check for plane enable/disable requirements (such as pipe > > enable, plane status, pll status etc). > > It is clear that this description is inadequate if we are actually > changing pipe init order. (Not that that is anything you should change > as it _is_ the upstream commit description.) Bad Jesse. > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > (Backport to 2.6.38.y) > > BugLink: http://bugs.launchpad.net/bugs/812638 > (backported from commit b24e71798871089da1a4ab049db2800afc1aac0c) > > "backported from" if its modified, "cherry-picked from" if its not ... ? Ok, having talked to Jose a bit on IRC I am now able to answer my own question. Actually this patch is the merge of _two_ upstream commits only retaining one of the descriptions. It is a merge of the two commits below: commit 65993d64a31844ad444694efb2d159eb9c883e49 Author: Jesse Barnes <jbarnes@virtuousgeek.org> Date: Tue Jan 4 15:09:29 2011 -0800 drm/i915: don't enable plane, pipe and PLL prematurely On Ironlake+ we need to enable these in a specific order. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> and: commit b24e71798871089da1a4ab049db2800afc1aac0c Author: Jesse Barnes <jbarnes@virtuousgeek.org> Date: Tue Jan 4 15:09:30 2011 -0800 drm/i915: add pipe/plane enable/disable functions Add plane enable/disable functions to prevent duplicated code and allow us to easily check for plane enable/disable requirements (such as pipe enable, plane status, pll status etc). Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> If we try applying these onto the current natty head the first is a clean cherry-pick, the second collised in context _only_ with the patch below, keeping both sides gives us the combined commit on this thread: commit bbeaf8811ba070fd186dfcabc957044c3a1149ac Author: Keng-Yu Lin <keng-yu.lin@canonical.com> Date: Wed Jun 22 09:49:34 2011 +0800 drm/i915: disable PCH ports if needed when disabling a CRTC For me this should have been submitted as two patched as clearly the change in the first one is a key component. From these discusssions it seems that this improves things from 100% failure to ~10% failure on boot. It looks a lot like the changes in the second patch, such as waiting for vblank on enabling the pipes fixes that final 10%. -apw
On Mon, 2011-10-03 at 14:23 +0100, Andy Whitcroft wrote: > On Mon, Oct 03, 2011 at 01:19:55PM +0100, Andy Whitcroft wrote: [...] > Ok, having talked to Jose a bit on IRC I am now able to answer my own > question. Actually this patch is the merge of _two_ upstream commits > only retaining one of the descriptions. [...] > For me this should have been submitted as two patched as clearly the > change in the first one is a key component. From these discusssions it > seems that this improves things from 100% failure to ~10% failure on > boot. It looks a lot like the changes in the second patch, such as > waiting for vblank on enabling the pipes fixes that final 10%. Thanks for the replies Andy, I will get the following done asap: - different patches for clean inclusion - also test on other than Ironlake/SandyBridge. And reply back to the list, Jose
On Mon, Oct 03, 2011 at 02:23:24PM +0100, Andy Whitcroft wrote: > On Mon, Oct 03, 2011 at 01:19:55PM +0100, Andy Whitcroft wrote: > > > Well its a vast and epic patch. I am left wondering whether this is > > a backport or a cherry-pick in this context, below its reported as a > > backport which implies it was modified, but you say here "as-is" implying > > the opposite. > [...] > > > Subject: [PATCH] drm/i915: add pipe/plane enable/disable functions > > > > > > Add plane enable/disable functions to prevent duplicated code and allow > > > us to easily check for plane enable/disable requirements (such as pipe > > > enable, plane status, pll status etc). > > > > It is clear that this description is inadequate if we are actually > > changing pipe init order. (Not that that is anything you should change > > as it _is_ the upstream commit description.) Bad Jesse. > > > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > (Backport to 2.6.38.y) > > > > BugLink: http://bugs.launchpad.net/bugs/812638 > > (backported from commit b24e71798871089da1a4ab049db2800afc1aac0c) > > > > "backported from" if its modified, "cherry-picked from" if its not ... ? > > Ok, having talked to Jose a bit on IRC I am now able to answer my own > question. Actually this patch is the merge of _two_ upstream commits > only retaining one of the descriptions. > > It is a merge of the two commits below: > > commit 65993d64a31844ad444694efb2d159eb9c883e49 > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > Date: Tue Jan 4 15:09:29 2011 -0800 > > drm/i915: don't enable plane, pipe and PLL prematurely > > On Ironlake+ we need to enable these in a specific order. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > and: > > commit b24e71798871089da1a4ab049db2800afc1aac0c > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > Date: Tue Jan 4 15:09:30 2011 -0800 > > drm/i915: add pipe/plane enable/disable functions > > Add plane enable/disable functions to prevent duplicated code and allow > us to easily check for plane enable/disable requirements (such as pipe > enable, plane status, pll status etc). > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > If we try applying these onto the current natty head the first is a clean > cherry-pick, the second collised in context _only_ with the patch below, > keeping both sides gives us the combined commit on this thread: > > commit bbeaf8811ba070fd186dfcabc957044c3a1149ac > Author: Keng-Yu Lin <keng-yu.lin@canonical.com> > Date: Wed Jun 22 09:49:34 2011 +0800 > > drm/i915: disable PCH ports if needed when disabling a CRTC > > For me this should have been submitted as two patched as clearly the > change in the first one is a key component. From these discusssions it > seems that this improves things from 100% failure to ~10% failure on > boot. It looks a lot like the changes in the second patch, such as > waiting for vblank on enabling the pipes fixes that final 10%. Just droping a note about the "drm/i915: disable PCH ports if needed when disabling a CRTC", it's now reverted on current natty master, because of regressions. So once pushed to master-next, I guess the second patch will not collide anymore. I was just waiting for the natty update to be copied to -proposed so I could push master to master-next and do a proper startnewrelease, but may be I'll do something to get the abis from the c-k-t ppa instead and sync master-next earlier. > > -apw >
On Mon, Oct 03, 2011 at 10:50:22AM -0300, Herton Ronaldo Krzesinski wrote: > On Mon, Oct 03, 2011 at 02:23:24PM +0100, Andy Whitcroft wrote: > > On Mon, Oct 03, 2011 at 01:19:55PM +0100, Andy Whitcroft wrote: > > > > > Well its a vast and epic patch. I am left wondering whether this is > > > a backport or a cherry-pick in this context, below its reported as a > > > backport which implies it was modified, but you say here "as-is" implying > > > the opposite. > > [...] > > > > Subject: [PATCH] drm/i915: add pipe/plane enable/disable functions > > > > > > > > Add plane enable/disable functions to prevent duplicated code and allow > > > > us to easily check for plane enable/disable requirements (such as pipe > > > > enable, plane status, pll status etc). > > > > > > It is clear that this description is inadequate if we are actually > > > changing pipe init order. (Not that that is anything you should change > > > as it _is_ the upstream commit description.) Bad Jesse. > > > > > > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > > > (Backport to 2.6.38.y) > > > > > > BugLink: http://bugs.launchpad.net/bugs/812638 > > > (backported from commit b24e71798871089da1a4ab049db2800afc1aac0c) > > > > > > "backported from" if its modified, "cherry-picked from" if its not ... ? > > > > Ok, having talked to Jose a bit on IRC I am now able to answer my own > > question. Actually this patch is the merge of _two_ upstream commits > > only retaining one of the descriptions. > > > > It is a merge of the two commits below: > > > > commit 65993d64a31844ad444694efb2d159eb9c883e49 > > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > > Date: Tue Jan 4 15:09:29 2011 -0800 > > > > drm/i915: don't enable plane, pipe and PLL prematurely > > > > On Ironlake+ we need to enable these in a specific order. > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > and: > > > > commit b24e71798871089da1a4ab049db2800afc1aac0c > > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > > Date: Tue Jan 4 15:09:30 2011 -0800 > > > > drm/i915: add pipe/plane enable/disable functions > > > > Add plane enable/disable functions to prevent duplicated code and allow > > us to easily check for plane enable/disable requirements (such as pipe > > enable, plane status, pll status etc). > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > If we try applying these onto the current natty head the first is a clean > > cherry-pick, the second collised in context _only_ with the patch below, > > keeping both sides gives us the combined commit on this thread: > > > > commit bbeaf8811ba070fd186dfcabc957044c3a1149ac > > Author: Keng-Yu Lin <keng-yu.lin@canonical.com> > > Date: Wed Jun 22 09:49:34 2011 +0800 > > > > drm/i915: disable PCH ports if needed when disabling a CRTC > > > > For me this should have been submitted as two patched as clearly the > > change in the first one is a key component. From these discusssions it > > seems that this improves things from 100% failure to ~10% failure on > > boot. It looks a lot like the changes in the second patch, such as > > waiting for vblank on enabling the pipes fixes that final 10%. > > Just droping a note about the "drm/i915: disable PCH ports if needed > when disabling a CRTC", it's now reverted on current natty master, > because of regressions. So once pushed to master-next, I guess the > second patch will not collide anymore. I was just waiting for the natty > update to be copied to -proposed so I could push master to master-next > and do a proper startnewrelease, but may be I'll do something to get > the abis from the c-k-t ppa instead and sync master-next earlier. Thanks for the heads up on that. -apw
On Mon, Oct 03, 2011 at 02:55:24PM +0100, Andy Whitcroft wrote: > On Mon, Oct 03, 2011 at 10:50:22AM -0300, Herton Ronaldo Krzesinski wrote: > > On Mon, Oct 03, 2011 at 02:23:24PM +0100, Andy Whitcroft wrote: > > > On Mon, Oct 03, 2011 at 01:19:55PM +0100, Andy Whitcroft wrote: > > > > > > > Well its a vast and epic patch. I am left wondering whether this is > > > > a backport or a cherry-pick in this context, below its reported as a > > > > backport which implies it was modified, but you say here "as-is" implying > > > > the opposite. > > > [...] > > > > > Subject: [PATCH] drm/i915: add pipe/plane enable/disable functions > > > > > > > > > > Add plane enable/disable functions to prevent duplicated code and allow > > > > > us to easily check for plane enable/disable requirements (such as pipe > > > > > enable, plane status, pll status etc). > > > > > > > > It is clear that this description is inadequate if we are actually > > > > changing pipe init order. (Not that that is anything you should change > > > > as it _is_ the upstream commit description.) Bad Jesse. > > > > > > > > > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > > > > > (Backport to 2.6.38.y) > > > > > > > > BugLink: http://bugs.launchpad.net/bugs/812638 > > > > (backported from commit b24e71798871089da1a4ab049db2800afc1aac0c) > > > > > > > > "backported from" if its modified, "cherry-picked from" if its not ... ? > > > > > > Ok, having talked to Jose a bit on IRC I am now able to answer my own > > > question. Actually this patch is the merge of _two_ upstream commits > > > only retaining one of the descriptions. > > > > > > It is a merge of the two commits below: > > > > > > commit 65993d64a31844ad444694efb2d159eb9c883e49 > > > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > > > Date: Tue Jan 4 15:09:29 2011 -0800 > > > > > > drm/i915: don't enable plane, pipe and PLL prematurely > > > > > > On Ironlake+ we need to enable these in a specific order. > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > and: > > > > > > commit b24e71798871089da1a4ab049db2800afc1aac0c > > > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > > > Date: Tue Jan 4 15:09:30 2011 -0800 > > > > > > drm/i915: add pipe/plane enable/disable functions > > > > > > Add plane enable/disable functions to prevent duplicated code and allow > > > us to easily check for plane enable/disable requirements (such as pipe > > > enable, plane status, pll status etc). > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > If we try applying these onto the current natty head the first is a clean > > > cherry-pick, the second collised in context _only_ with the patch below, > > > keeping both sides gives us the combined commit on this thread: > > > > > > commit bbeaf8811ba070fd186dfcabc957044c3a1149ac > > > Author: Keng-Yu Lin <keng-yu.lin@canonical.com> > > > Date: Wed Jun 22 09:49:34 2011 +0800 > > > > > > drm/i915: disable PCH ports if needed when disabling a CRTC > > > > > > For me this should have been submitted as two patched as clearly the > > > change in the first one is a key component. From these discusssions it > > > seems that this improves things from 100% failure to ~10% failure on > > > boot. It looks a lot like the changes in the second patch, such as > > > waiting for vblank on enabling the pipes fixes that final 10%. > > > > Just droping a note about the "drm/i915: disable PCH ports if needed > > when disabling a CRTC", it's now reverted on current natty master, > > because of regressions. So once pushed to master-next, I guess the > > second patch will not collide anymore. I was just waiting for the natty > > update to be copied to -proposed so I could push master to master-next > > and do a proper startnewrelease, but may be I'll do something to get > > the abis from the c-k-t ppa instead and sync master-next earlier. > > Thanks for the heads up on that. master-next should be up to date now, synced with master with new release getting abi from c-k-t ppa. > > -apw >
From: Jesse Barnes <jbarnes@virtuousgeek.org> Date: Tue, 4 Jan 2011 15:09:30 -0800 Subject: [PATCH] drm/i915: add pipe/plane enable/disable functions Add plane enable/disable functions to prevent duplicated code and allow us to easily check for plane enable/disable requirements (such as pipe enable, plane status, pll status etc). Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> (Backport to 2.6.38.y) Signed-off-by: Robert Hooker <robert.hooker@canonical.com> Signed-off-by: Jose Plans <jose.plans@canonical.com> --- drivers/gpu/drm/i915/i915_reg.h | 5 +- drivers/gpu/drm/i915/intel_display.c | 308 +++++++++++++++++++++++---------- 2 files changed, 217 insertions(+), 96 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 12c547a..d59df08 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2537,9 +2537,10 @@ #define DISPPLANE_32BPP_30BIT_NO_ALPHA (0xa<<26) #define DISPPLANE_STEREO_ENABLE (1<<25) #define DISPPLANE_STEREO_DISABLE 0 -#define DISPPLANE_SEL_PIPE_MASK (1<<24) +#define DISPPLANE_SEL_PIPE_SHIFT 24 +#define DISPPLANE_SEL_PIPE_MASK (3<<DISPPLANE_SEL_PIPE_SHIFT) #define DISPPLANE_SEL_PIPE_A 0 -#define DISPPLANE_SEL_PIPE_B (1<<24) +#define DISPPLANE_SEL_PIPE_B (1<<DISPPLANE_SEL_PIPE_SHIFT) #define DISPPLANE_SRC_KEY_ENABLE (1<<22) #define DISPPLANE_SRC_KEY_DISABLE 0 #define DISPPLANE_LINE_DOUBLE (1<<20) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 841558b..dc51881 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1058,6 +1058,203 @@ void intel_wait_for_pipe_off(struct drm_device *dev, int pipe) } } +static const char *state_string(bool enabled) +{ + return enabled ? "on" : "off"; +} + +/* Only for pre-ILK configs */ +static void assert_pll(struct drm_i915_private *dev_priv, + enum pipe pipe, bool state) +{ + int reg; + u32 val; + bool cur_state; + + reg = DPLL(pipe); + val = I915_READ(reg); + cur_state = !!(val & DPLL_VCO_ENABLE); + WARN(cur_state != state, + "PLL state assertion failure (expected %s, current %s)\n", + state_string(state), state_string(cur_state)); +} +#define assert_pll_enabled(d, p) assert_pll(d, p, true) +#define assert_pll_disabled(d, p) assert_pll(d, p, false) + +static void assert_pipe_enabled(struct drm_i915_private *dev_priv, + enum pipe pipe) +{ + int reg; + u32 val; + + reg = PIPECONF(pipe); + val = I915_READ(reg); + WARN(!(val & PIPECONF_ENABLE), + "pipe %c assertion failure, should be active but is disabled\n", + pipe ? 'B' : 'A'); +} + +static void assert_plane_enabled(struct drm_i915_private *dev_priv, + enum plane plane) +{ + int reg; + u32 val; + + reg = DSPCNTR(plane); + val = I915_READ(reg); + WARN(!(val & DISPLAY_PLANE_ENABLE), + "plane %c assertion failure, should be active but is disabled\n", + plane ? 'B' : 'A'); +} + +static void assert_planes_disabled(struct drm_i915_private *dev_priv, + enum pipe pipe) +{ + int reg, i; + u32 val; + int cur_pipe; + + /* Need to check both planes against the pipe */ + for (i = 0; i < 2; i++) { + reg = DSPCNTR(i); + val = I915_READ(reg); + cur_pipe = (val & DISPPLANE_SEL_PIPE_MASK) >> + DISPPLANE_SEL_PIPE_SHIFT; + WARN((val & DISPLAY_PLANE_ENABLE) && pipe == cur_pipe, + "plane %d assertion failure, should be off on pipe %c but is still active\n", + i, pipe ? 'B' : 'A'); + } +} + +/** + * intel_enable_pipe - enable a pipe, assertiing requirements + * @dev_priv: i915 private structure + * @pipe: pipe to enable + * + * Enable @pipe, making sure that various hardware specific requirements + * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc. + * + * @pipe should be %PIPE_A or %PIPE_B. + * + * Will wait until the pipe is actually running (i.e. first vblank) before + * returning. + */ +static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe) +{ + int reg; + u32 val; + + /* + * A pipe without a PLL won't actually be able to drive bits from + * a plane. On ILK+ the pipe PLLs are integrated, so we don't + * need the check. + */ + if (!HAS_PCH_SPLIT(dev_priv->dev)) + assert_pll_enabled(dev_priv, pipe); + + reg = PIPECONF(pipe); + val = I915_READ(reg); + val |= PIPECONF_ENABLE; + I915_WRITE(reg, val); + POSTING_READ(reg); + intel_wait_for_vblank(dev_priv->dev, pipe); +} + +/** + * intel_disable_pipe - disable a pipe, assertiing requirements + * @dev_priv: i915 private structure + * @pipe: pipe to disable + * + * Disable @pipe, making sure that various hardware specific requirements + * are met, if applicable, e.g. plane disabled, panel fitter off, etc. + * + * @pipe should be %PIPE_A or %PIPE_B. + * + * Will wait until the pipe has shut down before returning. + */ +static void intel_disable_pipe(struct drm_i915_private *dev_priv, + enum pipe pipe) +{ + int reg; + u32 val; + + /* + * Make sure planes won't keep trying to pump pixels to us, + * or we might hang the display. + */ + assert_planes_disabled(dev_priv, pipe); + + /* Don't disable pipe A or pipe A PLLs if needed */ + if (pipe == PIPE_A && (dev_priv->quirks & QUIRK_PIPEA_FORCE)) + return; + + reg = PIPECONF(pipe); + val = I915_READ(reg); + val &= ~PIPECONF_ENABLE; + I915_WRITE(reg, val); + POSTING_READ(reg); + intel_wait_for_pipe_off(dev_priv->dev, pipe); +} + +/** + * intel_enable_plane - enable a display plane on a given pipe + * @dev_priv: i915 private structure + * @plane: plane to enable + * @pipe: pipe being fed + * + * Enable @plane on @pipe, making sure that @pipe is running first. + */ +static void intel_enable_plane(struct drm_i915_private *dev_priv, + enum plane plane, enum pipe pipe) +{ + int reg; + u32 val; + + /* If the pipe isn't enabled, we can't pump pixels and may hang */ + assert_pipe_enabled(dev_priv, pipe); + + reg = DSPCNTR(plane); + val = I915_READ(reg); + val |= DISPLAY_PLANE_ENABLE; + I915_WRITE(reg, val); + POSTING_READ(reg); + intel_wait_for_vblank(dev_priv->dev, pipe); +} + +/* + * Plane regs are double buffered, going from enabled->disabled needs a + * trigger in order to latch. The display address reg provides this. + */ +static void intel_flush_display_plane(struct drm_i915_private *dev_priv, + enum plane plane) +{ + u32 reg = DSPADDR(plane); + I915_WRITE(reg, I915_READ(reg)); +} + +/** + * intel_disable_plane - disable a display plane + * @dev_priv: i915 private structure + * @plane: plane to disable + * @pipe: pipe consuming the data + * + * Disable @plane; should be an independent operation. + */ +static void intel_disable_plane(struct drm_i915_private *dev_priv, + enum plane plane, enum pipe pipe) +{ + int reg; + u32 val; + + reg = DSPCNTR(plane); + val = I915_READ(reg); + val &= ~DISPLAY_PLANE_ENABLE; + I915_WRITE(reg, val); + POSTING_READ(reg); + intel_flush_display_plane(dev_priv, plane); + intel_wait_for_vblank(dev_priv->dev, pipe); +} + static void i8xx_enable_fbc(struct drm_crtc *crtc, unsigned long interval) { struct drm_device *dev = crtc->dev; @@ -2003,14 +2200,6 @@ static void ironlake_fdi_enable(struct drm_crtc *crtc) } } -static void intel_flush_display_plane(struct drm_device *dev, - int plane) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - u32 reg = DSPADDR(plane); - I915_WRITE(reg, I915_READ(reg)); -} - /* * When we disable a pipe, we need to clear any pending scanline wait events * to avoid hanging the ring, which we assume we are waiting on. @@ -2158,22 +2347,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) dev_priv->pch_pf_size); } - /* Enable CPU pipe */ - reg = PIPECONF(pipe); - temp = I915_READ(reg); - if ((temp & PIPECONF_ENABLE) == 0) { - I915_WRITE(reg, temp | PIPECONF_ENABLE); - POSTING_READ(reg); - intel_wait_for_vblank(dev, intel_crtc->pipe); - } - - /* configure and enable CPU plane */ - reg = DSPCNTR(plane); - temp = I915_READ(reg); - if ((temp & DISPLAY_PLANE_ENABLE) == 0) { - I915_WRITE(reg, temp | DISPLAY_PLANE_ENABLE); - intel_flush_display_plane(dev, plane); - } + intel_enable_pipe(dev_priv, pipe); + intel_enable_plane(dev_priv, plane, pipe); /* Skip the PCH stuff if possible */ if (!is_pch_port) @@ -2285,27 +2460,13 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) drm_vblank_off(dev, pipe); intel_crtc_update_cursor(crtc, false); - /* Disable display plane */ - reg = DSPCNTR(plane); - temp = I915_READ(reg); - if (temp & DISPLAY_PLANE_ENABLE) { - I915_WRITE(reg, temp & ~DISPLAY_PLANE_ENABLE); - intel_flush_display_plane(dev, plane); - } + intel_disable_plane(dev_priv, plane, pipe); if (dev_priv->cfb_plane == plane && dev_priv->display.disable_fbc) dev_priv->display.disable_fbc(dev); - /* disable cpu pipe, disable after all planes disabled */ - reg = PIPECONF(pipe); - temp = I915_READ(reg); - if (temp & PIPECONF_ENABLE) { - I915_WRITE(reg, temp & ~PIPECONF_ENABLE); - POSTING_READ(reg); - /* wait for cpu pipe off, pipe state */ - intel_wait_for_pipe_off(dev, intel_crtc->pipe); - } + intel_disable_pipe(dev_priv, pipe); /* Disable PF */ I915_WRITE(pipe ? PFB_CTL_1 : PFA_CTL_1, 0); @@ -2500,19 +2661,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) udelay(150); } - /* Enable the pipe */ - reg = PIPECONF(pipe); - temp = I915_READ(reg); - if ((temp & PIPECONF_ENABLE) == 0) - I915_WRITE(reg, temp | PIPECONF_ENABLE); - - /* Enable the plane */ - reg = DSPCNTR(plane); - temp = I915_READ(reg); - if ((temp & DISPLAY_PLANE_ENABLE) == 0) { - I915_WRITE(reg, temp | DISPLAY_PLANE_ENABLE); - intel_flush_display_plane(dev, plane); - } + intel_enable_pipe(dev_priv, pipe); + intel_enable_plane(dev_priv, plane, pipe); intel_crtc_load_lut(crtc); intel_update_fbc(dev); @@ -2544,33 +2694,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) dev_priv->display.disable_fbc) dev_priv->display.disable_fbc(dev); - /* Disable display plane */ - reg = DSPCNTR(plane); - temp = I915_READ(reg); - if (temp & DISPLAY_PLANE_ENABLE) { - I915_WRITE(reg, temp & ~DISPLAY_PLANE_ENABLE); - /* Flush the plane changes */ - intel_flush_display_plane(dev, plane); - - /* Wait for vblank for the disable to take effect */ - if (IS_GEN2(dev)) - intel_wait_for_vblank(dev, pipe); - } + intel_disable_plane(dev_priv, plane, pipe); /* Don't disable pipe A or pipe A PLLs if needed */ if (pipe == 0 && (dev_priv->quirks & QUIRK_PIPEA_FORCE)) goto done; - /* Next, disable display pipes */ - reg = PIPECONF(pipe); - temp = I915_READ(reg); - if (temp & PIPECONF_ENABLE) { - I915_WRITE(reg, temp & ~PIPECONF_ENABLE); - - /* Wait for the pipe to turn off */ - POSTING_READ(reg); - intel_wait_for_pipe_off(dev, pipe); - } + intel_disable_pipe(dev_priv, pipe); reg = DPLL(pipe); temp = I915_READ(reg); @@ -4322,9 +4452,8 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, pipeconf &= ~PIPECONF_DOUBLE_WIDE; } - dspcntr |= DISPLAY_PLANE_ENABLE; - pipeconf |= PIPECONF_ENABLE; - dpll |= DPLL_VCO_ENABLE; + if (!HAS_PCH_SPLIT(dev)) + dpll |= DPLL_VCO_ENABLE; DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe == 0 ? 'A' : 'B'); drm_mode_debug_printmodeline(mode); @@ -4533,6 +4662,8 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, I915_WRITE(PIPECONF(pipe), pipeconf); POSTING_READ(PIPECONF(pipe)); + if (!HAS_PCH_SPLIT(dev)) + intel_enable_pipe(dev_priv, pipe); intel_wait_for_vblank(dev, pipe); @@ -4543,6 +4674,9 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, } I915_WRITE(DSPCNTR(plane), dspcntr); + POSTING_READ(DSPCNTR(plane)); + if (!HAS_PCH_SPLIT(dev)) + intel_enable_plane(dev_priv, plane, pipe); ret = intel_pipe_set_base(crtc, x, y, old_fb); @@ -5662,22 +5796,8 @@ static void intel_sanitize_modesetting(struct drm_device *dev, pipe = !pipe; /* Disable the plane and wait for it to stop reading from the pipe. */ - I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE); - intel_flush_display_plane(dev, plane); - - if (IS_GEN2(dev)) - intel_wait_for_vblank(dev, pipe); - - if (pipe == 0 && (dev_priv->quirks & QUIRK_PIPEA_FORCE)) - return; - - /* Switch off the pipe. */ - reg = PIPECONF(pipe); - val = I915_READ(reg); - if (val & PIPECONF_ENABLE) { - I915_WRITE(reg, val & ~PIPECONF_ENABLE); - intel_wait_for_pipe_off(dev, pipe); - } + intel_disable_plane(dev_priv, plane, pipe); + intel_disable_pipe(dev_priv, pipe); } static void intel_crtc_reset(struct drm_crtc *crtc) -- 1.7.4.1
SRU Justification: Impact: Every Lenovo x420s with Samsung panels will present a corrupted display when booting / mode setting. The systems are not usable. Fixes: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/812638 Fix: Backported patch commit from upstream: b24e71798871089da1a4ab049db2800afc1aac0c drm/i915: add pipe/plane enable/disable functions by Jesse Barnes. Test case: The reproducer is to simply boot the system with any kernels prior to Oneiric and using Lenovo x420 and it's Intel graphical chipset. Hardware tested was: - Lenovo x301 and on Lenovo x420s with Samsung panel. Note: This a very intrusive patch, I have spent some time studying it and I came to the conclusion it would be safer and easier to maintain if this patch was include as-is rather than providing a forked patch. I would like the list opinion on this. There are three major changes: (1) Addition of functions allowing to cleanly add or remove pipes / planes. (2) Addition of assertion functions to alert help prevent scenarios where pipes or planes are not present and they should be. For example a plane is being activated but there was no pipe active to contain it. (3) During crtc mode setting, now the pipe and planes are allocated later in the process conforming to intel specifications. Seen at the changes made in: intel_crtc_mode_set() only affecting ironlake +/sandybridge. (3) is the real fix for this panel issue, however (1) is necessary for it as well as (2) helps preventing further regressions. drivers/gpu/drm/i915/i915_reg.h | 5 +- drivers/gpu/drm/i915/intel_display.c | 308 +++++++++++++++++++++++---------- 2 files changed, 217 insertions(+), 96 deletions(-) Jose