| Message ID | 20181004231600.14101-5-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Watermarks small fixes/improvements | expand |
On Thu, Oct 04, 2018 at 04:15:58PM -0700, Paulo Zanoni wrote: > The transition watermarks ask for Selected Result Blocks (the real > value), not Result Blocks (the integer value). Given how ceilings are > applied in both the non-transition and the transition watermarks > calculations, we can get away with assuming that Selected Result > Blocks is actually Result Blocks minus 1 without any rounding errors. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Matches current bspec. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> I wonder if we should make trans_amount configurable via debugfs so that we can experiment with the impact of transitional watermarks. Matt > --- > drivers/gpu/drm/i915/intel_pm.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 40ce99c455f3..14f13a371989 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4874,7 +4874,7 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate, > const struct drm_i915_private *dev_priv = to_i915(dev); > uint16_t trans_min, trans_y_tile_min; > const uint16_t trans_amount = 10; /* This is configurable amount */ > - uint16_t trans_offset_b, res_blocks; > + uint16_t wm0_sel_res_b, trans_offset_b, res_blocks; > > if (!cstate->base.active) > goto exit; > @@ -4893,13 +4893,25 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate, > > trans_offset_b = trans_min + trans_amount; > > + /* > + * The spec asks for Selected Result Blocks for wm0 (the real value), > + * not Result Blocks (the integer value). Pay attention to the capital > + * letters. The value wm_l0->plane_res_b is actually Result Blocks, but > + * since Result Blocks is the ceiling of Selected Result Blocks plus 1, > + * and since we later will have to get the ceiling of the sum in the > + * transition watermarks calculation, we can just pretend Selected > + * Result Blocks is Result Blocks minus 1 and it should work for the > + * current platforms. > + */ > + wm0_sel_res_b = wm_l0->plane_res_b - 1; > + > if (wp->y_tiled) { > trans_y_tile_min = (uint16_t) mul_round_up_u32_fixed16(2, > wp->y_tile_minimum); > - res_blocks = max(wm_l0->plane_res_b, trans_y_tile_min) + > + res_blocks = max(wm0_sel_res_b, trans_y_tile_min) + > trans_offset_b; > } else { > - res_blocks = wm_l0->plane_res_b + trans_offset_b; > + res_blocks = wm0_sel_res_b + trans_offset_b; > > /* WA BUG:1938466 add one block for non y-tile planes */ > if (IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_A0)) > -- > 2.14.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 40ce99c455f3..14f13a371989 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4874,7 +4874,7 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate, const struct drm_i915_private *dev_priv = to_i915(dev); uint16_t trans_min, trans_y_tile_min; const uint16_t trans_amount = 10; /* This is configurable amount */ - uint16_t trans_offset_b, res_blocks; + uint16_t wm0_sel_res_b, trans_offset_b, res_blocks; if (!cstate->base.active) goto exit; @@ -4893,13 +4893,25 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate, trans_offset_b = trans_min + trans_amount; + /* + * The spec asks for Selected Result Blocks for wm0 (the real value), + * not Result Blocks (the integer value). Pay attention to the capital + * letters. The value wm_l0->plane_res_b is actually Result Blocks, but + * since Result Blocks is the ceiling of Selected Result Blocks plus 1, + * and since we later will have to get the ceiling of the sum in the + * transition watermarks calculation, we can just pretend Selected + * Result Blocks is Result Blocks minus 1 and it should work for the + * current platforms. + */ + wm0_sel_res_b = wm_l0->plane_res_b - 1; + if (wp->y_tiled) { trans_y_tile_min = (uint16_t) mul_round_up_u32_fixed16(2, wp->y_tile_minimum); - res_blocks = max(wm_l0->plane_res_b, trans_y_tile_min) + + res_blocks = max(wm0_sel_res_b, trans_y_tile_min) + trans_offset_b; } else { - res_blocks = wm_l0->plane_res_b + trans_offset_b; + res_blocks = wm0_sel_res_b + trans_offset_b; /* WA BUG:1938466 add one block for non y-tile planes */ if (IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_A0))
The transition watermarks ask for Selected Result Blocks (the real value), not Result Blocks (the integer value). Given how ceilings are applied in both the non-transition and the transition watermarks calculations, we can get away with assuming that Selected Result Blocks is actually Result Blocks minus 1 without any rounding errors. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)