| Message ID | 1302628011-16640-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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>
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
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 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
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,
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(-)