On Tue, Jan 31, 2023 at 02:21:24AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> --
> 2.39.1
>
