On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
If the only thing that is changing is SAGV vs. no SAGV but the number of active planes and the total data rates end up unchanged we currently bail out of intel_bw_atomic_check() early and forget to actually compute the new WGV point mask and thus won't actually enable/disable SAGV as requested. This ends up poorly if we end up running with SAGV enabled when we shouldn't. Usually ends up in underruns. To fix this let's go through the QGV point mask computation if anyone else already added the bw state for us.
Haven't been looking this in a while. Despite we have been looking like few revisions together still some bugs :(
I thought SAGV vs No SAGV can't change if active planes or data rate didn't change? Because it means we probably still have same ddb allocations, which means SAGV state will just stay the same.
SAGV can change due to watermarks/ddb allocations. The easiest way to trip this up is to try to use the async flip wm0/ddb optimization. That immediately forgets to turn off SAGV and we get underruns, whcih is how I noticed this. And I don't immediately see any easy proof that this couldn't also happen due to some other plane changes.
Stan
Cc: stable@vger.kernel.org Cc: Stanislav Lisovskiy stanislav.lisovskiy@intel.com Fixes: 20f505f22531 ("drm/i915: Restrict qgv points which don't have enough bandwidth.") Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/i915/display/intel_bw.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c index 23aa8e06de18..d72ccee7d53b 100644 --- a/drivers/gpu/drm/i915/display/intel_bw.c +++ b/drivers/gpu/drm/i915/display/intel_bw.c @@ -846,6 +846,13 @@ int intel_bw_atomic_check(struct intel_atomic_state *state) if (num_psf_gv_points > 0) mask |= REG_GENMASK(num_psf_gv_points - 1, 0) << ADLS_PSF_PT_SHIFT;
- /*
* If we already have the bw state then recompute everything* even if pipe data_rate / active_planes didn't change.* Other things (such as SAGV) may have changed.*/- new_bw_state = intel_atomic_get_new_bw_state(state);
- for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { unsigned int old_data_rate =
-- 2.34.1