| Message ID | 1369816889-22871-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 29, 2013 at 10:41:29AM +0200, Daniel Vetter wrote: > If we always force the pipe A to on we can't use the hw state to > decide whether it should be on. Hence quirk the quirk. > > The problem is that crtc->active tracks the state of the entire > display pipe, i.e. including planes, encoders and all. But our hw > state readout simply looks at the pipe. But with the pipe A quirk we > force-enable that (together with it's pll). To fix that mismatch we > have two options: > - Quirk the checked state to match what our sw tracking states if the > pipe A quirk is in effect. > - Improve the hw state readout to not get fooled by the pipe A quirk. > > Since we already have similar state clamping in e.g. assert_pipe I've > opted for the first variant. Also note that we don't really loose any > state checking: Individual pieces of the abstract crtc pipe are > checked in the enable/disable functions with the various asssert_* > checks we have, and the hw state check code doesn't check anything if > the pipe is off anyway. > > v2: Pimp commit message after discussion with Chris and only apply the > quirk for the quirk if we're checking pipe A. Otherwise we'll miss > state checking for pipe B on i830M ... > > v3: Make the code comment consistent with the improved commit message, > too (Chris). > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64764 > Cc: stable@vger.kernel.org > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Reported-and-Tested-by: mlsemon35@gmail.com (v1) > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Wed, May 29, 2013 at 10:06:20AM +0100, Chris Wilson wrote: > On Wed, May 29, 2013 at 10:41:29AM +0200, Daniel Vetter wrote: > > If we always force the pipe A to on we can't use the hw state to > > decide whether it should be on. Hence quirk the quirk. > > > > The problem is that crtc->active tracks the state of the entire > > display pipe, i.e. including planes, encoders and all. But our hw > > state readout simply looks at the pipe. But with the pipe A quirk we > > force-enable that (together with it's pll). To fix that mismatch we > > have two options: > > - Quirk the checked state to match what our sw tracking states if the > > pipe A quirk is in effect. > > - Improve the hw state readout to not get fooled by the pipe A quirk. > > > > Since we already have similar state clamping in e.g. assert_pipe I've > > opted for the first variant. Also note that we don't really loose any > > state checking: Individual pieces of the abstract crtc pipe are > > checked in the enable/disable functions with the various asssert_* > > checks we have, and the hw state check code doesn't check anything if > > the pipe is off anyway. > > > > v2: Pimp commit message after discussion with Chris and only apply the > > quirk for the quirk if we're checking pipe A. Otherwise we'll miss > > state checking for pipe B on i830M ... > > > > v3: Make the code comment consistent with the improved commit message, > > too (Chris). > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64764 > > Cc: stable@vger.kernel.org > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Reported-and-Tested-by: mlsemon35@gmail.com (v1) > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Picked up for -fixes, thanks for the review. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d63bb3fa..09eedfa 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8099,6 +8099,11 @@ intel_modeset_check_state(struct drm_device *dev) pipe_config.cpu_transcoder = crtc->config.cpu_transcoder; active = dev_priv->display.get_pipe_config(crtc, &pipe_config); + + /* hw state is inconsistent with the pipe A quirk */ + if (crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) + active = crtc->active; + WARN(crtc->active != active, "crtc active state doesn't match with hw state " "(expected %i, found %i)\n", crtc->active, active);
If we always force the pipe A to on we can't use the hw state to decide whether it should be on. Hence quirk the quirk. The problem is that crtc->active tracks the state of the entire display pipe, i.e. including planes, encoders and all. But our hw state readout simply looks at the pipe. But with the pipe A quirk we force-enable that (together with it's pll). To fix that mismatch we have two options: - Quirk the checked state to match what our sw tracking states if the pipe A quirk is in effect. - Improve the hw state readout to not get fooled by the pipe A quirk. Since we already have similar state clamping in e.g. assert_pipe I've opted for the first variant. Also note that we don't really loose any state checking: Individual pieces of the abstract crtc pipe are checked in the enable/disable functions with the various asssert_* checks we have, and the hw state check code doesn't check anything if the pipe is off anyway. v2: Pimp commit message after discussion with Chris and only apply the quirk for the quirk if we're checking pipe A. Otherwise we'll miss state checking for pipe B on i830M ... v3: Make the code comment consistent with the improved commit message, too (Chris). Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64764 Cc: stable@vger.kernel.org Cc: Chris Wilson <chris@chris-wilson.co.uk> Reported-and-Tested-by: mlsemon35@gmail.com (v1) Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 5 +++++ 1 file changed, 5 insertions(+)