[Natty-SRU] Lenovo x420s panel display corruption when mode-setting
Andy Whitcroft
apw at canonical.com
Mon Oct 3 12:19:55 UTC 2011
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 at 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 at virtuousgeek.org>
> Signed-off-by: Chris Wilson <chris at 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 at canonical.com>
> Signed-off-by: Jose Plans <jose.plans at 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
More information about the kernel-team
mailing list