[PATCH] drm/i915: Sanitize the output registers after resume

[PATCH] drm/i915: Sanitize the output registers after resume

[Chris Wilson]

Similar to booting, we need to inspect the state left by the BIOS and
remove any conflicting bits before we take over. The example reported by
Seth Forshee is very similar to the bug we encountered with the state left
by grub2, that the crtc pipe<->planning mapping was reversed from our
expectations and so we failed to turn off the outputs when booting or,
in this case, resuming. This may be in fact the same bug, but triggered
at resume time.

This patch rearranges the code we already have to clear up the
conflicting state upon init and calls it from reset (which is called
after we have lost control of the hardware, i.e. along both the boot and
resume paths) instead.

Reported-and-tested-by: Seth Forshee <seth.forshee@canonical.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35796
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@kernel.org
---
 drivers/gpu/drm/i915/intel_display.c |   68 ++++++++++++++++++----------------
 1 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b9e7776..3c4b378 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6199,36 +6199,6 @@ cleanup_work:
 	return ret;
 }
 
-static void intel_crtc_reset(struct drm_crtc *crtc)
-{
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	/* Reset flags back to the 'unknown' status so that they
-	 * will be correctly set on the initial modeset.
-	 */
-	intel_crtc->dpms_mode = -1;
-}
-
-static struct drm_crtc_helper_funcs intel_helper_funcs = {
-	.dpms = intel_crtc_dpms,
-	.mode_fixup = intel_crtc_mode_fixup,
-	.mode_set = intel_crtc_mode_set,
-	.mode_set_base = intel_pipe_set_base,
-	.mode_set_base_atomic = intel_pipe_set_base_atomic,
-	.load_lut = intel_crtc_load_lut,
-	.disable = intel_crtc_disable,
-};
-
-static const struct drm_crtc_funcs intel_crtc_funcs = {
-	.reset = intel_crtc_reset,
-	.cursor_set = intel_crtc_cursor_set,
-	.cursor_move = intel_crtc_cursor_move,
-	.gamma_set = intel_crtc_gamma_set,
-	.set_config = drm_crtc_helper_set_config,
-	.destroy = intel_crtc_destroy,
-	.page_flip = intel_crtc_page_flip,
-};
-
 static void intel_sanitize_modesetting(struct drm_device *dev,
 				       int pipe, int plane)
 {
@@ -6265,6 +6235,42 @@ static void intel_sanitize_modesetting(struct drm_device *dev,
 	intel_disable_pipe(dev_priv, pipe);
 }
 
+static void intel_crtc_reset(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	/* Reset flags back to the 'unknown' status so that they
+	 * will be correctly set on the initial modeset.
+	 */
+	intel_crtc->dpms_mode = -1;
+
+	/* We need to fix up any BIOS configuration that conflicts with
+	 * our expectations.
+	 */
+	intel_sanitize_modesetting(dev, intel_crtc->pipe, intel_crtc->plane);
+}
+
+static struct drm_crtc_helper_funcs intel_helper_funcs = {
+	.dpms = intel_crtc_dpms,
+	.mode_fixup = intel_crtc_mode_fixup,
+	.mode_set = intel_crtc_mode_set,
+	.mode_set_base = intel_pipe_set_base,
+	.mode_set_base_atomic = intel_pipe_set_base_atomic,
+	.load_lut = intel_crtc_load_lut,
+	.disable = intel_crtc_disable,
+};
+
+static const struct drm_crtc_funcs intel_crtc_funcs = {
+	.reset = intel_crtc_reset,
+	.cursor_set = intel_crtc_cursor_set,
+	.cursor_move = intel_crtc_cursor_move,
+	.gamma_set = intel_crtc_gamma_set,
+	.set_config = drm_crtc_helper_set_config,
+	.destroy = intel_crtc_destroy,
+	.page_flip = intel_crtc_page_flip,
+};
+
 static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -6314,8 +6320,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 
 	setup_timer(&intel_crtc->idle_timer, intel_crtc_idle_timer,
 		    (unsigned long)intel_crtc);
-
-	intel_sanitize_modesetting(dev, intel_crtc->pipe, intel_crtc->plane);
 }
 
 int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
-- 
1.7.4.1

Re: [PATCH] drm/i915: Sanitize the output registers after resume

[Chris Wilson]

After sending this patch, to the changelog I added:

