[Intel-gfx] [PATCH] drm/i915: Fix initial pipe underrun state tracking

[Daniel Vetter]

On Mon, Mar 24, 2014 at 02:05:37PM +0200, Jani Nikula wrote:
> On Mon, 24 Mar 2014, Daniel Vetter <daniel at 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 at ffwll.ch> wrote:
> >> > Since
> >> >
> >> > commit 5c673b60a9b3b23486f4eda75c72e91d31d26a2b
> >> > Author: Daniel Vetter <daniel.vetter at 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 at linux.intel.com>
> >> > Cc: Jani Nikula <jani.nikula at intel.com>
> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76150
> >> > Signed-off-by: Daniel Vetter <daniel.vetter at 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 at intel.com>

Thanks for your review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch