| Message ID | 1347307111-6887-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2012/9/10 Daniel Vetter <daniel.vetter@ffwll.ch>: > This has been added in > > commit de9a35abb3b343a25065449234e47a76c4f3454a > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Tue Jun 5 11:03:40 2012 +0200 > > drm/i915: assert that the IBX port transcoder select w/a is implemented > > Unfortunately I've failed to notice that these checks are not just > called for the port that is about to be disabled, but for all (which > makes sense for an assert ...), and the WARN missfired when disabling > another pipe than the one with the dp port. > > Hence also check whether the port is actually disabled. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54688 > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Looks fine: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@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 f26fb3f..b8e5a51 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1376,7 +1376,8 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv, > "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n", > reg, pipe_name(pipe)); > > - WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT), > + WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0 > + && (val & DP_PIPEB_SELECT), > "IBX PCH dp port still using transcoder B\n"); > } > > @@ -1388,7 +1389,8 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv, > "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n", > reg, pipe_name(pipe)); > > - WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT), > + WARN(HAS_PCH_IBX(dev_priv->dev) && (val & PORT_ENABLE) == 0 > + && (val & SDVO_PIPE_B_SELECT), > "IBX PCH hdmi port still using transcoder B\n"); > } > > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Sep 11, 2012 at 11:57:56AM -0300, Paulo Zanoni wrote: > 2012/9/10 Daniel Vetter <daniel.vetter@ffwll.ch>: > > This has been added in > > > > commit de9a35abb3b343a25065449234e47a76c4f3454a > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > Date: Tue Jun 5 11:03:40 2012 +0200 > > > > drm/i915: assert that the IBX port transcoder select w/a is implemented > > > > Unfortunately I've failed to notice that these checks are not just > > called for the port that is about to be disabled, but for all (which > > makes sense for an assert ...), and the WARN missfired when disabling > > another pipe than the one with the dp port. > > > > Hence also check whether the port is actually disabled. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54688 > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Looks fine: > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Picked up for -fixes, thanks for the review. -Daniel
On Mon, 10 Sep 2012 21:58:29 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > This has been added in > > commit de9a35abb3b343a25065449234e47a76c4f3454a > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Tue Jun 5 11:03:40 2012 +0200 > > drm/i915: assert that the IBX port transcoder select w/a is implemented > > Unfortunately I've failed to notice that these checks are not just > called for the port that is about to be disabled, but for all (which > makes sense for an assert ...), and the WARN missfired when disabling > another pipe than the one with the dp port. > > Hence also check whether the port is actually disabled. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54688 > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > 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 f26fb3f..b8e5a51 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1376,7 +1376,8 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv, > "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n", > reg, pipe_name(pipe)); > > - WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT), > + WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0 > + && (val & DP_PIPEB_SELECT), > "IBX PCH dp port still using transcoder B\n"); > } > > @@ -1388,7 +1389,8 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv, > "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n", > reg, pipe_name(pipe)); > > - WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT), > + WARN(HAS_PCH_IBX(dev_priv->dev) && (val & PORT_ENABLE) == 0 > + && (val & SDVO_PIPE_B_SELECT), > "IBX PCH hdmi port still using transcoder B\n"); > } > Won't these warn if the port is disabled rather than enabled? Shouldn't we be checking for (val & PORT_ENABLE) != 0 *and* pipe B is selected?
On Wed, Sep 12, 2012 at 08:20:12AM -0700, Jesse Barnes wrote: > On Mon, 10 Sep 2012 21:58:29 +0200 > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > This has been added in > > > > commit de9a35abb3b343a25065449234e47a76c4f3454a > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > Date: Tue Jun 5 11:03:40 2012 +0200 > > > > drm/i915: assert that the IBX port transcoder select w/a is implemented > > > > Unfortunately I've failed to notice that these checks are not just > > called for the port that is about to be disabled, but for all (which > > makes sense for an assert ...), and the WARN missfired when disabling > > another pipe than the one with the dp port. > > > > Hence also check whether the port is actually disabled. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54688 > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > 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 f26fb3f..b8e5a51 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -1376,7 +1376,8 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv, > > "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n", > > reg, pipe_name(pipe)); > > > > - WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT), > > + WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0 > > + && (val & DP_PIPEB_SELECT), > > "IBX PCH dp port still using transcoder B\n"); > > } > > > > @@ -1388,7 +1389,8 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv, > > "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n", > > reg, pipe_name(pipe)); > > > > - WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT), > > + WARN(HAS_PCH_IBX(dev_priv->dev) && (val & PORT_ENABLE) == 0 > > + && (val & SDVO_PIPE_B_SELECT), > > "IBX PCH hdmi port still using transcoder B\n"); > > } > > > > Won't these warn if the port is disabled rather than enabled? > Shouldn't we be checking for (val & PORT_ENABLE) != 0 *and* pipe B is > selected? Recap from our irc discussion: This is for an ibx workaround, where we can't let a disabled port stay on transcoder B (for it prevents the other encoder on the same port from reliably getting enabled). The "is this port properly disabled" check is above the diff context. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f26fb3f..b8e5a51 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1376,7 +1376,8 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv, "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n", reg, pipe_name(pipe)); - WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT), + WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0 + && (val & DP_PIPEB_SELECT), "IBX PCH dp port still using transcoder B\n"); } @@ -1388,7 +1389,8 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv, "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n", reg, pipe_name(pipe)); - WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT), + WARN(HAS_PCH_IBX(dev_priv->dev) && (val & PORT_ENABLE) == 0 + && (val & SDVO_PIPE_B_SELECT), "IBX PCH hdmi port still using transcoder B\n"); }
This has been added in commit de9a35abb3b343a25065449234e47a76c4f3454a Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Tue Jun 5 11:03:40 2012 +0200 drm/i915: assert that the IBX port transcoder select w/a is implemented Unfortunately I've failed to notice that these checks are not just called for the port that is about to be disabled, but for all (which makes sense for an assert ...), and the WARN missfired when disabling another pipe than the one with the dp port. Hence also check whether the port is actually disabled. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54688 Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)