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

[Maarten Lankhorst]

Op 24-02-16 om 02:24 schreef Matt Roper:
> On Tue, Feb 23, 2016 at 05:20:13PM -0800, Matt Roper wrote:
>> 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.
>>
>> v2: Significant rebasing/rewriting.
>>
>> v3:
>>  - Move 'need_postvbl_update' flag to CRTC state (Daniel)
>>  - Don't forget to check intermediate watermark values for validity
>>    (Maarten)
>>  - Don't due async watermark optimization; just do it at the end of the
>>    atomic transaction, after waiting for vblanks.  We do want it to be
>>    async eventually, but adding that now will cause more trouble for
>>    Maarten's in-progress work.  (Maarten)
>>  - Don't allocate space in crtc_state for intermediate watermarks on
>>    platforms that don't need it (gen9+).
>>  - Move WaCxSRDisabledForSpriteScaling:ivb into intel_begin_crtc_commit
>>    now that ilk_update_wm is gone.
>>
>> v4:
>>  - Add a wm_mutex to cover updates to intel_crtc->active and the
>>    need_postvbl_update flag.  Since we don't have async yet it isn't
>>    terribly important yet, but might as well add it now.
>>  - Change interface to program watermarks.  Platforms will now expose
>>    .initial_watermarks() and .optimize_watermarks() functions to do
>>    watermark programming.  These should lock wm_mutex, copy the
>>    appropriate state values into intel_crtc->active, and then call
>>    the internal program watermarks function.
>>
>> v5:
>>  - Skip intermediate watermark calculation/check during initial hardware
>>    readout since we don't trust the existing HW values (and don't have
>>    valid values of our own yet).
>>  - Don't try to call .optimize_watermarks() on platforms that don't have
>>    atomic watermarks yet.  (Maarten)
>>
>> v6:
>>  - Rebase
>>
>> v7:
>>  - Further rebase
>>
>> v8:
>>  - A few minor indentation and line length fixes
>>
>> v9:
>>  - Yet another rebase since Maarten's patches reworked a bunch of the
>>    code (wm_pre, wm_post, etc.) that this was previously based on.
>>
>> v10:
>>  - Move wm_mutex to dev_priv to protect against racing commits against
>>    disjoint CRTC sets. (Maarten)
>>  - Drop unnecessary clearing of cstate->wm.need_postvbl_update (Maarten)
>>
>> v11:
>>  - Now that we've moved to atomic watermark updates, make sure we call
>>    the proper function to program watermarks in
>>    {ironlake,haswell}_crtc_enable(); the failure to do so on the
>>    previous patch iteration led to us not actually programming the
>>    watermarks before turning on the CRTC, which was the cause of the
>>    underruns that the CI system was seeing.
>>  - Fix inverted logic for determining when to optimize watermarks.  We
>>    were needlessly optimizing when the intermediate/optimal values were
>>    the same (harmless), but not actually optimizing when they differed
>>    (also harmless, but wasteful from a power/bandwidth perspective).
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>> ---
> To assist with review, the non-rebasing changes in this iteration vs the
> last one are:
>
>> @@ -4925,7 +4960,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>>  	 */
>>  	intel_crtc_load_lut(crtc);
>>  
>> -	intel_update_watermarks(crtc);
>> +	dev_priv->display.initial_watermarks(intel_crtc->config);
>>  	intel_enable_pipe(intel_crtc);
>>  
>>  	if (intel_crtc->config->has_pch_encoder)
>> @@ -5024,7 +5059,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>>  	if (!intel_crtc->config->has_dsi_encoder)
>>  		intel_ddi_enable_transcoder_func(crtc);
>>  
>> -	intel_update_watermarks(crtc);
>> +	dev_priv->display.initial_watermarks(pipe_config);
>>  	intel_enable_pipe(intel_crtc);
>>  
>>  	if (intel_crtc->config->has_pch_encoder)
Are the intermediate watermarks identical to optimal watermarks during modeset?
> (both new additions to the patch)
>
> and:
>
>> +	/*
>> +	 * If our intermediate WM are identical to the final WM, then we can
>> +	 * omit the post-vblank programming; only update if it's different.
>> +	 */
>> +	if (memcmp(a, &newstate->wm.optimal.ilk, sizeof(*a)) == 0)
>> +		newstate->wm.need_postvbl_update = false;
> (replacement of a "!=" with "==")
Ah, good catch!

Lets just wait for CI before applying again