[PATCH] drm/i915: Quirk the pipe A quirk in the modeset state checker

[PATCH] drm/i915: Quirk the pipe A quirk in the modeset state checker

[Daniel Vetter]

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.

Note that in the hw state readout we don't really care since we have a
big hack to force-enable pipe A anyway.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64764
Cc: stable@vger.kernel.org
Reported-and-Tested-by: mlsemon35@gmail.com
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d63bb3fa..5f2b289 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 unreliable with the pipe A quirk */
+		if (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);
-- 
1.7.10.4

Re: [Intel-gfx] [PATCH] drm/i915: Quirk the pipe A quirk in the modeset state checker

[Chris Wilson]

On Wed, May 29, 2013 at 09:28:22AM +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.

This is misleading as it is not the hw state that is unreliable, but
crtc->active that is a misnomer. The quirk makes the hw state
inconsistent with the bookkeeping.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/i915: Quirk the pipe A quirk in the modeset state checker

[Daniel Vetter]

On Wed, May 29, 2013 at 9:54 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, May 29, 2013 at 09:28:22AM +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.
>
> This is misleading as it is not the hw state that is unreliable, but
> crtc->active that is a misnomer. The quirk makes the hw state
> inconsistent with the bookkeeping.

Well, crtc->active tracks much more than what we quirk, it means that
the entire display pipe is on (including encoders, planes and all) and
pumping pixels. The quirk only keeps pipe&pll running though. I've
seen two options:
- this one here
- pimping the get_config stuff to figure out we're quirked and e.g.
check whether the plane is running.

Clamping the tracked state seemed to be the quicker option, and we
already have similar (in spirit) hacks in assert_pipe.

Also, we shouldn't lose any assert coverage with this: The pipe
enabled state is independently asserted in the enable/disable
functions and the hw state doesn't really check anything if the pipe
is logically off.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: Quirk the pipe A quirk in the modeset state checker

[Chris Wilson]

On Wed, May 29, 2013 at 10:09:34AM +0200, Daniel Vetter wrote:
> On Wed, May 29, 2013 at 9:54 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Wed, May 29, 2013 at 09:28:22AM +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.
> >
> > This is misleading as it is not the hw state that is unreliable, but
> > crtc->active that is a misnomer. The quirk makes the hw state
> > inconsistent with the bookkeeping.
> 
> Well, crtc->active tracks much more than what we quirk, it means that
> the entire display pipe is on (including encoders, planes and all) and
> pumping pixels. The quirk only keeps pipe&pll running though. I've
> seen two options:
> - this one here
> - pimping the get_config stuff to figure out we're quirked and e.g.
> check whether the plane is running.
> 
> Clamping the tracked state seemed to be the quicker option, and we
> already have similar (in spirit) hacks in assert_pipe.
> 
> Also, we shouldn't lose any assert coverage with this: The pipe
> enabled state is independently asserted in the enable/disable
> functions and the hw state doesn't really check anything if the pipe
> is logically off.

Completely agree; I came to the same assessment myself wondering if you
were covering up some nasty bug. It's just the wording here that makes
me think that the hardware readback is broken, when in fact it is just
inconsistent with our expectations due to our misleading the state
tracking.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Quirk the pipe A quirk in the modeset state checker

[Daniel Vetter]

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(+)

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);
-- 
1.7.10.4

Re: [PATCH] drm/i915: Quirk the pipe A quirk in the modeset state checker

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/i915: Quirk the pipe A quirk in the modeset state checker

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch