Re: [PATCH v5 3/6] drm/i915/skl: Update plane watermarks atomically during plane updates

[Matt Roper]

On Tue, Aug 02, 2016 at 02:52:51PM -0400, Lyude wrote:
> Thanks to Ville for suggesting this as a potential solution to pipe
> underruns on Skylake.
> 
> On Skylake all of the registers for configuring planes, including the
> registers for configuring their watermarks, are double buffered. New
> values written to them won't take effect until said registers are
> "armed", which is done by writing to the PLANE_SURF (or in the case of
> cursor planes, the CURBASE register) register.
> 
> With this in mind, up until now we've been updating watermarks on skl
> like this:
> 
>   non-modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - intel_pre_plane_update:
>         - intel_update_watermarks()
>      - {vblank happens; new watermarks + old plane values => underrun }
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
>         - end vblank evasion
>   }
> 
>   or
> 
>   modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - crtc_enable:
>         - intel_update_watermarks()
>      - {vblank happens; new watermarks + old plane values => underrun }
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
>         - end vblank evasion
>   }
> 
> Now we update watermarks atomically like this:
> 
>   non-modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - intel_pre_plane_update:
>         - intel_update_watermarks() (wm values aren't written yet)
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
>         - write new wm values
>         - end vblank evasion
>   }
> 
>   modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - crtc_enable:
>         - intel_update_watermarks() (actual wm values aren't written
>           yet)
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
> 	- write new wm values
>         - end vblank evasion
>   }
> 
> So this patch moves all of the watermark writes into the right place;
> inside of the vblank evasion where we update all of the registers for
> each plane. While this patch doesn't fix everything, it does allow us to
> update the watermark values in the way the hardware expects us to.
> 
> Changes since original patch series:
>  - Remove mutex_lock/mutex_unlock since they don't do anything and we're
>    not touching global state
>  - Move skl_write_cursor_wm/skl_write_plane_wm functions into
>    intel_pm.c, make externally visible
>  - Add skl_write_plane_wm calls to skl_update_plane
>  - Fix conditional for for loop in skl_write_plane_wm (level < max_level
>    should be level <= max_level)
>  - Make diagram in commit more accurate to what's actually happening
>  - Add Fixes:
> 
> Changes since v1:
>  - Use IS_GEN9() instead of IS_SKYLAKE() since these fixes apply to more
>    then just Skylake
>  - Update description to make it clear this patch doesn't fix everything
>  - Check if pipes were actually changed before writing watermarks
> 
> Changes since v2:
>  - Write PIPE_WM_LINETIME during vblank evasion
> 
> Changes since v3:
>  - Rebase against new SAGV patch changes
> 
> Changes since v4:
>  - Add a parameter to choose what skl_wm_values struct to use when
>    writing new plane watermarks
> 
> Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
> Signed-off-by: Lyude <cpaul@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  8 +++++
>  drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
>  drivers/gpu/drm/i915/intel_pm.c      | 58 ++++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++
>  4 files changed, 61 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 76ba79f..79d146c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2980,6 +2980,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl, stride_div, stride;
>  	u32 tile_height, plane_offset, plane_size;
> @@ -3031,6 +3032,9 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	intel_crtc->adjusted_x = x_offset;
>  	intel_crtc->adjusted_y = y_offset;
>  
> +	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> +	    skl_write_plane_wm(intel_crtc, wm, 0);
> +
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
>  	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> @@ -10243,9 +10247,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> +	if (IS_GEN9(dev_priv) && wm->dirty_pipes & drm_crtc_mask(crtc))
> +		skl_write_cursor_wm(intel_crtc, wm);
> +
>  	if (plane_state && plane_state->visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
>  		switch (plane_state->base.crtc_w) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6b0532a..1b444d3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1711,6 +1711,11 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
>  int skl_enable_sagv(struct drm_i915_private *dev_priv);
>  int skl_disable_sagv(struct drm_i915_private *dev_priv);
> +void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> +			 const struct skl_wm_values *wm);
> +void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> +			const struct skl_wm_values *wm,
> +			int plane);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7fd299e..53adcbf 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3798,6 +3798,47 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
>  		I915_WRITE(reg, 0);
>  }
>  
> +void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> +			const struct skl_wm_values *wm,
> +			int plane)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	int level, max_level = ilk_wm_max_level(dev);
> +	enum pipe pipe = intel_crtc->pipe;
> +
> +	if (!(wm->dirty_pipes & drm_crtc_mask(crtc)))
> +		return;
> +
> +	I915_WRITE(PIPE_WM_LINETIME(pipe), wm->wm_linetime[pipe]);

