All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	stable@vger.kernel.org
Subject: [PATCH] drm/i915: Fixup Oops in the pipe config computation
Date: Fri, 12 Apr 2013 18:48:43 +0200	[thread overview]
Message-ID: <1365785323-4220-1-git-send-email-daniel.vetter@ffwll.ch> (raw)
In-Reply-To: <CAKMK7uGrq9CCnqi9i_O5PRA+ePn6eaJtbNos=-fuWgL9Vw6-ug@mail.gmail.com>

Yet again our current confusion between doing the modeset globally,
but only having the new parameters for one crtc at a time.

So that intel_set_mode essentially already does a global modeset:
intel_modeset_affected_pipes compares the current state with where we
want to go to (which is carefully set up by intel_crtc_set_config) and
then goes through the modeset sequence for any crtc which needs
updating.

Now the issue is that the actual interface with the remaining code
still only works on one crtc, and so we only pass in one fb and one
mode. In intel_set_mode we also only compute one intel_crtc_config
(which should be the one for the crtc we're doing a modeset on).

The reason for that mismatch is twofold:
- We want to eventually do all modeset as global state changes, so
it's just infrastructure prep.
- But even the old semantics can change more than one crtc when you
e.g. move a connector from crtc A to crtc B, then both crtc A and B
need to be updated. Usually that means one pipe is disabled and the
other enabled. This is also the reason why the hack doesn't touch the
disable_pipes mask.

Now hilarity ensued in our kms config restore paths when we actually
try to do a modeset on all crtcs: If the first crtc should be off and
the second should be on, then the call on the first crtc will notice
that the 2nd one should be switched on and so tries to compute the
pipe_config. But due to a lack of passed-in fb (crtc 1 should be off
after all) it only results in tears.

This case is ridiculously easy to hit on gen2/3 where the lvds output
is restricted to pipe B. Note that before the pipe_config bpp rework
gen2/3 didn't care really about the fb->depth, so this is a regression
brought to light with

commit 4e53c2e010e531b4a014692199e978482d471c7e
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed Mar 27 00:44:58 2013 +0100

    drm/i915: precompute pipe bpp before touching the hw

But apparently Ajax also managed to blow up pch platforms, probably
with some randomized configs, and pch platforms trip up over the lack
of an fb even in the old code. So this actually goes back to the first
introduction of the new modeset restore code in

commit 45e2b5f640b3766da3eda48f6c35f088155c06f3
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Nov 23 18:16:34 2012 +0100

    drm/i915: force restore on lid open

Fix this mess by now by justing shunting all the cool new global
modeset logic in intel_modeset_affected_pipes.

v2: Improve commit message and clean up all the comments in
intel_modeset_affected_pipes - since the introduction of the modeset
restore code they've been a bit outdated.

Bugzill: https://bugzilla.redhat.com/show_bug.cgi?id=917725
Cc: stable@vger.kernel.org
References: http://www.mail-archive.com/stable@vger.kernel.org/msg38084.html
Tested-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |   23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1b2c744..010115c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7613,22 +7613,25 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes,
 	if (crtc->enabled)
 		*prepare_pipes |= 1 << intel_crtc->pipe;
 
-	/* We only support modeset on one single crtc, hence we need to do that
-	 * only for the passed in crtc iff we change anything else than just
-	 * disable crtcs.
-	 *
-	 * This is actually not true, to be fully compatible with the old crtc
-	 * helper we automatically disable _any_ output (i.e. doesn't need to be
-	 * connected to the crtc we're modesetting on) if it's disconnected.
-	 * Which is a rather nutty api (since changed the output configuration
-	 * without userspace's explicit request can lead to confusion), but
-	 * alas. Hence we currently need to modeset on all pipes we prepare. */
+	/*
+	 * For simplicity do a full modeset on any pipe where the output routing
+	 * changed. We could be more clever, but that would require us to be
+	 * more careful with calling the relevant encoder->mode_set functions.
+	 */
 	if (*prepare_pipes)
 		*modeset_pipes = *prepare_pipes;
 
 	/* ... and mask these out. */
 	*modeset_pipes &= ~(*disable_pipes);
 	*prepare_pipes &= ~(*disable_pipes);
+
+	/*
+	 * HACK: We don't (yet) fully support global modesets. intel_set_config
+	 * obies this rule, but the modeset restore mode of
+	 * intel_modeset_setup_hw_state does not.
+	 */
+	*modeset_pipes &= 1 << intel_crtc->pipe;
+	*prepare_pipes &= 1 << intel_crtc->pipe;
 }
 
 static bool intel_crtc_in_use(struct drm_crtc *crtc)
-- 
1.7.10.4

  reply	other threads:[~2013-04-12 16:48 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-11 14:29 [PATCH 1/7] drm/i915: move debug output back to the right place Daniel Vetter
2013-04-11 14:29 ` [PATCH 2/7] drm/i915: Fixup Oops in the pipe config computation Daniel Vetter
2013-04-11 21:09   ` Daniel Vetter
2013-04-12  8:10     ` Daniel Vetter
2013-04-12  9:46   ` Chris Wilson
2013-04-12 15:24     ` Daniel Vetter
2013-04-12 16:48       ` Daniel Vetter [this message]
2013-04-12 17:46         ` [PATCH] " Chris Wilson
2013-04-11 14:29 ` [PATCH 3/7] drm/i915: Fixup pfit disabling for gen2/3 Daniel Vetter
2013-04-12 14:01   ` Chris Wilson
2013-04-11 14:29 ` [PATCH 4/7] drm/i915: don't enable the plane too early in i9xx_crtc_mode_set Daniel Vetter
2013-04-12 14:05   ` Chris Wilson
2013-04-11 14:29 ` [PATCH 5/7] drm/i915: drop redundant vblank waits Daniel Vetter
2013-04-11 17:47   ` Paulo Zanoni
2013-04-11 18:10     ` Daniel Vetter
2013-04-12 14:03   ` Chris Wilson
2013-04-11 14:29 ` [PATCH 6/7] drm/i915: add pipe asserts for the crtc enable sequence Daniel Vetter
2013-04-12 14:03   ` Chris Wilson
2013-04-11 14:29 ` [PATCH 7/7] drm/i915: add i9xx pfit pipe asserts Daniel Vetter
2013-04-11 16:37   ` Ville Syrjälä
2013-04-11 16:38     ` Daniel Vetter
2013-04-12  5:47     ` Jani Nikula
2013-04-12 14:02   ` Chris Wilson
2013-04-15  8:17     ` Daniel Vetter
2013-04-11 14:39 ` [PATCH] drm/i915: move debug output back to the right place Daniel Vetter
2013-04-11 17:27   ` Ville Syrjälä
2013-04-11 17:49     ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1365785323-4220-1-git-send-email-daniel.vetter@ffwll.ch \
    --to=daniel.vetter@ffwll.ch \
    --cc=chris@chris-wilson.co.uk \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.