| Message ID | 20200318174515.31637-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: Fix crtc nv12 etc. plane bitmasks for DPMS off | expand |
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 >
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)