[v2] drm/i915: Fix crtc nv12 etc. plane bitmasks for DPMS off
diff mbox series

Message ID 20200318174515.31637-1-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • [v2] drm/i915: Fix crtc nv12 etc. plane bitmasks for DPMS off
Related show

Commit Message

Ville Syrjälä March 18, 2020, 5:45 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

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@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../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(-)

Comments

Rodrigo Vivi March 18, 2020, 5:47 p.m. UTC | #1
On Wed, Mar 18, 2020 at 07:45:15PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  .../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
>

Patch
diff mbox series

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)