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.lisovs...@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,
