| Message ID | 1370432073-27634-4-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 05, 2013 at 01:34:05PM +0200, Daniel Vetter wrote: > Before I start to make a complete mess out of this, crank up > the paranoia level a bit. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 56fb6ed..39e977f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1440,6 +1440,7 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc) > } > > assert_pch_pll_enabled(dev_priv, pll, NULL); > + WARN_ON(!pll->on); > if (--pll->active) > return; Maybe a WARN_ON(pll->on) near the end of ironlake_enable_pch_pll() too? > > @@ -3031,12 +3032,18 @@ static void intel_put_pch_pll(struct intel_crtc *intel_crtc) > if (pll == NULL) > return; > > + WARN_ON(!intel_crtc->config.has_pch_encoder); Doesn't that trigger if we switch directly from PCH to CPU eDP? > + > if (pll->refcount == 0) { > WARN(1, "bad PCH PLL refcount\n"); > return; > } > > - --pll->refcount; > + if (--pll->refcount == 0) { > + WARN_ON(pll->on); > + WARN_ON(pll->active); > + } > + > intel_crtc->pch_pll = NULL; > } > > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jun 07, 2013 at 07:32:56PM +0300, Ville Syrjälä wrote: > On Wed, Jun 05, 2013 at 01:34:05PM +0200, Daniel Vetter wrote: > > Before I start to make a complete mess out of this, crank up > > the paranoia level a bit. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/intel_display.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 56fb6ed..39e977f 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -1440,6 +1440,7 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc) > > } > > > > assert_pch_pll_enabled(dev_priv, pll, NULL); > > + WARN_ON(!pll->on); > > if (--pll->active) > > return; > > Maybe a WARN_ON(pll->on) near the end of ironlake_enable_pch_pll() too? At the very end we set on = true, and the only non-error early return (when the active refcount is > 0 to begin with) has alreay a WARN_ON(!pll->on). Shouldn't that be good enough? > > > > > @@ -3031,12 +3032,18 @@ static void intel_put_pch_pll(struct intel_crtc *intel_crtc) > > if (pll == NULL) > > return; > > > > + WARN_ON(!intel_crtc->config.has_pch_encoder); > > Doesn't that trigger if we switch directly from PCH to CPU eDP? I've missed this case in testing somehow, and it's indeed broken. I don't hit the WARN here, but that's just because I've broken the refcounting somewhere. At least I've got plenty of backtraces, so the level of paranoia seems to be correct ;-) I'll fix this up and resend. -Daniel > > > + > > if (pll->refcount == 0) { > > WARN(1, "bad PCH PLL refcount\n"); > > return; > > } > > > > - --pll->refcount; > > + if (--pll->refcount == 0) { > > + WARN_ON(pll->on); > > + WARN_ON(pll->active); > > + } > > + > > intel_crtc->pch_pll = NULL; > > } > > > > -- > > 1.7.11.7 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
On Fri, Jun 07, 2013 at 10:03:20PM +0200, Daniel Vetter wrote: > On Fri, Jun 07, 2013 at 07:32:56PM +0300, Ville Syrjälä wrote: > > On Wed, Jun 05, 2013 at 01:34:05PM +0200, Daniel Vetter wrote: > > > Before I start to make a complete mess out of this, crank up > > > the paranoia level a bit. > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 56fb6ed..39e977f 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -1440,6 +1440,7 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc) > > > } > > > > > > assert_pch_pll_enabled(dev_priv, pll, NULL); > > > + WARN_ON(!pll->on); > > > if (--pll->active) > > > return; > > > > Maybe a WARN_ON(pll->on) near the end of ironlake_enable_pch_pll() too? > > At the very end we set on = true, and the only non-error early return > (when the active refcount is > 0 to begin with) has alreay a > WARN_ON(!pll->on). Shouldn't that be good enough? Well I was just thinking that since we have this dual bookeeping w/ active and on, maybe we want to warn if things go out of sync. > > > > > > > > > @@ -3031,12 +3032,18 @@ static void intel_put_pch_pll(struct intel_crtc *intel_crtc) > > > if (pll == NULL) > > > return; > > > > > > + WARN_ON(!intel_crtc->config.has_pch_encoder); > > > > Doesn't that trigger if we switch directly from PCH to CPU eDP? > > I've missed this case in testing somehow, and it's indeed broken. I don't > hit the WARN here, but that's just because I've broken the refcounting > somewhere. > > At least I've got plenty of backtraces, so the level of paranoia seems to > be correct ;-) > > I'll fix this up and resend. > -Daniel > > > > > > + > > > if (pll->refcount == 0) { > > > WARN(1, "bad PCH PLL refcount\n"); > > > return; > > > } > > > > > > - --pll->refcount; > > > + if (--pll->refcount == 0) { > > > + WARN_ON(pll->on); > > > + WARN_ON(pll->active); > > > + } > > > + > > > intel_crtc->pch_pll = NULL; > > > } > > > > > > -- > > > 1.7.11.7 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Fri, Jun 07, 2013 at 11:46:08PM +0300, Ville Syrjälä wrote: > On Fri, Jun 07, 2013 at 10:03:20PM +0200, Daniel Vetter wrote: > > On Fri, Jun 07, 2013 at 07:32:56PM +0300, Ville Syrjälä wrote: > > > On Wed, Jun 05, 2013 at 01:34:05PM +0200, Daniel Vetter wrote: > > > > Before I start to make a complete mess out of this, crank up > > > > the paranoia level a bit. > > > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 9 ++++++++- > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index 56fb6ed..39e977f 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -1440,6 +1440,7 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc) > > > > } > > > > > > > > assert_pch_pll_enabled(dev_priv, pll, NULL); > > > > + WARN_ON(!pll->on); > > > > if (--pll->active) > > > > return; > > > > > > Maybe a WARN_ON(pll->on) near the end of ironlake_enable_pch_pll() too? > > > > At the very end we set on = true, and the only non-error early return > > (when the active refcount is > 0 to begin with) has alreay a > > WARN_ON(!pll->on). Shouldn't that be good enough? > > Well I was just thinking that since we have this dual bookeeping w/ > active and on, maybe we want to warn if things go out of sync. Now I'm confused. I've tried to explain why I think we already have full checking of pll->on in enable_shared_dpll ... Can you maybe show in a diff where you'd want to add more? -Daniel
On Fri, Jun 07, 2013 at 11:13:32PM +0200, Daniel Vetter wrote: > On Fri, Jun 07, 2013 at 11:46:08PM +0300, Ville Syrjälä wrote: > > On Fri, Jun 07, 2013 at 10:03:20PM +0200, Daniel Vetter wrote: > > > On Fri, Jun 07, 2013 at 07:32:56PM +0300, Ville Syrjälä wrote: > > > > On Wed, Jun 05, 2013 at 01:34:05PM +0200, Daniel Vetter wrote: > > > > > Before I start to make a complete mess out of this, crank up > > > > > the paranoia level a bit. > > > > > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_display.c | 9 ++++++++- > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > > index 56fb6ed..39e977f 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -1440,6 +1440,7 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc) > > > > > } > > > > > > > > > > assert_pch_pll_enabled(dev_priv, pll, NULL); > > > > > + WARN_ON(!pll->on); > > > > > if (--pll->active) > > > > > return; > > > > > > > > Maybe a WARN_ON(pll->on) near the end of ironlake_enable_pch_pll() too? > > > > > > At the very end we set on = true, and the only non-error early return > > > (when the active refcount is > 0 to begin with) has alreay a > > > WARN_ON(!pll->on). Shouldn't that be good enough? > > > > Well I was just thinking that since we have this dual bookeeping w/ > > active and on, maybe we want to warn if things go out of sync. > > Now I'm confused. I've tried to explain why I think we already have full > checking of pll->on in enable_shared_dpll ... Can you maybe show in a diff > where you'd want to add more? Something like this: if (pll->active++) { WARN_ON(!pll->on); assert_pch_pll_enabled(dev_priv, pll, NULL); return; } + WARN_ON(pll->on); and maybe also: + assert_pch_pll_disabled(dev_priv, pll, NULL); Or maybe just kill 'pll->on' as it seems totally redundant. Also maybe we could move most of the asserts and WARNs to some central location. Currently there are quite a few early return paths from the pll enable/disable functions, and I don't think we perform the same checks for all of the branches. So maybe we could just have one function that would cross-check pll->on, pll->active and the real hardware state. We could call said function just before and after enable/disable_pch_pll().
On Mon, Jun 10, 2013 at 12:11 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Jun 07, 2013 at 11:13:32PM +0200, Daniel Vetter wrote: >> On Fri, Jun 07, 2013 at 11:46:08PM +0300, Ville Syrjälä wrote: >> > On Fri, Jun 07, 2013 at 10:03:20PM +0200, Daniel Vetter wrote: >> > > On Fri, Jun 07, 2013 at 07:32:56PM +0300, Ville Syrjälä wrote: >> > > > On Wed, Jun 05, 2013 at 01:34:05PM +0200, Daniel Vetter wrote: >> > > > > Before I start to make a complete mess out of this, crank up >> > > > > the paranoia level a bit. >> > > > > >> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> > > > > --- >> > > > > drivers/gpu/drm/i915/intel_display.c | 9 ++++++++- >> > > > > 1 file changed, 8 insertions(+), 1 deletion(-) >> > > > > >> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> > > > > index 56fb6ed..39e977f 100644 >> > > > > --- a/drivers/gpu/drm/i915/intel_display.c >> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c >> > > > > @@ -1440,6 +1440,7 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc) >> > > > > } >> > > > > >> > > > > assert_pch_pll_enabled(dev_priv, pll, NULL); >> > > > > + WARN_ON(!pll->on); >> > > > > if (--pll->active) >> > > > > return; >> > > > >> > > > Maybe a WARN_ON(pll->on) near the end of ironlake_enable_pch_pll() too? >> > > >> > > At the very end we set on = true, and the only non-error early return >> > > (when the active refcount is > 0 to begin with) has alreay a >> > > WARN_ON(!pll->on). Shouldn't that be good enough? >> > >> > Well I was just thinking that since we have this dual bookeeping w/ >> > active and on, maybe we want to warn if things go out of sync. >> >> Now I'm confused. I've tried to explain why I think we already have full >> checking of pll->on in enable_shared_dpll ... Can you maybe show in a diff >> where you'd want to add more? > > Something like this: > > if (pll->active++) { > WARN_ON(!pll->on); > assert_pch_pll_enabled(dev_priv, pll, NULL); > return; > } > + WARN_ON(pll->on); That one's gonna misfire every time since we set pll->on = true only at the end of the function in this case. > and maybe also: > + assert_pch_pll_disabled(dev_priv, pll, NULL); This one should already be in the platform-specific pll->enable hook. It's added in "drm/i915: enable/disable hooks for shared dplls" > Or maybe just kill 'pll->on' as it seems totally redundant. Yeah, I've considered that but independently checking pll->on with the hw state and pll->active with the number of crtc using it looked neated. I guess we could rip out pll->on as a follow-up though. > Also maybe we could move most of the asserts and WARNs to some > central location. Currently there are quite a few early return paths > from the pll enable/disable functions, and I don't think we perform the > same checks for all of the branches. So maybe we could just have one > function that would cross-check pll->on, pll->active and the real hardware > state. We could call said function just before and after > enable/disable_pch_pll(). The totally paranoid hw state cross checker does that at the very end of each modeset. The checks in here are simply to assert a bunch of edge transitions. And like I've said, I think I pretty much have it all covered. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Jun 10, 2013 at 04:34:05PM +0200, Daniel Vetter wrote: > On Mon, Jun 10, 2013 at 12:11 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Fri, Jun 07, 2013 at 11:13:32PM +0200, Daniel Vetter wrote: > >> On Fri, Jun 07, 2013 at 11:46:08PM +0300, Ville Syrjälä wrote: > >> > On Fri, Jun 07, 2013 at 10:03:20PM +0200, Daniel Vetter wrote: > >> > > On Fri, Jun 07, 2013 at 07:32:56PM +0300, Ville Syrjälä wrote: > >> > > > On Wed, Jun 05, 2013 at 01:34:05PM +0200, Daniel Vetter wrote: > >> > > > > Before I start to make a complete mess out of this, crank up > >> > > > > the paranoia level a bit. > >> > > > > > >> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > > > > --- > >> > > > > drivers/gpu/drm/i915/intel_display.c | 9 ++++++++- > >> > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > >> > > > > > >> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> > > > > index 56fb6ed..39e977f 100644 > >> > > > > --- a/drivers/gpu/drm/i915/intel_display.c > >> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > >> > > > > @@ -1440,6 +1440,7 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc) > >> > > > > } > >> > > > > > >> > > > > assert_pch_pll_enabled(dev_priv, pll, NULL); > >> > > > > + WARN_ON(!pll->on); > >> > > > > if (--pll->active) > >> > > > > return; > >> > > > > >> > > > Maybe a WARN_ON(pll->on) near the end of ironlake_enable_pch_pll() too? > >> > > > >> > > At the very end we set on = true, and the only non-error early return > >> > > (when the active refcount is > 0 to begin with) has alreay a > >> > > WARN_ON(!pll->on). Shouldn't that be good enough? > >> > > >> > Well I was just thinking that since we have this dual bookeeping w/ > >> > active and on, maybe we want to warn if things go out of sync. > >> > >> Now I'm confused. I've tried to explain why I think we already have full > >> checking of pll->on in enable_shared_dpll ... Can you maybe show in a diff > >> where you'd want to add more? > > > > Something like this: > > > > if (pll->active++) { > > WARN_ON(!pll->on); > > assert_pch_pll_enabled(dev_priv, pll, NULL); > > return; > > } > > + WARN_ON(pll->on); > > That one's gonna misfire every time since we set pll->on = true only > at the end of the function in this case. > > > and maybe also: > > + assert_pch_pll_disabled(dev_priv, pll, NULL); > > This one should already be in the platform-specific pll->enable hook. > It's added in "drm/i915: enable/disable hooks for shared dplls" > > > Or maybe just kill 'pll->on' as it seems totally redundant. > > Yeah, I've considered that but independently checking pll->on with the > hw state and pll->active with the number of crtc using it looked > neated. I guess we could rip out pll->on as a follow-up though. > > > Also maybe we could move most of the asserts and WARNs to some > > central location. Currently there are quite a few early return paths > > from the pll enable/disable functions, and I don't think we perform the > > same checks for all of the branches. So maybe we could just have one > > function that would cross-check pll->on, pll->active and the real hardware > > state. We could call said function just before and after > > enable/disable_pch_pll(). > > The totally paranoid hw state cross checker does that at the very end > of each modeset. The checks in here are simply to assert a bunch of > edge transitions. And like I've said, I think I pretty much have it > all covered. Before we set pll->on to true, pll->on should be false, no?
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 56fb6ed..39e977f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1440,6 +1440,7 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc) } assert_pch_pll_enabled(dev_priv, pll, NULL); + WARN_ON(!pll->on); if (--pll->active) return; @@ -3031,12 +3032,18 @@ static void intel_put_pch_pll(struct intel_crtc *intel_crtc) if (pll == NULL) return; + WARN_ON(!intel_crtc->config.has_pch_encoder); + if (pll->refcount == 0) { WARN(1, "bad PCH PLL refcount\n"); return; } - --pll->refcount; + if (--pll->refcount == 0) { + WARN_ON(pll->on); + WARN_ON(pll->active); + } + intel_crtc->pch_pll = NULL; }
Before I start to make a complete mess out of this, crank up the paranoia level a bit. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)