Re: [Intel-gfx] [PATCH 1/4] drm/i915: Don't do the WM0->WM1 copy w/a if WM1 is already enabled

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Tue, Jan 31, 2023 at 02:21:24AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Due to a workaround we have to make sure the WM1 watermarks block/lines
> values are sensible even when WM1 is disabled. To that end we copy those
> values from WM0.
> 
> However since we now keep each wm level enabled on a per-plane basis
> it doesn't seem necessary to do that copy when we already have an
> enabled WM1 on the current plane. That is, we might be in a situation
> where another plane can only do WM0 (and thus needs the copy) but
> the current plane's WM1 is still perfectly valid (ie. fits into the
> current DDB allocation).
> 
> Skipping the copy could avoid reprogramming the plane's registers
> needlessly in some cases.
> 
> Fixes: a301cb0fca2d ("drm/i915: Keep plane watermarks enabled more aggressively")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_watermark.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 261cdab390b4..0c605034356f 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -1586,7 +1586,8 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
>  				skl_check_wm_level(&wm->wm[level], ddb);
>  
>  			if (icl_need_wm1_wa(i915, plane_id) &&
> -			    level == 1 && wm->wm[0].enable) {
> +			    level == 1 && !wm->wm[level].enable &&
> +			    wm->wm[0].enable) {
>  				wm->wm[level].blocks = wm->wm[0].blocks;
>  				wm->wm[level].lines = wm->wm[0].lines;
>  				wm->wm[level].ignore_lines = wm->wm[0].ignore_lines;

Took some time to remember once again the logic here :)
We probably need to make this more easily readable, because the comment
kinda suggests that we disable all wms starting from level + 1 here, while
we actually do also check with ddb allocation first.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> -- 
> 2.39.1
>