there are two main reason I want to pass complete pipe_wm to compute function

1. for transition WM calculation, else we need to pass trans_wm.plane_res_b & trans_wm.plane_res_l also as a parameter,

that will increase the parameter-list. And trans_wm will be used (calculation will happen) only during level-0. All other levels (1-7) these will be extra unused variables.

If we want to calculate transition WM separately (after plane WM calculation) there we will have to duplicate some of the code for WM level-0 calculation.

2. This will be used in Render compression WA for (Gen9 & CNL) where we have to make sure WM level-(1-7) result blocks/lines are >= WM level-0 result blocks/lines.

agree, out_ variable should not be local variables.
If you think above points can be addressed, let me know, will update patches accordingly.


On Tuesday 20 September 2016 05:47 PM, Paulo Zanoni wrote:
Em Sex, 2016-09-09 às 13:30 +0530, Kumar, Mahesh escreveu:
From: Mahesh Kumar <>

This patch make use of plane_wm variable directly instead of passing
skl_plane_wm struct. this way reduces number of argument requirement
in watermark calculation functions.

It also gives more freedom of decision making to implement Bspec WM
This is just my personal opinion, but I don't think this patch is an
improvement to the codebase. The way things are currently organized,
there's no risk that we may end up reading some variable that's still
not set/computed, so there's less risk that some later patch may end up
using information it shouldn't. Also, by having these explicit out
parameters, we clearly highlight what's the goal of the function:
output those 3 values, instead of writing I-don't-know-what to a huge

Besides, the patch even keeps the out_ variables as local variables
instead of parameters, which makes things even more confusing IMHO,
since in_ and out_ variables are usually function parameters.

I also see that this patch is not necessary for the series. We kinda
use the new pipe_wm variable at patch 9, but we could just go with the
variables we have.

So, unless some new arguments are given, I'd suggest to just drop this

Signed-off-by: Mahesh Kumar <>
  drivers/gpu/drm/i915/intel_pm.c | 29 +++++++++++++++--------------
  1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c
index 86c6d66..3fdec4d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3542,9 +3542,7 @@ static int skl_compute_plane_wm(const struct
drm_i915_private *dev_priv,
                                struct intel_plane_state
                                uint16_t ddb_allocation,
                                int level,
-                               uint16_t *out_blocks, /* out */
-                               uint8_t *out_lines, /* out */
-                               bool *enabled /* out */)
+                               struct skl_pipe_wm *pipe_wm)
        struct drm_plane_state *pstate = &intel_pstate->base;
        struct drm_framebuffer *fb = pstate->fb;
@@ -3557,6 +3555,11 @@ static int skl_compute_plane_wm(const struct
drm_i915_private *dev_priv,
        uint32_t width = 0, height = 0;
        uint32_t plane_pixel_rate;
        uint32_t y_tile_minimum, y_min_scanlines;
+       int id = skl_wm_plane_id(to_intel_plane(pstate->plane));
+       struct skl_wm_level *result = &pipe_wm->wm[level];
+       uint16_t *out_blocks = &result->plane_res_b[id];
+       uint8_t *out_lines = &result->plane_res_l[id];
+       bool *enabled = &result->plane_en[id];
if (latency == 0 || !cstate-> || !intel_pstate-
base.visible) {
                *enabled = false;
@@ -3673,7 +3676,7 @@ skl_compute_wm_level(const struct
drm_i915_private *dev_priv,
                     struct skl_ddb_allocation *ddb,
                     struct intel_crtc_state *cstate,
                     int level,
-                    struct skl_wm_level *result)
+                    struct skl_pipe_wm *pipe_wm)
        struct drm_atomic_state *state = cstate->base.state;
        struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
@@ -3684,12 +3687,6 @@ skl_compute_wm_level(const struct
drm_i915_private *dev_priv,
        enum pipe pipe = intel_crtc->pipe;
        int ret;
- /*
-        * We'll only calculate watermarks for planes that are
-        * enabled, so make sure all other planes are set as
-        */
-       memset(result, 0, sizeof(*result));
                                  cstate->base.plane_mask) {
@@ -3727,9 +3724,7 @@ skl_compute_wm_level(const struct
drm_i915_private *dev_priv,
-                                          &result->plane_res_b[i],
-                                          &result->plane_res_l[i],
-                                          &result->plane_en[i]);
+                                          pipe_wm);
                if (ret)
                        return ret;
@@ -3777,9 +3772,15 @@ static int skl_build_pipe_wm(struct
intel_crtc_state *cstate,
        int level, max_level = ilk_wm_max_level(dev);
        int ret;
+ /*
+        * We'll only calculate watermarks for planes that are
+        * enabled, so make sure all other planes are set as
+        */
+       memset(pipe_wm, 0, sizeof(*pipe_wm));
        for (level = 0; level <= max_level; level++) {
                ret = skl_compute_wm_level(dev_priv, ddb, cstate,
-                                          level, &pipe_wm-
+                                          level, pipe_wm);
                if (ret)
                        return ret;

Intel-gfx mailing list

Reply via email to