* [PATCH] drm/i915: Fix initial pipe underrun state tracking
@ 2014-03-23 23:01 Daniel Vetter
2014-03-24 8:22 ` Jani Nikula
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2014-03-23 23:01 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Jani Nikula, Daniel Vetter
Since
commit 5c673b60a9b3b23486f4eda75c72e91d31d26a2b
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Fri Mar 7 20:34:46 2014 +0100
drm/i915: Don't enable display error interrupts from the start
we don't enable underrun interrupts any more at takeover time.
Unfortunately I've forgotten to also adjust the sw-side tracking.
Since the code assumes that disabled pipes have underrun reporting
enabled set the disable flag on all pipes which are active at takeover
time. Without this underrun reporting wasn't enabled correctly on the
first modeset. Note that for fastboot this is another piece of state
that needs to be fixed up by enabling the underrung reporting after
watermarks have beend fixed up.
On ivb/hsw an additional effect of this regression was that also all
cpu crc reporting stopped working since the master error interrupt it
shared across all pipes and sources.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76150
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_display.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7e4ea8d4e388..9b24ae4fb7bd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11502,6 +11502,14 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
encoder->base.crtc = NULL;
}
}
+ if (crtc->active) {
+ /*
+ * We start out with underrun reporting disabled to avoid races.
+ * For correct bookkeeping mark this on active crtcs.
+ */
+ crtc->cpu_fifo_underrun_disabled = true;
+ crtc->pch_fifo_underrun_disabled = true;
+ }
}
static void intel_sanitize_encoder(struct intel_encoder *encoder)
--
1.8.4.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: Fix initial pipe underrun state tracking 2014-03-23 23:01 [PATCH] drm/i915: Fix initial pipe underrun state tracking Daniel Vetter @ 2014-03-24 8:22 ` Jani Nikula 2014-03-24 9:28 ` Daniel Vetter 0 siblings, 1 reply; 5+ messages in thread From: Jani Nikula @ 2014-03-24 8:22 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter On Mon, 24 Mar 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Since > > commit 5c673b60a9b3b23486f4eda75c72e91d31d26a2b > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Fri Mar 7 20:34:46 2014 +0100 > > drm/i915: Don't enable display error interrupts from the start > > we don't enable underrun interrupts any more at takeover time. > Unfortunately I've forgotten to also adjust the sw-side tracking. > > Since the code assumes that disabled pipes have underrun reporting > enabled set the disable flag on all pipes which are active at takeover > time. Without this underrun reporting wasn't enabled correctly on the > first modeset. Note that for fastboot this is another piece of state > that needs to be fixed up by enabling the underrung reporting after > watermarks have beend fixed up. > > On ivb/hsw an additional effect of this regression was that also all > cpu crc reporting stopped working since the master error interrupt it > shared across all pipes and sources. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76150 > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 7e4ea8d4e388..9b24ae4fb7bd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11502,6 +11502,14 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > encoder->base.crtc = NULL; > } > } > + if (crtc->active) { > + /* > + * We start out with underrun reporting disabled to avoid races. > + * For correct bookkeeping mark this on active crtcs. > + */ Why only on active crtcs? The state remains in inactive crtcs. Won't the problem appear when such an crtc is actived? Quoth a comment in struct intel_crtc: /* Access to these should be protected by dev_priv->irq_lock. */ Either you need to hold the lock or explain in a comment why it's not necessary. BR, Jani. > + crtc->cpu_fifo_underrun_disabled = true; > + crtc->pch_fifo_underrun_disabled = true; > + } > } > > static void intel_sanitize_encoder(struct intel_encoder *encoder) > -- > 1.8.4.rc3 > -- 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] 5+ messages in thread
* Re: [PATCH] drm/i915: Fix initial pipe underrun state tracking 2014-03-24 8:22 ` Jani Nikula @ 2014-03-24 9:28 ` Daniel Vetter 2014-03-24 12:05 ` Jani Nikula 0 siblings, 1 reply; 5+ messages in thread From: Daniel Vetter @ 2014-03-24 9:28 UTC (permalink / raw) To: Jani Nikula; +Cc: Daniel Vetter, Intel Graphics Development On Mon, Mar 24, 2014 at 10:22:59AM +0200, Jani Nikula wrote: > On Mon, 24 Mar 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Since > > > > commit 5c673b60a9b3b23486f4eda75c72e91d31d26a2b > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > Date: Fri Mar 7 20:34:46 2014 +0100 > > > > drm/i915: Don't enable display error interrupts from the start > > > > we don't enable underrun interrupts any more at takeover time. > > Unfortunately I've forgotten to also adjust the sw-side tracking. > > > > Since the code assumes that disabled pipes have underrun reporting > > enabled set the disable flag on all pipes which are active at takeover > > time. Without this underrun reporting wasn't enabled correctly on the > > first modeset. Note that for fastboot this is another piece of state > > that needs to be fixed up by enabling the underrung reporting after > > watermarks have beend fixed up. > > > > On ivb/hsw an additional effect of this regression was that also all > > cpu crc reporting stopped working since the master error interrupt it > > shared across all pipes and sources. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76150 > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/intel_display.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 7e4ea8d4e388..9b24ae4fb7bd 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -11502,6 +11502,14 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > > encoder->base.crtc = NULL; > > } > > } > > + if (crtc->active) { > > + /* > > + * We start out with underrun reporting disabled to avoid races. > > + * For correct bookkeeping mark this on active crtcs. > > + */ > > Why only on active crtcs? The state remains in inactive crtcs. Won't the > problem appear when such an crtc is actived? Setting this for inactive crtcs breaks the code, which I've learned the hard way ;-) The issue is that on ivb/hsw we can _only_ enable the err interrupt if all crtcs allow it. But if some disabled crtc disallows it that condition is never true until userspace has lit up all for crtcs at least once. I've tried to capture this in the commit message, but seem to have failed. Can you please suggest a rewording to clarify things? > Quoth a comment in struct intel_crtc: > > /* Access to these should be protected by dev_priv->irq_lock. */ > > Either you need to hold the lock or explain in a comment why it's not > necessary. At worst the interrupt handler runs concurrently (on platforms where we don't yet hide fifo underruns from the start), also setting this to false in the case of an underrun. What about: "No protection against concurrent access is required - at wors a fifo underrun happens which also sets this to false." Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Fix initial pipe underrun state tracking 2014-03-24 9:28 ` Daniel Vetter @ 2014-03-24 12:05 ` Jani Nikula 2014-03-24 17:23 ` Daniel Vetter 0 siblings, 1 reply; 5+ messages in thread From: Jani Nikula @ 2014-03-24 12:05 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development On Mon, 24 Mar 2014, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Mar 24, 2014 at 10:22:59AM +0200, Jani Nikula wrote: >> On Mon, 24 Mar 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> > Since >> > >> > commit 5c673b60a9b3b23486f4eda75c72e91d31d26a2b >> > Author: Daniel Vetter <daniel.vetter@ffwll.ch> >> > Date: Fri Mar 7 20:34:46 2014 +0100 >> > >> > drm/i915: Don't enable display error interrupts from the start >> > >> > we don't enable underrun interrupts any more at takeover time. >> > Unfortunately I've forgotten to also adjust the sw-side tracking. >> > >> > Since the code assumes that disabled pipes have underrun reporting >> > enabled set the disable flag on all pipes which are active at takeover >> > time. Without this underrun reporting wasn't enabled correctly on the >> > first modeset. Note that for fastboot this is another piece of state >> > that needs to be fixed up by enabling the underrung reporting after >> > watermarks have beend fixed up. >> > >> > On ivb/hsw an additional effect of this regression was that also all >> > cpu crc reporting stopped working since the master error interrupt it >> > shared across all pipes and sources. >> > >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > Cc: Jani Nikula <jani.nikula@intel.com> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76150 >> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> > --- >> > drivers/gpu/drm/i915/intel_display.c | 8 ++++++++ >> > 1 file changed, 8 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> > index 7e4ea8d4e388..9b24ae4fb7bd 100644 >> > --- a/drivers/gpu/drm/i915/intel_display.c >> > +++ b/drivers/gpu/drm/i915/intel_display.c >> > @@ -11502,6 +11502,14 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) >> > encoder->base.crtc = NULL; >> > } >> > } >> > + if (crtc->active) { >> > + /* >> > + * We start out with underrun reporting disabled to avoid races. >> > + * For correct bookkeeping mark this on active crtcs. >> > + */ >> >> Why only on active crtcs? The state remains in inactive crtcs. Won't the >> problem appear when such an crtc is actived? > > Setting this for inactive crtcs breaks the code, which I've learned the > hard way ;-) The issue is that on ivb/hsw we can _only_ enable the err > interrupt if all crtcs allow it. But if some disabled crtc disallows it > that condition is never true until userspace has lit up all for crtcs at > least once. I've tried to capture this in the commit message, but seem to > have failed. Can you please suggest a rewording to clarify things? "Jani, please read the commit message" would clarify things. ;) > >> Quoth a comment in struct intel_crtc: >> >> /* Access to these should be protected by dev_priv->irq_lock. */ >> >> Either you need to hold the lock or explain in a comment why it's not >> necessary. > > At worst the interrupt handler runs concurrently (on platforms where we > don't yet hide fifo underruns from the start), also setting this to false > in the case of an underrun. What about: > > "No protection against concurrent access is required - at wors a fifo > underrun happens which also sets this to false." s/wors/worst/ Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > Thanks, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- 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] 5+ messages in thread
* Re: [PATCH] drm/i915: Fix initial pipe underrun state tracking 2014-03-24 12:05 ` Jani Nikula @ 2014-03-24 17:23 ` Daniel Vetter 0 siblings, 0 replies; 5+ messages in thread From: Daniel Vetter @ 2014-03-24 17:23 UTC (permalink / raw) To: Jani Nikula; +Cc: Daniel Vetter, Intel Graphics Development On Mon, Mar 24, 2014 at 02:05:37PM +0200, Jani Nikula wrote: > On Mon, 24 Mar 2014, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Mar 24, 2014 at 10:22:59AM +0200, Jani Nikula wrote: > >> On Mon, 24 Mar 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > >> > Since > >> > > >> > commit 5c673b60a9b3b23486f4eda75c72e91d31d26a2b > >> > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > Date: Fri Mar 7 20:34:46 2014 +0100 > >> > > >> > drm/i915: Don't enable display error interrupts from the start > >> > > >> > we don't enable underrun interrupts any more at takeover time. > >> > Unfortunately I've forgotten to also adjust the sw-side tracking. > >> > > >> > Since the code assumes that disabled pipes have underrun reporting > >> > enabled set the disable flag on all pipes which are active at takeover > >> > time. Without this underrun reporting wasn't enabled correctly on the > >> > first modeset. Note that for fastboot this is another piece of state > >> > that needs to be fixed up by enabling the underrung reporting after > >> > watermarks have beend fixed up. > >> > > >> > On ivb/hsw an additional effect of this regression was that also all > >> > cpu crc reporting stopped working since the master error interrupt it > >> > shared across all pipes and sources. > >> > > >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > Cc: Jani Nikula <jani.nikula@intel.com> > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76150 > >> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > --- > >> > drivers/gpu/drm/i915/intel_display.c | 8 ++++++++ > >> > 1 file changed, 8 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> > index 7e4ea8d4e388..9b24ae4fb7bd 100644 > >> > --- a/drivers/gpu/drm/i915/intel_display.c > >> > +++ b/drivers/gpu/drm/i915/intel_display.c > >> > @@ -11502,6 +11502,14 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > >> > encoder->base.crtc = NULL; > >> > } > >> > } > >> > + if (crtc->active) { > >> > + /* > >> > + * We start out with underrun reporting disabled to avoid races. > >> > + * For correct bookkeeping mark this on active crtcs. > >> > + */ > >> > >> Why only on active crtcs? The state remains in inactive crtcs. Won't the > >> problem appear when such an crtc is actived? > > > > Setting this for inactive crtcs breaks the code, which I've learned the > > hard way ;-) The issue is that on ivb/hsw we can _only_ enable the err > > interrupt if all crtcs allow it. But if some disabled crtc disallows it > > that condition is never true until userspace has lit up all for crtcs at > > least once. I've tried to capture this in the commit message, but seem to > > have failed. Can you please suggest a rewording to clarify things? > > "Jani, please read the commit message" would clarify things. ;) I've polished it a bit myself by adding an "only" to emphasis that only setting this for active crtc is indeed intentionally. > > > > >> Quoth a comment in struct intel_crtc: > >> > >> /* Access to these should be protected by dev_priv->irq_lock. */ > >> > >> Either you need to hold the lock or explain in a comment why it's not > >> necessary. > > > > At worst the interrupt handler runs concurrently (on platforms where we > > don't yet hide fifo underruns from the start), also setting this to false > > in the case of an underrun. What about: > > > > "No protection against concurrent access is required - at wors a fifo > > underrun happens which also sets this to false." > > s/wors/worst/ Fixed and comment augmented. > > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> Thanks for your review. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-03-24 17:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-03-23 23:01 [PATCH] drm/i915: Fix initial pipe underrun state tracking Daniel Vetter 2014-03-24 8:22 ` Jani Nikula 2014-03-24 9:28 ` Daniel Vetter 2014-03-24 12:05 ` Jani Nikula 2014-03-24 17:23 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.