See commit 47f1c6c9ffdec0c0e5a2c2709bd63c7380b325c4 ("drm/i915:
Clean conflicting modesetting registers upon init") for further details
on the original bug.

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Sanitize the output registers after resume

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 537 bytes --]

On Tue, 12 Apr 2011 18:06:51 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> This patch rearranges the code we already have to clear up the
> conflicting state upon init and calls it from reset (which is called
> after we have lost control of the hardware, i.e. along both the boot and
> resume paths) instead.

Note that intel_sanitize_modesetting does not do anything on PCH
hardware yet. Otherwise, this looks like a reasonable plan.

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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

Re: [PATCH] drm/i915: Sanitize the output registers after resume

[Chris Wilson]

On Tue, 12 Apr 2011 10:26:36 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Tue, 12 Apr 2011 18:06:51 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > This patch rearranges the code we already have to clear up the
> > conflicting state upon init and calls it from reset (which is called
> > after we have lost control of the hardware, i.e. along both the boot and
> > resume paths) instead.
> 
> Note that intel_sanitize_modesetting does not do anything on PCH
> hardware yet. Otherwise, this looks like a reasonable plan.

Yes, at the moment the only scenario we've discovered is where the BIOS
sets up a conflicting pipe<->plane mapping, which is only possible before
PCH.

If we go hunting, we can probably find many more. :|
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Sanitize the output registers after resume

[Jesse Barnes]

On Tue, 12 Apr 2011 18:06:51 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Similar to booting, we need to inspect the state left by the BIOS and
> remove any conflicting bits before we take over. The example reported by
> Seth Forshee is very similar to the bug we encountered with the state left
> by grub2, that the crtc pipe<->planning mapping was reversed from our
> expectations and so we failed to turn off the outputs when booting or,
> in this case, resuming. This may be in fact the same bug, but triggered
> at resume time.
> 
> This patch rearranges the code we already have to clear up the
> conflicting state upon init and calls it from reset (which is called
> after we have lost control of the hardware, i.e. along both the boot and
> resume paths) instead.
> 
> Reported-and-tested-by: Seth Forshee <seth.forshee@canonical.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35796
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@kernel.org
> ---

It's a bigger change, but I'd really rather we have functions to probe
the existing config and copy it into our mode config structures.  That
way we can re-use the code to minimize flicker and transitions, and
potentially just leave things alone if the config is valid (it should
be since the BIOS provided it) and we just need to switch the fb around
or disable VGA.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Sanitize the output registers after resume

[Chris Wilson]

On Tue, 12 Apr 2011 17:00:05 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Tue, 12 Apr 2011 18:06:51 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Similar to booting, we need to inspect the state left by the BIOS and
> > remove any conflicting bits before we take over. The example reported by
> > Seth Forshee is very similar to the bug we encountered with the state left
> > by grub2, that the crtc pipe<->planning mapping was reversed from our
> > expectations and so we failed to turn off the outputs when booting or,
> > in this case, resuming. This may be in fact the same bug, but triggered
> > at resume time.
> > 
> > This patch rearranges the code we already have to clear up the
> > conflicting state upon init and calls it from reset (which is called
> > after we have lost control of the hardware, i.e. along both the boot and
> > resume paths) instead.
> > 
> > Reported-and-tested-by: Seth Forshee <seth.forshee@canonical.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35796
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: stable@kernel.org
> > ---
> 
> It's a bigger change, but I'd really rather we have functions to probe
> the existing config and copy it into our mode config structures.  That
> way we can re-use the code to minimize flicker and transitions, and
> potentially just leave things alone if the config is valid (it should
> be since the BIOS provided it) and we just need to switch the fb around
> or disable VGA.

On the one hand, we should just be able to trust a BIOS config, right? On
the other hand did you just see what that BIOS did to Sitosfe's machine?
Or the PGTBL_ERs it generates on other machines?

Though we can reduce this particular issue by aligning ourselves better
with the state left by the BIOS, I don't think we can lose a function
whose job it is to make sure that the hw state is consistent with KMS. And
that function needs to be called whenever we takeover from the BIOS -
which is the purpose of this patch.

At the moment, we have a real problem where outputs are continuing to use
GTT as we rewrite it from underneath them. The simplest solution for that
is to disable the outputs right at the start. The next step is then to
recreate the GEM and KMS state currently occupied by the BIOS and preserve
that across the takeover.  So yes, to do it completely flicker free is a
much harder job.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre