* [PATCH 0/3] drm/i915: HSW modeset hang fix
@ 2013-09-12 19:45 ville.syrjala
2013-09-12 19:45 ` [PATCH 1/3] drm/i915: do not update cursor in crtc mode set ville.syrjala
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: ville.syrjala @ 2013-09-12 19:45 UTC (permalink / raw)
To: intel-gfx; +Cc: stable
As Paulo explained at [1], there's a modeset related hang occuring on HSW.
I bisected it down to removing a workaround meant for pre-production hardware,
and Paulo cooked up a workaround that worked for him. Unfortunately I was still
able to trigger the hangs by disabling all planes on pipe B but leaving the pipe
active, and the doing a modeset on pipe A.
The actual problem can occur when we enable any plane on a pipe that is not
fully running yet. When a pipe is considered running depends on a bit on the
type of encoder used. But in any case we should not really be enabling planes
on disabled pipes anyway, so implementing an actual workaround should not
even be necessary. We just need to be careful when we enable planes.
For me it seems Jani already fixed the problem when he proposed killing the cursor
enable calls from the .mode_set hooks. So it looks like our normal plane enabling
during .crtc_enable happens late enough for the pipe to be fully up.
I actually want to refactor the plane enabling/disabling during our
.crtc_{enable,disable} a bit. But as that doesn't seem necessary to prevent the
hangs, I'm posting these minimal fixes first, and then we can go crazy later.
[1] http://lists.freedesktop.org/archives/intel-gfx/2013-September/033040.html
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 1/3] drm/i915: do not update cursor in crtc mode set 2013-09-12 19:45 [PATCH 0/3] drm/i915: HSW modeset hang fix ville.syrjala @ 2013-09-12 19:45 ` ville.syrjala 2013-09-12 20:09 ` Chris Wilson 2013-09-12 19:45 ` [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe ville.syrjala 2013-09-12 19:45 ` [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe ville.syrjala 2 siblings, 1 reply; 22+ messages in thread From: ville.syrjala @ 2013-09-12 19:45 UTC (permalink / raw) To: intel-gfx; +Cc: stable 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 3c0e0cf..18043a2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4888,9 +4888,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. @@ -5780,9 +5777,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); @@ -6270,9 +6264,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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] drm/i915: do not update cursor in crtc mode set 2013-09-12 19:45 ` [PATCH 1/3] drm/i915: do not update cursor in crtc mode set ville.syrjala @ 2013-09-12 20:09 ` Chris Wilson 2013-09-13 7:30 ` [Intel-gfx] " Ville Syrjälä 0 siblings, 1 reply; 22+ messages in thread From: Chris Wilson @ 2013-09-12 20:09 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx, stable On Thu, Sep 12, 2013 at 10:45:41PM +0300, ville.syrjala@linux.intel.com wrote: > 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> Where did the asserts go? :) -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915: do not update cursor in crtc mode set 2013-09-12 20:09 ` Chris Wilson @ 2013-09-13 7:30 ` Ville Syrjälä 2013-09-13 8:03 ` [PATCH 1/2] drm/i915: add asserts for cursor disabled Jani Nikula 0 siblings, 1 reply; 22+ messages in thread From: Ville Syrjälä @ 2013-09-13 7:30 UTC (permalink / raw) To: Chris Wilson, intel-gfx, stable On Thu, Sep 12, 2013 at 09:09:05PM +0100, Chris Wilson wrote: > On Thu, Sep 12, 2013 at 10:45:41PM +0300, ville.syrjala@linux.intel.com wrote: > > 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> > > Where did the asserts go? :) I asked Jani to move them around a bit, which AFAIK he didn't yet. So I figured we'd just go with a minimal fix for stable first and then slap on more asserts later in dinq. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] drm/i915: add asserts for cursor disabled 2013-09-13 7:30 ` [Intel-gfx] " Ville Syrjälä @ 2013-09-13 8:03 ` Jani Nikula 2013-09-13 8:03 ` [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling Jani Nikula ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Jani Nikula @ 2013-09-13 8:03 UTC (permalink / raw) To: intel-gfx The cursor is supposed to be disabled during crtc mode set (disabled by ctrc disable). Assert this is the case. v2: move cursor disabled assert next to plane asserts (Ville) Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d0137b6..0446dc7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1069,6 +1069,26 @@ static void assert_panel_unlocked(struct drm_i915_private *dev_priv, pipe_name(pipe)); } +static void assert_cursor(struct drm_i915_private *dev_priv, + enum pipe pipe, bool state) +{ + struct drm_device *dev = dev_priv->dev; + bool cur_state; + + if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) + cur_state = I915_READ(CURCNTR_IVB(pipe)) & CURSOR_MODE; + else if (IS_845G(dev) || IS_I865G(dev)) + cur_state = I915_READ(_CURACNTR) & CURSOR_ENABLE; + else + cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE; + + WARN(cur_state != state, + "cursor on pipe %c assertion failure (expected %s, current %s)\n", + pipe_name(pipe), state_string(state), state_string(cur_state)); +} +#define assert_cursor_enabled(d, p) assert_cursor(d, p, true) +#define assert_cursor_disabled(d, p) assert_cursor(d, p, false) + void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, bool state) { @@ -1670,6 +1690,7 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, u32 val; assert_planes_disabled(dev_priv, pipe); + assert_cursor_disabled(dev_priv, pipe); assert_sprites_disabled(dev_priv, pipe); if (HAS_PCH_LPT(dev_priv->dev)) @@ -1731,6 +1752,7 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv, * or we might hang the display. */ assert_planes_disabled(dev_priv, pipe); + assert_cursor_disabled(dev_priv, pipe); assert_sprites_disabled(dev_priv, pipe); /* Don't disable pipe A or pipe A PLLs if needed */ @@ -3867,6 +3889,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc) dev_priv->display.off(crtc); assert_plane_disabled(dev->dev_private, to_intel_crtc(crtc)->plane); + assert_cursor_disabled(dev_priv, to_intel_crtc(crtc)->pipe); assert_pipe_disabled(dev->dev_private, to_intel_crtc(crtc)->pipe); if (crtc->fb) { -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling 2013-09-13 8:03 ` [PATCH 1/2] drm/i915: add asserts for cursor disabled Jani Nikula @ 2013-09-13 8:03 ` Jani Nikula 2013-09-13 13:00 ` Daniel Vetter 2013-09-13 8:48 ` [PATCH 1/2] drm/i915: add asserts for cursor disabled Chris Wilson 2013-09-13 11:57 ` Ville Syrjälä 2 siblings, 1 reply; 22+ messages in thread From: Jani Nikula @ 2013-09-13 8:03 UTC (permalink / raw) To: intel-gfx Flat out skip anything to do with PLL if we have a DSI encoder (and thus DSI PLL). Also skip PLL computation if the encoder has already set clocks. This allows for some tidying up of the code, including a superfluous call to intel_limit() for LVDS downclock path. Signed-off-by: Jani Nikula <jani.nikula@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++----------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0446dc7..6a51cc2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, num_connectors++; } - refclk = i9xx_get_refclk(crtc, num_connectors); + if (is_dsi) + goto skip_dpll; + + if (!intel_crtc->config.clock_set) { + refclk = i9xx_get_refclk(crtc, num_connectors); - if (!is_dsi && !intel_crtc->config.clock_set) { /* * Returns a set of divisors for the desired target clock with * the given refclk, or FALSE. The returned values represent @@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, ok = dev_priv->display.find_dpll(limit, crtc, intel_crtc->config.port_clock, refclk, NULL, &clock); - if (!ok && !intel_crtc->config.clock_set) { + if (!ok) { DRM_ERROR("Couldn't find PLL settings for mode!\n"); return -EINVAL; } - } - if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) { - /* - * Ensure we match the reduced clock's P to the target clock. - * If the clocks don't match, we can't switch the display clock - * by using the FP0/FP1. In such case we will disable the LVDS - * downclock feature. - */ - limit = intel_limit(crtc, refclk); - has_reduced_clock = - dev_priv->display.find_dpll(limit, crtc, - dev_priv->lvds_downclock, - refclk, &clock, - &reduced_clock); - } - /* Compat-code for transition, will disappear. */ - if (!intel_crtc->config.clock_set) { + if (is_lvds && dev_priv->lvds_downclock_avail) { + /* + * Ensure we match the reduced clock's P to the target + * clock. If the clocks don't match, we can't switch + * the display clock by using the FP0/FP1. In such case + * we will disable the LVDS downclock feature. + */ + has_reduced_clock = + dev_priv->display.find_dpll(limit, crtc, + dev_priv->lvds_downclock, + refclk, &clock, + &reduced_clock); + } + /* Compat-code for transition, will disappear. */ intel_crtc->config.dpll.n = clock.n; intel_crtc->config.dpll.m1 = clock.m1; intel_crtc->config.dpll.m2 = clock.m2; @@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, has_reduced_clock ? &reduced_clock : NULL, num_connectors); } else if (IS_VALLEYVIEW(dev)) { - if (!is_dsi) - vlv_update_pll(intel_crtc); + vlv_update_pll(intel_crtc); } else { i9xx_update_pll(intel_crtc, has_reduced_clock ? &reduced_clock : NULL, num_connectors); } +skip_dpll: /* Set up the display plane register */ dspcntr = DISPPLANE_GAMMA_ENABLE; -- 1.7.9.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling 2013-09-13 8:03 ` [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling Jani Nikula @ 2013-09-13 13:00 ` Daniel Vetter 2013-09-13 13:38 ` Jani Nikula 0 siblings, 1 reply; 22+ messages in thread From: Daniel Vetter @ 2013-09-13 13:00 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote: > Flat out skip anything to do with PLL if we have a DSI encoder (and thus > DSI PLL). Also skip PLL computation if the encoder has already set > clocks. This allows for some tidying up of the code, including a > superfluous call to intel_limit() for LVDS downclock path. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Tried to merge this but the baseline seems to be off, at least wrt dinq. Do I miss some patches that I should apply first? -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++----------------- > 1 file changed, 22 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0446dc7..6a51cc2 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > num_connectors++; > } > > - refclk = i9xx_get_refclk(crtc, num_connectors); > + if (is_dsi) > + goto skip_dpll; > + > + if (!intel_crtc->config.clock_set) { > + refclk = i9xx_get_refclk(crtc, num_connectors); > > - if (!is_dsi && !intel_crtc->config.clock_set) { > /* > * Returns a set of divisors for the desired target clock with > * the given refclk, or FALSE. The returned values represent > @@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > ok = dev_priv->display.find_dpll(limit, crtc, > intel_crtc->config.port_clock, > refclk, NULL, &clock); > - if (!ok && !intel_crtc->config.clock_set) { > + if (!ok) { > DRM_ERROR("Couldn't find PLL settings for mode!\n"); > return -EINVAL; > } > - } > > - if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) { > - /* > - * Ensure we match the reduced clock's P to the target clock. > - * If the clocks don't match, we can't switch the display clock > - * by using the FP0/FP1. In such case we will disable the LVDS > - * downclock feature. > - */ > - limit = intel_limit(crtc, refclk); > - has_reduced_clock = > - dev_priv->display.find_dpll(limit, crtc, > - dev_priv->lvds_downclock, > - refclk, &clock, > - &reduced_clock); > - } > - /* Compat-code for transition, will disappear. */ > - if (!intel_crtc->config.clock_set) { > + if (is_lvds && dev_priv->lvds_downclock_avail) { > + /* > + * Ensure we match the reduced clock's P to the target > + * clock. If the clocks don't match, we can't switch > + * the display clock by using the FP0/FP1. In such case > + * we will disable the LVDS downclock feature. > + */ > + has_reduced_clock = > + dev_priv->display.find_dpll(limit, crtc, > + dev_priv->lvds_downclock, > + refclk, &clock, > + &reduced_clock); > + } > + /* Compat-code for transition, will disappear. */ > intel_crtc->config.dpll.n = clock.n; > intel_crtc->config.dpll.m1 = clock.m1; > intel_crtc->config.dpll.m2 = clock.m2; > @@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > has_reduced_clock ? &reduced_clock : NULL, > num_connectors); > } else if (IS_VALLEYVIEW(dev)) { > - if (!is_dsi) > - vlv_update_pll(intel_crtc); > + vlv_update_pll(intel_crtc); > } else { > i9xx_update_pll(intel_crtc, > has_reduced_clock ? &reduced_clock : NULL, > num_connectors); > } > > +skip_dpll: > /* Set up the display plane register */ > dspcntr = DISPPLANE_GAMMA_ENABLE; > > -- > 1.7.9.5 > > _______________________________________________ > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling 2013-09-13 13:00 ` Daniel Vetter @ 2013-09-13 13:38 ` Jani Nikula 2013-09-13 14:07 ` Daniel Vetter 0 siblings, 1 reply; 22+ messages in thread From: Jani Nikula @ 2013-09-13 13:38 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Fri, 13 Sep 2013, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote: >> Flat out skip anything to do with PLL if we have a DSI encoder (and thus >> DSI PLL). Also skip PLL computation if the encoder has already set >> clocks. This allows for some tidying up of the code, including a >> superfluous call to intel_limit() for LVDS downclock path. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Tried to merge this but the baseline seems to be off, at least wrt dinq. > Do I miss some patches that I should apply first? This one depends on "drm/i915: do not update cursor in crtc mode set" in this thread (my patch but with Ville's commit message amendmend). Did you push the assert patch before that? It's a good order. Jani. > -Daniel > >> --- >> drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++----------------- >> 1 file changed, 22 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 0446dc7..6a51cc2 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, >> num_connectors++; >> } >> >> - refclk = i9xx_get_refclk(crtc, num_connectors); >> + if (is_dsi) >> + goto skip_dpll; >> + >> + if (!intel_crtc->config.clock_set) { >> + refclk = i9xx_get_refclk(crtc, num_connectors); >> >> - if (!is_dsi && !intel_crtc->config.clock_set) { >> /* >> * Returns a set of divisors for the desired target clock with >> * the given refclk, or FALSE. The returned values represent >> @@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, >> ok = dev_priv->display.find_dpll(limit, crtc, >> intel_crtc->config.port_clock, >> refclk, NULL, &clock); >> - if (!ok && !intel_crtc->config.clock_set) { >> + if (!ok) { >> DRM_ERROR("Couldn't find PLL settings for mode!\n"); >> return -EINVAL; >> } >> - } >> >> - if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) { >> - /* >> - * Ensure we match the reduced clock's P to the target clock. >> - * If the clocks don't match, we can't switch the display clock >> - * by using the FP0/FP1. In such case we will disable the LVDS >> - * downclock feature. >> - */ >> - limit = intel_limit(crtc, refclk); >> - has_reduced_clock = >> - dev_priv->display.find_dpll(limit, crtc, >> - dev_priv->lvds_downclock, >> - refclk, &clock, >> - &reduced_clock); >> - } >> - /* Compat-code for transition, will disappear. */ >> - if (!intel_crtc->config.clock_set) { >> + if (is_lvds && dev_priv->lvds_downclock_avail) { >> + /* >> + * Ensure we match the reduced clock's P to the target >> + * clock. If the clocks don't match, we can't switch >> + * the display clock by using the FP0/FP1. In such case >> + * we will disable the LVDS downclock feature. >> + */ >> + has_reduced_clock = >> + dev_priv->display.find_dpll(limit, crtc, >> + dev_priv->lvds_downclock, >> + refclk, &clock, >> + &reduced_clock); >> + } >> + /* Compat-code for transition, will disappear. */ >> intel_crtc->config.dpll.n = clock.n; >> intel_crtc->config.dpll.m1 = clock.m1; >> intel_crtc->config.dpll.m2 = clock.m2; >> @@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, >> has_reduced_clock ? &reduced_clock : NULL, >> num_connectors); >> } else if (IS_VALLEYVIEW(dev)) { >> - if (!is_dsi) >> - vlv_update_pll(intel_crtc); >> + vlv_update_pll(intel_crtc); >> } else { >> i9xx_update_pll(intel_crtc, >> has_reduced_clock ? &reduced_clock : NULL, >> num_connectors); >> } >> >> +skip_dpll: >> /* Set up the display plane register */ >> dspcntr = DISPPLANE_GAMMA_ENABLE; >> >> -- >> 1.7.9.5 >> >> _______________________________________________ >> 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 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling 2013-09-13 13:38 ` Jani Nikula @ 2013-09-13 14:07 ` Daniel Vetter 2013-09-13 14:47 ` Ville Syrjälä 0 siblings, 1 reply; 22+ messages in thread From: Daniel Vetter @ 2013-09-13 14:07 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx On Fri, Sep 13, 2013 at 04:38:28PM +0300, Jani Nikula wrote: > On Fri, 13 Sep 2013, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote: > >> Flat out skip anything to do with PLL if we have a DSI encoder (and thus > >> DSI PLL). Also skip PLL computation if the encoder has already set > >> clocks. This allows for some tidying up of the code, including a > >> superfluous call to intel_limit() for LVDS downclock path. > >> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Tried to merge this but the baseline seems to be off, at least wrt dinq. > > Do I miss some patches that I should apply first? > > This one depends on "drm/i915: do not update cursor in crtc mode set" in > this thread (my patch but with Ville's commit message amendmend). Did > you push the assert patch before that? It's a good order. I've pushed the assert patch to dinq, the real fixes need to go to -fixes. Tbh I'm a bit confused about what to put where, so please scream ;-) For Ville's patches I'm waiting a bit for Paulo to take a look and ack them. -Daniel > > Jani. > > > > > -Daniel > > > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++----------------- > >> 1 file changed, 22 insertions(+), 22 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 0446dc7..6a51cc2 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > >> num_connectors++; > >> } > >> > >> - refclk = i9xx_get_refclk(crtc, num_connectors); > >> + if (is_dsi) > >> + goto skip_dpll; > >> + > >> + if (!intel_crtc->config.clock_set) { > >> + refclk = i9xx_get_refclk(crtc, num_connectors); > >> > >> - if (!is_dsi && !intel_crtc->config.clock_set) { > >> /* > >> * Returns a set of divisors for the desired target clock with > >> * the given refclk, or FALSE. The returned values represent > >> @@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > >> ok = dev_priv->display.find_dpll(limit, crtc, > >> intel_crtc->config.port_clock, > >> refclk, NULL, &clock); > >> - if (!ok && !intel_crtc->config.clock_set) { > >> + if (!ok) { > >> DRM_ERROR("Couldn't find PLL settings for mode!\n"); > >> return -EINVAL; > >> } > >> - } > >> > >> - if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) { > >> - /* > >> - * Ensure we match the reduced clock's P to the target clock. > >> - * If the clocks don't match, we can't switch the display clock > >> - * by using the FP0/FP1. In such case we will disable the LVDS > >> - * downclock feature. > >> - */ > >> - limit = intel_limit(crtc, refclk); > >> - has_reduced_clock = > >> - dev_priv->display.find_dpll(limit, crtc, > >> - dev_priv->lvds_downclock, > >> - refclk, &clock, > >> - &reduced_clock); > >> - } > >> - /* Compat-code for transition, will disappear. */ > >> - if (!intel_crtc->config.clock_set) { > >> + if (is_lvds && dev_priv->lvds_downclock_avail) { > >> + /* > >> + * Ensure we match the reduced clock's P to the target > >> + * clock. If the clocks don't match, we can't switch > >> + * the display clock by using the FP0/FP1. In such case > >> + * we will disable the LVDS downclock feature. > >> + */ > >> + has_reduced_clock = > >> + dev_priv->display.find_dpll(limit, crtc, > >> + dev_priv->lvds_downclock, > >> + refclk, &clock, > >> + &reduced_clock); > >> + } > >> + /* Compat-code for transition, will disappear. */ > >> intel_crtc->config.dpll.n = clock.n; > >> intel_crtc->config.dpll.m1 = clock.m1; > >> intel_crtc->config.dpll.m2 = clock.m2; > >> @@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > >> has_reduced_clock ? &reduced_clock : NULL, > >> num_connectors); > >> } else if (IS_VALLEYVIEW(dev)) { > >> - if (!is_dsi) > >> - vlv_update_pll(intel_crtc); > >> + vlv_update_pll(intel_crtc); > >> } else { > >> i9xx_update_pll(intel_crtc, > >> has_reduced_clock ? &reduced_clock : NULL, > >> num_connectors); > >> } > >> > >> +skip_dpll: > >> /* Set up the display plane register */ > >> dspcntr = DISPPLANE_GAMMA_ENABLE; > >> > >> -- > >> 1.7.9.5 > >> > >> _______________________________________________ > >> 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 > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling 2013-09-13 14:07 ` Daniel Vetter @ 2013-09-13 14:47 ` Ville Syrjälä 0 siblings, 0 replies; 22+ messages in thread From: Ville Syrjälä @ 2013-09-13 14:47 UTC (permalink / raw) To: Daniel Vetter; +Cc: Jani Nikula, intel-gfx On Fri, Sep 13, 2013 at 04:07:19PM +0200, Daniel Vetter wrote: > On Fri, Sep 13, 2013 at 04:38:28PM +0300, Jani Nikula wrote: > > On Fri, 13 Sep 2013, Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote: > > >> Flat out skip anything to do with PLL if we have a DSI encoder (and thus > > >> DSI PLL). Also skip PLL computation if the encoder has already set > > >> clocks. This allows for some tidying up of the code, including a > > >> superfluous call to intel_limit() for LVDS downclock path. > > >> > > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Tried to merge this but the baseline seems to be off, at least wrt dinq. > > > Do I miss some patches that I should apply first? > > > > This one depends on "drm/i915: do not update cursor in crtc mode set" in > > this thread (my patch but with Ville's commit message amendmend). Did > > you push the assert patch before that? It's a good order. > > I've pushed the assert patch to dinq, the real fixes need to go to -fixes. > Tbh I'm a bit confused about what to put where, so please scream ;-) > > For Ville's patches I'm waiting a bit for Paulo to take a look and ack > them. For stable we need just (assuming it also fixes Paulo's hangs): drm/i915: do not update cursor in crtc mode set On top of that we can add the cursor assert patch (not sure if we want that to go to stable): drm/i915: add asserts for cursor disabled I get the impression you applied the assert patch alone. That's probably not a very good idea since it will (or at least should) scream every time you do a modeset, unless the other patch is applied as well. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] drm/i915: add asserts for cursor disabled 2013-09-13 8:03 ` [PATCH 1/2] drm/i915: add asserts for cursor disabled Jani Nikula 2013-09-13 8:03 ` [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling Jani Nikula @ 2013-09-13 8:48 ` Chris Wilson 2013-09-13 11:57 ` Ville Syrjälä 2 siblings, 0 replies; 22+ messages in thread From: Chris Wilson @ 2013-09-13 8:48 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx On Fri, Sep 13, 2013 at 11:03:08AM +0300, Jani Nikula wrote: > The cursor is supposed to be disabled during crtc mode set (disabled by > ctrc disable). Assert this is the case. > > v2: move cursor disabled assert next to plane asserts (Ville) > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Lgtm. There's one extra place for paranoia that I spotted, in *_update_cursor() we can assert that the cursor in the expected visibility when we enter the function. That might be overkill for such a frequently called operation, so just limiting it to the change in visibility should be paranoid enough. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] drm/i915: add asserts for cursor disabled 2013-09-13 8:03 ` [PATCH 1/2] drm/i915: add asserts for cursor disabled Jani Nikula 2013-09-13 8:03 ` [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling Jani Nikula 2013-09-13 8:48 ` [PATCH 1/2] drm/i915: add asserts for cursor disabled Chris Wilson @ 2013-09-13 11:57 ` Ville Syrjälä 2013-09-13 12:59 ` Daniel Vetter 2 siblings, 1 reply; 22+ messages in thread From: Ville Syrjälä @ 2013-09-13 11:57 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx On Fri, Sep 13, 2013 at 11:03:08AM +0300, Jani Nikula wrote: > The cursor is supposed to be disabled during crtc mode set (disabled by > ctrc disable). Assert this is the case. > > v2: move cursor disabled assert next to plane asserts (Ville) > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index d0137b6..0446dc7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1069,6 +1069,26 @@ static void assert_panel_unlocked(struct drm_i915_private *dev_priv, > pipe_name(pipe)); > } > > +static void assert_cursor(struct drm_i915_private *dev_priv, > + enum pipe pipe, bool state) > +{ > + struct drm_device *dev = dev_priv->dev; > + bool cur_state; > + > + if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) > + cur_state = I915_READ(CURCNTR_IVB(pipe)) & CURSOR_MODE; > + else if (IS_845G(dev) || IS_I865G(dev)) > + cur_state = I915_READ(_CURACNTR) & CURSOR_ENABLE; > + else > + cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE; > + > + WARN(cur_state != state, > + "cursor on pipe %c assertion failure (expected %s, current %s)\n", > + pipe_name(pipe), state_string(state), state_string(cur_state)); > +} > +#define assert_cursor_enabled(d, p) assert_cursor(d, p, true) > +#define assert_cursor_disabled(d, p) assert_cursor(d, p, false) > + > void assert_pipe(struct drm_i915_private *dev_priv, > enum pipe pipe, bool state) > { > @@ -1670,6 +1690,7 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, > u32 val; > > assert_planes_disabled(dev_priv, pipe); > + assert_cursor_disabled(dev_priv, pipe); > assert_sprites_disabled(dev_priv, pipe); > > if (HAS_PCH_LPT(dev_priv->dev)) > @@ -1731,6 +1752,7 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv, > * or we might hang the display. > */ > assert_planes_disabled(dev_priv, pipe); > + assert_cursor_disabled(dev_priv, pipe); > assert_sprites_disabled(dev_priv, pipe); > > /* Don't disable pipe A or pipe A PLLs if needed */ > @@ -3867,6 +3889,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc) > dev_priv->display.off(crtc); > > assert_plane_disabled(dev->dev_private, to_intel_crtc(crtc)->plane); > + assert_cursor_disabled(dev_priv, to_intel_crtc(crtc)->pipe); > assert_pipe_disabled(dev->dev_private, to_intel_crtc(crtc)->pipe); > > if (crtc->fb) { > -- > 1.7.9.5 -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] drm/i915: add asserts for cursor disabled 2013-09-13 11:57 ` Ville Syrjälä @ 2013-09-13 12:59 ` Daniel Vetter 0 siblings, 0 replies; 22+ messages in thread From: Daniel Vetter @ 2013-09-13 12:59 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Jani Nikula, intel-gfx On Fri, Sep 13, 2013 at 02:57:37PM +0300, Ville Syrjälä wrote: > On Fri, Sep 13, 2013 at 11:03:08AM +0300, Jani Nikula wrote: > > The cursor is supposed to be disabled during crtc mode set (disabled by > > ctrc disable). Assert this is the case. > > > > v2: move cursor disabled assert next to plane asserts (Ville) > > > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe 2013-09-12 19:45 [PATCH 0/3] drm/i915: HSW modeset hang fix ville.syrjala 2013-09-12 19:45 ` [PATCH 1/3] drm/i915: do not update cursor in crtc mode set ville.syrjala @ 2013-09-12 19:45 ` ville.syrjala 2013-09-12 20:08 ` Chris Wilson 2013-09-12 19:45 ` [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe ville.syrjala 2 siblings, 1 reply; 22+ messages in thread From: ville.syrjala @ 2013-09-12 19:45 UTC (permalink / raw) To: intel-gfx; +Cc: stable 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. Cc: stable@vger.kernel.org Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 18043a2..d0137b6 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6793,6 +6793,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, u32 base, pos; bool visible; + if (!intel_crtc->active) + return; + pos = 0; if (on && crtc->enabled && crtc->fb) { -- 1.8.1.5 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe 2013-09-12 19:45 ` [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe ville.syrjala @ 2013-09-12 20:08 ` Chris Wilson 2013-09-13 7:28 ` [Intel-gfx] " Ville Syrjälä 0 siblings, 1 reply; 22+ messages in thread From: Chris Wilson @ 2013-09-12 20:08 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx, stable On Thu, Sep 12, 2013 at 10:45:42PM +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. > > Cc: stable@vger.kernel.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 18043a2..d0137b6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6793,6 +6793,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, > u32 base, pos; > bool visible; > > + if (!intel_crtc->active) > + return; This is misleading since we do expect to call this function whilst turning off the crtc. This check makes it appear that such calls might be wrong. Also the !crtc->enabled following intel_crtc->active makes ones question their sanity. So I feel this check detracts from readability of the function. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe 2013-09-12 20:08 ` Chris Wilson @ 2013-09-13 7:28 ` Ville Syrjälä 0 siblings, 0 replies; 22+ messages in thread From: Ville Syrjälä @ 2013-09-13 7:28 UTC (permalink / raw) To: Chris Wilson, intel-gfx, stable On Thu, Sep 12, 2013 at 09:08:17PM +0100, Chris Wilson wrote: > On Thu, Sep 12, 2013 at 10:45:42PM +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. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 18043a2..d0137b6 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -6793,6 +6793,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, > > u32 base, pos; > > bool visible; > > > > + if (!intel_crtc->active) > > + return; > > This is misleading since we do expect to call this function whilst > turning off the crtc. This check makes it appear that such calls might > be wrong. crtc->active is true until the pipe has been fully turned off. > Also the !crtc->enabled following intel_crtc->active makes > ones question their sanity. I actually forgot we had that there. If you recall I'm actually removing it in my cursor visibility fixes. But I didn't realize that we don't actually clear crtc->config to zero for inactive pipes, so I actually would have introduced a bug there. > So I feel this check detracts from readability of the function. I just wanted something minimal for stable. But since we have the ->enabled check there currently I think this patch could just be dropped as ->enabled should match ->active outside modeset operations, and during modeset operations we're covered by Jani's patch to remove the extra cursor calls. But I need to amend my earlier visibility fixes to add either ->active or ->enabled check back. Or I need to make it so that crtc->config gets cleared for disabled pipes. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe 2013-09-12 19:45 [PATCH 0/3] drm/i915: HSW modeset hang fix ville.syrjala 2013-09-12 19:45 ` [PATCH 1/3] drm/i915: do not update cursor in crtc mode set ville.syrjala 2013-09-12 19:45 ` [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe ville.syrjala @ 2013-09-12 19:45 ` ville.syrjala 2013-09-12 20:13 ` [Intel-gfx] " Chris Wilson 2 siblings, 1 reply; 22+ messages in thread From: ville.syrjala @ 2013-09-12 19:45 UTC (permalink / raw) To: intel-gfx; +Cc: stable 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. Cc: stable@vger.kernel.org Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_sprite.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index d9c7a66..4f11eb1 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, .y2 = crtc_y + crtc_h, }; const struct drm_rect clip = { - .x2 = crtc->mode.hdisplay, - .y2 = crtc->mode.vdisplay, + .x2 = intel_crtc->active ? crtc->mode.hdisplay : 0, + .y2 = intel_crtc->active ? crtc->mode.vdisplay : 0, }; intel_fb = to_intel_framebuffer(fb); -- 1.8.1.5 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe 2013-09-12 19:45 ` [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe ville.syrjala @ 2013-09-12 20:13 ` Chris Wilson 2013-09-13 7:40 ` Ville Syrjälä 0 siblings, 1 reply; 22+ messages in thread From: Chris Wilson @ 2013-09-12 20:13 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx, stable On Thu, Sep 12, 2013 at 10:45:43PM +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. > > Cc: stable@vger.kernel.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index d9c7a66..4f11eb1 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > .y2 = crtc_y + crtc_h, > }; > const struct drm_rect clip = { > - .x2 = crtc->mode.hdisplay, > - .y2 = crtc->mode.vdisplay, > + .x2 = intel_crtc->active ? crtc->mode.hdisplay : 0, > + .y2 = intel_crtc->active ? crtc->mode.vdisplay : 0, > }; Too much magic that looks like it would have interesting effects later in the function. This function should only be called on an active CRTC, so declare it so: if (WARN_ON(!intel_crtc->active)) return -EMONKEY; -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe 2013-09-12 20:13 ` [Intel-gfx] " Chris Wilson @ 2013-09-13 7:40 ` Ville Syrjälä 2013-09-13 7:50 ` Ville Syrjälä 0 siblings, 1 reply; 22+ messages in thread From: Ville Syrjälä @ 2013-09-13 7:40 UTC (permalink / raw) To: Chris Wilson, intel-gfx, stable On Thu, Sep 12, 2013 at 09:13:56PM +0100, Chris Wilson wrote: > On Thu, Sep 12, 2013 at 10:45:43PM +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. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_sprite.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index d9c7a66..4f11eb1 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > .y2 = crtc_y + crtc_h, > > }; > > const struct drm_rect clip = { > > - .x2 = crtc->mode.hdisplay, > > - .y2 = crtc->mode.vdisplay, > > + .x2 = intel_crtc->active ? crtc->mode.hdisplay : 0, > > + .y2 = intel_crtc->active ? crtc->mode.vdisplay : 0, > > }; > > Too much magic that looks like it would have interesting effects later > in the function. This function should only be called on an active CRTC, > so declare it so: > > if (WARN_ON(!intel_crtc->active)) > return -EMONKEY; I'm actually perfectly happy to let users set up planes on a disabled pipe. We allow it for cursors already, so why not all planes? Especially as cursors will be just drm_planes in the future. And actually we do have a PIPECONF_ENABLED check in there that I was going to remove but forgot. That already prevents setting up a plane when the pipe is off outside modeset operations. During modeset even if we add an ->active check we might be left with a race window when the pipe is not fully up and running yet. I think to catch those we just need to slap asserts at the appropriate place in modeset. So I gues we don't really need this patch in stable either. Hmm. fixing bugs is easier than I thought ;) -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe 2013-09-13 7:40 ` Ville Syrjälä @ 2013-09-13 7:50 ` Ville Syrjälä 2013-09-13 7:54 ` Ville Syrjälä 0 siblings, 1 reply; 22+ messages in thread From: Ville Syrjälä @ 2013-09-13 7:50 UTC (permalink / raw) To: Chris Wilson, intel-gfx, stable On Fri, Sep 13, 2013 at 10:40:16AM +0300, Ville Syrjälä wrote: > On Thu, Sep 12, 2013 at 09:13:56PM +0100, Chris Wilson wrote: > > On Thu, Sep 12, 2013 at 10:45:43PM +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. > > > > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_sprite.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > > index d9c7a66..4f11eb1 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > .y2 = crtc_y + crtc_h, > > > }; > > > const struct drm_rect clip = { > > > - .x2 = crtc->mode.hdisplay, > > > - .y2 = crtc->mode.vdisplay, > > > + .x2 = intel_crtc->active ? crtc->mode.hdisplay : 0, > > > + .y2 = intel_crtc->active ? crtc->mode.vdisplay : 0, > > > }; > > > > Too much magic that looks like it would have interesting effects later > > in the function. This function should only be called on an active CRTC, > > so declare it so: > > > > if (WARN_ON(!intel_crtc->active)) > > return -EMONKEY; > > I'm actually perfectly happy to let users set up planes on a disabled > pipe. We allow it for cursors already, so why not all planes? Especially > as cursors will be just drm_planes in the future. Oh and BTW the clipping/scaling stuff works perfectly well when stuff gets fully clipped. dst region will come out as zero, and visible will be false. That already happens when you simply move the plane outside the pipe src dimensions. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe 2013-09-13 7:50 ` Ville Syrjälä @ 2013-09-13 7:54 ` Ville Syrjälä 2013-09-13 8:43 ` Chris Wilson 0 siblings, 1 reply; 22+ messages in thread From: Ville Syrjälä @ 2013-09-13 7:54 UTC (permalink / raw) To: Chris Wilson, intel-gfx, stable On Fri, Sep 13, 2013 at 10:50:02AM +0300, Ville Syrjälä wrote: > On Fri, Sep 13, 2013 at 10:40:16AM +0300, Ville Syrjälä wrote: > > On Thu, Sep 12, 2013 at 09:13:56PM +0100, Chris Wilson wrote: > > > On Thu, Sep 12, 2013 at 10:45:43PM +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. > > > > > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_sprite.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > > > index d9c7a66..4f11eb1 100644 > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > > .y2 = crtc_y + crtc_h, > > > > }; > > > > const struct drm_rect clip = { > > > > - .x2 = crtc->mode.hdisplay, > > > > - .y2 = crtc->mode.vdisplay, > > > > + .x2 = intel_crtc->active ? crtc->mode.hdisplay : 0, > > > > + .y2 = intel_crtc->active ? crtc->mode.vdisplay : 0, > > > > }; > > > > > > Too much magic that looks like it would have interesting effects later > > > in the function. This function should only be called on an active CRTC, > > > so declare it so: > > > > > > if (WARN_ON(!intel_crtc->active)) > > > return -EMONKEY; > > > > I'm actually perfectly happy to let users set up planes on a disabled > > pipe. We allow it for cursors already, so why not all planes? Especially > > as cursors will be just drm_planes in the future. > > Oh and BTW the clipping/scaling stuff works perfectly well when stuff > gets fully clipped. dst region will come out as zero, and visible will > be false. That already happens when you simply move the plane outside > the pipe src dimensions. Hmm. Except the primary disable logic is fscked up in that case. Oh well, I already knew I have to fix that stuff up. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe 2013-09-13 7:54 ` Ville Syrjälä @ 2013-09-13 8:43 ` Chris Wilson 0 siblings, 0 replies; 22+ messages in thread From: Chris Wilson @ 2013-09-13 8:43 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, stable On Fri, Sep 13, 2013 at 10:54:45AM +0300, Ville Syrjälä wrote: > On Fri, Sep 13, 2013 at 10:50:02AM +0300, Ville Syrjälä wrote: > > On Fri, Sep 13, 2013 at 10:40:16AM +0300, Ville Syrjälä wrote: > > > On Thu, Sep 12, 2013 at 09:13:56PM +0100, Chris Wilson wrote: > > > > On Thu, Sep 12, 2013 at 10:45:43PM +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. > > > > > > > > > > Cc: stable@vger.kernel.org > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_sprite.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > > > > index d9c7a66..4f11eb1 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > > @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > > > .y2 = crtc_y + crtc_h, > > > > > }; > > > > > const struct drm_rect clip = { > > > > > - .x2 = crtc->mode.hdisplay, > > > > > - .y2 = crtc->mode.vdisplay, > > > > > + .x2 = intel_crtc->active ? crtc->mode.hdisplay : 0, > > > > > + .y2 = intel_crtc->active ? crtc->mode.vdisplay : 0, > > > > > }; > > > > > > > > Too much magic that looks like it would have interesting effects later > > > > in the function. This function should only be called on an active CRTC, > > > > so declare it so: > > > > > > > > if (WARN_ON(!intel_crtc->active)) > > > > return -EMONKEY; > > > > > > I'm actually perfectly happy to let users set up planes on a disabled > > > pipe. We allow it for cursors already, so why not all planes? Especially > > > as cursors will be just drm_planes in the future. > > > > Oh and BTW the clipping/scaling stuff works perfectly well when stuff > > gets fully clipped. dst region will come out as zero, and visible will > > be false. That already happens when you simply move the plane outside > > the pipe src dimensions. > > Hmm. Except the primary disable logic is fscked up in that case. Oh > well, I already knew I have to fix that stuff up. That's what caught my eye. I wasn't convinced it would always do the right thing and erroring out earlier was easier than thinking. :) -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2013-09-13 14:47 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-12 19:45 [PATCH 0/3] drm/i915: HSW modeset hang fix ville.syrjala 2013-09-12 19:45 ` [PATCH 1/3] drm/i915: do not update cursor in crtc mode set ville.syrjala 2013-09-12 20:09 ` Chris Wilson 2013-09-13 7:30 ` [Intel-gfx] " Ville Syrjälä 2013-09-13 8:03 ` [PATCH 1/2] drm/i915: add asserts for cursor disabled Jani Nikula 2013-09-13 8:03 ` [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling Jani Nikula 2013-09-13 13:00 ` Daniel Vetter 2013-09-13 13:38 ` Jani Nikula 2013-09-13 14:07 ` Daniel Vetter 2013-09-13 14:47 ` Ville Syrjälä 2013-09-13 8:48 ` [PATCH 1/2] drm/i915: add asserts for cursor disabled Chris Wilson 2013-09-13 11:57 ` Ville Syrjälä 2013-09-13 12:59 ` Daniel Vetter 2013-09-12 19:45 ` [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe ville.syrjala 2013-09-12 20:08 ` Chris Wilson 2013-09-13 7:28 ` [Intel-gfx] " Ville Syrjälä 2013-09-12 19:45 ` [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe ville.syrjala 2013-09-12 20:13 ` [Intel-gfx] " Chris Wilson 2013-09-13 7:40 ` Ville Syrjälä 2013-09-13 7:50 ` Ville Syrjälä 2013-09-13 7:54 ` Ville Syrjälä 2013-09-13 8:43 ` Chris Wilson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox