Re: [PATCH 7/7] drm/i915: Add two-stage ILK-style watermark programming (v9)

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

Op 15-12-15 om 00:51 schreef Matt Roper:
> In addition to calculating final watermarks, let's also pre-calculate a
> set of intermediate watermark values at atomic check time.  These
> intermediate watermarks are a combination of the watermarks for the old
> state and the new state; they should satisfy the requirements of both
> states which means they can be programmed immediately when we commit the
> atomic state (without waiting for a vblank).  Once the vblank does
> happen, we can then re-program watermarks to the more optimal final
> value.
<snip>
> -static void ilk_update_wm(struct drm_crtc *crtc)
> +static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> -
> -	WARN_ON(cstate->base.active != intel_crtc->active);
> +	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
>  
> -	/*
> -	 * IVB workaround: must disable low power watermarks for at least
> -	 * one frame before enabling scaling.  LP watermarks can be re-enabled
> -	 * when scaling is disabled.
> -	 *
> -	 * WaCxSRDisabledForSpriteScaling:ivb
> -	 */
> -	if (cstate->disable_lp_wm) {
> -		ilk_disable_lp_wm(crtc->dev);
> -		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
> -	}
> +	mutex_lock(&intel_crtc->wm.wm_mutex);
> +	intel_crtc->wm.active.ilk = cstate->wm.intermediate;
> +	ilk_program_watermarks(dev_priv);
> +	mutex_unlock(&intel_crtc->wm.wm_mutex);
> +}
Do the locks even protect anything correctly? It seems to me like this mutex may
need to be in dev_priv.

ilk_program_watermarks seems to be depending on all crtc's wm state.

Not a criticism on this patch, but is there an way to hammer in per crtc only wm's without having to look at the exact details of the global wm state outside of modesets?
>  
> -	intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
> +static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
>  
> -	ilk_program_watermarks(cstate);
> +	mutex_lock(&intel_crtc->wm.wm_mutex);
> +	if (cstate->wm.need_postvbl_update) {
> +		intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
> +		ilk_program_watermarks(dev_priv);
> +		cstate->wm.need_postvbl_update = false;
You don't need to reset the flag here, it's already cleared when duplicating the state..

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx