diff mbox series

[v2] drm/i915: Fix dbuf slice mask when turning off all the pipes

Message ID 20200518121354.20401-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [v2] drm/i915: Fix dbuf slice mask when turning off all the pipes | expand

Commit Message

Ville Syrjala May 18, 2020, 12:13 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The current dbuf slice computation only happens when there are
active pipes. If we are turning off all the pipes we just leave
the dbuf slice mask at it's previous value, which may be something
other that BIT(S1). If runtime PM will kick in it will however
turn off everything but S1. Then on the next atomic commit (if
the new dbuf slice mask matches the stale value we left behind)
the code will not turn on the other slices we now need. This will
lead to underruns as the planes are trying to use a dbuf slice
that's not powered up.

To work around let's just just explicitly set the dbuf slice mask
to BIT(S1) when we are turning off all the pipes. Really the code
should just calculate this stuff the same way regardless whether
the pipes are on or off, but we're not quite there yet (need a
bit more work on the dbuf state for that).

v2: Let's not put the fix into dead code

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk> #v1
Fixes: 3cf43cdc63fb ("drm/i915: Introduce proper dbuf state")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Chris Wilson May 18, 2020, 1:14 p.m. UTC | #1 
Quoting Ville Syrjala (2020-05-18 13:13:54)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The current dbuf slice computation only happens when there are
> active pipes. If we are turning off all the pipes we just leave
> the dbuf slice mask at it's previous value, which may be something
> other that BIT(S1). If runtime PM will kick in it will however
> turn off everything but S1. Then on the next atomic commit (if
> the new dbuf slice mask matches the stale value we left behind)
> the code will not turn on the other slices we now need. This will
> lead to underruns as the planes are trying to use a dbuf slice
> that's not powered up.
> 
> To work around let's just just explicitly set the dbuf slice mask
> to BIT(S1) when we are turning off all the pipes. Really the code
> should just calculate this stuff the same way regardless whether
> the pipes are on or off, but we're not quite there yet (need a
> bit more work on the dbuf state for that).
> 
> v2: Let's not put the fix into dead code
> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk> #v1
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
> Fixes: 3cf43cdc63fb ("drm/i915: Introduce proper dbuf state")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
-Chris
Ville Syrjala May 18, 2020, 6:01 p.m. UTC | #2 
On Mon, May 18, 2020 at 02:14:15PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2020-05-18 13:13:54)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The current dbuf slice computation only happens when there are
> > active pipes. If we are turning off all the pipes we just leave
> > the dbuf slice mask at it's previous value, which may be something
> > other that BIT(S1). If runtime PM will kick in it will however
> > turn off everything but S1. Then on the next atomic commit (if
> > the new dbuf slice mask matches the stale value we left behind)
> > the code will not turn on the other slices we now need. This will
> > lead to underruns as the planes are trying to use a dbuf slice
> > that's not powered up.
> > 
> > To work around let's just just explicitly set the dbuf slice mask
> > to BIT(S1) when we are turning off all the pipes. Really the code
> > should just calculate this stuff the same way regardless whether
> > the pipes are on or off, but we're not quite there yet (need a
> > bit more work on the dbuf state for that).
> > 
> > v2: Let's not put the fix into dead code
> > 
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> #v1
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Fixes: 3cf43cdc63fb ("drm/i915: Introduce proper dbuf state")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> -Chris

v2 seems to have done the trick. CI gave up on the reverts anyway so
let's go with this one then. Pushed along with Chris's smatch fix.

Apologies for the massive cockup.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a21e36ed1a77..0082582d8352 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4764,6 +4764,30 @@  skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
 	memset(crtc_state->wm.skl.plane_ddb_uv, 0, sizeof(crtc_state->wm.skl.plane_ddb_uv));
 
 	if (!crtc_state->hw.active) {
+		struct intel_atomic_state *state =
+			to_intel_atomic_state(crtc_state->uapi.state);
+		struct intel_dbuf_state *new_dbuf_state =
+			intel_atomic_get_new_dbuf_state(state);
+		const struct intel_dbuf_state *old_dbuf_state =
+			intel_atomic_get_old_dbuf_state(state);
+
+		/*
+		 * FIXME hack to make sure we compute this sensibly when
+		 * turning off all the pipes. Otherwise we leave it at
+		 * whatever we had previously, and then runtime PM will
+		 * mess it up by turning off all but S1. Remove this
+		 * once the dbuf state computation flow becomes sane.
+		 */
+		if (new_dbuf_state->active_pipes == 0) {
+			new_dbuf_state->enabled_slices = BIT(DBUF_S1);
+
+			if (old_dbuf_state->enabled_slices != new_dbuf_state->enabled_slices) {
+				ret = intel_atomic_serialize_global_state(&new_dbuf_state->base);
+				if (ret)
+					return ret;
+			}
+		}
+
 		alloc->start = alloc->end = 0;
 		return 0;
 	}