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

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

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