On Mon, 24 Mar 2014, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Mon, Mar 24, 2014 at 10:22:59AM +0200, Jani Nikula wrote: >> On Mon, 24 Mar 2014, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote: >> > Since >> > >> > commit 5c673b60a9b3b23486f4eda75c72e91d31d26a2b >> > Author: Daniel Vetter <daniel.vetter@xxxxxxxx> >> > 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@xxxxxxxxxxxxxxx> >> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76150 >> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx> >> > --- >> > 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@xxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
