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

[Matt Roper]

On Wed, Jan 06, 2016 at 11:38:32AM +0100, Maarten Lankhorst wrote:
> 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.

My original reasoning for the wm_mutex was to ensure that once we allow
post-vblank operations to happen via a workqueue that they don't race
with the 'initial' step of a subsequent commit.  We don't actually have
that async postvbl support yet, so you're right that the mutex doesn't
really protect anything yet.

But you're right that we should move the mutex to dev_priv instead of
having it per-crtc; I guess the other case we need to worry about (which
my patch isn't currently protecting) is two racing atomic commits
against two disjoint sets of CRTC's.  In that case the
ilk_write_wm_values() needs to happen under under a lock.  I'll send an
update shortly with this change.

> 
> 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?

I don't think so; most of the watermark logic has several different
methods of calculating various watermark values; the decision for which
method is supposed to be used is dependent on the global state.  For
example, turning on a sprite on CRTC 1 could change the method that
needs to be used to calculate the values of CRTC 2.


Matt

> >  
> > -	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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795