[PATCH 0/2] drm/i915: HSW modeset hang fix v2

[PATCH 0/2] drm/i915: HSW modeset hang fix v2

[ville.syrjala]

v2 as requested.

The first patch just got rebased, the second one I adjusted a bit.

[PATCH v2 1/2] drm/i915: do not update cursor in crtc mode set

[ville.syrjala]

From: Jani Nikula <jani.nikula@intel.com>

The cursor is disabled before crtc mode set in crtc disable (and we
assert this is the case), and enabled afterwards in crtc enable. Do not
update it in crtc mode set.

On HSW enabling a plane on a disabled pipe may hang the entire system.
And there's no good reason for doing it ever, so just don't.

v2: Add note about HSW hangs - vsyrjala

Cc: stable@vger.kernel.org
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bb3bcc7..b4f8f71 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4932,9 +4932,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 		}
 	}
 
-	/* Ensure that the cursor is valid for the new mode before changing... */
-	intel_crtc_update_cursor(crtc, true);
-
 	if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {
 		/*
 		 * Ensure we match the reduced clock's P to the target clock.
@@ -5845,9 +5842,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		intel_crtc->config.dpll.p2 = clock.p2;
 	}
 
-	/* Ensure that the cursor is valid for the new mode before changing... */
-	intel_crtc_update_cursor(crtc, true);
-
 	/* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
 	if (intel_crtc->config.has_pch_encoder) {
 		fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
@@ -6398,9 +6392,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	if (!intel_ddi_pll_mode_set(crtc))
 		return -EINVAL;
 
-	/* Ensure that the cursor is valid for the new mode before changing... */
-	intel_crtc_update_cursor(crtc, true);
-
 	if (intel_crtc->config.has_dp_encoder)
 		intel_dp_set_m_n(intel_crtc);
 
-- 
1.8.1.5

[PATCH v2 2/2] drm/i915: Don't enable the cursor on a disable pipe

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On HSW enabling a plane on a disabled pipe may hang the entire system.
And there's no good reason for doing it ever, so just don't.

v2: Move the crtc active checks to intel_crtc_cursor_{set,move} to
    avoid confusing people during modeset

Cc: stable@vger.kernel.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b4f8f71..8f485e4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7066,7 +7066,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 	intel_crtc->cursor_width = width;
 	intel_crtc->cursor_height = height;
 
-	intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
+	if (intel_crtc->active)
+		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
 
 	return 0;
 fail_unpin:
@@ -7085,7 +7086,8 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 	intel_crtc->cursor_x = x;
 	intel_crtc->cursor_y = y;
 
-	intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
+	if (intel_crtc->active)
+		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
 
 	return 0;
 }
-- 
1.8.1.5

Re: [PATCH 0/2] drm/i915: HSW modeset hang fix v2

[Jani Nikula]

On Tue, 17 Sep 2013, ville.syrjala@linux.intel.com wrote:
> v2 as requested.
>
> The first patch just got rebased, the second one I adjusted a bit.

[dropping cc: stable]

So these two are for -fixes, right?

Unrelated to any hangs, there's still [1] that's destined to dinq, but
depends on patch 1/2 from this series. This is because Ville lifted
patch 1/2 from my original series of three, when I didn't know it would
fix a hang... Daniel, have fun. ;)

Cheers,
Jani.


[1] http://mid.gmane.org/1379059389-2890-2-git-send-email-jani.nikula@intel.com

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Don't enable the cursor on a disable pipe

[Chris Wilson]

On Tue, Sep 17, 2013 at 06:33:44PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On HSW enabling a plane on a disabled pipe may hang the entire system.
> And there's no good reason for doing it ever, so just don't.
> 
> v2: Move the crtc active checks to intel_crtc_cursor_{set,move} to
>     avoid confusing people during modeset

But outside of modeset the existing checks are accurate.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Don't enable the cursor on a disable pipe

[Ville Syrjälä]

On Tue, Sep 17, 2013 at 05:24:01PM +0100, Chris Wilson wrote:
> On Tue, Sep 17, 2013 at 06:33:44PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On HSW enabling a plane on a disabled pipe may hang the entire system.
> > And there's no good reason for doing it ever, so just don't.
> > 
> > v2: Move the crtc active checks to intel_crtc_cursor_{set,move} to
> >     avoid confusing people during modeset
> 
> But outside of modeset the existing checks are accurate.

There are no existing checks anymore. The crtc->enabled check ended up
as collateral damage in the cursor visibility patches.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 0/2] drm/i915: HSW modeset hang fix v2

[Paulo Zanoni]

2013/9/17  <ville.syrjala@linux.intel.com>:
> v2 as requested.
>
> The first patch just got rebased, the second one I adjusted a bit.

I've been testing this for the last 5 minutes and it seems the problem
is fixed. Now instead of a hard hang I get a FIFO underrun :)

Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I'll keep using these patches (because this machine is almost unusable
without them) and I'll report if I see the hang again. I'll also start
investigating the underrun.

Thanks,
Paulo

> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Don't enable the cursor on a disable pipe

[Paulo Zanoni]

2013/9/17 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Tue, Sep 17, 2013 at 05:24:01PM +0100, Chris Wilson wrote:
>> On Tue, Sep 17, 2013 at 06:33:44PM +0300, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > On HSW enabling a plane on a disabled pipe may hang the entire system.
>> > And there's no good reason for doing it ever, so just don't.
>> >
>> > v2: Move the crtc active checks to intel_crtc_cursor_{set,move} to
>> >     avoid confusing people during modeset
>>
>> But outside of modeset the existing checks are accurate.
>
> There are no existing checks anymore. The crtc->enabled check ended up
> as collateral damage in the cursor visibility patches.

I'm not really an expert in the cursor code, but as far as I could
understand, this patch looks correct.
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

(I propose a "Weak-reviewed-by:" tag for cases like this!)

Also, isn't this one of those cases where we can try to write an IGT
test? (unset all modes, call the cursor IOCTLs)

>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 0/2] drm/i915: HSW modeset hang fix v2

[Daniel Vetter]

On Tue, Sep 17, 2013 at 07:07:45PM +0300, Jani Nikula wrote:
> On Tue, 17 Sep 2013, ville.syrjala@linux.intel.com wrote:
> > v2 as requested.
> >
> > The first patch just got rebased, the second one I adjusted a bit.
> 
> [dropping cc: stable]
> 
> So these two are for -fixes, right?
> 
> Unrelated to any hangs, there's still [1] that's destined to dinq, but
> depends on patch 1/2 from this series. This is because Ville lifted
> patch 1/2 from my original series of three, when I didn't know it would
> fix a hang... Daniel, have fun. ;)

I'll do a backmerge when -rc2 is out or so. Both patches merged to -fixes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch