On Wed, Mar 18, 2020 at 07:45:15PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> We only consider crtc_state->enable when initially calculating plane
> visibility. Later on we try to override the plane's state to invisible
> if the crtc is in DPMS off state (crtc_state->active==false).
> Unfortunately the code doing that only updates the plane_state.visible
> flag and the crtc_state.active_planes bimask, but forgets to update
> some of the other plane bitmasks stored in the crtc_state. Namely
> crtc_state.nv12_planes is left set up based on the original visibility
> check which makes icl_check_nv12_planes() pick a slave plane for the
> flagged plane in the bitmask. Later on we hit the watermark code
> which sees a plane with a slave assigned and it then makes the
> logical assumption that the master plane must itself be visible.
> Since the master's plane_state.visible flag was already cleared
> we get a WARN.
>
> Fix the problem by clearing all the plane bitmasks for DPMS off.
> This is more or less the wrong approach and instead we should
> calculate all the plane related state purely based crtc_state->enable
> (to guarantee that the subsequent DPMS on can't fail). However in
> the past we definitely had some roadblocks to making that happen.
> Not sure how many are left these days, but let's stick to the current
> approach since it's a much simpler fix to the immediate problem
> (the WARN).
>
> v2: Keep the visible=false, it's important (Rodrigo)
>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
> .../gpu/drm/i915/display/intel_atomic_plane.c | 21 +++++++++++++------
> .../gpu/drm/i915/display/intel_atomic_plane.h | 2 ++
> drivers/gpu/drm/i915/display/intel_display.c | 6 ++----
> 3 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 457b258683d3..25dfeb3197aa 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -264,6 +264,20 @@ void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
> plane_state->hw.color_range = from_plane_state->uapi.color_range;
> }
>
> +void intel_plane_set_invisible(struct intel_crtc_state *crtc_state,
> + struct intel_plane_state *plane_state)
> +{
> + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> +
> + crtc_state->active_planes &= ~BIT(plane->id);
> + crtc_state->nv12_planes &= ~BIT(plane->id);
> + crtc_state->c8_planes &= ~BIT(plane->id);
> + crtc_state->data_rate[plane->id] = 0;
> + crtc_state->min_cdclk[plane->id] = 0;
> +
> + plane_state->uapi.visible = false;
> +}
> +
> int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state,
> struct intel_crtc_state *new_crtc_state,
> const struct intel_plane_state *old_plane_state,
> @@ -273,12 +287,7 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> const struct drm_framebuffer *fb = new_plane_state->hw.fb;
> int ret;
>
> - new_crtc_state->active_planes &= ~BIT(plane->id);
> - new_crtc_state->nv12_planes &= ~BIT(plane->id);
> - new_crtc_state->c8_planes &= ~BIT(plane->id);
> - new_crtc_state->data_rate[plane->id] = 0;
> - new_crtc_state->min_cdclk[plane->id] = 0;
> - new_plane_state->uapi.visible = false;
> + intel_plane_set_invisible(new_crtc_state, new_plane_state);
>
> if (!new_plane_state->hw.crtc && !old_plane_state->hw.crtc)
> return 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> index a6bbf42bae1f..59dd1fbb02ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> @@ -52,5 +52,7 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
> int intel_plane_calc_min_cdclk(struct intel_atomic_state *state,
> struct intel_plane *plane,
> bool *need_cdclk_calc);
> +void intel_plane_set_invisible(struct intel_crtc_state *crtc_state,
> + struct intel_plane_state *plane_state);
>
> #endif /* __INTEL_ATOMIC_PLANE_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 8f23c4d51c33..37bd7ce88ecd 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12377,10 +12377,8 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
> * only combine the results from all planes in the current place?
> */
> if (!is_crtc_enabled) {
> - plane_state->uapi.visible = visible = false;
> - crtc_state->active_planes &= ~BIT(plane->id);
> - crtc_state->data_rate[plane->id] = 0;
> - crtc_state->min_cdclk[plane->id] = 0;
> + intel_plane_set_invisible(crtc_state, plane_state);
> + visible = false;
> }
>
> if (!was_visible && !visible)
> --
> 2.24.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
