On Tue, Oct 16, 2018 at 03:01:29PM -0700, Paulo Zanoni wrote:
> The goal of struct skl_wm_params is to cache every watermark
> parameter so the other functions can just use them without worrying
> about the appropriate place to fetch each parameter requested by the
> spec, and without having to recompute parameters that are used in
> different steps of the calculation.
>
> The ddb_blocks parameter is one that is used by both the the plane
> watermarks and the transition watermarks. Move ddb_blocks to the
> parameter struct so we can simplify the code.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++-------------------------
> 2 files changed, 18 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4b1e8471609b..b32d680d9bf0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1255,6 +1255,7 @@ struct skl_wm_params {
> bool rc_surface;
> bool is_planar;
> uint32_t width;
> + uint16_t ddb_blocks;
> uint8_t cpp;
> uint32_t plane_pixel_rate;
> uint32_t y_min_scanlines;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f388bfa99a97..4053f4a68657 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4522,11 +4522,13 @@ static int
> skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
> struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate,
> + const struct skl_ddb_allocation *ddb,
> struct skl_wm_params *wp, int plane_id)
> {
> struct intel_plane *plane = to_intel_plane(intel_pstate->base.plane);
> const struct drm_plane_state *pstate = &intel_pstate->base;
> const struct drm_framebuffer *fb = pstate->fb;
> + enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
> uint32_t interm_pbpl;
> struct intel_atomic_state *state =
> to_intel_atomic_state(cstate->base.state);
> @@ -4624,13 +4626,16 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
> wp->linetime_us = fixed16_to_u32_round_up(
> intel_get_linetime_us(cstate));
>
> + wp->ddb_blocks = plane_id ?
Ugh. That 'plane_id' really should be renamed to something else.
'color_plane' would be the newfangled name I started to use elsewhere.
Patch looks good to me.
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> + skl_ddb_entry_size(&ddb->uv_plane[pipe][plane->id]) :
> + skl_ddb_entry_size(&ddb->plane[pipe][plane->id]);
> +
> return 0;
> }
>
> static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> 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,
> @@ -4674,7 +4679,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> wp->dbuf_block_size < 1) &&
> (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
> selected_result = method2;
> - } else if (ddb_allocation >=
> + } else if (wp->ddb_blocks >=
> fixed16_to_u32_round_up(wp->plane_blocks_per_line)) {
> if (INTEL_GEN(dev_priv) == 9 &&
> !IS_GEMINILAKE(dev_priv))
> @@ -4747,8 +4752,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> }
>
> if ((level > 0 && res_lines > 31) ||
> - res_blocks >= ddb_allocation ||
> - min_disp_buf_needed >= ddb_allocation) {
> + res_blocks >= wp->ddb_blocks ||
> + min_disp_buf_needed >= wp->ddb_blocks) {
> result->plane_en = false;
>
> /*
> @@ -4763,7 +4768,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> 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);
> + res_blocks, wp->ddb_blocks, res_lines);
> return -EINVAL;
> }
> }
> @@ -4789,26 +4794,15 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>
> static int
> skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> - struct skl_ddb_allocation *ddb,
> struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate,
> const struct skl_wm_params *wm_params,
> struct skl_plane_wm *wm,
> int plane_id)
> {
> - struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> - struct drm_plane *plane = intel_pstate->base.plane;
> - struct intel_plane *intel_plane = to_intel_plane(plane);
> - uint16_t ddb_blocks;
> - enum pipe pipe = intel_crtc->pipe;
> int level, max_level = ilk_wm_max_level(dev_priv);
> - enum plane_id intel_plane_id = intel_plane->id;
> int ret;
>
> - ddb_blocks = plane_id ?
> - skl_ddb_entry_size(&ddb->uv_plane[pipe][intel_plane_id]) :
> - skl_ddb_entry_size(&ddb->plane[pipe][intel_plane_id]);
> -
> for (level = 0; level <= max_level; level++) {
> struct skl_wm_level *result = plane_id ? &wm->uv_wm[level] :
> &wm->wm[level];
> @@ -4823,7 +4817,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> ret = skl_compute_plane_wm(dev_priv,
> cstate,
> intel_pstate,
> - ddb_blocks,
> level,
> wm_params,
> result_prev,
> @@ -4863,7 +4856,6 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate)
> static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
> struct skl_wm_params *wp,
> struct skl_wm_level *wm_l0,
> - uint16_t ddb_allocation,
> struct skl_wm_level *trans_wm /* out */)
> {
> struct drm_device *dev = cstate->base.crtc->dev;
> @@ -4914,7 +4906,7 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
>
> res_blocks += 1;
>
> - if (res_blocks < ddb_allocation) {
> + if (res_blocks < wp->ddb_blocks) {
> trans_wm->plane_res_b = res_blocks;
> trans_wm->plane_en = true;
> return;
> @@ -4947,37 +4939,35 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> to_intel_plane_state(pstate);
> enum plane_id plane_id = to_intel_plane(plane)->id;
> struct skl_wm_params wm_params;
> - enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
> - uint16_t ddb_blocks;
>
> wm = &pipe_wm->planes[plane_id];
> - ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][plane_id]);
>
> ret = skl_compute_plane_wm_params(dev_priv, cstate,
> - intel_pstate, &wm_params, 0);
> + intel_pstate, ddb,
> + &wm_params, 0);
> if (ret)
> return ret;
>
> if (!wm_params.plane_visible)
> continue;
>
> - ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> + ret = skl_compute_wm_levels(dev_priv, cstate,
> intel_pstate, &wm_params, wm, 0);
> if (ret)
> return ret;
>
> skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
> - ddb_blocks, &wm->trans_wm);
> + &wm->trans_wm);
>
> /* uv plane watermarks must also be validated for NV12/Planar */
> if (wm_params.is_planar) {
> ret = skl_compute_plane_wm_params(dev_priv, cstate,
> - intel_pstate,
> + intel_pstate, ddb,
> &wm_params, 1);
> if (ret)
> return ret;
>
> - ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> + ret = skl_compute_wm_levels(dev_priv, cstate,
> intel_pstate, &wm_params,
> wm, 1);
> if (ret)
> --
> 2.14.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
