On Mon, Mar 24, 2014 at 02:05:37PM +0200, Jani Nikula wrote: > 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. ;) 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@xxxxxxxxx> Thanks for your review. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
