Re: [Intel-gfx] [PATCH] drm/i915: Write zero wms if we disable planes for icl+

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

Hi Stan

Pls see some comments inline..

BR
Vinod

On Wed, 2022-05-18 at 13:59 +0300, Stanislav Lisovskiy wrote:
> Otherwise we seem to get FIFO underruns. It is being disabled
> anyway, so kind of logical to write those as zeroes, even if
> disabling is temporary.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  .../drm/i915/display/skl_universal_plane.c    |  2 +-
>  drivers/gpu/drm/i915/intel_pm.c               | 46 +++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.h               |  2 +
>  3 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index caa03324a733..c0251189c042 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -633,7 +633,7 @@ icl_plane_disable_arm(struct intel_plane *plane,
>  	if (icl_is_hdr_plane(dev_priv, plane_id))
>  		intel_de_write_fw(dev_priv, PLANE_CUS_CTL(pipe, plane_id), 0);
>  
> -	skl_write_plane_wm(plane, crtc_state);
> +	skl_write_zero_plane_wm(plane, crtc_state);
>  
>  	intel_psr2_disable_plane_sel_fetch(plane, crtc_state);
>  	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), 0);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ee0047fdc95d..2470c037bfae 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5885,6 +5885,52 @@ void skl_write_plane_wm(struct intel_plane *plane,
>  				    PLANE_NV12_BUF_CFG(pipe, plane_id), ddb_y);
>  }
>  
> +void skl_write_zero_plane_wm(struct intel_plane *plane,
> +			     const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	int level, max_level = ilk_wm_max_level(dev_priv);
> +	enum plane_id plane_id = plane->id;
> +	enum pipe pipe = plane->pipe;
> +	struct skl_pipe_wm pipe_wm;
> +	const struct skl_ddb_entry *ddb =
> +		&crtc_state->wm.skl.plane_ddb[plane_id];
> +	const struct skl_ddb_entry *ddb_y =
> +		&crtc_state->wm.skl.plane_ddb_y[plane_id];
> +
> +	for (level = 0; level <= max_level; level++) {
> +		pipe_wm.planes[plane_id].wm[level].blocks = 0;
> +		pipe_wm.planes[plane_id].wm[level].lines = 0;
> +	}
> +
> +	pipe_wm.planes[plane_id].trans_wm.blocks = 0;
> +	pipe_wm.planes[plane_id].trans_wm.lines = 0;

What about the other members of struct skl_plane_wm - uv_wm, sagv.wm, sagv.trans_wm?
Or memset to 0 to the sizeof(skl_wm_level) better?

> +
> +	for (level = 0; level <= max_level; level++)
> +		skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, level),
> +				   skl_plane_wm_level(&pipe_wm, plane_id, level));
> +
> +	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
> +			   skl_plane_trans_wm(&pipe_wm, plane_id));
> +
> +	if (HAS_HW_SAGV_WM(dev_priv)) {
> +		const struct skl_plane_wm *wm = &pipe_wm.planes[plane_id];
> +
> +		skl_write_wm_level(dev_priv, PLANE_WM_SAGV(pipe, plane_id),
> +				   &wm->sagv.wm0);
> +		skl_write_wm_level(dev_priv, PLANE_WM_SAGV_TRANS(pipe, plane_id),
> +				   &wm->sagv.trans_wm);
> +	}
> +
> +	skl_ddb_entry_write(dev_priv,
> +			    PLANE_BUF_CFG(pipe, plane_id), ddb);

As the plane wm values are hardcode to 0 just before this line, these ddb entries might not be
representing the accurate information anymore. Does that even matter?

Or is it better if we ignore/skip zero-ing the wm values during the disable planes completely if it
doesnt matter and just rely on disabling the plane bit in plane_ctl?

> +
> +	if (DISPLAY_VER(dev_priv) < 11)
> +		skl_ddb_entry_write(dev_priv,
> +				    PLANE_NV12_BUF_CFG(pipe, plane_id), ddb_y);
> +}
> +
> +
>  void skl_write_cursor_wm(struct intel_plane *plane,
>  			 const struct intel_crtc_state *crtc_state)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index 50604cf7398c..fb0ac4f143ab 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -61,6 +61,8 @@ bool skl_wm_level_equals(const struct skl_wm_level *l1,
>  bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb,
>  				 const struct skl_ddb_entry *entries,
>  				 int num_entries, int ignore_idx);
> +void skl_write_zero_plane_wm(struct intel_plane *plane,
> +			     const struct intel_crtc_state *crtc_state);
>  void skl_write_plane_wm(struct intel_plane *plane,
>  			const struct intel_crtc_state *crtc_state);
>  void skl_write_cursor_wm(struct intel_plane *plane,