Re: [PATCH] drm/i915: Fix initial pipe underrun state tracking

[Jani Nikula <jani.nikula@intel.com>]

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