[PATCH] drm/i915: check the power wells on assert_{cursor, plane}

[PATCH] drm/i915: check the power wells on assert_{cursor, plane}

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Since we merged runtime PM support for DPMS, it is possible that these
functions will be called when the power wells are disabled but a mode
is "set", resulting in "failed assertion" and "device suspended while
reading register" WARNs.

To reproduce the bug: disable all screens using mode unset, do a
modeset on one screen, disable it using DPMS, then try to do a mode
unset on it again to see the WARNs.

Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp
Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 54e3af9..7ad46e2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1216,7 +1216,9 @@ static void assert_cursor(struct drm_i915_private *dev_priv,
 	struct drm_device *dev = dev_priv->dev;
 	bool cur_state;
 
-	if (IS_845G(dev) || IS_I865G(dev))
+	if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe)))
+		cur_state = false;
+	else if (IS_845G(dev) || IS_I865G(dev))
 		cur_state = I915_READ(_CURACNTR) & CURSOR_ENABLE;
 	else
 		cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
@@ -1262,9 +1264,13 @@ static void assert_plane(struct drm_i915_private *dev_priv,
 	u32 val;
 	bool cur_state;
 
-	reg = DSPCNTR(plane);
-	val = I915_READ(reg);
-	cur_state = !!(val & DISPLAY_PLANE_ENABLE);
+	if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PIPE(plane))) {
+		cur_state = false;
+	} else {
+		reg = DSPCNTR(plane);
+		val = I915_READ(reg);
+		cur_state = !!(val & DISPLAY_PLANE_ENABLE);
+	}
 	WARN(cur_state != state,
 	     "plane %c assertion failure (expected %s, current %s)\n",
 	     plane_name(plane), state_string(state), state_string(cur_state));
-- 
2.0.0

[PATCH] tests/pm_rpm: add dpms-mode-unset{, -non}-lpsp subtests

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

These tests currently trigger WARNs on our Kernel. Let's make sure we
fix the bug and it never comes back.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/pm_rpm.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 323e072..1e63bc8 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1390,6 +1390,23 @@ static void system_suspend_subtest(void)
 	igt_assert(wait_for_suspended());
 }
 
+/* Enable a screen, activate DPMS, then do a modeset. At some point our driver
+ * produced WARNs on this case. */
+static void dpms_mode_unset_subtest(enum screen_type type)
+{
+	disable_all_screens(&ms_data);
+	igt_assert(wait_for_suspended());
+
+	igt_require(enable_one_screen_with_type(&ms_data, type));
+	igt_assert(wait_for_active());
+
+	disable_all_screens_dpms(&ms_data);
+	igt_assert(wait_for_suspended());
+
+	disable_all_screens(&ms_data);
+	igt_assert(wait_for_suspended());
+}
+
 int main(int argc, char *argv[])
 {
 	int rounds = 50;
@@ -1462,6 +1479,10 @@ int main(int argc, char *argv[])
 		debugfs_forcewake_user_subtest();
 	igt_subtest("sysfs-read")
 		sysfs_read_subtest();
+	igt_subtest("dpms-mode-unset-lpsp")
+		dpms_mode_unset_subtest(SCREEN_TYPE_LPSP);
+	igt_subtest("dpms-mode-unset-non-lpsp")
+		dpms_mode_unset_subtest(SCREEN_TYPE_NON_LPSP);
 
 	/* Modeset stress */
 	igt_subtest("modeset-lpsp-stress")
-- 
2.0.0

Re: [PATCH] drm/i915: check the power wells on assert_{cursor, plane}

[Daniel Vetter]

On Wed, Jul 16, 2014 at 05:06:34PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Since we merged runtime PM support for DPMS, it is possible that these
> functions will be called when the power wells are disabled but a mode
> is "set", resulting in "failed assertion" and "device suspended while
> reading register" WARNs.
> 
> To reproduce the bug: disable all screens using mode unset, do a
> modeset on one screen, disable it using DPMS, then try to do a mode
> unset on it again to see the WARNs.
> 
> Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp
> Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hm, where do we call these asserts while the pipe is off? Do you have some
example backtraces at hand?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 54e3af9..7ad46e2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1216,7 +1216,9 @@ static void assert_cursor(struct drm_i915_private *dev_priv,
>  	struct drm_device *dev = dev_priv->dev;
>  	bool cur_state;
>  
> -	if (IS_845G(dev) || IS_I865G(dev))
> +	if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe)))
> +		cur_state = false;
> +	else if (IS_845G(dev) || IS_I865G(dev))
>  		cur_state = I915_READ(_CURACNTR) & CURSOR_ENABLE;
>  	else
>  		cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
> @@ -1262,9 +1264,13 @@ static void assert_plane(struct drm_i915_private *dev_priv,
>  	u32 val;
>  	bool cur_state;
>  
> -	reg = DSPCNTR(plane);
> -	val = I915_READ(reg);
> -	cur_state = !!(val & DISPLAY_PLANE_ENABLE);
> +	if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PIPE(plane))) {
> +		cur_state = false;
> +	} else {
> +		reg = DSPCNTR(plane);
> +		val = I915_READ(reg);
> +		cur_state = !!(val & DISPLAY_PLANE_ENABLE);
> +	}
>  	WARN(cur_state != state,
>  	     "plane %c assertion failure (expected %s, current %s)\n",
>  	     plane_name(plane), state_string(state), state_string(cur_state));
> -- 
> 2.0.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: check the power wells on assert_{cursor, plane}

[Paulo Zanoni]

2014-07-17 5:38 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Jul 16, 2014 at 05:06:34PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Since we merged runtime PM support for DPMS, it is possible that these
>> functions will be called when the power wells are disabled but a mode
>> is "set", resulting in "failed assertion" and "device suspended while
>> reading register" WARNs.
>>
>> To reproduce the bug: disable all screens using mode unset, do a
>> modeset on one screen, disable it using DPMS, then try to do a mode
>> unset on it again to see the WARNs.
>>
>> Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp
>> Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Hm, where do we call these asserts while the pipe is off? Do you have some
> example backtraces at hand?

Function __intel_set_mode() directly calls intel_crtc_disable(), which
calls both assert_plane_disabled() and assert_cursor_disabled().


> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 54e3af9..7ad46e2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -1216,7 +1216,9 @@ static void assert_cursor(struct drm_i915_private *dev_priv,
>>       struct drm_device *dev = dev_priv->dev;
>>       bool cur_state;
>>
>> -     if (IS_845G(dev) || IS_I865G(dev))
>> +     if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe)))
>> +             cur_state = false;
>> +     else if (IS_845G(dev) || IS_I865G(dev))
>>               cur_state = I915_READ(_CURACNTR) & CURSOR_ENABLE;
>>       else
>>               cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
>> @@ -1262,9 +1264,13 @@ static void assert_plane(struct drm_i915_private *dev_priv,
>>       u32 val;
>>       bool cur_state;
>>
>> -     reg = DSPCNTR(plane);
>> -     val = I915_READ(reg);
>> -     cur_state = !!(val & DISPLAY_PLANE_ENABLE);
>> +     if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PIPE(plane))) {
>> +             cur_state = false;
>> +     } else {
>> +             reg = DSPCNTR(plane);
>> +             val = I915_READ(reg);
>> +             cur_state = !!(val & DISPLAY_PLANE_ENABLE);
>> +     }
>>       WARN(cur_state != state,
>>            "plane %c assertion failure (expected %s, current %s)\n",
>>            plane_name(plane), state_string(state), state_string(cur_state));
>> --
>> 2.0.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: check the power wells on assert_{cursor, plane}

[Daniel Vetter]

On Thu, Jul 17, 2014 at 2:53 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2014-07-17 5:38 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
>> On Wed, Jul 16, 2014 at 05:06:34PM -0300, Paulo Zanoni wrote:
>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>
>>> Since we merged runtime PM support for DPMS, it is possible that these
>>> functions will be called when the power wells are disabled but a mode
>>> is "set", resulting in "failed assertion" and "device suspended while
>>> reading register" WARNs.
>>>
>>> To reproduce the bug: disable all screens using mode unset, do a
>>> modeset on one screen, disable it using DPMS, then try to do a mode
>>> unset on it again to see the WARNs.
>>>
>>> Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp
>>> Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp
>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Hm, where do we call these asserts while the pipe is off? Do you have some
>> example backtraces at hand?
>
> Function __intel_set_mode() directly calls intel_crtc_disable(), which
> calls both assert_plane_disabled() and assert_cursor_disabled().

Hm, I think it makes more sense to drop the three asserts in there.
The modeset state checker will already notice when we've failed to
turn off the pipe. And we check cursors and plane state in the enable
sequence, too.

Since we use these asserts a lot to lock down the precise modeset
sequence I actually prefer if they're a bit dumb and don't check the
power wells.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: check the power wells on assert_{cursor, plane}

[Paulo Zanoni]