I think I mentioned this on the previous version, but if we program this
here then it only gets programmed when a plane is being updated.  It's
possible that we could have the linetime value change due to a scaler
update or a modeset, but without corresponding plane updates (e.g.,
maybe all planes are off).  So we probably need to make sure this value
gets written outside the plane updates (but still under vblank evasion).

> +
> +	for (level = 0; level <= max_level; level++) {
> +		I915_WRITE(PLANE_WM(pipe, plane, level),
> +			   wm->plane[pipe][plane][level]);
> +	}
> +	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);

I notice that you moved the DDB update into skl_write_cursor_wm() down
below, but didn't move the plane DDB update in here.  I realize you
remedy that in patch #6 where you finally fix up the flushing order, but
the inconsistency between how cursor vs !cursor planes are handled seems
confusing.  Can we either move the plane DDB update here in this patch
(we don't flush properly yet, but we're really no worse off than we
already are), or can we defer the move of the cursor DDB to patch 6 to
keep them consistent?


Matt

> +}
> +
> +void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> +			 const struct skl_wm_values *wm)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	int level, max_level = ilk_wm_max_level(dev);
> +	enum pipe pipe = intel_crtc->pipe;
> +
> +	for (level = 0; level <= max_level; level++) {
> +		I915_WRITE(CUR_WM(pipe, level),
> +			   wm->plane[pipe][PLANE_CURSOR][level]);
> +	}
> +	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
> +
> +	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> +			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
> +}
> +
>  static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  				const struct skl_wm_values *new)
>  {
> @@ -3805,7 +3846,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  	struct intel_crtc *crtc;
>  
>  	for_each_intel_crtc(dev, crtc) {
> -		int i, level, max_level = ilk_wm_max_level(dev);
> +		int i;
>  		enum pipe pipe = crtc->pipe;
>  
>  		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
> @@ -3813,21 +3854,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  		if (!crtc->active)
>  			continue;
>  
> -		I915_WRITE(PIPE_WM_LINETIME(pipe), new->wm_linetime[pipe]);
> -
> -		for (level = 0; level <= max_level; level++) {
> -			for (i = 0; i < intel_num_planes(crtc); i++)
> -				I915_WRITE(PLANE_WM(pipe, i, level),
> -					   new->plane[pipe][i][level]);
> -			I915_WRITE(CUR_WM(pipe, level),
> -				   new->plane[pipe][PLANE_CURSOR][level]);
> -		}
> -		for (i = 0; i < intel_num_planes(crtc); i++)
> -			I915_WRITE(PLANE_WM_TRANS(pipe, i),
> -				   new->plane_trans[pipe][i]);
> -		I915_WRITE(CUR_WM_TRANS(pipe),
> -			   new->plane_trans[pipe][PLANE_CURSOR]);
> -
>  		for (i = 0; i < intel_num_planes(crtc); i++) {
>  			skl_ddb_entry_write(dev_priv,
>  					    PLANE_BUF_CFG(pipe, i),
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0de935a..55d173f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,6 +203,9 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	struct drm_crtc *crtc = crtc_state->base.crtc;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  	u32 plane_ctl, stride_div, stride;
> @@ -238,6 +241,9 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	crtc_w--;
>  	crtc_h--;
>  
> +	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> +	    skl_write_plane_wm(intel_crtc, wm, plane);
> +
>  	if (key->flags) {
>  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
>  		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
> -- 
> 2.7.4
> 

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