* [PATCH v2 1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method
@ 2018-12-11 1:05 Matt Roper
2018-12-11 1:05 ` [PATCH v2 2/2] drm/i915: Switch to level-based DDB allocation algorithm (v3) Matt Roper
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Matt Roper @ 2018-12-11 1:05 UTC (permalink / raw)
To: intel-gfx
The bspec gives an if/else chain for choosing whether to use "method 1"
or "method 2" for calculating the watermark "Selected Result Blocks"
value for a plane. One of the branches of the if chain is:
"Else If ('plane buffer allocation' is known and (plane buffer
allocation / plane blocks per line) >=1)"
Since our driver currently calculates DDB allocations first and the
actual watermark values second, the plane buffer allocation is known at
this point in our code and we include this test in our driver's logic.
However we plan to soon move to a "watermarks first, ddb allocation
second" sequence where we won't know the DDB allocation at this point.
Let's drop this arm of the if/else statement (effectively considering
the DDB allocation unknown) as an independent patch so that any
regressions can be more accurately bisected to either the different
watermark value (in this patch) or the new DDB allocation (in the next
patch).
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2bba5315b764..bf970cf7b8a5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4761,13 +4761,6 @@ static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
wp->dbuf_block_size < 1) &&
(wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
selected_result = method2;
- } else if (ddb_allocation >=
- fixed16_to_u32_round_up(wp->plane_blocks_per_line)) {
- if (IS_GEN9(dev_priv) &&
- !IS_GEMINILAKE(dev_priv))
- selected_result = min_fixed16(method1, method2);
- else
- selected_result = method2;
} else if (latency >= wp->linetime_us) {
if (IS_GEN9(dev_priv) &&
!IS_GEMINILAKE(dev_priv))
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] drm/i915: Switch to level-based DDB allocation algorithm (v3)
2018-12-11 1:05 [PATCH v2 1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method Matt Roper
@ 2018-12-11 1:05 ` Matt Roper
2018-12-11 15:59 ` Ville Syrjälä
2018-12-11 1:27 ` ✗ Fi.CI.SPARSE: warning for series starting with [v2,1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method Patchwork
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Matt Roper @ 2018-12-11 1:05 UTC (permalink / raw)
To: intel-gfx
The DDB allocation algorithm currently used by the driver grants each
plane a very small minimum allocation of DDB blocks and then divies up
all of the remaining blocks based on the percentage of the total data
rate that the plane makes up. It turns out that this proportional
allocation approach is overly-generous with the larger planes and can
leave very small planes wthout a big enough allocation to even hit their
level 0 watermark requirements (especially on APL, which has a smaller
DDB in general than other gen9 platforms). Or there can be situations
where the smallest planes hit a lower watermark level than they should
have been able to hit with a more equitable division of DDB blocks, thus
limiting the overall system sleep state that can be achieved.
The bspec now describes an alternate algorithm that can be used to
overcome these types of issues. With the new algorithm, we calculate
all plane watermark values for all wm levels first, then go back and
partition a pipe's DDB space second. The DDB allocation will calculate
what the highest watermark level that can be achieved on *all* active
planes, and then grant the blocks necessary to hit that level to each
plane. Any remaining blocks are then divided up proportionally
according to data rate, similar to the old algorithm.
There was a previous attempt to implement this algorithm a couple years
ago in bb9d85f6e9d ("drm/i915/skl: New ddb allocation algorithm"), but
some regressions were reported, the patch was reverted, and nobody
ever got around to figuring out exactly where the bug was in that
version. Our watermark code has evolved significantly in the meantime,
but we're still getting bug reports caused by the unfair proportional
algorithm, so let's give this another shot.
v2:
- Make sure cursor allocation stays constant and fixed at the end of
the pipe allocation.
- Fix some watermark level iterators that weren't handling the max
level.
v3:
- Ensure we don't leave any DDB blocks unused by using DIV_ROUND_UP+min
to calculate the extra blocks for each plane. (Ville)
- Replace a while() loop with a for() loop to be more consistent with
surrounding code. (Ville)
- Clean unattainable watermark levels with memset rather than directly
clearing the member fields. Also do the same for the transition
watermark values if they can't be achieved. (Ville)
- Drop min_disp_buf_needed calculations in skl_compute_plane_wm() since
the results are no longer needed or used. (Ville)
- Drop skl_latency[0] != 0 sanity check; both watermark methods already
account for an invalid 0 latency by returning FP_16_16_MAX. (Ville)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105458
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 376 ++++++++++++++--------------------------
1 file changed, 132 insertions(+), 244 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index bf970cf7b8a5..f5f86757457d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4301,102 +4301,6 @@ icl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
return total_data_rate;
}
-static uint16_t
-skl_ddb_min_alloc(const struct intel_plane_state *plane_state, const int plane)
-{
- struct drm_framebuffer *fb = plane_state->base.fb;
- uint32_t src_w, src_h;
- uint32_t min_scanlines = 8;
- uint8_t plane_bpp;
-
- if (WARN_ON(!fb))
- return 0;
-
- /* For packed formats, and uv-plane, return 0 */
- if (plane == 1 && fb->format->format != DRM_FORMAT_NV12)
- return 0;
-
- /* For Non Y-tile return 8-blocks */
- if (fb->modifier != I915_FORMAT_MOD_Y_TILED &&
- fb->modifier != I915_FORMAT_MOD_Yf_TILED &&
- fb->modifier != I915_FORMAT_MOD_Y_TILED_CCS &&
- fb->modifier != I915_FORMAT_MOD_Yf_TILED_CCS)
- return 8;
-
- /*
- * Src coordinates are already rotated by 270 degrees for
- * the 90/270 degree plane rotation cases (to match the
- * GTT mapping), hence no need to account for rotation here.
- */
- src_w = drm_rect_width(&plane_state->base.src) >> 16;
- src_h = drm_rect_height(&plane_state->base.src) >> 16;
-
- /* Halve UV plane width and height for NV12 */
- if (plane == 1) {
- src_w /= 2;
- src_h /= 2;
- }
-
- plane_bpp = fb->format->cpp[plane];
-
- if (drm_rotation_90_or_270(plane_state->base.rotation)) {
- switch (plane_bpp) {
- case 1:
- min_scanlines = 32;
- break;
- case 2:
- min_scanlines = 16;
- break;
- case 4:
- min_scanlines = 8;
- break;
- case 8:
- min_scanlines = 4;
- break;
- default:
- WARN(1, "Unsupported pixel depth %u for rotation",
- plane_bpp);
- min_scanlines = 32;
- }
- }
-
- return DIV_ROUND_UP((4 * src_w * plane_bpp), 512) * min_scanlines/4 + 3;
-}
-
-static void
-skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
- uint16_t *minimum, uint16_t *uv_minimum)
-{
- const struct drm_plane_state *pstate;
- struct drm_plane *plane;
-
- drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
- enum plane_id plane_id = to_intel_plane(plane)->id;
- struct intel_plane_state *plane_state = to_intel_plane_state(pstate);
-
- if (plane_id == PLANE_CURSOR)
- continue;
-
- /* slave plane must be invisible and calculated from master */
- if (!pstate->visible || WARN_ON(plane_state->slave))
- continue;
-
- if (!plane_state->linked_plane) {
- minimum[plane_id] = skl_ddb_min_alloc(plane_state, 0);
- uv_minimum[plane_id] =
- skl_ddb_min_alloc(plane_state, 1);
- } else {
- enum plane_id y_plane_id =
- plane_state->linked_plane->id;
-
- minimum[y_plane_id] = skl_ddb_min_alloc(plane_state, 0);
- minimum[plane_id] = skl_ddb_min_alloc(plane_state, 1);
- }
- }
-
- minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
-}
-
static int
skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
struct skl_ddb_allocation *ddb /* out */)
@@ -4406,15 +4310,17 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
struct drm_i915_private *dev_priv = to_i915(crtc->dev);
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
- uint16_t alloc_size, start;
- uint16_t minimum[I915_MAX_PLANES] = {};
- uint16_t uv_minimum[I915_MAX_PLANES] = {};
+ struct skl_plane_wm *wm;
+ uint16_t alloc_size, start = 0;
+ uint16_t total[I915_MAX_PLANES] = {};
+ uint16_t uv_total[I915_MAX_PLANES] = {};
u64 total_data_rate;
enum plane_id plane_id;
int num_active;
u64 plane_data_rate[I915_MAX_PLANES] = {};
u64 uv_plane_data_rate[I915_MAX_PLANES] = {};
- uint16_t total_min_blocks = 0;
+ uint16_t blocks = 0;
+ int level;
/* Clear the partitioning for disabled planes. */
memset(cstate->wm.skl.plane_ddb_y, 0, sizeof(cstate->wm.skl.plane_ddb_y));
@@ -4444,81 +4350,125 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
if (alloc_size == 0)
return 0;
- skl_ddb_calc_min(cstate, num_active, minimum, uv_minimum);
+ /* Allocate fixed number of blocks for cursor. */
+ total[PLANE_CURSOR] = skl_cursor_allocation(num_active);
+ alloc_size -= total[PLANE_CURSOR];
+ cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].start =
+ alloc->end - total[PLANE_CURSOR];
+ cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].end = alloc->end;
+
+ if (total_data_rate == 0)
+ return 0;
/*
- * 1. Allocate the mininum required blocks for each active plane
- * and allocate the cursor, it doesn't require extra allocation
- * proportional to the data rate.
+ * Find the highest watermark level for which we can satisfy the block
+ * requirement of active planes.
*/
+ for (level = ilk_wm_max_level(dev_priv); level >= 0; level--) {
+ for_each_plane_id_on_crtc(intel_crtc, plane_id) {
+ if (plane_id == PLANE_CURSOR)
+ continue;
- for_each_plane_id_on_crtc(intel_crtc, plane_id) {
- total_min_blocks += minimum[plane_id];
- total_min_blocks += uv_minimum[plane_id];
+ wm = &cstate->wm.skl.optimal.planes[plane_id];
+ blocks += wm->wm[level].plane_res_b;
+ blocks += wm->uv_wm[level].plane_res_b;
+ }
+
+ if (blocks < alloc_size) {
+ alloc_size -= blocks;
+ break;
+ }
}
- if (total_min_blocks > alloc_size) {
+ if (level < 0) {
DRM_DEBUG_KMS("Requested display configuration exceeds system DDB limitations");
- DRM_DEBUG_KMS("minimum required %d/%d\n", total_min_blocks,
- alloc_size);
+ DRM_DEBUG_KMS("minimum required %d/%d\n", blocks,
+ alloc_size);
return -EINVAL;
}
- alloc_size -= total_min_blocks;
- cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
- cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].end = alloc->end;
-
/*
- * 2. Distribute the remaining space in proportion to the amount of
- * data each plane needs to fetch from memory.
- *
- * FIXME: we may not allocate every single block here.
+ * Grant each plane the blocks it requires at the highest achievable
+ * watermark level, plus an extra share of the leftover blocks
+ * proportional to its relative data rate.
*/
- if (total_data_rate == 0)
- return 0;
-
- start = alloc->start;
for_each_plane_id_on_crtc(intel_crtc, plane_id) {
- u64 data_rate, uv_data_rate;
- uint16_t plane_blocks, uv_plane_blocks;
+ u64 rate;
+ u16 extra;
if (plane_id == PLANE_CURSOR)
continue;
+ if (alloc_size == 0)
+ continue;
- data_rate = plane_data_rate[plane_id];
+ wm = &cstate->wm.skl.optimal.planes[plane_id];
- /*
- * allocation for (packed formats) or (uv-plane part of planar format):
- * promote the expression to 64 bits to avoid overflowing, the
- * result is < available as data_rate / total_data_rate < 1
- */
- plane_blocks = minimum[plane_id];
- plane_blocks += div64_u64(alloc_size * data_rate, total_data_rate);
+ rate = plane_data_rate[plane_id];
+ extra = min_t(u16, alloc_size,
+ DIV_ROUND_UP(alloc_size * rate, total_data_rate));
+ total[plane_id] = wm->wm[level].plane_res_b + extra;
+ alloc_size -= extra;
+ total_data_rate -= rate;
- /* Leave disabled planes at (0,0) */
- if (data_rate) {
- cstate->wm.skl.plane_ddb_y[plane_id].start = start;
- cstate->wm.skl.plane_ddb_y[plane_id].end = start + plane_blocks;
- }
+ if (alloc_size == 0)
+ continue;
- start += plane_blocks;
+ rate = uv_plane_data_rate[plane_id];
+ extra = min_t(u16, alloc_size,
+ DIV_ROUND_UP(alloc_size * rate, total_data_rate));
+ uv_total[plane_id] = wm->uv_wm[level].plane_res_b + extra;
+ alloc_size -= extra;
+ total_data_rate -= rate;
+ }
+ WARN_ON(alloc_size != 0 || total_data_rate != 0);
- /* Allocate DDB for UV plane for planar format/NV12 */
- uv_data_rate = uv_plane_data_rate[plane_id];
+ /* Set the actual DDB start/end points for each plane */
+ start = alloc->start;
+ for_each_plane_id_on_crtc(intel_crtc, plane_id) {
+ struct skl_ddb_entry *plane_alloc, *uv_plane_alloc;
- uv_plane_blocks = uv_minimum[plane_id];
- uv_plane_blocks += div64_u64(alloc_size * uv_data_rate, total_data_rate);
+ if (plane_id == PLANE_CURSOR)
+ continue;
+
+ plane_alloc = &cstate->wm.skl.plane_ddb_y[plane_id];
+ uv_plane_alloc = &cstate->wm.skl.plane_ddb_uv[plane_id];
/* Gen11+ uses a separate plane for UV watermarks */
- WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_plane_blocks);
+ WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_total[plane_id]);
+
+ /* Leave disabled planes at (0,0) */
+ if (total[plane_id]) {
+ plane_alloc->start = start;
+ plane_alloc->end = start += total[plane_id];
+ }
- if (uv_data_rate) {
- cstate->wm.skl.plane_ddb_uv[plane_id].start = start;
- cstate->wm.skl.plane_ddb_uv[plane_id].end =
- start + uv_plane_blocks;
+ if (uv_total[plane_id]) {
+ uv_plane_alloc->start = start;
+ uv_plane_alloc->end = start + uv_total[plane_id];
}
+ }
- start += uv_plane_blocks;
+ /*
+ * When we calculated watermark values we didn't know how high
+ * of a level we'd actually be able to hit, so we just marked
+ * all levels as "enabled." Go back now and disable the ones
+ * that aren't actually possible.
+ */
+ for (level++; level <= ilk_wm_max_level(dev_priv); level++) {
+ for_each_plane_id_on_crtc(intel_crtc, plane_id) {
+ wm = &cstate->wm.skl.optimal.planes[plane_id];
+ memset(&wm->wm[level], 0, sizeof(wm->wm[level]));
+ }
+ }
+
+ /*
+ * Go back and disable the transition watermark if it turns out we
+ * don't have enough DDB blocks for it.
+ */
+ for_each_plane_id_on_crtc(intel_crtc, plane_id) {
+ wm = &cstate->wm.skl.optimal.planes[plane_id];
+ if (wm->trans_wm.plane_res_b > total[plane_id])
+ memset(&wm->trans_wm, 0, sizeof(wm->trans_wm));
}
return 0;
@@ -4715,17 +4665,15 @@ skl_compute_plane_wm_params(const struct intel_crtc_state *cstate,
return 0;
}
-static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
- const struct intel_plane_state *intel_pstate,
- uint16_t ddb_allocation,
- int level,
- const struct skl_wm_params *wp,
- const struct skl_wm_level *result_prev,
- struct skl_wm_level *result /* out */)
+static void skl_compute_plane_wm(const struct intel_crtc_state *cstate,
+ const struct intel_plane_state *intel_pstate,
+ int level,
+ const struct skl_wm_params *wp,
+ const struct skl_wm_level *result_prev,
+ struct skl_wm_level *result /* out */)
{
struct drm_i915_private *dev_priv =
to_i915(intel_pstate->base.plane->dev);
- const struct drm_plane_state *pstate = &intel_pstate->base;
uint32_t latency = dev_priv->wm.skl_latency[level];
uint_fixed_16_16_t method1, method2;
uint_fixed_16_16_t selected_result;
@@ -4733,10 +4681,6 @@ static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
struct intel_atomic_state *state =
to_intel_atomic_state(cstate->base.state);
bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
- uint32_t min_disp_buf_needed;
-
- if (latency == 0)
- return level == 0 ? -EINVAL : 0;
/* Display WA #1141: kbl,cfl */
if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
@@ -4800,61 +4744,24 @@ static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
res_blocks = result_prev->plane_res_b;
}
- if (INTEL_GEN(dev_priv) >= 11) {
- if (wp->y_tiled) {
- uint32_t extra_lines;
- uint_fixed_16_16_t fp_min_disp_buf_needed;
-
- if (res_lines % wp->y_min_scanlines == 0)
- extra_lines = wp->y_min_scanlines;
- else
- extra_lines = wp->y_min_scanlines * 2 -
- res_lines % wp->y_min_scanlines;
-
- fp_min_disp_buf_needed = mul_u32_fixed16(res_lines +
- extra_lines,
- wp->plane_blocks_per_line);
- min_disp_buf_needed = fixed16_to_u32_round_up(
- fp_min_disp_buf_needed);
- } else {
- min_disp_buf_needed = DIV_ROUND_UP(res_blocks * 11, 10);
- }
- } else {
- min_disp_buf_needed = res_blocks;
- }
-
- if ((level > 0 && res_lines > 31) ||
- res_blocks >= ddb_allocation ||
- min_disp_buf_needed >= ddb_allocation) {
- /*
- * If there are no valid level 0 watermarks, then we can't
- * support this display configuration.
- */
- if (level) {
- return 0;
- } else {
- struct drm_plane *plane = pstate->plane;
-
- DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
- DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
- plane->base.id, plane->name,
- res_blocks, ddb_allocation, res_lines);
- return -EINVAL;
- }
- }
-
/* The number of lines are ignored for the level 0 watermark. */
+ if (level > 0 && res_lines > 31)
+ return;
+
+ /*
+ * If res_lines is valid, assume we can use this watermark level
+ * for now. We'll come back and disable it after we calculate the
+ * DDB allocation if it turns out we don't actually have enough
+ * blocks to satisfy it.
+ */
result->plane_res_b = res_blocks;
result->plane_res_l = res_lines;
result->plane_en = true;
-
- return 0;
}
-static int
+static void
skl_compute_wm_levels(const struct intel_crtc_state *cstate,
const struct intel_plane_state *intel_pstate,
- uint16_t ddb_blocks,
const struct skl_wm_params *wm_params,
struct skl_wm_level *levels)
{
@@ -4862,25 +4769,15 @@ skl_compute_wm_levels(const struct intel_crtc_state *cstate,
to_i915(intel_pstate->base.plane->dev);
int level, max_level = ilk_wm_max_level(dev_priv);
struct skl_wm_level *result_prev = &levels[0];
- int ret;
for (level = 0; level <= max_level; level++) {
struct skl_wm_level *result = &levels[level];
- ret = skl_compute_plane_wm(cstate,
- intel_pstate,
- ddb_blocks,
- level,
- wm_params,
- result_prev,
- result);
- if (ret)
- return ret;
+ skl_compute_plane_wm(cstate, intel_pstate, level, wm_params,
+ result_prev, result);
result_prev = result;
}
-
- return 0;
}
static uint32_t
@@ -4908,8 +4805,7 @@ skl_compute_linetime_wm(const struct intel_crtc_state *cstate)
static void skl_compute_transition_wm(const struct intel_crtc_state *cstate,
const struct skl_wm_params *wp,
- struct skl_plane_wm *wm,
- uint16_t ddb_allocation)
+ struct skl_plane_wm *wm)
{
struct drm_device *dev = cstate->base.crtc->dev;
const struct drm_i915_private *dev_priv = to_i915(dev);
@@ -4957,12 +4853,13 @@ static void skl_compute_transition_wm(const struct intel_crtc_state *cstate,
}
- res_blocks += 1;
-
- if (res_blocks < ddb_allocation) {
- wm->trans_wm.plane_res_b = res_blocks;
- wm->trans_wm.plane_en = true;
- }
+ /*
+ * Just assume we can enable the transition watermark. After
+ * computing the DDB we'll come back and disable it if that
+ * assumption turns out to be false.
+ */
+ wm->trans_wm.plane_res_b = res_blocks + 1;
+ wm->trans_wm.plane_en = true;
}
static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
@@ -4970,7 +4867,6 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
enum plane_id plane_id, int color_plane)
{
struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
- u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_y[plane_id]);
struct skl_wm_params wm_params;
int ret;
@@ -4979,12 +4875,8 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
if (ret)
return ret;
- ret = skl_compute_wm_levels(crtc_state, plane_state,
- ddb_blocks, &wm_params, wm->wm);
- if (ret)
- return ret;
-
- skl_compute_transition_wm(crtc_state, &wm_params, wm, ddb_blocks);
+ skl_compute_wm_levels(crtc_state, plane_state, &wm_params, wm->wm);
+ skl_compute_transition_wm(crtc_state, &wm_params, wm);
return 0;
}
@@ -4994,7 +4886,6 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
enum plane_id plane_id)
{
struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
- u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_uv[plane_id]);
struct skl_wm_params wm_params;
int ret;
@@ -5006,10 +4897,7 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
if (ret)
return ret;
- ret = skl_compute_wm_levels(crtc_state, plane_state,
- ddb_blocks, &wm_params, wm->uv_wm);
- if (ret)
- return ret;
+ skl_compute_wm_levels(crtc_state, plane_state, &wm_params, wm->uv_wm);
return 0;
}
@@ -5521,13 +5409,9 @@ skl_compute_wm(struct intel_atomic_state *state)
if (ret || !changed)
return ret;
- ret = skl_compute_ddb(state);
- if (ret)
- return ret;
-
/*
* Calculate WM's for all pipes that are part of this transaction.
- * Note that the DDB allocation above may have added more CRTC's that
+ * Note that skl_ddb_add_affected_pipes may have added more CRTC's that
* weren't otherwise being modified (and set bits in dirty_pipes) if
* pipe allocations had to change.
*/
@@ -5549,6 +5433,10 @@ skl_compute_wm(struct intel_atomic_state *state)
results->dirty_pipes |= drm_crtc_mask(&crtc->base);
}
+ ret = skl_compute_ddb(state);
+ if (ret)
+ return ret;
+
skl_print_wm_changes(state);
return 0;
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✗ Fi.CI.SPARSE: warning for series starting with [v2,1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method
2018-12-11 1:05 [PATCH v2 1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method Matt Roper
2018-12-11 1:05 ` [PATCH v2 2/2] drm/i915: Switch to level-based DDB allocation algorithm (v3) Matt Roper
@ 2018-12-11 1:27 ` Patchwork
2018-12-11 1:45 ` ✓ Fi.CI.BAT: success " Patchwork
2018-12-11 3:37 ` ✗ Fi.CI.IGT: failure " Patchwork
3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-12-11 1:27 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method
URL : https://patchwork.freedesktop.org/series/53862/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Don't use DDB allocation when choosing gen9 watermark method
-drivers/gpu/drm/i915/i915_fixed.h:42:43: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/i915_fixed.h:42:43: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_pm.c:6724:35: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_pm.c:6724:35: warning: expression using sizeof(void)
Commit: drm/i915: Switch to level-based DDB allocation algorithm (v3)
+drivers/gpu/drm/i915/intel_pm.c:4407:25: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_pm.c:4407:25: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_pm.c:4417:25: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_pm.c:4417:25: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_pm.c:6608:24: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_pm.c:6608:24: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_pm.c:6612:35: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_pm.c:6612:35: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/intel_pm.c:6612:35: warning: too many warnings
+drivers/gpu/drm/i915/intel_pm.c:6608:24: warning: too many warnings
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v2,1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method
2018-12-11 1:05 [PATCH v2 1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method Matt Roper
2018-12-11 1:05 ` [PATCH v2 2/2] drm/i915: Switch to level-based DDB allocation algorithm (v3) Matt Roper
2018-12-11 1:27 ` ✗ Fi.CI.SPARSE: warning for series starting with [v2,1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method Patchwork
@ 2018-12-11 1:45 ` Patchwork
2018-12-11 3:37 ` ✗ Fi.CI.IGT: failure " Patchwork
3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-12-11 1:45 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method
URL : https://patchwork.freedesktop.org/series/53862/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5294 -> Patchwork_11061
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/53862/revisions/1/mbox/
Known issues
------------
Here are the changes found in Patchwork_11061 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live_hangcheck:
- fi-bwr-2160: PASS -> DMESG-FAIL [fdo#108735]
#### Possible fixes ####
* igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
- fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
Participating hosts (49 -> 44)
------------------------------
Missing (5): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-ctg-p8600
Build changes
-------------
* Linux: CI_DRM_5294 -> Patchwork_11061
CI_DRM_5294: 791e2289df7e8ec34fb65251bb8ad30ceb28aba3 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4744: 4579ac1d445cf39f6de474071b20db790db575bd @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_11061: 847ef79c984b4068d82649abfaa6f99261af5a99 @ git://anongit.freedesktop.org/gfx-ci/linux
== Kernel 32bit build ==
Warning: Kernel 32bit buildtest failed:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11061/build_32bit.log
CALL scripts/checksyscalls.sh
CHK include/generated/compile.h
Kernel: arch/x86/boot/bzImage is ready (#1)
Building modules, stage 2.
MODPOST 109 modules
ERROR: "__udivdi3" [drivers/gpu/drm/i915/i915.ko] undefined!
scripts/Makefile.modpost:92: recipe for target '__modpost' failed
make[1]: *** [__modpost] Error 1
Makefile:1269: recipe for target 'modules' failed
make: *** [modules] Error 2
== Linux commits ==
847ef79c984b drm/i915: Switch to level-based DDB allocation algorithm (v3)
419d5bd8b79d drm/i915: Don't use DDB allocation when choosing gen9 watermark method
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11061/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* ✗ Fi.CI.IGT: failure for series starting with [v2,1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method
2018-12-11 1:05 [PATCH v2 1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method Matt Roper
` (2 preceding siblings ...)
2018-12-11 1:45 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-12-11 3:37 ` Patchwork
3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-12-11 3:37 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method
URL : https://patchwork.freedesktop.org/series/53862/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_5294_full -> Patchwork_11061_full
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_11061_full absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_11061_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_11061_full:
### IGT changes ###
#### Possible regressions ####
* igt@kms_plane@plane-position-hole-pipe-c-planes:
- shard-apl: PASS -> DMESG-WARN
* igt@kms_plane_alpha_blend@pipe-c-alpha-transparant-fb:
- {shard-iclb}: PASS -> FAIL +4
* {igt@runner@aborted}:
- shard-glk: NOTRUN -> ( 15 FAIL )
- shard-skl: NOTRUN -> ( 22 FAIL )
- {shard-iclb}: NOTRUN -> ( 6 FAIL )
- shard-apl: NOTRUN -> ( 18 FAIL )
#### Warnings ####
* igt@kms_plane_lowres@pipe-b-tiling-y:
- {shard-iclb}: PASS -> SKIP
* igt@pm_rc6_residency@rc6-accuracy:
- shard-snb: SKIP -> PASS
Known issues
------------
Here are the changes found in Patchwork_11061_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_eio@in-flight-internal-1us:
- shard-glk: PASS -> FAIL [fdo#107799]
* igt@i915_selftest@live_contexts:
- {shard-iclb}: NOTRUN -> DMESG-FAIL [fdo#108569]
* igt@kms_available_modes_crc@available_mode_test_crc:
- shard-apl: PASS -> FAIL [fdo#106641]
* igt@kms_chv_cursor_fail@pipe-a-64x64-bottom-edge:
- shard-skl: PASS -> FAIL [fdo#104671]
* igt@kms_cursor_crc@cursor-128x128-offscreen:
- shard-skl: PASS -> FAIL [fdo#103232]
* igt@kms_cursor_crc@cursor-128x128-suspend:
- shard-skl: PASS -> INCOMPLETE [fdo#104108]
* igt@kms_draw_crc@draw-method-xrgb8888-mmap-wc-ytiled:
- shard-skl: PASS -> FAIL [fdo#103184]
* igt@kms_flip@flip-vs-expired-vblank:
- shard-skl: PASS -> FAIL [fdo#105363]
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
- shard-apl: PASS -> DMESG-FAIL [fdo#103167] +4
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
- {shard-iclb}: PASS -> FAIL [fdo#103167] +7
- shard-skl: NOTRUN -> DMESG-FAIL [fdo#103167] / [fdo#105541]
* igt@kms_frontbuffer_tracking@fbc-1p-rte:
- {shard-iclb}: PASS -> DMESG-FAIL [fdo#103167] / [fdo#107724] +3
* igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-blt:
- shard-glk: PASS -> DMESG-FAIL [fdo#105681] / [fdo#105763] / [fdo#106538]
* igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
- shard-glk: PASS -> DMESG-FAIL [fdo#103167] / [fdo#105763] / [fdo#106538] +10
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-pwrite:
- shard-skl: PASS -> DMESG-FAIL [fdo#103167] / [fdo#105541] +14
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-indfb-draw-mmap-cpu:
- shard-skl: PASS -> FAIL [fdo#103167] +1
* igt@kms_plane@pixel-format-pipe-b-planes:
- shard-skl: NOTRUN -> DMESG-WARN [fdo#106885]
* {igt@kms_plane@pixel-format-pipe-c-planes-source-clamping}:
- shard-glk: PASS -> FAIL [fdo#108948]
* igt@kms_plane@plane-position-hole-dpms-pipe-a-planes:
- shard-skl: NOTRUN -> DMESG-WARN [fdo#105541]
* igt@kms_plane@plane-position-hole-pipe-a-planes:
- shard-apl: PASS -> DMESG-FAIL [fdo#103166] +3
* igt@kms_plane@plane-position-hole-pipe-c-planes:
- shard-skl: PASS -> DMESG-FAIL [fdo#103166] / [fdo#105541] +1
* igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
- shard-skl: PASS -> FAIL [fdo#107815] / [fdo#108145]
* igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
- shard-skl: NOTRUN -> FAIL [fdo#107815]
* igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
- shard-glk: PASS -> FAIL [fdo#103166] +1
* igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
- shard-apl: PASS -> FAIL [fdo#103166]
* igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
- {shard-iclb}: PASS -> FAIL [fdo#103166]
* igt@kms_plane_multiple@atomic-pipe-c-tiling-y:
- shard-skl: NOTRUN -> FAIL [fdo#103166] / [fdo#107815]
* {igt@kms_rotation_crc@multiplane-rotation-cropping-top}:
- shard-kbl: PASS -> DMESG-FAIL [fdo#108950]
* igt@kms_setmode@basic:
- shard-apl: PASS -> FAIL [fdo#99912]
- shard-kbl: PASS -> FAIL [fdo#99912]
* igt@pm_rpm@debugfs-read:
- shard-skl: PASS -> INCOMPLETE [fdo#107807]
* igt@pm_rpm@dpms-lpsp:
- {shard-iclb}: PASS -> DMESG-WARN [fdo#108654]
* igt@pm_rpm@universal-planes-dpms:
- shard-skl: PASS -> DMESG-WARN [fdo#105541] +1
- shard-apl: PASS -> DMESG-WARN [fdo#108337] +3
#### Possible fixes ####
* igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
- shard-apl: FAIL [fdo#105458] / [fdo#106510] -> PASS
* igt@kms_color@pipe-c-legacy-gamma:
- shard-apl: FAIL [fdo#104782] -> PASS
* igt@kms_cursor_crc@cursor-128x42-onscreen:
- shard-glk: FAIL [fdo#103232] -> PASS +2
- shard-apl: FAIL [fdo#103232] -> PASS +1
* igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-untiled:
- {shard-iclb}: WARN [fdo#108336] -> PASS
* igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
- shard-glk: FAIL [fdo#105363] -> PASS
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt:
- {shard-iclb}: DMESG-FAIL [fdo#107724] -> PASS +4
* igt@kms_frontbuffer_tracking@fbc-rgb565-draw-render:
- shard-glk: FAIL [fdo#103167] -> PASS +1
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-blt:
- {shard-iclb}: FAIL [fdo#103167] -> PASS
* igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
- shard-apl: FAIL [fdo#103166] -> PASS +2
* igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
- shard-glk: FAIL [fdo#103166] -> PASS +1
* igt@kms_plane_scaling@pipe-b-plane-scaling:
- {shard-iclb}: DMESG-WARN [fdo#107724] -> PASS +8
* igt@kms_psr@no_drrs:
- {shard-iclb}: FAIL [fdo#108341] -> PASS
* igt@kms_rotation_crc@primary-rotation-180:
- {shard-iclb}: DMESG-WARN [fdo#107724] / [fdo#108336] -> PASS +4
* igt@kms_setmode@basic:
- shard-glk: FAIL [fdo#99912] -> PASS
* igt@kms_universal_plane@universal-plane-pipe-a-functional:
- {shard-iclb}: DMESG-FAIL [fdo#103166] / [fdo#107724] -> PASS
* igt@perf@polling:
- shard-hsw: FAIL [fdo#102252] -> PASS
#### Warnings ####
* igt@i915_suspend@shrink:
- shard-glk: DMESG-WARN [fdo#108784] -> INCOMPLETE [fdo#103359] / [fdo#106886] / [k.org#198133]
* igt@kms_cursor_crc@cursor-64x64-onscreen:
- {shard-iclb}: DMESG-WARN [fdo#107724] / [fdo#108336] -> FAIL [fdo#103232]
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
- shard-apl: FAIL [fdo#103167] -> DMESG-FAIL [fdo#103167] +1
* igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-fullscreen:
- shard-glk: FAIL [fdo#103167] -> DMESG-FAIL [fdo#103167] / [fdo#105763] / [fdo#106538] +2
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-mmap-gtt:
- shard-skl: FAIL [fdo#103167] -> DMESG-FAIL [fdo#103167] / [fdo#105541]
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-pwrite:
- {shard-iclb}: FAIL [fdo#103167] -> DMESG-FAIL [fdo#103167] / [fdo#107724]
* igt@kms_plane@plane-position-covered-pipe-c-planes:
- shard-apl: FAIL [fdo#103166] -> DMESG-FAIL [fdo#103166] +1
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#102252]: https://bugs.freedesktop.org/show_bug.cgi?id=102252
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
[fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
[fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#104671]: https://bugs.freedesktop.org/show_bug.cgi?id=104671
[fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#105458]: https://bugs.freedesktop.org/show_bug.cgi?id=105458
[fdo#105541]: https://bugs.freedesktop.org/show_bug.cgi?id=105541
[fdo#105681]: https://bugs.freedesktop.org/show_bug.cgi?id=105681
[fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
[fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
[fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
[fdo#106641]: https://bugs.freedesktop.org/show_bug.cgi?id=106641
[fdo#106885]: https://bugs.freedesktop.org/show_bug.cgi?id=106885
[fdo#106886]: https://bugs.freedesktop.org/show_bug.cgi?id=106886
[fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
[fdo#107799]: https://bugs.freedesktop.org/show_bug.cgi?id=107799
[fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
[fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108336]: https://bugs.freedesktop.org/show_bug.cgi?id=108336
[fdo#108337]: https://bugs.freedesktop.org/show_bug.cgi?id=108337
[fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
[fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
[fdo#108654]: https://bugs.freedesktop.org/show_bug.cgi?id=108654
[fdo#108784]: https://bugs.freedesktop.org/show_bug.cgi?id=108784
[fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
[fdo#108950]: https://bugs.freedesktop.org/show_bug.cgi?id=108950
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
[k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133
Participating hosts (7 -> 7)
------------------------------
No changes in participating hosts
Build changes
-------------
* Linux: CI_DRM_5294 -> Patchwork_11061
CI_DRM_5294: 791e2289df7e8ec34fb65251bb8ad30ceb28aba3 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4744: 4579ac1d445cf39f6de474071b20db790db575bd @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_11061: 847ef79c984b4068d82649abfaa6f99261af5a99 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11061/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Switch to level-based DDB allocation algorithm (v3)
2018-12-11 1:05 ` [PATCH v2 2/2] drm/i915: Switch to level-based DDB allocation algorithm (v3) Matt Roper
@ 2018-12-11 15:59 ` Ville Syrjälä
2018-12-11 16:11 ` Matt Roper
0 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2018-12-11 15:59 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
On Mon, Dec 10, 2018 at 05:05:43PM -0800, Matt Roper wrote:
> The DDB allocation algorithm currently used by the driver grants each
> plane a very small minimum allocation of DDB blocks and then divies up
> all of the remaining blocks based on the percentage of the total data
> rate that the plane makes up. It turns out that this proportional
> allocation approach is overly-generous with the larger planes and can
> leave very small planes wthout a big enough allocation to even hit their
> level 0 watermark requirements (especially on APL, which has a smaller
> DDB in general than other gen9 platforms). Or there can be situations
> where the smallest planes hit a lower watermark level than they should
> have been able to hit with a more equitable division of DDB blocks, thus
> limiting the overall system sleep state that can be achieved.
>
> The bspec now describes an alternate algorithm that can be used to
> overcome these types of issues. With the new algorithm, we calculate
> all plane watermark values for all wm levels first, then go back and
> partition a pipe's DDB space second. The DDB allocation will calculate
> what the highest watermark level that can be achieved on *all* active
> planes, and then grant the blocks necessary to hit that level to each
> plane. Any remaining blocks are then divided up proportionally
> according to data rate, similar to the old algorithm.
>
> There was a previous attempt to implement this algorithm a couple years
> ago in bb9d85f6e9d ("drm/i915/skl: New ddb allocation algorithm"), but
> some regressions were reported, the patch was reverted, and nobody
> ever got around to figuring out exactly where the bug was in that
> version. Our watermark code has evolved significantly in the meantime,
> but we're still getting bug reports caused by the unfair proportional
> algorithm, so let's give this another shot.
>
> v2:
> - Make sure cursor allocation stays constant and fixed at the end of
> the pipe allocation.
> - Fix some watermark level iterators that weren't handling the max
> level.
>
> v3:
> - Ensure we don't leave any DDB blocks unused by using DIV_ROUND_UP+min
> to calculate the extra blocks for each plane. (Ville)
> - Replace a while() loop with a for() loop to be more consistent with
> surrounding code. (Ville)
> - Clean unattainable watermark levels with memset rather than directly
> clearing the member fields. Also do the same for the transition
> watermark values if they can't be achieved. (Ville)
> - Drop min_disp_buf_needed calculations in skl_compute_plane_wm() since
> the results are no longer needed or used. (Ville)
> - Drop skl_latency[0] != 0 sanity check; both watermark methods already
> account for an invalid 0 latency by returning FP_16_16_MAX. (Ville)
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105458
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 376 ++++++++++++++--------------------------
> 1 file changed, 132 insertions(+), 244 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bf970cf7b8a5..f5f86757457d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4301,102 +4301,6 @@ icl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
> return total_data_rate;
> }
>
> -static uint16_t
> -skl_ddb_min_alloc(const struct intel_plane_state *plane_state, const int plane)
> -{
> - struct drm_framebuffer *fb = plane_state->base.fb;
> - uint32_t src_w, src_h;
> - uint32_t min_scanlines = 8;
> - uint8_t plane_bpp;
> -
> - if (WARN_ON(!fb))
> - return 0;
> -
> - /* For packed formats, and uv-plane, return 0 */
> - if (plane == 1 && fb->format->format != DRM_FORMAT_NV12)
> - return 0;
> -
> - /* For Non Y-tile return 8-blocks */
> - if (fb->modifier != I915_FORMAT_MOD_Y_TILED &&
> - fb->modifier != I915_FORMAT_MOD_Yf_TILED &&
> - fb->modifier != I915_FORMAT_MOD_Y_TILED_CCS &&
> - fb->modifier != I915_FORMAT_MOD_Yf_TILED_CCS)
> - return 8;
> -
> - /*
> - * Src coordinates are already rotated by 270 degrees for
> - * the 90/270 degree plane rotation cases (to match the
> - * GTT mapping), hence no need to account for rotation here.
> - */
> - src_w = drm_rect_width(&plane_state->base.src) >> 16;
> - src_h = drm_rect_height(&plane_state->base.src) >> 16;
> -
> - /* Halve UV plane width and height for NV12 */
> - if (plane == 1) {
> - src_w /= 2;
> - src_h /= 2;
> - }
> -
> - plane_bpp = fb->format->cpp[plane];
> -
> - if (drm_rotation_90_or_270(plane_state->base.rotation)) {
> - switch (plane_bpp) {
> - case 1:
> - min_scanlines = 32;
> - break;
> - case 2:
> - min_scanlines = 16;
> - break;
> - case 4:
> - min_scanlines = 8;
> - break;
> - case 8:
> - min_scanlines = 4;
> - break;
> - default:
> - WARN(1, "Unsupported pixel depth %u for rotation",
> - plane_bpp);
> - min_scanlines = 32;
> - }
> - }
> -
> - return DIV_ROUND_UP((4 * src_w * plane_bpp), 512) * min_scanlines/4 + 3;
> -}
> -
> -static void
> -skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
> - uint16_t *minimum, uint16_t *uv_minimum)
> -{
> - const struct drm_plane_state *pstate;
> - struct drm_plane *plane;
> -
> - drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
> - enum plane_id plane_id = to_intel_plane(plane)->id;
> - struct intel_plane_state *plane_state = to_intel_plane_state(pstate);
> -
> - if (plane_id == PLANE_CURSOR)
> - continue;
> -
> - /* slave plane must be invisible and calculated from master */
> - if (!pstate->visible || WARN_ON(plane_state->slave))
> - continue;
> -
> - if (!plane_state->linked_plane) {
> - minimum[plane_id] = skl_ddb_min_alloc(plane_state, 0);
> - uv_minimum[plane_id] =
> - skl_ddb_min_alloc(plane_state, 1);
> - } else {
> - enum plane_id y_plane_id =
> - plane_state->linked_plane->id;
> -
> - minimum[y_plane_id] = skl_ddb_min_alloc(plane_state, 0);
> - minimum[plane_id] = skl_ddb_min_alloc(plane_state, 1);
> - }
> - }
> -
> - minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
> -}
> -
> static int
> skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> struct skl_ddb_allocation *ddb /* out */)
> @@ -4406,15 +4310,17 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
> - uint16_t alloc_size, start;
> - uint16_t minimum[I915_MAX_PLANES] = {};
> - uint16_t uv_minimum[I915_MAX_PLANES] = {};
> + struct skl_plane_wm *wm;
> + uint16_t alloc_size, start = 0;
> + uint16_t total[I915_MAX_PLANES] = {};
> + uint16_t uv_total[I915_MAX_PLANES] = {};
> u64 total_data_rate;
> enum plane_id plane_id;
> int num_active;
> u64 plane_data_rate[I915_MAX_PLANES] = {};
> u64 uv_plane_data_rate[I915_MAX_PLANES] = {};
> - uint16_t total_min_blocks = 0;
> + uint16_t blocks = 0;
> + int level;
>
> /* Clear the partitioning for disabled planes. */
> memset(cstate->wm.skl.plane_ddb_y, 0, sizeof(cstate->wm.skl.plane_ddb_y));
> @@ -4444,81 +4350,125 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> if (alloc_size == 0)
> return 0;
>
> - skl_ddb_calc_min(cstate, num_active, minimum, uv_minimum);
> + /* Allocate fixed number of blocks for cursor. */
> + total[PLANE_CURSOR] = skl_cursor_allocation(num_active);
> + alloc_size -= total[PLANE_CURSOR];
> + cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].start =
> + alloc->end - total[PLANE_CURSOR];
> + cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].end = alloc->end;
> +
> + if (total_data_rate == 0)
> + return 0;
>
> /*
> - * 1. Allocate the mininum required blocks for each active plane
> - * and allocate the cursor, it doesn't require extra allocation
> - * proportional to the data rate.
> + * Find the highest watermark level for which we can satisfy the block
> + * requirement of active planes.
> */
> + for (level = ilk_wm_max_level(dev_priv); level >= 0; level--) {
> + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> + if (plane_id == PLANE_CURSOR)
> + continue;
>
> - for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> - total_min_blocks += minimum[plane_id];
> - total_min_blocks += uv_minimum[plane_id];
> + wm = &cstate->wm.skl.optimal.planes[plane_id];
> + blocks += wm->wm[level].plane_res_b;
> + blocks += wm->uv_wm[level].plane_res_b;
> + }
> +
> + if (blocks < alloc_size) {
> + alloc_size -= blocks;
> + break;
> + }
> }
>
> - if (total_min_blocks > alloc_size) {
> + if (level < 0) {
> DRM_DEBUG_KMS("Requested display configuration exceeds system DDB limitations");
> - DRM_DEBUG_KMS("minimum required %d/%d\n", total_min_blocks,
> - alloc_size);
> + DRM_DEBUG_KMS("minimum required %d/%d\n", blocks,
> + alloc_size);
> return -EINVAL;
> }
>
> - alloc_size -= total_min_blocks;
> - cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
> - cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].end = alloc->end;
> -
> /*
> - * 2. Distribute the remaining space in proportion to the amount of
> - * data each plane needs to fetch from memory.
> - *
> - * FIXME: we may not allocate every single block here.
> + * Grant each plane the blocks it requires at the highest achievable
> + * watermark level, plus an extra share of the leftover blocks
> + * proportional to its relative data rate.
> */
> - if (total_data_rate == 0)
> - return 0;
> -
> - start = alloc->start;
> for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> - u64 data_rate, uv_data_rate;
> - uint16_t plane_blocks, uv_plane_blocks;
> + u64 rate;
> + u16 extra;
>
> if (plane_id == PLANE_CURSOR)
> continue;
> + if (alloc_size == 0)
> + continue;
This seems wrong. We wouldn't assign anything to total/uv_total for the
plane in this case. I guess you could move those assignments to the
earlier loop and then s/continue/break/ here? Or we just remove the
continue entirely and let the calculations go through even if
alloc_size==0.
>
> - data_rate = plane_data_rate[plane_id];
> + wm = &cstate->wm.skl.optimal.planes[plane_id];
>
> - /*
> - * allocation for (packed formats) or (uv-plane part of planar format):
> - * promote the expression to 64 bits to avoid overflowing, the
> - * result is < available as data_rate / total_data_rate < 1
> - */
> - plane_blocks = minimum[plane_id];
> - plane_blocks += div64_u64(alloc_size * data_rate, total_data_rate);
> + rate = plane_data_rate[plane_id];
> + extra = min_t(u16, alloc_size,
> + DIV_ROUND_UP(alloc_size * rate, total_data_rate));
> + total[plane_id] = wm->wm[level].plane_res_b + extra;
> + alloc_size -= extra;
> + total_data_rate -= rate;
>
> - /* Leave disabled planes at (0,0) */
> - if (data_rate) {
> - cstate->wm.skl.plane_ddb_y[plane_id].start = start;
> - cstate->wm.skl.plane_ddb_y[plane_id].end = start + plane_blocks;
> - }
> + if (alloc_size == 0)
> + continue;
>
> - start += plane_blocks;
> + rate = uv_plane_data_rate[plane_id];
> + extra = min_t(u16, alloc_size,
> + DIV_ROUND_UP(alloc_size * rate, total_data_rate));
> + uv_total[plane_id] = wm->uv_wm[level].plane_res_b + extra;
> + alloc_size -= extra;
> + total_data_rate -= rate;
> + }
> + WARN_ON(alloc_size != 0 || total_data_rate != 0);
>
> - /* Allocate DDB for UV plane for planar format/NV12 */
> - uv_data_rate = uv_plane_data_rate[plane_id];
> + /* Set the actual DDB start/end points for each plane */
> + start = alloc->start;
> + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> + struct skl_ddb_entry *plane_alloc, *uv_plane_alloc;
>
> - uv_plane_blocks = uv_minimum[plane_id];
> - uv_plane_blocks += div64_u64(alloc_size * uv_data_rate, total_data_rate);
> + if (plane_id == PLANE_CURSOR)
> + continue;
> +
> + plane_alloc = &cstate->wm.skl.plane_ddb_y[plane_id];
> + uv_plane_alloc = &cstate->wm.skl.plane_ddb_uv[plane_id];
>
> /* Gen11+ uses a separate plane for UV watermarks */
> - WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_plane_blocks);
> + WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_total[plane_id]);
> +
> + /* Leave disabled planes at (0,0) */
> + if (total[plane_id]) {
> + plane_alloc->start = start;
> + plane_alloc->end = start += total[plane_id];
> + }
>
> - if (uv_data_rate) {
> - cstate->wm.skl.plane_ddb_uv[plane_id].start = start;
> - cstate->wm.skl.plane_ddb_uv[plane_id].end =
> - start + uv_plane_blocks;
> + if (uv_total[plane_id]) {
> + uv_plane_alloc->start = start;
> + uv_plane_alloc->end = start + uv_total[plane_id];
> }
> + }
>
> - start += uv_plane_blocks;
> + /*
> + * When we calculated watermark values we didn't know how high
> + * of a level we'd actually be able to hit, so we just marked
> + * all levels as "enabled." Go back now and disable the ones
> + * that aren't actually possible.
> + */
> + for (level++; level <= ilk_wm_max_level(dev_priv); level++) {
> + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> + wm = &cstate->wm.skl.optimal.planes[plane_id];
> + memset(&wm->wm[level], 0, sizeof(wm->wm[level]));
> + }
> + }
> +
> + /*
> + * Go back and disable the transition watermark if it turns out we
> + * don't have enough DDB blocks for it.
> + */
> + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> + wm = &cstate->wm.skl.optimal.planes[plane_id];
> + if (wm->trans_wm.plane_res_b > total[plane_id])
> + memset(&wm->trans_wm, 0, sizeof(wm->trans_wm));
> }
>
> return 0;
> @@ -4715,17 +4665,15 @@ skl_compute_plane_wm_params(const struct intel_crtc_state *cstate,
> return 0;
> }
>
> -static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> - const struct intel_plane_state *intel_pstate,
> - uint16_t ddb_allocation,
> - int level,
> - const struct skl_wm_params *wp,
> - const struct skl_wm_level *result_prev,
> - struct skl_wm_level *result /* out */)
> +static void skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> + const struct intel_plane_state *intel_pstate,
> + int level,
> + const struct skl_wm_params *wp,
> + const struct skl_wm_level *result_prev,
> + struct skl_wm_level *result /* out */)
> {
> struct drm_i915_private *dev_priv =
> to_i915(intel_pstate->base.plane->dev);
> - const struct drm_plane_state *pstate = &intel_pstate->base;
> uint32_t latency = dev_priv->wm.skl_latency[level];
> uint_fixed_16_16_t method1, method2;
> uint_fixed_16_16_t selected_result;
> @@ -4733,10 +4681,6 @@ static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> struct intel_atomic_state *state =
> to_intel_atomic_state(cstate->base.state);
> bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
> - uint32_t min_disp_buf_needed;
> -
> - if (latency == 0)
> - return level == 0 ? -EINVAL : 0;
>
> /* Display WA #1141: kbl,cfl */
> if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
> @@ -4800,61 +4744,24 @@ static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> res_blocks = result_prev->plane_res_b;
> }
>
> - if (INTEL_GEN(dev_priv) >= 11) {
> - if (wp->y_tiled) {
> - uint32_t extra_lines;
> - uint_fixed_16_16_t fp_min_disp_buf_needed;
> -
> - if (res_lines % wp->y_min_scanlines == 0)
> - extra_lines = wp->y_min_scanlines;
> - else
> - extra_lines = wp->y_min_scanlines * 2 -
> - res_lines % wp->y_min_scanlines;
> -
> - fp_min_disp_buf_needed = mul_u32_fixed16(res_lines +
> - extra_lines,
> - wp->plane_blocks_per_line);
> - min_disp_buf_needed = fixed16_to_u32_round_up(
> - fp_min_disp_buf_needed);
> - } else {
> - min_disp_buf_needed = DIV_ROUND_UP(res_blocks * 11, 10);
> - }
> - } else {
> - min_disp_buf_needed = res_blocks;
> - }
> -
> - if ((level > 0 && res_lines > 31) ||
> - res_blocks >= ddb_allocation ||
> - min_disp_buf_needed >= ddb_allocation) {
> - /*
> - * If there are no valid level 0 watermarks, then we can't
> - * support this display configuration.
> - */
> - if (level) {
> - return 0;
> - } else {
> - struct drm_plane *plane = pstate->plane;
> -
> - DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
> - DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
> - plane->base.id, plane->name,
> - res_blocks, ddb_allocation, res_lines);
> - return -EINVAL;
> - }
> - }
> -
> /* The number of lines are ignored for the level 0 watermark. */
> + if (level > 0 && res_lines > 31)
> + return;
> +
> + /*
> + * If res_lines is valid, assume we can use this watermark level
> + * for now. We'll come back and disable it after we calculate the
> + * DDB allocation if it turns out we don't actually have enough
> + * blocks to satisfy it.
> + */
> result->plane_res_b = res_blocks;
> result->plane_res_l = res_lines;
> result->plane_en = true;
> -
> - return 0;
> }
>
> -static int
> +static void
> skl_compute_wm_levels(const struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate,
> - uint16_t ddb_blocks,
> const struct skl_wm_params *wm_params,
> struct skl_wm_level *levels)
> {
> @@ -4862,25 +4769,15 @@ skl_compute_wm_levels(const struct intel_crtc_state *cstate,
> to_i915(intel_pstate->base.plane->dev);
> int level, max_level = ilk_wm_max_level(dev_priv);
> struct skl_wm_level *result_prev = &levels[0];
> - int ret;
>
> for (level = 0; level <= max_level; level++) {
> struct skl_wm_level *result = &levels[level];
>
> - ret = skl_compute_plane_wm(cstate,
> - intel_pstate,
> - ddb_blocks,
> - level,
> - wm_params,
> - result_prev,
> - result);
> - if (ret)
> - return ret;
> + skl_compute_plane_wm(cstate, intel_pstate, level, wm_params,
> + result_prev, result);
>
> result_prev = result;
> }
> -
> - return 0;
> }
>
> static uint32_t
> @@ -4908,8 +4805,7 @@ skl_compute_linetime_wm(const struct intel_crtc_state *cstate)
>
> static void skl_compute_transition_wm(const struct intel_crtc_state *cstate,
> const struct skl_wm_params *wp,
> - struct skl_plane_wm *wm,
> - uint16_t ddb_allocation)
> + struct skl_plane_wm *wm)
> {
> struct drm_device *dev = cstate->base.crtc->dev;
> const struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -4957,12 +4853,13 @@ static void skl_compute_transition_wm(const struct intel_crtc_state *cstate,
>
> }
>
> - res_blocks += 1;
> -
> - if (res_blocks < ddb_allocation) {
> - wm->trans_wm.plane_res_b = res_blocks;
> - wm->trans_wm.plane_en = true;
> - }
> + /*
> + * Just assume we can enable the transition watermark. After
> + * computing the DDB we'll come back and disable it if that
> + * assumption turns out to be false.
> + */
> + wm->trans_wm.plane_res_b = res_blocks + 1;
> + wm->trans_wm.plane_en = true;
> }
>
> static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> @@ -4970,7 +4867,6 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> enum plane_id plane_id, int color_plane)
> {
> struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> - u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_y[plane_id]);
> struct skl_wm_params wm_params;
> int ret;
>
> @@ -4979,12 +4875,8 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> if (ret)
> return ret;
>
> - ret = skl_compute_wm_levels(crtc_state, plane_state,
> - ddb_blocks, &wm_params, wm->wm);
> - if (ret)
> - return ret;
> -
> - skl_compute_transition_wm(crtc_state, &wm_params, wm, ddb_blocks);
> + skl_compute_wm_levels(crtc_state, plane_state, &wm_params, wm->wm);
> + skl_compute_transition_wm(crtc_state, &wm_params, wm);
>
> return 0;
> }
> @@ -4994,7 +4886,6 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
> enum plane_id plane_id)
> {
> struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> - u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_uv[plane_id]);
> struct skl_wm_params wm_params;
> int ret;
>
> @@ -5006,10 +4897,7 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
> if (ret)
> return ret;
>
> - ret = skl_compute_wm_levels(crtc_state, plane_state,
> - ddb_blocks, &wm_params, wm->uv_wm);
> - if (ret)
> - return ret;
> + skl_compute_wm_levels(crtc_state, plane_state, &wm_params, wm->uv_wm);
>
> return 0;
> }
> @@ -5521,13 +5409,9 @@ skl_compute_wm(struct intel_atomic_state *state)
> if (ret || !changed)
> return ret;
>
> - ret = skl_compute_ddb(state);
> - if (ret)
> - return ret;
> -
> /*
> * Calculate WM's for all pipes that are part of this transaction.
> - * Note that the DDB allocation above may have added more CRTC's that
> + * Note that skl_ddb_add_affected_pipes may have added more CRTC's that
> * weren't otherwise being modified (and set bits in dirty_pipes) if
> * pipe allocations had to change.
> */
> @@ -5549,6 +5433,10 @@ skl_compute_wm(struct intel_atomic_state *state)
> results->dirty_pipes |= drm_crtc_mask(&crtc->base);
> }
>
> + ret = skl_compute_ddb(state);
> + if (ret)
> + return ret;
> +
> skl_print_wm_changes(state);
>
> return 0;
> --
> 2.14.4
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Switch to level-based DDB allocation algorithm (v3)
2018-12-11 15:59 ` Ville Syrjälä
@ 2018-12-11 16:11 ` Matt Roper
2018-12-11 16:21 ` Ville Syrjälä
0 siblings, 1 reply; 9+ messages in thread
From: Matt Roper @ 2018-12-11 16:11 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Tue, Dec 11, 2018 at 05:59:56PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 10, 2018 at 05:05:43PM -0800, Matt Roper wrote:
...snip...
> >
> > - alloc_size -= total_min_blocks;
> > - cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
> > - cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].end = alloc->end;
> > -
> > /*
> > - * 2. Distribute the remaining space in proportion to the amount of
> > - * data each plane needs to fetch from memory.
> > - *
> > - * FIXME: we may not allocate every single block here.
> > + * Grant each plane the blocks it requires at the highest achievable
> > + * watermark level, plus an extra share of the leftover blocks
> > + * proportional to its relative data rate.
> > */
> > - if (total_data_rate == 0)
> > - return 0;
> > -
> > - start = alloc->start;
> > for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > - u64 data_rate, uv_data_rate;
> > - uint16_t plane_blocks, uv_plane_blocks;
> > + u64 rate;
> > + u16 extra;
> >
> > if (plane_id == PLANE_CURSOR)
> > continue;
> > + if (alloc_size == 0)
> > + continue;
>
> This seems wrong. We wouldn't assign anything to total/uv_total for the
> plane in this case. I guess you could move those assignments to the
> earlier loop and then s/continue/break/ here? Or we just remove the
> continue entirely and let the calculations go through even if
> alloc_size==0.
It should probably be a break, since we'll only hit this on a loop
iteration where we've handed the whole pipe allocation and all remaining
planes are disabled. The total and uv_total were initialized to 0 at
initialization time, so that should be correct for all remaining planes.
Also, we can't let the calculation proceed here, otherwise we'll divide
by 0 (total_data_rate) farther down since that value also decreases with
each loop iteration.
Matt
>
> >
> > - data_rate = plane_data_rate[plane_id];
> > + wm = &cstate->wm.skl.optimal.planes[plane_id];
> >
> > - /*
> > - * allocation for (packed formats) or (uv-plane part of planar format):
> > - * promote the expression to 64 bits to avoid overflowing, the
> > - * result is < available as data_rate / total_data_rate < 1
> > - */
> > - plane_blocks = minimum[plane_id];
> > - plane_blocks += div64_u64(alloc_size * data_rate, total_data_rate);
> > + rate = plane_data_rate[plane_id];
> > + extra = min_t(u16, alloc_size,
> > + DIV_ROUND_UP(alloc_size * rate, total_data_rate));
> > + total[plane_id] = wm->wm[level].plane_res_b + extra;
> > + alloc_size -= extra;
> > + total_data_rate -= rate;
> >
> > - /* Leave disabled planes at (0,0) */
> > - if (data_rate) {
> > - cstate->wm.skl.plane_ddb_y[plane_id].start = start;
> > - cstate->wm.skl.plane_ddb_y[plane_id].end = start + plane_blocks;
> > - }
> > + if (alloc_size == 0)
> > + continue;
> >
> > - start += plane_blocks;
> > + rate = uv_plane_data_rate[plane_id];
> > + extra = min_t(u16, alloc_size,
> > + DIV_ROUND_UP(alloc_size * rate, total_data_rate));
> > + uv_total[plane_id] = wm->uv_wm[level].plane_res_b + extra;
> > + alloc_size -= extra;
> > + total_data_rate -= rate;
> > + }
> > + WARN_ON(alloc_size != 0 || total_data_rate != 0);
> >
> > - /* Allocate DDB for UV plane for planar format/NV12 */
> > - uv_data_rate = uv_plane_data_rate[plane_id];
> > + /* Set the actual DDB start/end points for each plane */
> > + start = alloc->start;
> > + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > + struct skl_ddb_entry *plane_alloc, *uv_plane_alloc;
> >
> > - uv_plane_blocks = uv_minimum[plane_id];
> > - uv_plane_blocks += div64_u64(alloc_size * uv_data_rate, total_data_rate);
> > + if (plane_id == PLANE_CURSOR)
> > + continue;
> > +
> > + plane_alloc = &cstate->wm.skl.plane_ddb_y[plane_id];
> > + uv_plane_alloc = &cstate->wm.skl.plane_ddb_uv[plane_id];
> >
> > /* Gen11+ uses a separate plane for UV watermarks */
> > - WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_plane_blocks);
> > + WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_total[plane_id]);
> > +
> > + /* Leave disabled planes at (0,0) */
> > + if (total[plane_id]) {
> > + plane_alloc->start = start;
> > + plane_alloc->end = start += total[plane_id];
> > + }
> >
> > - if (uv_data_rate) {
> > - cstate->wm.skl.plane_ddb_uv[plane_id].start = start;
> > - cstate->wm.skl.plane_ddb_uv[plane_id].end =
> > - start + uv_plane_blocks;
> > + if (uv_total[plane_id]) {
> > + uv_plane_alloc->start = start;
> > + uv_plane_alloc->end = start + uv_total[plane_id];
> > }
> > + }
> >
> > - start += uv_plane_blocks;
> > + /*
> > + * When we calculated watermark values we didn't know how high
> > + * of a level we'd actually be able to hit, so we just marked
> > + * all levels as "enabled." Go back now and disable the ones
> > + * that aren't actually possible.
> > + */
> > + for (level++; level <= ilk_wm_max_level(dev_priv); level++) {
> > + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > + wm = &cstate->wm.skl.optimal.planes[plane_id];
> > + memset(&wm->wm[level], 0, sizeof(wm->wm[level]));
> > + }
> > + }
> > +
> > + /*
> > + * Go back and disable the transition watermark if it turns out we
> > + * don't have enough DDB blocks for it.
> > + */
> > + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > + wm = &cstate->wm.skl.optimal.planes[plane_id];
> > + if (wm->trans_wm.plane_res_b > total[plane_id])
> > + memset(&wm->trans_wm, 0, sizeof(wm->trans_wm));
> > }
> >
> > return 0;
> > @@ -4715,17 +4665,15 @@ skl_compute_plane_wm_params(const struct intel_crtc_state *cstate,
> > return 0;
> > }
> >
> > -static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > - const struct intel_plane_state *intel_pstate,
> > - uint16_t ddb_allocation,
> > - int level,
> > - const struct skl_wm_params *wp,
> > - const struct skl_wm_level *result_prev,
> > - struct skl_wm_level *result /* out */)
> > +static void skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > + const struct intel_plane_state *intel_pstate,
> > + int level,
> > + const struct skl_wm_params *wp,
> > + const struct skl_wm_level *result_prev,
> > + struct skl_wm_level *result /* out */)
> > {
> > struct drm_i915_private *dev_priv =
> > to_i915(intel_pstate->base.plane->dev);
> > - const struct drm_plane_state *pstate = &intel_pstate->base;
> > uint32_t latency = dev_priv->wm.skl_latency[level];
> > uint_fixed_16_16_t method1, method2;
> > uint_fixed_16_16_t selected_result;
> > @@ -4733,10 +4681,6 @@ static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > struct intel_atomic_state *state =
> > to_intel_atomic_state(cstate->base.state);
> > bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
> > - uint32_t min_disp_buf_needed;
> > -
> > - if (latency == 0)
> > - return level == 0 ? -EINVAL : 0;
> >
> > /* Display WA #1141: kbl,cfl */
> > if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
> > @@ -4800,61 +4744,24 @@ static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > res_blocks = result_prev->plane_res_b;
> > }
> >
> > - if (INTEL_GEN(dev_priv) >= 11) {
> > - if (wp->y_tiled) {
> > - uint32_t extra_lines;
> > - uint_fixed_16_16_t fp_min_disp_buf_needed;
> > -
> > - if (res_lines % wp->y_min_scanlines == 0)
> > - extra_lines = wp->y_min_scanlines;
> > - else
> > - extra_lines = wp->y_min_scanlines * 2 -
> > - res_lines % wp->y_min_scanlines;
> > -
> > - fp_min_disp_buf_needed = mul_u32_fixed16(res_lines +
> > - extra_lines,
> > - wp->plane_blocks_per_line);
> > - min_disp_buf_needed = fixed16_to_u32_round_up(
> > - fp_min_disp_buf_needed);
> > - } else {
> > - min_disp_buf_needed = DIV_ROUND_UP(res_blocks * 11, 10);
> > - }
> > - } else {
> > - min_disp_buf_needed = res_blocks;
> > - }
> > -
> > - if ((level > 0 && res_lines > 31) ||
> > - res_blocks >= ddb_allocation ||
> > - min_disp_buf_needed >= ddb_allocation) {
> > - /*
> > - * If there are no valid level 0 watermarks, then we can't
> > - * support this display configuration.
> > - */
> > - if (level) {
> > - return 0;
> > - } else {
> > - struct drm_plane *plane = pstate->plane;
> > -
> > - DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
> > - DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
> > - plane->base.id, plane->name,
> > - res_blocks, ddb_allocation, res_lines);
> > - return -EINVAL;
> > - }
> > - }
> > -
> > /* The number of lines are ignored for the level 0 watermark. */
> > + if (level > 0 && res_lines > 31)
> > + return;
> > +
> > + /*
> > + * If res_lines is valid, assume we can use this watermark level
> > + * for now. We'll come back and disable it after we calculate the
> > + * DDB allocation if it turns out we don't actually have enough
> > + * blocks to satisfy it.
> > + */
> > result->plane_res_b = res_blocks;
> > result->plane_res_l = res_lines;
> > result->plane_en = true;
> > -
> > - return 0;
> > }
> >
> > -static int
> > +static void
> > skl_compute_wm_levels(const struct intel_crtc_state *cstate,
> > const struct intel_plane_state *intel_pstate,
> > - uint16_t ddb_blocks,
> > const struct skl_wm_params *wm_params,
> > struct skl_wm_level *levels)
> > {
> > @@ -4862,25 +4769,15 @@ skl_compute_wm_levels(const struct intel_crtc_state *cstate,
> > to_i915(intel_pstate->base.plane->dev);
> > int level, max_level = ilk_wm_max_level(dev_priv);
> > struct skl_wm_level *result_prev = &levels[0];
> > - int ret;
> >
> > for (level = 0; level <= max_level; level++) {
> > struct skl_wm_level *result = &levels[level];
> >
> > - ret = skl_compute_plane_wm(cstate,
> > - intel_pstate,
> > - ddb_blocks,
> > - level,
> > - wm_params,
> > - result_prev,
> > - result);
> > - if (ret)
> > - return ret;
> > + skl_compute_plane_wm(cstate, intel_pstate, level, wm_params,
> > + result_prev, result);
> >
> > result_prev = result;
> > }
> > -
> > - return 0;
> > }
> >
> > static uint32_t
> > @@ -4908,8 +4805,7 @@ skl_compute_linetime_wm(const struct intel_crtc_state *cstate)
> >
> > static void skl_compute_transition_wm(const struct intel_crtc_state *cstate,
> > const struct skl_wm_params *wp,
> > - struct skl_plane_wm *wm,
> > - uint16_t ddb_allocation)
> > + struct skl_plane_wm *wm)
> > {
> > struct drm_device *dev = cstate->base.crtc->dev;
> > const struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -4957,12 +4853,13 @@ static void skl_compute_transition_wm(const struct intel_crtc_state *cstate,
> >
> > }
> >
> > - res_blocks += 1;
> > -
> > - if (res_blocks < ddb_allocation) {
> > - wm->trans_wm.plane_res_b = res_blocks;
> > - wm->trans_wm.plane_en = true;
> > - }
> > + /*
> > + * Just assume we can enable the transition watermark. After
> > + * computing the DDB we'll come back and disable it if that
> > + * assumption turns out to be false.
> > + */
> > + wm->trans_wm.plane_res_b = res_blocks + 1;
> > + wm->trans_wm.plane_en = true;
> > }
> >
> > static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> > @@ -4970,7 +4867,6 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> > enum plane_id plane_id, int color_plane)
> > {
> > struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> > - u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_y[plane_id]);
> > struct skl_wm_params wm_params;
> > int ret;
> >
> > @@ -4979,12 +4875,8 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> > if (ret)
> > return ret;
> >
> > - ret = skl_compute_wm_levels(crtc_state, plane_state,
> > - ddb_blocks, &wm_params, wm->wm);
> > - if (ret)
> > - return ret;
> > -
> > - skl_compute_transition_wm(crtc_state, &wm_params, wm, ddb_blocks);
> > + skl_compute_wm_levels(crtc_state, plane_state, &wm_params, wm->wm);
> > + skl_compute_transition_wm(crtc_state, &wm_params, wm);
> >
> > return 0;
> > }
> > @@ -4994,7 +4886,6 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
> > enum plane_id plane_id)
> > {
> > struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> > - u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_uv[plane_id]);
> > struct skl_wm_params wm_params;
> > int ret;
> >
> > @@ -5006,10 +4897,7 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
> > if (ret)
> > return ret;
> >
> > - ret = skl_compute_wm_levels(crtc_state, plane_state,
> > - ddb_blocks, &wm_params, wm->uv_wm);
> > - if (ret)
> > - return ret;
> > + skl_compute_wm_levels(crtc_state, plane_state, &wm_params, wm->uv_wm);
> >
> > return 0;
> > }
> > @@ -5521,13 +5409,9 @@ skl_compute_wm(struct intel_atomic_state *state)
> > if (ret || !changed)
> > return ret;
> >
> > - ret = skl_compute_ddb(state);
> > - if (ret)
> > - return ret;
> > -
> > /*
> > * Calculate WM's for all pipes that are part of this transaction.
> > - * Note that the DDB allocation above may have added more CRTC's that
> > + * Note that skl_ddb_add_affected_pipes may have added more CRTC's that
> > * weren't otherwise being modified (and set bits in dirty_pipes) if
> > * pipe allocations had to change.
> > */
> > @@ -5549,6 +5433,10 @@ skl_compute_wm(struct intel_atomic_state *state)
> > results->dirty_pipes |= drm_crtc_mask(&crtc->base);
> > }
> >
> > + ret = skl_compute_ddb(state);
> > + if (ret)
> > + return ret;
> > +
> > skl_print_wm_changes(state);
> >
> > return 0;
> > --
> > 2.14.4
>
> --
> Ville Syrjälä
> Intel
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Switch to level-based DDB allocation algorithm (v3)
2018-12-11 16:11 ` Matt Roper
@ 2018-12-11 16:21 ` Ville Syrjälä
2018-12-11 16:28 ` Ville Syrjälä
0 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2018-12-11 16:21 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
On Tue, Dec 11, 2018 at 08:11:16AM -0800, Matt Roper wrote:
> On Tue, Dec 11, 2018 at 05:59:56PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 10, 2018 at 05:05:43PM -0800, Matt Roper wrote:
> ...snip...
> > >
> > > - alloc_size -= total_min_blocks;
> > > - cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
> > > - cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].end = alloc->end;
> > > -
> > > /*
> > > - * 2. Distribute the remaining space in proportion to the amount of
> > > - * data each plane needs to fetch from memory.
> > > - *
> > > - * FIXME: we may not allocate every single block here.
> > > + * Grant each plane the blocks it requires at the highest achievable
> > > + * watermark level, plus an extra share of the leftover blocks
> > > + * proportional to its relative data rate.
> > > */
> > > - if (total_data_rate == 0)
> > > - return 0;
> > > -
> > > - start = alloc->start;
> > > for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > > - u64 data_rate, uv_data_rate;
> > > - uint16_t plane_blocks, uv_plane_blocks;
> > > + u64 rate;
> > > + u16 extra;
> > >
> > > if (plane_id == PLANE_CURSOR)
> > > continue;
> > > + if (alloc_size == 0)
> > > + continue;
> >
> > This seems wrong. We wouldn't assign anything to total/uv_total for the
> > plane in this case. I guess you could move those assignments to the
> > earlier loop and then s/continue/break/ here? Or we just remove the
> > continue entirely and let the calculations go through even if
> > alloc_size==0.
>
> It should probably be a break, since we'll only hit this on a loop
> iteration where we've handed the whole pipe allocation and all remaining
> planes are disabled. The total and uv_total were initialized to 0 at
> initialization time, so that should be correct for all remaining planes.
Not sure we can trust all the remaining planes to be really off due to
the round_up.
>
> Also, we can't let the calculation proceed here, otherwise we'll divide
> by 0 (total_data_rate) farther down since that value also decreases with
> each loop iteration.
Just keep the 'if (total_data_rate==0) return 0;' before the loop?
>
>
> Matt
>
> >
> > >
> > > - data_rate = plane_data_rate[plane_id];
> > > + wm = &cstate->wm.skl.optimal.planes[plane_id];
> > >
> > > - /*
> > > - * allocation for (packed formats) or (uv-plane part of planar format):
> > > - * promote the expression to 64 bits to avoid overflowing, the
> > > - * result is < available as data_rate / total_data_rate < 1
> > > - */
> > > - plane_blocks = minimum[plane_id];
> > > - plane_blocks += div64_u64(alloc_size * data_rate, total_data_rate);
> > > + rate = plane_data_rate[plane_id];
> > > + extra = min_t(u16, alloc_size,
> > > + DIV_ROUND_UP(alloc_size * rate, total_data_rate));
Needs DIV64_U64_ROUND_UP() on 32bit.
> > > + total[plane_id] = wm->wm[level].plane_res_b + extra;
> > > + alloc_size -= extra;
> > > + total_data_rate -= rate;
> > >
> > > - /* Leave disabled planes at (0,0) */
> > > - if (data_rate) {
> > > - cstate->wm.skl.plane_ddb_y[plane_id].start = start;
> > > - cstate->wm.skl.plane_ddb_y[plane_id].end = start + plane_blocks;
> > > - }
> > > + if (alloc_size == 0)
> > > + continue;
> > >
> > > - start += plane_blocks;
> > > + rate = uv_plane_data_rate[plane_id];
> > > + extra = min_t(u16, alloc_size,
> > > + DIV_ROUND_UP(alloc_size * rate, total_data_rate));
> > > + uv_total[plane_id] = wm->uv_wm[level].plane_res_b + extra;
> > > + alloc_size -= extra;
> > > + total_data_rate -= rate;
> > > + }
> > > + WARN_ON(alloc_size != 0 || total_data_rate != 0);
> > >
> > > - /* Allocate DDB for UV plane for planar format/NV12 */
> > > - uv_data_rate = uv_plane_data_rate[plane_id];
> > > + /* Set the actual DDB start/end points for each plane */
> > > + start = alloc->start;
> > > + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > > + struct skl_ddb_entry *plane_alloc, *uv_plane_alloc;
> > >
> > > - uv_plane_blocks = uv_minimum[plane_id];
> > > - uv_plane_blocks += div64_u64(alloc_size * uv_data_rate, total_data_rate);
> > > + if (plane_id == PLANE_CURSOR)
> > > + continue;
> > > +
> > > + plane_alloc = &cstate->wm.skl.plane_ddb_y[plane_id];
> > > + uv_plane_alloc = &cstate->wm.skl.plane_ddb_uv[plane_id];
> > >
> > > /* Gen11+ uses a separate plane for UV watermarks */
> > > - WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_plane_blocks);
> > > + WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_total[plane_id]);
> > > +
> > > + /* Leave disabled planes at (0,0) */
> > > + if (total[plane_id]) {
> > > + plane_alloc->start = start;
> > > + plane_alloc->end = start += total[plane_id];
> > > + }
> > >
> > > - if (uv_data_rate) {
> > > - cstate->wm.skl.plane_ddb_uv[plane_id].start = start;
> > > - cstate->wm.skl.plane_ddb_uv[plane_id].end =
> > > - start + uv_plane_blocks;
> > > + if (uv_total[plane_id]) {
> > > + uv_plane_alloc->start = start;
> > > + uv_plane_alloc->end = start + uv_total[plane_id];
> > > }
> > > + }
> > >
> > > - start += uv_plane_blocks;
> > > + /*
> > > + * When we calculated watermark values we didn't know how high
> > > + * of a level we'd actually be able to hit, so we just marked
> > > + * all levels as "enabled." Go back now and disable the ones
> > > + * that aren't actually possible.
> > > + */
> > > + for (level++; level <= ilk_wm_max_level(dev_priv); level++) {
> > > + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > > + wm = &cstate->wm.skl.optimal.planes[plane_id];
> > > + memset(&wm->wm[level], 0, sizeof(wm->wm[level]));
> > > + }
> > > + }
> > > +
> > > + /*
> > > + * Go back and disable the transition watermark if it turns out we
> > > + * don't have enough DDB blocks for it.
> > > + */
> > > + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > > + wm = &cstate->wm.skl.optimal.planes[plane_id];
> > > + if (wm->trans_wm.plane_res_b > total[plane_id])
> > > + memset(&wm->trans_wm, 0, sizeof(wm->trans_wm));
> > > }
> > >
> > > return 0;
> > > @@ -4715,17 +4665,15 @@ skl_compute_plane_wm_params(const struct intel_crtc_state *cstate,
> > > return 0;
> > > }
> > >
> > > -static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > > - const struct intel_plane_state *intel_pstate,
> > > - uint16_t ddb_allocation,
> > > - int level,
> > > - const struct skl_wm_params *wp,
> > > - const struct skl_wm_level *result_prev,
> > > - struct skl_wm_level *result /* out */)
> > > +static void skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > > + const struct intel_plane_state *intel_pstate,
> > > + int level,
> > > + const struct skl_wm_params *wp,
> > > + const struct skl_wm_level *result_prev,
> > > + struct skl_wm_level *result /* out */)
> > > {
> > > struct drm_i915_private *dev_priv =
> > > to_i915(intel_pstate->base.plane->dev);
> > > - const struct drm_plane_state *pstate = &intel_pstate->base;
> > > uint32_t latency = dev_priv->wm.skl_latency[level];
> > > uint_fixed_16_16_t method1, method2;
> > > uint_fixed_16_16_t selected_result;
> > > @@ -4733,10 +4681,6 @@ static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > > struct intel_atomic_state *state =
> > > to_intel_atomic_state(cstate->base.state);
> > > bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
> > > - uint32_t min_disp_buf_needed;
> > > -
> > > - if (latency == 0)
> > > - return level == 0 ? -EINVAL : 0;
> > >
> > > /* Display WA #1141: kbl,cfl */
> > > if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
> > > @@ -4800,61 +4744,24 @@ static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > > res_blocks = result_prev->plane_res_b;
> > > }
> > >
> > > - if (INTEL_GEN(dev_priv) >= 11) {
> > > - if (wp->y_tiled) {
> > > - uint32_t extra_lines;
> > > - uint_fixed_16_16_t fp_min_disp_buf_needed;
> > > -
> > > - if (res_lines % wp->y_min_scanlines == 0)
> > > - extra_lines = wp->y_min_scanlines;
> > > - else
> > > - extra_lines = wp->y_min_scanlines * 2 -
> > > - res_lines % wp->y_min_scanlines;
> > > -
> > > - fp_min_disp_buf_needed = mul_u32_fixed16(res_lines +
> > > - extra_lines,
> > > - wp->plane_blocks_per_line);
> > > - min_disp_buf_needed = fixed16_to_u32_round_up(
> > > - fp_min_disp_buf_needed);
> > > - } else {
> > > - min_disp_buf_needed = DIV_ROUND_UP(res_blocks * 11, 10);
> > > - }
> > > - } else {
> > > - min_disp_buf_needed = res_blocks;
> > > - }
> > > -
> > > - if ((level > 0 && res_lines > 31) ||
> > > - res_blocks >= ddb_allocation ||
> > > - min_disp_buf_needed >= ddb_allocation) {
> > > - /*
> > > - * If there are no valid level 0 watermarks, then we can't
> > > - * support this display configuration.
> > > - */
> > > - if (level) {
> > > - return 0;
> > > - } else {
> > > - struct drm_plane *plane = pstate->plane;
> > > -
> > > - DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
> > > - DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
> > > - plane->base.id, plane->name,
> > > - res_blocks, ddb_allocation, res_lines);
> > > - return -EINVAL;
> > > - }
> > > - }
> > > -
> > > /* The number of lines are ignored for the level 0 watermark. */
> > > + if (level > 0 && res_lines > 31)
> > > + return;
> > > +
> > > + /*
> > > + * If res_lines is valid, assume we can use this watermark level
> > > + * for now. We'll come back and disable it after we calculate the
> > > + * DDB allocation if it turns out we don't actually have enough
> > > + * blocks to satisfy it.
> > > + */
> > > result->plane_res_b = res_blocks;
> > > result->plane_res_l = res_lines;
> > > result->plane_en = true;
> > > -
> > > - return 0;
> > > }
> > >
> > > -static int
> > > +static void
> > > skl_compute_wm_levels(const struct intel_crtc_state *cstate,
> > > const struct intel_plane_state *intel_pstate,
> > > - uint16_t ddb_blocks,
> > > const struct skl_wm_params *wm_params,
> > > struct skl_wm_level *levels)
> > > {
> > > @@ -4862,25 +4769,15 @@ skl_compute_wm_levels(const struct intel_crtc_state *cstate,
> > > to_i915(intel_pstate->base.plane->dev);
> > > int level, max_level = ilk_wm_max_level(dev_priv);
> > > struct skl_wm_level *result_prev = &levels[0];
> > > - int ret;
> > >
> > > for (level = 0; level <= max_level; level++) {
> > > struct skl_wm_level *result = &levels[level];
> > >
> > > - ret = skl_compute_plane_wm(cstate,
> > > - intel_pstate,
> > > - ddb_blocks,
> > > - level,
> > > - wm_params,
> > > - result_prev,
> > > - result);
> > > - if (ret)
> > > - return ret;
> > > + skl_compute_plane_wm(cstate, intel_pstate, level, wm_params,
> > > + result_prev, result);
> > >
> > > result_prev = result;
> > > }
> > > -
> > > - return 0;
> > > }
> > >
> > > static uint32_t
> > > @@ -4908,8 +4805,7 @@ skl_compute_linetime_wm(const struct intel_crtc_state *cstate)
> > >
> > > static void skl_compute_transition_wm(const struct intel_crtc_state *cstate,
> > > const struct skl_wm_params *wp,
> > > - struct skl_plane_wm *wm,
> > > - uint16_t ddb_allocation)
> > > + struct skl_plane_wm *wm)
> > > {
> > > struct drm_device *dev = cstate->base.crtc->dev;
> > > const struct drm_i915_private *dev_priv = to_i915(dev);
> > > @@ -4957,12 +4853,13 @@ static void skl_compute_transition_wm(const struct intel_crtc_state *cstate,
> > >
> > > }
> > >
> > > - res_blocks += 1;
> > > -
> > > - if (res_blocks < ddb_allocation) {
> > > - wm->trans_wm.plane_res_b = res_blocks;
> > > - wm->trans_wm.plane_en = true;
> > > - }
> > > + /*
> > > + * Just assume we can enable the transition watermark. After
> > > + * computing the DDB we'll come back and disable it if that
> > > + * assumption turns out to be false.
> > > + */
> > > + wm->trans_wm.plane_res_b = res_blocks + 1;
> > > + wm->trans_wm.plane_en = true;
> > > }
> > >
> > > static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> > > @@ -4970,7 +4867,6 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> > > enum plane_id plane_id, int color_plane)
> > > {
> > > struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> > > - u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_y[plane_id]);
> > > struct skl_wm_params wm_params;
> > > int ret;
> > >
> > > @@ -4979,12 +4875,8 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> > > if (ret)
> > > return ret;
> > >
> > > - ret = skl_compute_wm_levels(crtc_state, plane_state,
> > > - ddb_blocks, &wm_params, wm->wm);
> > > - if (ret)
> > > - return ret;
> > > -
> > > - skl_compute_transition_wm(crtc_state, &wm_params, wm, ddb_blocks);
> > > + skl_compute_wm_levels(crtc_state, plane_state, &wm_params, wm->wm);
> > > + skl_compute_transition_wm(crtc_state, &wm_params, wm);
> > >
> > > return 0;
> > > }
> > > @@ -4994,7 +4886,6 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
> > > enum plane_id plane_id)
> > > {
> > > struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> > > - u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_uv[plane_id]);
> > > struct skl_wm_params wm_params;
> > > int ret;
> > >
> > > @@ -5006,10 +4897,7 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
> > > if (ret)
> > > return ret;
> > >
> > > - ret = skl_compute_wm_levels(crtc_state, plane_state,
> > > - ddb_blocks, &wm_params, wm->uv_wm);
> > > - if (ret)
> > > - return ret;
> > > + skl_compute_wm_levels(crtc_state, plane_state, &wm_params, wm->uv_wm);
> > >
> > > return 0;
> > > }
> > > @@ -5521,13 +5409,9 @@ skl_compute_wm(struct intel_atomic_state *state)
> > > if (ret || !changed)
> > > return ret;
> > >
> > > - ret = skl_compute_ddb(state);
> > > - if (ret)
> > > - return ret;
> > > -
> > > /*
> > > * Calculate WM's for all pipes that are part of this transaction.
> > > - * Note that the DDB allocation above may have added more CRTC's that
> > > + * Note that skl_ddb_add_affected_pipes may have added more CRTC's that
> > > * weren't otherwise being modified (and set bits in dirty_pipes) if
> > > * pipe allocations had to change.
> > > */
> > > @@ -5549,6 +5433,10 @@ skl_compute_wm(struct intel_atomic_state *state)
> > > results->dirty_pipes |= drm_crtc_mask(&crtc->base);
> > > }
> > >
> > > + ret = skl_compute_ddb(state);
> > > + if (ret)
> > > + return ret;
> > > +
> > > skl_print_wm_changes(state);
> > >
> > > return 0;
> > > --
> > > 2.14.4
> >
> > --
> > Ville Syrjälä
> > Intel
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Switch to level-based DDB allocation algorithm (v3)
2018-12-11 16:21 ` Ville Syrjälä
@ 2018-12-11 16:28 ` Ville Syrjälä
0 siblings, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2018-12-11 16:28 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
On Tue, Dec 11, 2018 at 06:21:29PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 11, 2018 at 08:11:16AM -0800, Matt Roper wrote:
> > On Tue, Dec 11, 2018 at 05:59:56PM +0200, Ville Syrjälä wrote:
> > > On Mon, Dec 10, 2018 at 05:05:43PM -0800, Matt Roper wrote:
> > ...snip...
> > > >
> > > > - alloc_size -= total_min_blocks;
> > > > - cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
> > > > - cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].end = alloc->end;
> > > > -
> > > > /*
> > > > - * 2. Distribute the remaining space in proportion to the amount of
> > > > - * data each plane needs to fetch from memory.
> > > > - *
> > > > - * FIXME: we may not allocate every single block here.
> > > > + * Grant each plane the blocks it requires at the highest achievable
> > > > + * watermark level, plus an extra share of the leftover blocks
> > > > + * proportional to its relative data rate.
> > > > */
> > > > - if (total_data_rate == 0)
> > > > - return 0;
> > > > -
> > > > - start = alloc->start;
> > > > for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > > > - u64 data_rate, uv_data_rate;
> > > > - uint16_t plane_blocks, uv_plane_blocks;
> > > > + u64 rate;
> > > > + u16 extra;
> > > >
> > > > if (plane_id == PLANE_CURSOR)
> > > > continue;
> > > > + if (alloc_size == 0)
> > > > + continue;
> > >
> > > This seems wrong. We wouldn't assign anything to total/uv_total for the
> > > plane in this case. I guess you could move those assignments to the
> > > earlier loop and then s/continue/break/ here? Or we just remove the
> > > continue entirely and let the calculations go through even if
> > > alloc_size==0.
> >
> > It should probably be a break, since we'll only hit this on a loop
> > iteration where we've handed the whole pipe allocation and all remaining
> > planes are disabled. The total and uv_total were initialized to 0 at
> > initialization time, so that should be correct for all remaining planes.
>
> Not sure we can trust all the remaining planes to be really off due to
> the round_up.
>
> >
> > Also, we can't let the calculation proceed here, otherwise we'll divide
> > by 0 (total_data_rate) farther down since that value also decreases with
> > each loop iteration.
Oh and there's actually no guarantee that we have any extra blocks left
after accounting for the watermarks anyway.
>
> Just keep the 'if (total_data_rate==0) return 0;' before the loop?
>
> >
> >
> > Matt
> >
> > >
> > > >
> > > > - data_rate = plane_data_rate[plane_id];
> > > > + wm = &cstate->wm.skl.optimal.planes[plane_id];
> > > >
> > > > - /*
> > > > - * allocation for (packed formats) or (uv-plane part of planar format):
> > > > - * promote the expression to 64 bits to avoid overflowing, the
> > > > - * result is < available as data_rate / total_data_rate < 1
> > > > - */
> > > > - plane_blocks = minimum[plane_id];
> > > > - plane_blocks += div64_u64(alloc_size * data_rate, total_data_rate);
> > > > + rate = plane_data_rate[plane_id];
> > > > + extra = min_t(u16, alloc_size,
> > > > + DIV_ROUND_UP(alloc_size * rate, total_data_rate));
>
> Needs DIV64_U64_ROUND_UP() on 32bit.
>
> > > > + total[plane_id] = wm->wm[level].plane_res_b + extra;
> > > > + alloc_size -= extra;
> > > > + total_data_rate -= rate;
> > > >
> > > > - /* Leave disabled planes at (0,0) */
> > > > - if (data_rate) {
> > > > - cstate->wm.skl.plane_ddb_y[plane_id].start = start;
> > > > - cstate->wm.skl.plane_ddb_y[plane_id].end = start + plane_blocks;
> > > > - }
> > > > + if (alloc_size == 0)
> > > > + continue;
> > > >
> > > > - start += plane_blocks;
> > > > + rate = uv_plane_data_rate[plane_id];
> > > > + extra = min_t(u16, alloc_size,
> > > > + DIV_ROUND_UP(alloc_size * rate, total_data_rate));
> > > > + uv_total[plane_id] = wm->uv_wm[level].plane_res_b + extra;
> > > > + alloc_size -= extra;
> > > > + total_data_rate -= rate;
> > > > + }
> > > > + WARN_ON(alloc_size != 0 || total_data_rate != 0);
> > > >
> > > > - /* Allocate DDB for UV plane for planar format/NV12 */
> > > > - uv_data_rate = uv_plane_data_rate[plane_id];
> > > > + /* Set the actual DDB start/end points for each plane */
> > > > + start = alloc->start;
> > > > + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > > > + struct skl_ddb_entry *plane_alloc, *uv_plane_alloc;
> > > >
> > > > - uv_plane_blocks = uv_minimum[plane_id];
> > > > - uv_plane_blocks += div64_u64(alloc_size * uv_data_rate, total_data_rate);
> > > > + if (plane_id == PLANE_CURSOR)
> > > > + continue;
> > > > +
> > > > + plane_alloc = &cstate->wm.skl.plane_ddb_y[plane_id];
> > > > + uv_plane_alloc = &cstate->wm.skl.plane_ddb_uv[plane_id];
> > > >
> > > > /* Gen11+ uses a separate plane for UV watermarks */
> > > > - WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_plane_blocks);
> > > > + WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_total[plane_id]);
> > > > +
> > > > + /* Leave disabled planes at (0,0) */
> > > > + if (total[plane_id]) {
> > > > + plane_alloc->start = start;
> > > > + plane_alloc->end = start += total[plane_id];
> > > > + }
> > > >
> > > > - if (uv_data_rate) {
> > > > - cstate->wm.skl.plane_ddb_uv[plane_id].start = start;
> > > > - cstate->wm.skl.plane_ddb_uv[plane_id].end =
> > > > - start + uv_plane_blocks;
> > > > + if (uv_total[plane_id]) {
> > > > + uv_plane_alloc->start = start;
> > > > + uv_plane_alloc->end = start + uv_total[plane_id];
> > > > }
> > > > + }
> > > >
> > > > - start += uv_plane_blocks;
> > > > + /*
> > > > + * When we calculated watermark values we didn't know how high
> > > > + * of a level we'd actually be able to hit, so we just marked
> > > > + * all levels as "enabled." Go back now and disable the ones
> > > > + * that aren't actually possible.
> > > > + */
> > > > + for (level++; level <= ilk_wm_max_level(dev_priv); level++) {
> > > > + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > > > + wm = &cstate->wm.skl.optimal.planes[plane_id];
> > > > + memset(&wm->wm[level], 0, sizeof(wm->wm[level]));
> > > > + }
> > > > + }
> > > > +
> > > > + /*
> > > > + * Go back and disable the transition watermark if it turns out we
> > > > + * don't have enough DDB blocks for it.
> > > > + */
> > > > + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> > > > + wm = &cstate->wm.skl.optimal.planes[plane_id];
> > > > + if (wm->trans_wm.plane_res_b > total[plane_id])
> > > > + memset(&wm->trans_wm, 0, sizeof(wm->trans_wm));
> > > > }
> > > >
> > > > return 0;
> > > > @@ -4715,17 +4665,15 @@ skl_compute_plane_wm_params(const struct intel_crtc_state *cstate,
> > > > return 0;
> > > > }
> > > >
> > > > -static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > > > - const struct intel_plane_state *intel_pstate,
> > > > - uint16_t ddb_allocation,
> > > > - int level,
> > > > - const struct skl_wm_params *wp,
> > > > - const struct skl_wm_level *result_prev,
> > > > - struct skl_wm_level *result /* out */)
> > > > +static void skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > > > + const struct intel_plane_state *intel_pstate,
> > > > + int level,
> > > > + const struct skl_wm_params *wp,
> > > > + const struct skl_wm_level *result_prev,
> > > > + struct skl_wm_level *result /* out */)
> > > > {
> > > > struct drm_i915_private *dev_priv =
> > > > to_i915(intel_pstate->base.plane->dev);
> > > > - const struct drm_plane_state *pstate = &intel_pstate->base;
> > > > uint32_t latency = dev_priv->wm.skl_latency[level];
> > > > uint_fixed_16_16_t method1, method2;
> > > > uint_fixed_16_16_t selected_result;
> > > > @@ -4733,10 +4681,6 @@ static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > > > struct intel_atomic_state *state =
> > > > to_intel_atomic_state(cstate->base.state);
> > > > bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
> > > > - uint32_t min_disp_buf_needed;
> > > > -
> > > > - if (latency == 0)
> > > > - return level == 0 ? -EINVAL : 0;
> > > >
> > > > /* Display WA #1141: kbl,cfl */
> > > > if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
> > > > @@ -4800,61 +4744,24 @@ static int skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> > > > res_blocks = result_prev->plane_res_b;
> > > > }
> > > >
> > > > - if (INTEL_GEN(dev_priv) >= 11) {
> > > > - if (wp->y_tiled) {
> > > > - uint32_t extra_lines;
> > > > - uint_fixed_16_16_t fp_min_disp_buf_needed;
> > > > -
> > > > - if (res_lines % wp->y_min_scanlines == 0)
> > > > - extra_lines = wp->y_min_scanlines;
> > > > - else
> > > > - extra_lines = wp->y_min_scanlines * 2 -
> > > > - res_lines % wp->y_min_scanlines;
> > > > -
> > > > - fp_min_disp_buf_needed = mul_u32_fixed16(res_lines +
> > > > - extra_lines,
> > > > - wp->plane_blocks_per_line);
> > > > - min_disp_buf_needed = fixed16_to_u32_round_up(
> > > > - fp_min_disp_buf_needed);
> > > > - } else {
> > > > - min_disp_buf_needed = DIV_ROUND_UP(res_blocks * 11, 10);
> > > > - }
> > > > - } else {
> > > > - min_disp_buf_needed = res_blocks;
> > > > - }
> > > > -
> > > > - if ((level > 0 && res_lines > 31) ||
> > > > - res_blocks >= ddb_allocation ||
> > > > - min_disp_buf_needed >= ddb_allocation) {
> > > > - /*
> > > > - * If there are no valid level 0 watermarks, then we can't
> > > > - * support this display configuration.
> > > > - */
> > > > - if (level) {
> > > > - return 0;
> > > > - } else {
> > > > - struct drm_plane *plane = pstate->plane;
> > > > -
> > > > - DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
> > > > - DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
> > > > - plane->base.id, plane->name,
> > > > - res_blocks, ddb_allocation, res_lines);
> > > > - return -EINVAL;
> > > > - }
> > > > - }
> > > > -
> > > > /* The number of lines are ignored for the level 0 watermark. */
> > > > + if (level > 0 && res_lines > 31)
> > > > + return;
> > > > +
> > > > + /*
> > > > + * If res_lines is valid, assume we can use this watermark level
> > > > + * for now. We'll come back and disable it after we calculate the
> > > > + * DDB allocation if it turns out we don't actually have enough
> > > > + * blocks to satisfy it.
> > > > + */
> > > > result->plane_res_b = res_blocks;
> > > > result->plane_res_l = res_lines;
> > > > result->plane_en = true;
> > > > -
> > > > - return 0;
> > > > }
> > > >
> > > > -static int
> > > > +static void
> > > > skl_compute_wm_levels(const struct intel_crtc_state *cstate,
> > > > const struct intel_plane_state *intel_pstate,
> > > > - uint16_t ddb_blocks,
> > > > const struct skl_wm_params *wm_params,
> > > > struct skl_wm_level *levels)
> > > > {
> > > > @@ -4862,25 +4769,15 @@ skl_compute_wm_levels(const struct intel_crtc_state *cstate,
> > > > to_i915(intel_pstate->base.plane->dev);
> > > > int level, max_level = ilk_wm_max_level(dev_priv);
> > > > struct skl_wm_level *result_prev = &levels[0];
> > > > - int ret;
> > > >
> > > > for (level = 0; level <= max_level; level++) {
> > > > struct skl_wm_level *result = &levels[level];
> > > >
> > > > - ret = skl_compute_plane_wm(cstate,
> > > > - intel_pstate,
> > > > - ddb_blocks,
> > > > - level,
> > > > - wm_params,
> > > > - result_prev,
> > > > - result);
> > > > - if (ret)
> > > > - return ret;
> > > > + skl_compute_plane_wm(cstate, intel_pstate, level, wm_params,
> > > > + result_prev, result);
> > > >
> > > > result_prev = result;
> > > > }
> > > > -
> > > > - return 0;
> > > > }
> > > >
> > > > static uint32_t
> > > > @@ -4908,8 +4805,7 @@ skl_compute_linetime_wm(const struct intel_crtc_state *cstate)
> > > >
> > > > static void skl_compute_transition_wm(const struct intel_crtc_state *cstate,
> > > > const struct skl_wm_params *wp,
> > > > - struct skl_plane_wm *wm,
> > > > - uint16_t ddb_allocation)
> > > > + struct skl_plane_wm *wm)
> > > > {
> > > > struct drm_device *dev = cstate->base.crtc->dev;
> > > > const struct drm_i915_private *dev_priv = to_i915(dev);
> > > > @@ -4957,12 +4853,13 @@ static void skl_compute_transition_wm(const struct intel_crtc_state *cstate,
> > > >
> > > > }
> > > >
> > > > - res_blocks += 1;
> > > > -
> > > > - if (res_blocks < ddb_allocation) {
> > > > - wm->trans_wm.plane_res_b = res_blocks;
> > > > - wm->trans_wm.plane_en = true;
> > > > - }
> > > > + /*
> > > > + * Just assume we can enable the transition watermark. After
> > > > + * computing the DDB we'll come back and disable it if that
> > > > + * assumption turns out to be false.
> > > > + */
> > > > + wm->trans_wm.plane_res_b = res_blocks + 1;
> > > > + wm->trans_wm.plane_en = true;
> > > > }
> > > >
> > > > static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> > > > @@ -4970,7 +4867,6 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> > > > enum plane_id plane_id, int color_plane)
> > > > {
> > > > struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> > > > - u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_y[plane_id]);
> > > > struct skl_wm_params wm_params;
> > > > int ret;
> > > >
> > > > @@ -4979,12 +4875,8 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - ret = skl_compute_wm_levels(crtc_state, plane_state,
> > > > - ddb_blocks, &wm_params, wm->wm);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > - skl_compute_transition_wm(crtc_state, &wm_params, wm, ddb_blocks);
> > > > + skl_compute_wm_levels(crtc_state, plane_state, &wm_params, wm->wm);
> > > > + skl_compute_transition_wm(crtc_state, &wm_params, wm);
> > > >
> > > > return 0;
> > > > }
> > > > @@ -4994,7 +4886,6 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
> > > > enum plane_id plane_id)
> > > > {
> > > > struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> > > > - u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_uv[plane_id]);
> > > > struct skl_wm_params wm_params;
> > > > int ret;
> > > >
> > > > @@ -5006,10 +4897,7 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - ret = skl_compute_wm_levels(crtc_state, plane_state,
> > > > - ddb_blocks, &wm_params, wm->uv_wm);
> > > > - if (ret)
> > > > - return ret;
> > > > + skl_compute_wm_levels(crtc_state, plane_state, &wm_params, wm->uv_wm);
> > > >
> > > > return 0;
> > > > }
> > > > @@ -5521,13 +5409,9 @@ skl_compute_wm(struct intel_atomic_state *state)
> > > > if (ret || !changed)
> > > > return ret;
> > > >
> > > > - ret = skl_compute_ddb(state);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > /*
> > > > * Calculate WM's for all pipes that are part of this transaction.
> > > > - * Note that the DDB allocation above may have added more CRTC's that
> > > > + * Note that skl_ddb_add_affected_pipes may have added more CRTC's that
> > > > * weren't otherwise being modified (and set bits in dirty_pipes) if
> > > > * pipe allocations had to change.
> > > > */
> > > > @@ -5549,6 +5433,10 @@ skl_compute_wm(struct intel_atomic_state *state)
> > > > results->dirty_pipes |= drm_crtc_mask(&crtc->base);
> > > > }
> > > >
> > > > + ret = skl_compute_ddb(state);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > skl_print_wm_changes(state);
> > > >
> > > > return 0;
> > > > --
> > > > 2.14.4
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
>
> --
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-12-11 16:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-11 1:05 [PATCH v2 1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method Matt Roper
2018-12-11 1:05 ` [PATCH v2 2/2] drm/i915: Switch to level-based DDB allocation algorithm (v3) Matt Roper
2018-12-11 15:59 ` Ville Syrjälä
2018-12-11 16:11 ` Matt Roper
2018-12-11 16:21 ` Ville Syrjälä
2018-12-11 16:28 ` Ville Syrjälä
2018-12-11 1:27 ` ✗ Fi.CI.SPARSE: warning for series starting with [v2,1/2] drm/i915: Don't use DDB allocation when choosing gen9 watermark method Patchwork
2018-12-11 1:45 ` ✓ Fi.CI.BAT: success " Patchwork
2018-12-11 3:37 ` ✗ Fi.CI.IGT: failure " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.