2014-07-17 10:23 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Jul 17, 2014 at 2:53 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> 2014-07-17 5:38 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
>>> On Wed, Jul 16, 2014 at 05:06:34PM -0300, Paulo Zanoni wrote:
>>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>
>>>> Since we merged runtime PM support for DPMS, it is possible that these
>>>> functions will be called when the power wells are disabled but a mode
>>>> is "set", resulting in "failed assertion" and "device suspended while
>>>> reading register" WARNs.
>>>>
>>>> To reproduce the bug: disable all screens using mode unset, do a
>>>> modeset on one screen, disable it using DPMS, then try to do a mode
>>>> unset on it again to see the WARNs.
>>>>
>>>> Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp
>>>> Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp
>>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>
>>> Hm, where do we call these asserts while the pipe is off? Do you have some
>>> example backtraces at hand?
>>
>> Function __intel_set_mode() directly calls intel_crtc_disable(), which
>> calls both assert_plane_disabled() and assert_cursor_disabled().
>
> Hm, I think it makes more sense to drop the three asserts in there.
> The modeset state checker will already notice when we've failed to
> turn off the pipe. And we check cursors and plane state in the enable
> sequence, too.
>
> Since we use these asserts a lot to lock down the precise modeset
> sequence I actually prefer if they're a bit dumb and don't check the
> power wells.

I can do this, but please notice that we already have
power-well-checking code in many of the other assertions on our
driver... And it will probably be just a matter of time since someone
starts using the assertions again on a place where the power well can
be disabled :)

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



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: check the power wells on assert_{cursor, plane}

[Daniel Vetter]

On Thu, Jul 17, 2014 at 3:29 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> I can do this, but please notice that we already have
> power-well-checking code in many of the other assertions on our
> driver... And it will probably be just a matter of time since someone
> starts using the assertions again on a place where the power well can
> be disabled :)

The only one I've found outside of the hw state readout code and error
capture code (and there we obviously need them) is in assert_pipe.
This was added in 693101618a4be. Tbh I wonder whether we could revert
that with the patch to ditch the assert from intel_crtc_disable?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: check the power wells on assert_{cursor, plane}

[Paulo Zanoni]

2014-07-17 13:58 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Jul 17, 2014 at 3:29 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> I can do this, but please notice that we already have
>> power-well-checking code in many of the other assertions on our
>> driver... And it will probably be just a matter of time since someone
>> starts using the assertions again on a place where the power well can
>> be disabled :)
>
> The only one I've found outside of the hw state readout code and error
> capture code (and there we obviously need them) is in assert_pipe.
> This was added in 693101618a4be. Tbh I wonder whether we could revert
> that with the patch to ditch the assert from intel_crtc_disable?

I was talking about all that hw state readout code (which are also
used as assertions) and basically every single caller of
intel_display_power_enabled().

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



-- 
Paulo Zanoni

[PATCH] drm/i915: remove plane/cursor/pipe assertions from intel_crtc_disable

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Since we merged runtime PM support for DPMS, it is possible that these
assertions will be called when the power wells are disabled but a mode
is "set", resulting in "failed assertion" and "device suspended while
reading register" WARNs.

To reproduce the bug: disable all screens using mode unset, do a
modeset on one screen, disable it using DPMS, then try to do a mode
unset on it again to see the WARNs.

v2: The first version of this patch changed the assertions to also
check the power domains. Daniel suggested that it would be better to
just remove the assertions: "The modeset state checker
will already notice when we've failed to turn off the pipe. And we
check cursors and plane state in the enable sequence, too. Since we
use these asserts a lot to lock down the precise modeset sequence I
actually prefer if they're a bit dumb and don't check the power
wells."

Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp
Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cb40a94..f4ef59e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4930,10 +4930,6 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc_update_sarea(crtc, false);
 	dev_priv->display.off(crtc);
 
-	assert_plane_disabled(dev->dev_private, to_intel_crtc(crtc)->plane);
-	assert_cursor_disabled(dev_priv, pipe);
-	assert_pipe_disabled(dev->dev_private, pipe);
-
 	if (crtc->primary->fb) {
 		mutex_lock(&dev->struct_mutex);
 		intel_unpin_fb_obj(old_obj);
-- 
2.0.0

Re: [PATCH] drm/i915: remove plane/cursor/pipe assertions from intel_crtc_disable

[Daniel Vetter]

On Thu, Jul 17, 2014 at 11:16:39AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Since we merged runtime PM support for DPMS, it is possible that these
> assertions will be called when the power wells are disabled but a mode
> is "set", resulting in "failed assertion" and "device suspended while
> reading register" WARNs.
> 
> To reproduce the bug: disable all screens using mode unset, do a
> modeset on one screen, disable it using DPMS, then try to do a mode
> unset on it again to see the WARNs.
> 
> v2: The first version of this patch changed the assertions to also
> check the power domains. Daniel suggested that it would be better to
> just remove the assertions: "The modeset state checker
> will already notice when we've failed to turn off the pipe. And we
> check cursors and plane state in the enable sequence, too. Since we
> use these asserts a lot to lock down the precise modeset sequence I
> actually prefer if they're a bit dumb and don't check the power
> wells."
> 
> Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp
> Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch