[Intel-gfx] [PATCH 01/11] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

[Paulo Zanoni]

BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.

Cc: Matt Roper <***@intel.com>
Signed-off-by: Paulo Zanoni <***@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 67a4d0735291..18157c6ee126 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
res_lines = div_round_up_fixed16(selected_result,
wp->plane_blocks_per_line);

- /* Display WA #1125: skl,bxt,kbl,glk */
- if (level == 0 && wp->rc_surface)
- res_blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
+ if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {
+ /* Display WA #1125: skl,bxt,kbl */
+ if (level == 0 && wp->rc_surface)
+ res_blocks +=
+ fixed16_to_u32_round_up(wp->y_tile_minimum);
+
+ /* Display WA #1126: skl,bxt,kbl */
+ if (level >= 1 && level <= 7) {
+ if (wp->y_tiled) {
+ res_blocks +=
+ fixed16_to_u32_round_up(wp->y_tile_minimum);
+ res_lines += wp->y_min_scanlines;
+ } else {
+ res_blocks++;
+ }

- /* Display WA #1126: skl,bxt,kbl,glk */
- if (level >= 1 && level <= 7) {
- if (wp->y_tiled) {
- res_blocks += fixed16_to_u32_round_up(
- wp->y_tile_minimum);
- res_lines += wp->y_min_scanlines;
- } else {
- res_blocks++;
+ /*
+ * Make sure result blocks for higher latency levels are
+ * atleast as high as level below the current level.
+ * Assumption in DDB algorithm optimization for special
+ * cases. Also covers Display WA #1125 for RC.
+ */
+ if (result_prev->plane_res_b > res_blocks)
+ res_blocks = result_prev->plane_res_b;
}
-
- /*
- * Make sure result blocks for higher latency levels are atleast
- * as high as level below the current level.
- * Assumption in DDB algorithm optimization for special cases.
- * Also covers Display WA #1125 for RC.
- */
- if (result_prev->plane_res_b > res_blocks)
- res_blocks = result_prev->plane_res_b;
}

if (INTEL_GEN(dev_priv) >= 11) {

[Paulo Zanoni]

This reduces the size of struct skl_wm_level from 6 to 4, which
reduces the size of struct skl_plane_wm from 104 to 70, which reduces
the size of struct skl_pipe_wm from 524 to 356. A reduction of 168
padding bytes per pipe. This will increase even more the next time we
bump I915_MAX_PLANES.

Signed-off-by: Paulo Zanoni <***@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3017ef037fed..3616b718b5d2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1243,9 +1243,9 @@ struct skl_ddb_values {
};

struct skl_wm_level {
- bool plane_en;
uint16_t plane_res_b;
uint8_t plane_res_l;
+ bool plane_en;
};

/* Stores plane specific WM parameters */

[Lucas De Marchi]

This post might be inappropriate. Click to display it.

[Paulo Zanoni]

We're currently doing it in two different ways, none of them based on
the wm_params struct. Both places are correct, so I chose to keep the
one in skl_compute_wm_levels() since it's the function that sets the
other values for the same struct. But I'm open to better suggestions
on the place to assign it.

Signed-off-by: Paulo Zanoni <***@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d1dd3ae408f9..7fd344b81d66 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4832,8 +4832,7 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
return ret;
}

- if (intel_pstate->base.fb->format->format == DRM_FORMAT_NV12)
- wm->is_planar = true;
+ wm->is_planar = wm_params->is_planar;

return 0;
}
@@ -4972,8 +4971,6 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,

/* uv plane watermarks must also be validated for NV12/Planar */
if (wm_params.is_planar) {
- wm->is_planar = true;
-
ret = skl_compute_plane_wm_params(dev_priv, cstate,
intel_pstate,
&wm_params, 1);

[Ville Syrjälä]

Post by Paulo Zanoni
We're currently doing it in two different ways, none of them based on
the wm_params struct. Both places are correct, so I chose to keep the
one in skl_compute_wm_levels() since it's the function that sets the
other values for the same struct. But I'm open to better suggestions
on the place to assign it.

I'm not a huge fan of the entire wm_params thing. Lots of
duplicated infromation for dubious gain. But given that we have
it I think this patch is fine.

Post by Paulo Zanoni
---
drivers/gpu/drm/i915/intel_pm.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d1dd3ae408f9..7fd344b81d66 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4832,8 +4832,7 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
return ret;
}
- if (intel_pstate->base.fb->format->format == DRM_FORMAT_NV12)
- wm->is_planar = true;
+ wm->is_planar = wm_params->is_planar;
return 0;
}
@@ -4972,8 +4971,6 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
/* uv plane watermarks must also be validated for NV12/Planar */
if (wm_params.is_planar) {
- wm->is_planar = true;
-
ret = skl_compute_plane_wm_params(dev_priv, cstate,
intel_pstate,
&wm_params, 1);
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Paulo Zanoni]

Print a more generic "failed to compute watermark levels" whenever any
of skl_compute_wm_levels() fail, and print only the specific error
message for the specific cases. This allows us to stop passing pstate
everywhere, making the watermarks computation code a little less
dependent on random intel state structs.

Signed-off-by: Paulo Zanoni <***@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4053f4a68657..1290efc64869 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,

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,
int level,
const struct skl_wm_params *wp,
const struct skl_wm_level *result_prev,
struct skl_wm_level *result /* out */)
{
- 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;
@@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
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,
+ DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations: blocks required = %u/%u, lines required = %u/31\n",
res_blocks, wp->ddb_blocks, res_lines);
return -EINVAL;
}
@@ -4795,7 +4789,6 @@ 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 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)
@@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,

ret = skl_compute_plane_wm(dev_priv,
cstate,
- intel_pstate,
level,
wm_params,
result_prev,
@@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
if (!wm_params.plane_visible)
continue;

- ret = skl_compute_wm_levels(dev_priv, cstate,
- intel_pstate, &wm_params, wm, 0);
- if (ret)
+ ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm,
+ 0);
+ if (ret) {
+ DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n",
+ plane->base.id, plane->name);
return ret;
+ }

skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
&wm->trans_wm);
@@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
return ret;

ret = skl_compute_wm_levels(dev_priv, cstate,
- intel_pstate, &wm_params,
- wm, 1);
- if (ret)
+ &wm_params, wm, 1);
+ if (ret) {
+ DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n",
+ plane->base.id, plane->name);
return ret;
+ }
}
}

[Ville Syrjälä]

Post by Paulo Zanoni
Print a more generic "failed to compute watermark levels" whenever any
of skl_compute_wm_levels() fail, and print only the specific error
message for the specific cases. This allows us to stop passing pstate
everywhere, making the watermarks computation code a little less
dependent on random intel state structs.

Nothing random about those structs. They are *the* state.
Using them directly rather than duplicating informationa in the
wm_params struct would probably make the code look a bit less alien.

But given that the information is duplicated I guess we don't have to
pass in the plane state. So the patch seems fine in that sense.

Post by Paulo Zanoni
---
drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4053f4a68657..1290efc64869 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
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,
int level,
const struct skl_wm_params *wp,
const struct skl_wm_level *result_prev,
struct skl_wm_level *result /* out */)
{
- 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;
@@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
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,
+ DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations: blocks required = %u/%u, lines required = %u/31\n",
res_blocks, wp->ddb_blocks, res_lines);
return -EINVAL;
}
@@ -4795,7 +4789,6 @@ 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 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)
@@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
ret = skl_compute_plane_wm(dev_priv,
cstate,
- intel_pstate,
level,
wm_params,
result_prev,
@@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
if (!wm_params.plane_visible)
continue;
- ret = skl_compute_wm_levels(dev_priv, cstate,
- intel_pstate, &wm_params, wm, 0);
- if (ret)
+ ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm,
+ 0);
+ if (ret) {
+ DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n",
+ plane->base.id, plane->name);
return ret;
+ }
skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
&wm->trans_wm);
@@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
return ret;
ret = skl_compute_wm_levels(dev_priv, cstate,
- intel_pstate, &wm_params,
- wm, 1);
- if (ret)
+ &wm_params, wm, 1);
+ if (ret) {
+ DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n",
+ plane->base.id, plane->name);
return ret;
+ }
}
}
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Ville Syrjälä]

Post by Ville Syrjälä

Post by Paulo Zanoni
Print a more generic "failed to compute watermark levels" whenever any
of skl_compute_wm_levels() fail, and print only the specific error
message for the specific cases. This allows us to stop passing pstate
everywhere, making the watermarks computation code a little less
dependent on random intel state structs.

Nothing random about those structs. They are *the* state.
Using them directly rather than duplicating informationa in the
wm_params struct would probably make the code look a bit less alien.
But given that the information is duplicated I guess we don't have to
pass in the plane state. So the patch seems fine in that sense.

And this will definitely conflict with Maarten's NV12 work. Either we
need to duplicate even more stuff or just pass the proper states around.

Post by Ville Syrjälä

Post by Paulo Zanoni
---
drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4053f4a68657..1290efc64869 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
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,
int level,
const struct skl_wm_params *wp,
const struct skl_wm_level *result_prev,
struct skl_wm_level *result /* out */)
{
- 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;
@@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
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,
+ DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations: blocks required = %u/%u, lines required = %u/31\n",
res_blocks, wp->ddb_blocks, res_lines);
return -EINVAL;
}
@@ -4795,7 +4789,6 @@ 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 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)
@@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
ret = skl_compute_plane_wm(dev_priv,
cstate,
- intel_pstate,
level,
wm_params,
result_prev,
@@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
if (!wm_params.plane_visible)
continue;
- ret = skl_compute_wm_levels(dev_priv, cstate,
- intel_pstate, &wm_params, wm, 0);
- if (ret)
+ ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm,
+ 0);
+ if (ret) {
+ DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n",
+ plane->base.id, plane->name);
return ret;
+ }
skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
&wm->trans_wm);
@@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
return ret;
ret = skl_compute_wm_levels(dev_priv, cstate,
- intel_pstate, &wm_params,
- wm, 1);
- if (ret)
+ &wm_params, wm, 1);
+ if (ret) {
+ DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n",
+ plane->base.id, plane->name);
return ret;
+ }
}
}
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrjälä
Intel

[Paulo Zanoni]

Post by Ville Syrjälä

Post by Paulo Zanoni
Print a more generic "failed to compute watermark levels" whenever any
of skl_compute_wm_levels() fail, and print only the specific error
message for the specific cases. This allows us to stop passing pstate
everywhere, making the watermarks computation code a little less
dependent on random intel state structs.

Nothing random about those structs. They are *the* state.

What I'm about to say is all probably a reflex of my own incompetence
to understand the flows of our code, but here it goes.

1. There's duplication in those structs. At any given point of our
source code, should you use state structs passed as parameters, or
should you use object->state (e.g., intel_crtc->state)? Sometimes one
thing is the new state and the other thing is the old state, sometimes
it is the opposite, and checking which one is which is never trivial. I
always have to go back to intel_display.c and try to find that point in
the modeset where we assign crtc_config to crtc->config and then try to
figure out if my code runs before or after that point. And I'm never
100% confident I'm using the correct struct.

2. There's a lot of duplication in members of those structs. There are
like 5 different things that could mean "pixel rate" (for real mode,
for adjusted mode, considering/not-considering scaling, rotation, etc),
there are many things that could mean "source width", etc. So when you
need a specific value, it's not always clear where to get it from in
our driver.

3. The names inside the watermarks calculation page (or anywhere else
in our spec) don't easily translate to any of those struct members
mentioned in item 2. E.g., plane pixel rate.

I mean, look at how many times the spec (not only for watermarks, but
for other stuff too, like FBC and PSR) had wording like "source size"
and we fetched the value from one place, but then we learned that we
needed to fetch the value from this other place that was the same in
most cases except for these X and Y corner cases. You reviewed some of
those patches.

So, to conclude the argument, the nice thing about struct skl_wm_params
is that it allows us to have a single point where we translate "i915
terminology" to "watermarks calculation algorithm terminology", so we
can fix that single place if/when needed. And then everywhere else in
watermarks code we just fetch the value from this struct without having
to worry "at this point in the sequence, where should this value come
from?", allowing us to focus on the algorithm specifically. On top of
that, there's the fact that we only compute things once instead of up
to 8 times per plane (7 levels + transition watermarks).

Of course, the downside is that we address the problem of "too many
places to fetch information from" by introducing a new place where
information is fetched from, so I definitely can't argue that the
current solution is good either. I would really like to know what are
your proposals here: if we decide to remove the params struct, would
you suggest we simply fetch everything directly from the passed structs
and recompute all the values that are computed multiple times? I'm
totally willing to implement something better if you have it in your
head.

Post by Ville Syrjälä
Using them directly rather than duplicating informationa in the
wm_params struct would probably make the code look a bit less alien.
But given that the information is duplicated I guess we don't have to
pass in the plane state. So the patch seems fine in that sense.

Thanks for the reviews!

Post by Ville Syrjälä

Post by Paulo Zanoni
---
drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index 4053f4a68657..1290efc64869 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct
drm_i915_private *dev_priv,
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,
int level,
const struct skl_wm_params *wp,
const struct skl_wm_level
*result_prev,
struct skl_wm_level *result /* out
*/)
{
- const struct drm_plane_state *pstate = &intel_pstate-

Post by Paulo Zanoni
base;

uint32_t latency = dev_priv->wm.skl_latency[level];
uint_fixed_16_16_t method1, method2;
uint_fixed_16_16_t selected_result;
@@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct
drm_i915_private *dev_priv,
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,
+ DRM_DEBUG_KMS("Requested display
configuration exceeds system watermark limitations: blocks required
= %u/%u, lines required = %u/31\n",
res_blocks, wp->ddb_blocks,
res_lines);
return -EINVAL;
}
@@ -4795,7 +4789,6 @@ 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 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)
@@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct
drm_i915_private *dev_priv,
ret = skl_compute_plane_wm(dev_priv,
cstate,
- intel_pstate,
level,
wm_params,
result_prev,
@@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct
intel_crtc_state *cstate,
if (!wm_params.plane_visible)
continue;
- ret = skl_compute_wm_levels(dev_priv, cstate,
- intel_pstate,
&wm_params, wm, 0);
- if (ret)
+ ret = skl_compute_wm_levels(dev_priv, cstate,
&wm_params, wm,
+ 0);
+ if (ret) {
+ DRM_DEBUG_KMS("[PLANE:%d:%s] failed to
compute watermark levels\n",
+ plane->base.id, plane-

Post by Paulo Zanoni
name);

return ret;
+ }
skl_compute_transition_wm(cstate, &wm_params, &wm-

Post by Paulo Zanoni
wm[0],

&wm->trans_wm);
@@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct
intel_crtc_state *cstate,
return ret;
ret = skl_compute_wm_levels(dev_priv,
cstate,
- intel_pstate,
&wm_params,
- wm, 1);
- if (ret)
+ &wm_params,
wm, 1);
+ if (ret) {
+ DRM_DEBUG_KMS("[PLANE:%d:%s]
failed to compute planar watermark levels\n",
+ plane->base.id,
plane->name);
return ret;
+ }
}
}
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Ville Syrjälä]

Post by Paulo Zanoni

Post by Ville Syrjälä

Post by Paulo Zanoni
Print a more generic "failed to compute watermark levels" whenever any
of skl_compute_wm_levels() fail, and print only the specific error
message for the specific cases. This allows us to stop passing pstate
everywhere, making the watermarks computation code a little less
dependent on random intel state structs.

Nothing random about those structs. They are *the* state.

What I'm about to say is all probably a reflex of my own incompetence
to understand the flows of our code, but here it goes.
1. There's duplication in those structs. At any given point of our
source code, should you use state structs passed as parameters, or
should you use object->state (e.g., intel_crtc->state)? Sometimes one
thing is the new state and the other thing is the old state, sometimes
it is the opposite, and checking which one is which is never trivial.

That was solved with the mlankhorst iterators. So no more foo->state,
except during readout since we don't currently allocate new states
for that.

Post by Paulo Zanoni
I
always have to go back to intel_display.c and try to find that point in
the modeset where we assign crtc_config to crtc->config and then try to
figure out if my code runs before or after that point. And I'm never
100% confident I'm using the correct struct.

And Maarten has been busy elimninating crtc->config as well. Not too
many uses left I hope.

Post by Paulo Zanoni
2. There's a lot of duplication in members of those structs. There are
like 5 different things that could mean "pixel rate" (for real mode,
for adjusted mode, considering/not-considering scaling, rotation, etc),
there are many things that could mean "source width", etc. So when you
need a specific value, it's not always clear where to get it from in
our driver.
3. The names inside the watermarks calculation page (or anywhere else
in our spec) don't easily translate to any of those struct members
mentioned in item 2. E.g., plane pixel rate.
I mean, look at how many times the spec (not only for watermarks, but
for other stuff too, like FBC and PSR) had wording like "source size"
and we fetched the value from one place, but then we learned that we
needed to fetch the value from this other place that was the same in
most cases except for these X and Y corner cases. You reviewed some of
those patches.

I don't recall many corner cases. Well, I guess you can count the
cursor as one. But that's about it I think.

Rotation was maybe the one that caused the most confusion but I think
that was because the spec was very vague. One actually has to think
a bit to figure out if it's referring to the fb orientation or the
scanout orientation.

Post by Paulo Zanoni
So, to conclude the argument, the nice thing about struct skl_wm_params
is that it allows us to have a single point where we translate "i915
terminology" to "watermarks calculation algorithm terminology", so we
can fix that single place if/when needed.

That can usually be solved with functions as well.

Post by Paulo Zanoni
And then everywhere else in
watermarks code we just fetch the value from this struct without having
to worry "at this point in the sequence, where should this value come
from?", allowing us to focus on the algorithm specifically. On top of
that, there's the fact that we only compute things once instead of up
to 8 times per plane (7 levels + transition watermarks).
Of course, the downside is that we address the problem of "too many
places to fetch information from" by introducing a new place where
information is fetched from, so I definitely can't argue that the
current solution is good either. I would really like to know what are
your proposals here: if we decide to remove the params struct, would
you suggest we simply fetch everything directly from the passed structs
and recompute all the values that are computed multiple times? I'm
totally willing to implement something better if you have it in your
head.

The "let's not recompute the same thing many times" argument may
have some merit. Though most things are perhaps cheap enough to not
matter much. And we could always stick more stuff into the plane/crtc
states.

But I don't have any concrete proposals at this time. I've tried to
stay away from the skl wm code as much as possible so as to not get
an urge to rewrite it :P

[Paulo Zanoni]

The skl_compute_plane_wm_params() already completely sets the contents
of its struct, or returns plane_visible=0 or returns an error code.
There's no need to memset() it at this point for the same reason we
don't zero-initialize it up when dealing with plane 0.

If we want to keep the memset "just to be safe", then we should also
zero initialize it when we use it for plane 0.

Signed-off-by: Paulo Zanoni <***@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9043ffe40ce8..d1dd3ae408f9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4972,7 +4972,6 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,

/* uv plane watermarks must also be validated for NV12/Planar */
if (wm_params.is_planar) {
- memset(&wm_params, 0, sizeof(struct skl_wm_params));
wm->is_planar = true;

ret = skl_compute_plane_wm_params(dev_priv, cstate,

[Ville Syrjälä]

Post by Paulo Zanoni
The skl_compute_plane_wm_params() already completely sets the contents
of its struct, or returns plane_visible=0 or returns an error code.
There's no need to memset() it at this point for the same reason we
don't zero-initialize it up when dealing with plane 0.

If old garbage in the struct is OK for the luma plane then it
should be OK for chroma as well.

Post by Paulo Zanoni
If we want to keep the memset "just to be safe", then we should also
zero initialize it when we use it for plane 0.
---
drivers/gpu/drm/i915/intel_pm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9043ffe40ce8..d1dd3ae408f9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4972,7 +4972,6 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
/* uv plane watermarks must also be validated for NV12/Planar */
if (wm_params.is_planar) {
- memset(&wm_params, 0, sizeof(struct skl_wm_params));
wm->is_planar = true;
ret = skl_compute_plane_wm_params(dev_priv, cstate,
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Paulo Zanoni]

With this one here we can finally drop the intel state structures from
the functions that compute watermark values: they all rely on struct
skl_wm_params now. This should help the watermarks code be a little
more clear on its intent and also match the spec a little bit more
with the carefully chosen names for its parameters.

Signed-off-by: Paulo Zanoni <***@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++------------
2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b32d680d9bf0..4712eaea9744 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1254,6 +1254,7 @@ struct skl_wm_params {
bool x_tiled, y_tiled;
bool rc_surface;
bool is_planar;
+ uint32_t pipe_htotal;
uint32_t width;
uint16_t ddb_blocks;
uint8_t cpp;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d101c542f10d..b01f3d807ff6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4549,6 +4549,8 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS;
wp->is_planar = fb->format->format == DRM_FORMAT_NV12;

+ wp->pipe_htotal = cstate->base.adjusted_mode.crtc_htotal;
+
if (plane->id == PLANE_CURSOR) {
wp->width = intel_pstate->base.crtc_w;
} else {
@@ -4630,7 +4632,6 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
}

static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
- struct intel_crtc_state *cstate,
int level,
const struct skl_wm_params *wp,
const struct skl_wm_level *result_prev,
@@ -4659,17 +4660,15 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,

method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
wp->cpp, latency, wp->dbuf_block_size);
- method2 = skl_wm_method2(wp->plane_pixel_rate,
- cstate->base.adjusted_mode.crtc_htotal,
+ method2 = skl_wm_method2(wp->plane_pixel_rate, wp->pipe_htotal,
latency,
wp->plane_blocks_per_line);

if (wp->y_tiled) {
selected_result = max_fixed16(method2, wp->y_tile_minimum);
} else {
- if ((wp->cpp * cstate->base.adjusted_mode.crtc_htotal /
- wp->dbuf_block_size < 1) &&
- (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
+ if ((wp->cpp * wp->pipe_htotal / wp->dbuf_block_size < 1) &&
+ (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
selected_result = method2;
} else if (wp->ddb_blocks >=
fixed16_to_u32_round_up(wp->plane_blocks_per_line)) {
@@ -4782,7 +4781,6 @@ 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 intel_crtc_state *cstate,
const struct skl_wm_params *wm_params,
struct skl_plane_wm *wm,
int plane_id)
@@ -4802,7 +4800,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
result_prev = plane_id ? &wm->uv_wm[0] : &wm->wm[0];

ret = skl_compute_plane_wm(dev_priv,
- cstate,
level,
wm_params,
result_prev,
@@ -4937,8 +4934,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
if (!wm_params.plane_visible)
continue;

- ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm,
- 0);
+ ret = skl_compute_wm_levels(dev_priv, &wm_params, wm, 0);
if (ret) {
DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n",
plane->base.id, plane->name);
@@ -4956,8 +4952,8 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
if (ret)
return ret;

- ret = skl_compute_wm_levels(dev_priv, cstate,
- &wm_params, wm, 1);
+ ret = skl_compute_wm_levels(dev_priv, &wm_params, wm,
+ 1);
if (ret) {
DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n",
plane->base.id, plane->name);

[Ville Syrjälä]

Post by Paulo Zanoni
With this one here we can finally drop the intel state structures from
the functions that compute watermark values: they all rely on struct
skl_wm_params now. This should help the watermarks code be a little
more clear on its intent and also match the spec a little bit more
with the carefully chosen names for its parameters.

I guess we'll just roll with the duplication.

Post by Paulo Zanoni
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++------------
2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b32d680d9bf0..4712eaea9744 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1254,6 +1254,7 @@ struct skl_wm_params {
bool x_tiled, y_tiled;
bool rc_surface;
bool is_planar;
+ uint32_t pipe_htotal;
uint32_t width;
uint16_t ddb_blocks;
uint8_t cpp;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d101c542f10d..b01f3d807ff6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4549,6 +4549,8 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS;
wp->is_planar = fb->format->format == DRM_FORMAT_NV12;
+ wp->pipe_htotal = cstate->base.adjusted_mode.crtc_htotal;
+
if (plane->id == PLANE_CURSOR) {
wp->width = intel_pstate->base.crtc_w;
} else {
@@ -4630,7 +4632,6 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
}
static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
- struct intel_crtc_state *cstate,
int level,
const struct skl_wm_params *wp,
const struct skl_wm_level *result_prev,
@@ -4659,17 +4660,15 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
wp->cpp, latency, wp->dbuf_block_size);
- method2 = skl_wm_method2(wp->plane_pixel_rate,
- cstate->base.adjusted_mode.crtc_htotal,
+ method2 = skl_wm_method2(wp->plane_pixel_rate, wp->pipe_htotal,
latency,
wp->plane_blocks_per_line);
if (wp->y_tiled) {
selected_result = max_fixed16(method2, wp->y_tile_minimum);
} else {
- if ((wp->cpp * cstate->base.adjusted_mode.crtc_htotal /
- wp->dbuf_block_size < 1) &&
- (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
+ if ((wp->cpp * wp->pipe_htotal / wp->dbuf_block_size < 1) &&
+ (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
selected_result = method2;
} else if (wp->ddb_blocks >=
fixed16_to_u32_round_up(wp->plane_blocks_per_line)) {
@@ -4782,7 +4781,6 @@ 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 intel_crtc_state *cstate,
const struct skl_wm_params *wm_params,
struct skl_plane_wm *wm,
int plane_id)
@@ -4802,7 +4800,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
result_prev = plane_id ? &wm->uv_wm[0] : &wm->wm[0];
ret = skl_compute_plane_wm(dev_priv,
- cstate,
level,
wm_params,
result_prev,
@@ -4937,8 +4934,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
if (!wm_params.plane_visible)
continue;
- ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm,
- 0);
+ ret = skl_compute_wm_levels(dev_priv, &wm_params, wm, 0);
if (ret) {
DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n",
plane->base.id, plane->name);
@@ -4956,8 +4952,8 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
if (ret)
return ret;
- ret = skl_compute_wm_levels(dev_priv, cstate,
- &wm_params, wm, 1);
+ ret = skl_compute_wm_levels(dev_priv, &wm_params, wm,
+ 1);
if (ret) {
DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n",
plane->base.id, plane->name);
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Paulo Zanoni]

Stop passing modeset state structures to functions that should work
only with the skl_wm_params. The only use for cstate there was to
reach dev_priv, so pass it directly.

Signed-off-by: Paulo Zanoni <***@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b01f3d807ff6..dac2613aaaa3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4836,13 +4836,11 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate)
return linetime_wm;
}

-static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
+static void skl_compute_transition_wm(const struct drm_i915_private *dev_priv,
struct skl_wm_params *wp,
struct skl_wm_level *wm_l0,
struct skl_wm_level *trans_wm /* out */)
{
- struct drm_device *dev = cstate->base.crtc->dev;
- 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 wm0_sel_res_b, trans_offset_b, res_blocks;
@@ -4941,7 +4939,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
return ret;
}

- skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
+ skl_compute_transition_wm(dev_priv, &wm_params, &wm->wm[0],
&wm->trans_wm);

/* uv plane watermarks must also be validated for NV12/Planar */

[Ville Syrjälä]

Post by Paulo Zanoni
Stop passing modeset state structures to functions that should work
only with the skl_wm_params. The only use for cstate there was to
reach dev_priv, so pass it directly.
---
drivers/gpu/drm/i915/intel_pm.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b01f3d807ff6..dac2613aaaa3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4836,13 +4836,11 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate)
return linetime_wm;
}
-static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
+static void skl_compute_transition_wm(const struct drm_i915_private *dev_priv,
struct skl_wm_params *wp,
struct skl_wm_level *wm_l0,
struct skl_wm_level *trans_wm /* out */)
{
- struct drm_device *dev = cstate->base.crtc->dev;
- 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 wm0_sel_res_b, trans_offset_b, res_blocks;
@@ -4941,7 +4939,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
return ret;
}
- skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
+ skl_compute_transition_wm(dev_priv, &wm_params, &wm->wm[0],
&wm->trans_wm);
/* uv plane watermarks must also be validated for NV12/Planar */
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Paulo Zanoni]

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 <***@intel.com>
---
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 ?
+ 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)

[Ville Syrjälä]

Post by Paulo Zanoni
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.
---
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.

Post by Paulo Zanoni
+ 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->plane[pipe][intel_plane_id]);
-
for (level = 0; level <= max_level; 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
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Paulo Zanoni]

Post by Ville Syrjälä

Post by Paulo Zanoni
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.
---
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-

Post by Paulo Zanoni
base.plane);

const struct drm_plane_state *pstate = &intel_pstate-

Post by Paulo Zanoni
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(csta
te));
+ 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.

I completely agree: there's plane_id and plane->id and they mean
different things.

If you're already using color_plane I can totally go with it so we have
consistency. But, well, for RGB planes, plane 0 does contain color
information... But can I suggest us to use "subplane" terminology when
talking about the planes of a plane? What about subplane_index?
Something that does not use the word "plane" is probably better.

Post by Ville Syrjälä
Patch looks good to me.

Post by Paulo Zanoni
+ skl_ddb_entry_size(&ddb-
+ skl_ddb_entry_size(&ddb->plane[pipe][plane-

Post by Paulo Zanoni
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-

Post by Paulo Zanoni
dbuf_block_size < 1)) {

selected_result = method2;
- } else if (ddb_allocation >=
+ } else if (wp->ddb_blocks >=
fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
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-

Post by Paulo Zanoni
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-
- skl_ddb_entry_size(&ddb-

Post by Paulo Zanoni
plane[pipe][intel_plane_id]);

-
for (level = 0; level <= max_level; level++) {
struct skl_wm_level *result = plane_id ? &wm-
&wm-

Post by Paulo Zanoni
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_sta
te(pstate);
enum plane_id plane_id = to_intel_plane(plane)-

Post by Paulo Zanoni
id;

struct skl_wm_params wm_params;
- enum pipe pipe = to_intel_crtc(cstate->base.crtc)-

Post by Paulo Zanoni
pipe;

- uint16_t ddb_blocks;
wm = &pipe_wm->planes[plane_id];
- ddb_blocks = skl_ddb_entry_size(&ddb-

Post by Paulo Zanoni
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-

Post by Paulo Zanoni
wm[0],

- ddb_blocks, &wm-

Post by Paulo Zanoni
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_ps
tate,
+ intel_ps
tate, ddb,
&wm_para
ms, 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
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Ville Syrjälä]

Post by Paulo Zanoni

Post by Ville Syrjälä

Post by Paulo Zanoni
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.
---
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-

Post by Paulo Zanoni
base.plane);

const struct drm_plane_state *pstate = &intel_pstate-

Post by Paulo Zanoni
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(csta
te));
+ 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.

I completely agree: there's plane_id and plane->id and they mean
different things.
If you're already using color_plane I can totally go with it so we have
consistency. But, well, for RGB planes, plane 0 does contain color
information... But can I suggest us to use "subplane" terminology when
talking about the planes of a plane? What about subplane_index?
Something that does not use the word "plane" is probably better.

What is "planes of a plane"?

There are drm_planes, hardware display planes (1:1 with drm_plane
for us), and there are the planes of a planar framebuffer (luma
plane, chroma plane, etc.). The latter is what I christened
"color plane".

Post by Paulo Zanoni

Post by Ville Syrjälä
Patch looks good to me.

Post by Paulo Zanoni
+ skl_ddb_entry_size(&ddb-
+ skl_ddb_entry_size(&ddb->plane[pipe][plane-

Post by Paulo Zanoni
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-

Post by Paulo Zanoni
dbuf_block_size < 1)) {

selected_result = method2;
- } else if (ddb_allocation >=
+ } else if (wp->ddb_blocks >=
fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
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-

Post by Paulo Zanoni
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-
- skl_ddb_entry_size(&ddb-

Post by Paulo Zanoni
plane[pipe][intel_plane_id]);

-
for (level = 0; level <= max_level; level++) {
struct skl_wm_level *result = plane_id ? &wm-
&wm-

Post by Paulo Zanoni
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_sta
te(pstate);
enum plane_id plane_id = to_intel_plane(plane)-

Post by Paulo Zanoni
id;

struct skl_wm_params wm_params;
- enum pipe pipe = to_intel_crtc(cstate->base.crtc)-

Post by Paulo Zanoni
pipe;

- uint16_t ddb_blocks;
wm = &pipe_wm->planes[plane_id];
- ddb_blocks = skl_ddb_entry_size(&ddb-

Post by Paulo Zanoni
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-

Post by Paulo Zanoni
wm[0],

- ddb_blocks, &wm-

Post by Paulo Zanoni
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_ps
tate,
+ intel_ps
tate, ddb,
&wm_para
ms, 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
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Paulo Zanoni]

The function only really needs dev_priv to make its decision. If we
ever need more, we can change it again. But then, in this case we
should make needs_memory_bw_wa be a variable inside struct
skl_wm_params so we won't need to keep passing intel states deep
inside pure watermark value calculation functions.

Signed-off-by: Paulo Zanoni <***@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1290efc64869..d101c542f10d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3599,10 +3599,8 @@ static u8 intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv)
* FIXME: We still don't have the proper code detect if we need to apply the WA,
* so assume we'll always need it in order to avoid underruns.
*/
-static bool skl_needs_memory_bw_wa(struct intel_atomic_state *state)
+static bool skl_needs_memory_bw_wa(const struct drm_i915_private *dev_priv)
{
- struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-
if (IS_GEN9_BC(dev_priv) || IS_BROXTON(dev_priv))
return true;

@@ -3765,7 +3763,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)

latency = dev_priv->wm.skl_latency[level];

- if (skl_needs_memory_bw_wa(intel_state) &&
+ if (skl_needs_memory_bw_wa(dev_priv) &&
plane->base.state->fb->modifier ==
I915_FORMAT_MOD_X_TILED)
latency += 15;
@@ -4530,9 +4528,7 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
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);
- bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
+ bool apply_memory_bw_wa = skl_needs_memory_bw_wa(dev_priv);

wp->plane_visible = intel_wm_plane_visible(cstate, intel_pstate);
if (!wp->plane_visible)
@@ -4644,9 +4640,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
uint_fixed_16_16_t method1, method2;
uint_fixed_16_16_t selected_result;
uint32_t res_blocks, res_lines;
- struct intel_atomic_state *state =
- to_intel_atomic_state(cstate->base.state);
- bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
+ bool apply_memory_bw_wa = skl_needs_memory_bw_wa(dev_priv);
uint32_t min_disp_buf_needed;

if (latency == 0) {

[Ville Syrjälä]

Post by Paulo Zanoni
The function only really needs dev_priv to make its decision. If we
ever need more, we can change it again. But then, in this case we
should make needs_memory_bw_wa be a variable inside struct
skl_wm_params so we won't need to keep passing intel states deep
inside pure watermark value calculation functions.

Again, I tend to disagree with the notion that wm_params is
somehow better than the atomic state(s) proper.

But the patch itself looks fine so

Post by Paulo Zanoni
---
drivers/gpu/drm/i915/intel_pm.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1290efc64869..d101c542f10d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3599,10 +3599,8 @@ static u8 intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv)
* FIXME: We still don't have the proper code detect if we need to apply the WA,
* so assume we'll always need it in order to avoid underruns.
*/
-static bool skl_needs_memory_bw_wa(struct intel_atomic_state *state)
+static bool skl_needs_memory_bw_wa(const struct drm_i915_private *dev_priv)
{
- struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-
if (IS_GEN9_BC(dev_priv) || IS_BROXTON(dev_priv))
return true;
@@ -3765,7 +3763,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
latency = dev_priv->wm.skl_latency[level];
- if (skl_needs_memory_bw_wa(intel_state) &&
+ if (skl_needs_memory_bw_wa(dev_priv) &&
plane->base.state->fb->modifier ==
I915_FORMAT_MOD_X_TILED)
latency += 15;
@@ -4530,9 +4528,7 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
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);
- bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
+ bool apply_memory_bw_wa = skl_needs_memory_bw_wa(dev_priv);
wp->plane_visible = intel_wm_plane_visible(cstate, intel_pstate);
if (!wp->plane_visible)
@@ -4644,9 +4640,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
uint_fixed_16_16_t method1, method2;
uint_fixed_16_16_t selected_result;
uint32_t res_blocks, res_lines;
- struct intel_atomic_state *state =
- to_intel_atomic_state(cstate->base.state);
- bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
+ bool apply_memory_bw_wa = skl_needs_memory_bw_wa(dev_priv);
uint32_t min_disp_buf_needed;
if (latency == 0) {
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Paulo Zanoni]

Before the patch, if a plane was not visible,
skl_compute_plane_wm_params() would return early without writing
anything to the wm_params struct. This would leave garbage in the
struct since it is not previously zeroed, and then we would later
check for wm_params.is_planar, which could be true due to the usage of
uninitialized memory. This would lead us to calculate the zeroed
watermarks for the second inexistent plane and mark the plane as a
planar plane. Then later this check would affect our decisions in
skl_write_plane_wm().

I can't see how this would lead to a noticeable bug in our code, but
it leads us to calculate watermarks for every level + transition
watermarks, twice (due to the is_planar bug). So the fix saves us a
lot of instructions.

This problem was found when I decided to add a DRM_ERROR for the
currently unsupported planar formats on ICL: kms_cursor_legacy would
trigger the error message without using planar formats.

So the fix we adopt in this patch is to create a new watermark
parameter called plane_visible and use it to avoid computing the
watermarks for invisible planes later. We also remove the checks that
are now made redundant by it.

Testcase: igt/kms_cursor_legacy/nonblocking-modeset-vs-cursor-atomic
Signed-off-by: Paulo Zanoni <***@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_pm.c | 15 ++++++---------
2 files changed, 7 insertions(+), 9 deletions(-)

The error message mentioned above isd the one added by patch 06 of the
series.

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3616b718b5d2..4b1e8471609b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1250,6 +1250,7 @@ struct skl_wm_level {

/* Stores plane specific WM parameters */
struct skl_wm_params {
+ bool plane_visible;
bool x_tiled, y_tiled;
bool rc_surface;
bool is_planar;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 18157c6ee126..9043ffe40ce8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4532,7 +4532,8 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
to_intel_atomic_state(cstate->base.state);
bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);

- if (!intel_wm_plane_visible(cstate, intel_pstate))
+ wp->plane_visible = intel_wm_plane_visible(cstate, intel_pstate);
+ if (!wp->plane_visible)
return 0;

/* only NV12 format has two planes */
@@ -4645,8 +4646,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
uint32_t min_disp_buf_needed;

- if (latency == 0 ||
- !intel_wm_plane_visible(cstate, intel_pstate)) {
+ if (latency == 0) {
result->plane_en = false;
return 0;
}
@@ -4805,9 +4805,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
enum plane_id intel_plane_id = intel_plane->id;
int ret;

- if (WARN_ON(!intel_pstate->base.fb))
- return -EINVAL;
-
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]);
@@ -4876,9 +4873,6 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
const uint16_t trans_amount = 10; /* This is configurable amount */
uint16_t wm0_sel_res_b, trans_offset_b, res_blocks;

- if (!cstate->base.active)
- goto exit;
-
/* Transition WM are not recommended by HW team for GEN9 */
if (INTEL_GEN(dev_priv) <= 9)
goto exit;
@@ -4965,6 +4959,9 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
if (ret)
return ret;

+ if (!wm_params.plane_visible)
+ continue;
+
ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
intel_pstate, &wm_params, wm, 0);
if (ret)

[Ville Syrjälä]

Post by Paulo Zanoni
Before the patch, if a plane was not visible,
skl_compute_plane_wm_params() would return early without writing
anything to the wm_params struct. This would leave garbage in the
struct since it is not previously zeroed, and then we would later
check for wm_params.is_planar, which could be true due to the usage of
uninitialized memory. This would lead us to calculate the zeroed
watermarks for the second inexistent plane and mark the plane as a
planar plane. Then later this check would affect our decisions in
skl_write_plane_wm().
I can't see how this would lead to a noticeable bug in our code, but
it leads us to calculate watermarks for every level + transition
watermarks, twice (due to the is_planar bug). So the fix saves us a
lot of instructions.
This problem was found when I decided to add a DRM_ERROR for the
currently unsupported planar formats on ICL: kms_cursor_legacy would
trigger the error message without using planar formats.
So the fix we adopt in this patch is to create a new watermark
parameter called plane_visible and use it to avoid computing the
watermarks for invisible planes later. We also remove the checks that
are now made redundant by it.
Testcase: igt/kms_cursor_legacy/nonblocking-modeset-vs-cursor-atomic
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_pm.c | 15 ++++++---------
2 files changed, 7 insertions(+), 9 deletions(-)
The error message mentioned above isd the one added by patch 06 of the
series.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3616b718b5d2..4b1e8471609b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1250,6 +1250,7 @@ struct skl_wm_level {
/* Stores plane specific WM parameters */
struct skl_wm_params {
+ bool plane_visible;
bool x_tiled, y_tiled;
bool rc_surface;
bool is_planar;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 18157c6ee126..9043ffe40ce8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4532,7 +4532,8 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
to_intel_atomic_state(cstate->base.state);
bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
- if (!intel_wm_plane_visible(cstate, intel_pstate))
+ wp->plane_visible = intel_wm_plane_visible(cstate, intel_pstate);
+ if (!wp->plane_visible)
return 0;
/* only NV12 format has two planes */
@@ -4645,8 +4646,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
uint32_t min_disp_buf_needed;
- if (latency == 0 ||
- !intel_wm_plane_visible(cstate, intel_pstate)) {
+ if (latency == 0) {
result->plane_en = false;
return 0;
}
@@ -4805,9 +4805,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
enum plane_id intel_plane_id = intel_plane->id;
int ret;
- if (WARN_ON(!intel_pstate->base.fb))
- return -EINVAL;
-
ddb_blocks = plane_id ?
skl_ddb_entry_size(&ddb->plane[pipe][intel_plane_id]);
@@ -4876,9 +4873,6 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
const uint16_t trans_amount = 10; /* This is configurable amount */
uint16_t wm0_sel_res_b, trans_offset_b, res_blocks;
- if (!cstate->base.active)
- goto exit;
-
/* Transition WM are not recommended by HW team for GEN9 */
if (INTEL_GEN(dev_priv) <= 9)
goto exit;
@@ -4965,6 +4959,9 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
if (ret)
return ret;
+ if (!wm_params.plane_visible)
+ continue;
+

So this will now skip both skl_compute_wm_levels() and
skl_compute_transition_wm() for this plane, and we've
zeroed the entire pipe_wm->planes thing earlier.
Seems good.

Since we're only checking this in this one place I don't
think we really need to add wm_params.plane_visible. Could
just do the intel_wm_plane_visible() check right here no?

Post by Paulo Zanoni
ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
intel_pstate, &wm_params, wm, 0);
if (ret)
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Paulo Zanoni]

This post might be inappropriate. Click to display it.

[Ville Syrjälä]

Post by Paulo Zanoni
Its control flow is not as easy to follow as it could be. We recently
even had a double register write that went unnoticed until commit
9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time")
fixed it. The return statement in the middle along with the fact that
it's on a void function really doesn't help readability IMHO.
Refactor the function so that the first level of checks is per
platform and the second level is for planar planes. IMHO that makes
the code much more readable.
---
drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7fd344b81d66..f388bfa99a97 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5033,21 +5033,28 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
&wm->trans_wm);
- /* FIXME: add proper NV12 support for ICL. */
- if (INTEL_GEN(dev_priv) >= 11)
- return skl_ddb_entry_write(dev_priv,
- PLANE_BUF_CFG(pipe, plane_id),
- &ddb->plane[pipe][plane_id]);
- if (wm->is_planar) {
- skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
- &ddb->uv_plane[pipe][plane_id]);
+ if (INTEL_GEN(dev_priv) >= 11) {
+ /* FIXME: add proper NV12 support for ICL. */
+ if (wm->is_planar)
+ DRM_ERROR("No DDB support for planar formats yet\n");
+
skl_ddb_entry_write(dev_priv,
- PLANE_NV12_BUF_CFG(pipe, plane_id),
- &ddb->plane[pipe][plane_id]);
+ PLANE_BUF_CFG(pipe, plane_id),
+ &ddb->plane[pipe][plane_id]);
} else {
- skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
- &ddb->plane[pipe][plane_id]);
- I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
+ if (wm->is_planar) {
+ skl_ddb_entry_write(dev_priv,
+ PLANE_BUF_CFG(pipe, plane_id),
+ &ddb->uv_plane[pipe][plane_id]);
+ skl_ddb_entry_write(dev_priv,
+ PLANE_NV12_BUF_CFG(pipe, plane_id),
+ &ddb->plane[pipe][plane_id]);
+ } else {
+ skl_ddb_entry_write(dev_priv,
+ PLANE_BUF_CFG(pipe, plane_id),
+ &ddb->plane[pipe][plane_id]);
+ I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
+ }
}
}
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Patchwork]

== Series Details ==

Series: More watermarks improvements
URL : https://patchwork.freedesktop.org/series/51086/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
6a8c3f3d3663 drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
6165f4257098 drm/i915: remove padding from struct skl_wm_level
-:25: CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384
#25: FILE: drivers/gpu/drm/i915/i915_drv.h:1248:
+ bool plane_en;

total: 0 errors, 0 warnings, 1 checks, 10 lines checked
7d4eced125cf drm/i915: fix handling of invisible planes in watermarks code
-:41: CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384
#41: FILE: drivers/gpu/drm/i915/i915_drv.h:1253:
+ bool plane_visible;

total: 0 errors, 0 warnings, 1 checks, 52 lines checked
20ce35c3029a drm/i915: remove useless memset() for watermarks parameters
10134ecdb23d drm/i915: simplify wm->is_planar assignment
b6f917785676 drm/i915: refactor skl_write_plane_wm()
-:8: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time")'
#8:
9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time")

-:16: WARNING:BAD_SIGN_OFF: Non-standard signature: Requested-by:
#16:
Requested-by: Matt Roper <***@intel.com>

total: 1 errors, 1 warnings, 0 checks, 41 lines checked
52262264df7e drm/i915: move ddb_blocks to be a watermark parameter
c801352ac21a drm/i915: reorganize the error message for invalid watermarks
e541090ceec3 drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state
03f7a31192d8 drm/i915: add pipe_htotal to struct skl_wm_params
4a4d7716aeaa drm/i915: pass dev_priv instead of cstate to skl_compute_transition_wm()

[Paulo Zanoni]

Post by Patchwork
== Series Details ==
Series: More watermarks improvements
URL : https://patchwork.freedesktop.org/series/51086/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
6a8c3f3d3663 drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
6165f4257098 drm/i915: remove padding from struct skl_wm_level
-:25: CHECK:BOOL_MEMBER: Avoid using bool structure members because
of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/
384
+ bool plane_en;
total: 0 errors, 0 warnings, 1 checks, 10 lines checked

That won't save us anything in this specific case, and it's not our
usual coding standard. If we conclude i915.ko should go this way, I'd
be happy to comply. Maintainers?

Post by Patchwork
7d4eced125cf drm/i915: fix handling of invisible planes in watermarks code
-:41: CHECK:BOOL_MEMBER: Avoid using bool structure members because
of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/
384
+ bool plane_visible;

The rest of the struct also uses bools like this. If we go this route,
we should probably change the whole struct.

Post by Patchwork
total: 0 errors, 0 warnings, 1 checks, 52 lines checked
20ce35c3029a drm/i915: remove useless memset() for watermarks
parameters
10134ecdb23d drm/i915: simplify wm->is_planar assignment
b6f917785676 drm/i915: refactor skl_write_plane_wm()
-:8: ERROR:GIT_COMMIT_ID: Please use git commit description style
'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit
9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every
time")'
9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time")

Well, I used that description style, but it's in the middle of a
paragraph so the word "commit" is in the line above. If that's not what
you're asking, then I don't know.
Is this really undesirable?

Post by Patchwork
total: 1 errors, 1 warnings, 0 checks, 41 lines checked
52262264df7e drm/i915: move ddb_blocks to be a watermark parameter
c801352ac21a drm/i915: reorganize the error message for invalid watermarks
e541090ceec3 drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state
03f7a31192d8 drm/i915: add pipe_htotal to struct skl_wm_params
4a4d7716aeaa drm/i915: pass dev_priv instead of cstate to
skl_compute_transition_wm()

[Patchwork]

== Series Details ==

Series: More watermarks improvements
URL : https://patchwork.freedesktop.org/series/51086/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
Okay!

Commit: drm/i915: remove padding from struct skl_wm_level
Okay!

Commit: drm/i915: fix handling of invisible planes in watermarks code
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3725:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3726:16: warning: expression using sizeof(void)

Commit: drm/i915: remove useless memset() for watermarks parameters
Okay!

Commit: drm/i915: simplify wm->is_planar assignment
Okay!

Commit: drm/i915: refactor skl_write_plane_wm()
Okay!

Commit: drm/i915: move ddb_blocks to be a watermark parameter
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3726:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3727:16: warning: expression using sizeof(void)

Commit: drm/i915: reorganize the error message for invalid watermarks
Okay!

Commit: drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state
Okay!

Commit: drm/i915: add pipe_htotal to struct skl_wm_params
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3727:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3728:16: warning: expression using sizeof(void)

Commit: drm/i915: pass dev_priv instead of cstate to skl_compute_transition_wm()
Okay!

[Patchwork]

== Series Details ==

Series: More watermarks improvements
URL : https://patchwork.freedesktop.org/series/51086/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4990 -> Patchwork_10481 =

== Summary - FAILURE ==

Serious unknown changes coming with Patchwork_10481 absolutely need to be
verified manually.

If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10481, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.

External URL: https://patchwork.freedesktop.org/api/1.0/series/51086/revisions/1/mbox/

== Possible new issues ==

Here are the unknown changes that may have been introduced in Patchwork_10481:

=== IGT changes ===

==== Possible regressions ====

***@pm_rpm@module-reload:
fi-glk-j4005: PASS -> DMESG-FAIL


== Known issues ==

Here are the changes found in Patchwork_10481 that come from known issues:

=== IGT changes ===

==== Issues hit ====

***@gem_exec_suspend@basic-s3:
fi-kbl-soraka: NOTRUN -> INCOMPLETE (fdo#107859, fdo#107774, fdo#107556)

***@kms_flip@basic-flip-vs-dpms:
fi-glk-j4005: PASS -> DMESG-WARN (fdo#106000, fdo#106097)

***@kms_flip@basic-plain-flip:
fi-ilk-650: PASS -> DMESG-WARN (fdo#106387) +2

***@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
fi-byt-clapper: PASS -> FAIL (fdo#103191, fdo#107362)


==== Possible fixes ====

***@drv_module_reload@basic-reload:
fi-glk-j4005: DMESG-WARN (fdo#106248, fdo#106725) -> PASS

***@drv_selftest@live_hangcheck:
fi-icl-u2: INCOMPLETE (fdo#108315) -> PASS

***@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
fi-glk-j4005: FAIL (fdo#106765) -> PASS

***@kms_flip@basic-flip-vs-dpms:
fi-hsw-4770r: DMESG-WARN (fdo#105602) -> PASS

***@kms_flip@basic-flip-vs-wf_vblank:
fi-glk-j4005: FAIL (fdo#100368) -> PASS

***@kms_flip@basic-plain-flip:
fi-glk-j4005: DMESG-WARN (fdo#106097) -> PASS

***@kms_frontbuffer_tracking@basic:
fi-icl-u2: FAIL (fdo#103167) -> PASS
fi-byt-clapper: FAIL (fdo#103167) -> PASS


fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387
fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
fdo#106765 https://bugs.freedesktop.org/show_bug.cgi?id=106765
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
fdo#107859 https://bugs.freedesktop.org/show_bug.cgi?id=107859
fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315


== Participating hosts (46 -> 38) ==

Additional (1): fi-kbl-soraka
Missing (9): fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-skl-guc fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-pnv-d510 fi-kbl-7560u


== Build changes ==

* Linux: CI_DRM_4990 -> Patchwork_10481

CI_DRM_4990: 0bd34d92e9a27784cb988a300619f497ca0e99ec @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4681: 959d6f95cb1344e0c0dace5b236e17755826fac1 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10481: 4a4d7716aeaa870a7d0aaa1e541706cb919096cd @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

4a4d7716aeaa drm/i915: pass dev_priv instead of cstate to skl_compute_transition_wm()
03f7a31192d8 drm/i915: add pipe_htotal to struct skl_wm_params
e541090ceec3 drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state
c801352ac21a drm/i915: reorganize the error message for invalid watermarks
52262264df7e drm/i915: move ddb_blocks to be a watermark parameter
b6f917785676 drm/i915: refactor skl_write_plane_wm()
10134ecdb23d drm/i915: simplify wm->is_planar assignment
20ce35c3029a drm/i915: remove useless memset() for watermarks parameters
7d4eced125cf drm/i915: fix handling of invisible planes in watermarks code
6165f4257098 drm/i915: remove padding from struct skl_wm_level
6a8c3f3d3663 drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10481/issues.html

[Paulo Zanoni]

Post by Patchwork
== Series Details ==
Series: More watermarks improvements
URL : https://patchwork.freedesktop.org/series/51086/
State : failure
== Summary ==
= CI Bug Log - changes from CI_DRM_4990 -> Patchwork_10481 =
== Summary - FAILURE ==
Serious unknown changes coming with Patchwork_10481 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10481, please notify your bug team to allow them
to document this new failure mode, which will reduce false
positives in CI.
External URL: https://patchwork.freedesktop.org/api/1.0/series/5108
6/revisions/1/mbox/
== Possible new issues ==
=== IGT changes ===
==== Possible regressions ====
fi-glk-j4005: PASS -> DMESG-FAIL

EDID shows invalid data after module reload, triggering error during
mode comparison in the subtest. Hard to think how this series could
cause that.

Post by Patchwork
== Known issues ==
=== IGT changes ===
==== Issues hit ====
fi-kbl-soraka: NOTRUN -> INCOMPLETE (fdo#107859,
fdo#107774, fdo#107556)
fi-glk-j4005: PASS -> DMESG-WARN (fdo#106000, fdo#106097)
fi-ilk-650: PASS -> DMESG-WARN (fdo#106387) +2
fi-byt-clapper: PASS -> FAIL (fdo#103191, fdo#107362)
==== Possible fixes ====
fi-glk-j4005: DMESG-WARN (fdo#106248, fdo#106725) -> PASS
fi-icl-u2: INCOMPLETE (fdo#108315) -> PASS
fi-glk-j4005: FAIL (fdo#106765) -> PASS
fi-hsw-4770r: DMESG-WARN (fdo#105602) -> PASS
fi-glk-j4005: FAIL (fdo#100368) -> PASS
fi-glk-j4005: DMESG-WARN (fdo#106097) -> PASS
fi-icl-u2: FAIL (fdo#103167) -> PASS
fi-byt-clapper: FAIL (fdo#103167) -> PASS
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387
fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
fdo#106765 https://bugs.freedesktop.org/show_bug.cgi?id=106765
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
fdo#107859 https://bugs.freedesktop.org/show_bug.cgi?id=107859
fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315
== Participating hosts (46 -> 38) ==
Additional (1): fi-kbl-soraka
Missing (9): fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-skl-guc
fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-pnv-d510 fi-kbl-7560u
== Build changes ==
* Linux: CI_DRM_4990 -> Patchwork_10481
git://anongit.freedesktop.org/gfx-ci/linux
git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
4a4d7716aeaa drm/i915: pass dev_priv instead of cstate to
skl_compute_transition_wm()
03f7a31192d8 drm/i915: add pipe_htotal to struct skl_wm_params
e541090ceec3 drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state
c801352ac21a drm/i915: reorganize the error message for invalid watermarks
52262264df7e drm/i915: move ddb_blocks to be a watermark parameter
b6f917785676 drm/i915: refactor skl_write_plane_wm()
10134ecdb23d drm/i915: simplify wm->is_planar assignment
20ce35c3029a drm/i915: remove useless memset() for watermarks
parameters
7d4eced125cf drm/i915: fix handling of invisible planes in watermarks code
6165f4257098 drm/i915: remove padding from struct skl_wm_level
6a8c3f3d3663 drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchw
ork_10481/issues.html

[Saarinen, Jani]

HI,

-----Original Message-----
Paulo Zanoni
Sent: keskiviikko 17. lokakuuta 2018 1.53
Subject: Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for More watermarks improvements

Post by Patchwork
== Series Details ==
Series: More watermarks improvements
URL : https://patchwork.freedesktop.org/series/51086/
State : failure
== Summary ==
= CI Bug Log - changes from CI_DRM_4990 -> Patchwork_10481 =
== Summary - FAILURE ==
Serious unknown changes coming with Patchwork_10481 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10481, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://patchwork.freedesktop.org/api/1.0/series/5108
6/revisions/1/mbox/
== Possible new issues ==
=== IGT changes ===
==== Possible regressions ====
fi-glk-j4005: PASS -> DMESG-FAIL

EDID shows invalid data after module reload, triggering error during mode
comparison in the subtest. Hard to think how this series could cause that.

Re-run?

Post by Patchwork
== Known issues ==
=== IGT changes ===
==== Issues hit ====
fi-kbl-soraka: NOTRUN -> INCOMPLETE (fdo#107859,
fdo#107774, fdo#107556)
fi-glk-j4005: PASS -> DMESG-WARN (fdo#106000, fdo#106097)
fi-ilk-650: PASS -> DMESG-WARN (fdo#106387) +2
fi-byt-clapper: PASS -> FAIL (fdo#103191, fdo#107362)
==== Possible fixes ====
fi-glk-j4005: DMESG-WARN (fdo#106248, fdo#106725) -> PASS
fi-icl-u2: INCOMPLETE (fdo#108315) -> PASS
fi-glk-j4005: FAIL (fdo#106765) -> PASS
fi-hsw-4770r: DMESG-WARN (fdo#105602) -> PASS
fi-glk-j4005: FAIL (fdo#100368) -> PASS
fi-glk-j4005: DMESG-WARN (fdo#106097) -> PASS
fi-icl-u2: FAIL (fdo#103167) -> PASS
fi-byt-clapper: FAIL (fdo#103167) -> PASS
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387
fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
fdo#106765 https://bugs.freedesktop.org/show_bug.cgi?id=106765
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
fdo#107859 https://bugs.freedesktop.org/show_bug.cgi?id=107859
fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315
== Participating hosts (46 -> 38) ==
Additional (1): fi-kbl-soraka
Missing (9): fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-skl-guc
fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-pnv-d510 fi-kbl-7560u
== Build changes ==
* Linux: CI_DRM_4990 -> Patchwork_10481
git://anongit.freedesktop.org/gfx-ci/linux
git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
4a4d7716aeaa drm/i915: pass dev_priv instead of cstate to
skl_compute_transition_wm()
03f7a31192d8 drm/i915: add pipe_htotal to struct skl_wm_params
e541090ceec3 drm/i915: make skl_needs_memory_bw_wa() take dev_priv
instead of state c801352ac21a drm/i915: reorganize the error message
for invalid watermarks 52262264df7e drm/i915: move ddb_blocks to be a
watermark parameter
b6f917785676 drm/i915: refactor skl_write_plane_wm() 10134ecdb23d
remove useless memset() for watermarks parameters 7d4eced125cf
drm/i915: fix handling of invisible planes in watermarks code
6165f4257098 drm/i915: remove padding from struct skl_wm_level
6a8c3f3d3663 drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchw
ork_10481/issues.html

_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Ville Syrjälä]

Post by Paulo Zanoni
BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.
---
drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 67a4d0735291..18157c6ee126 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
res_lines = div_round_up_fixed16(selected_result,
wp->plane_blocks_per_line);
- /* Display WA #1125: skl,bxt,kbl,glk */
- if (level == 0 && wp->rc_surface)
- res_blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
+ if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {

IS_GEN9_BC || IS_BXT

would be a little easier to parse, me thinks.

Post by Paulo Zanoni
+ /* Display WA #1125: skl,bxt,kbl */
+ if (level == 0 && wp->rc_surface)
+ res_blocks +=
+ fixed16_to_u32_round_up(wp->y_tile_minimum);
+
+ /* Display WA #1126: skl,bxt,kbl */
+ if (level >= 1 && level <= 7) {
+ if (wp->y_tiled) {
+ res_blocks +=
+ fixed16_to_u32_round_up(wp->y_tile_minimum);
+ res_lines += wp->y_min_scanlines;
+ } else {
+ res_blocks++;
+ }
- /* Display WA #1126: skl,bxt,kbl,glk */
- if (level >= 1 && level <= 7) {
- if (wp->y_tiled) {
- res_blocks += fixed16_to_u32_round_up(
- wp->y_tile_minimum);
- res_lines += wp->y_min_scanlines;
- } else {
- res_blocks++;
+ /*
+ * Make sure result blocks for higher latency levels are
+ * atleast as high as level below the current level.
+ * Assumption in DDB algorithm optimization for special
+ * cases. Also covers Display WA #1125 for RC.
+ */
+ if (result_prev->plane_res_b > res_blocks)
+ res_blocks = result_prev->plane_res_b;
}
-
- /*
- * Make sure result blocks for higher latency levels are atleast
- * as high as level below the current level.
- * Assumption in DDB algorithm optimization for special cases.
- * Also covers Display WA #1125 for RC.
- */
- if (result_prev->plane_res_b > res_blocks)
- res_blocks = result_prev->plane_res_b;

This last thing is part of the glk+ watermark formula as well. But
I'm not 100% convinced that it's needed. One might assume that the the
non-decrasing latency values guarantee that the resulting watermarks
are also non-decreasing. But I've not actually done the math to see if
that's true.

Hmm. It might not actually be true on account of the 'memory latency
microseconds >= line time microseconds' check when selecting which
method to use to calculate the watermark. Not quite sure which way
that would make things go.

We also seem to be missing the res_lines handling here. But given
that we only did this for compressed fbs before I don't think this
patch is making things much worse by limiting this to pre-glk only.
The glk+ handling and res_lines fix could be done as a followup.

Post by Paulo Zanoni
}
if (INTEL_GEN(dev_priv) >= 11) {
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Paulo Zanoni]

Post by Ville Syrjälä

Post by Paulo Zanoni
BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production
stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.
---
drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------
------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index 67a4d0735291..18157c6ee126 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
struct drm_i915_private *dev_priv,
res_lines = div_round_up_fixed16(selected_result,
wp-

Post by Paulo Zanoni
plane_blocks_per_line);

- /* Display WA #1125: skl,bxt,kbl,glk */
- if (level == 0 && wp->rc_surface)
- res_blocks += fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+ if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {

IS_GEN9_BC || IS_BXT
would be a little easier to parse, me thinks.

I can do that, but what I really want is "DISPLAY_GEN(dev_priv) == 9".

/me looks at Rodrigo

Post by Ville Syrjälä

Post by Paulo Zanoni
+ /* Display WA #1125: skl,bxt,kbl */
+ if (level == 0 && wp->rc_surface)
+ res_blocks +=
+ fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+
+ /* Display WA #1126: skl,bxt,kbl */
+ if (level >= 1 && level <= 7) {
+ if (wp->y_tiled) {
+ res_blocks +=
+ fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+ res_lines += wp->y_min_scanlines;
+ } else {
+ res_blocks++;
+ }
- /* Display WA #1126: skl,bxt,kbl,glk */
- if (level >= 1 && level <= 7) {
- if (wp->y_tiled) {
- res_blocks += fixed16_to_u32_round_up(
- wp-

Post by Paulo Zanoni
y_tile_minimum);

- res_lines += wp->y_min_scanlines;
- } else {
- res_blocks++;
+ /*
+ * Make sure result blocks for higher
latency levels are
+ * atleast as high as level below the
current level.
+ * Assumption in DDB algorithm
optimization for special
+ * cases. Also covers Display WA #1125 for
RC.
+ */
+ if (result_prev->plane_res_b > res_blocks)
+ res_blocks = result_prev-

Post by Paulo Zanoni
plane_res_b;

}
-
- /*
- * Make sure result blocks for higher latency
levels are atleast
- * as high as level below the current level.
- * Assumption in DDB algorithm optimization for
special cases.
- * Also covers Display WA #1125 for RC.
- */
- if (result_prev->plane_res_b > res_blocks)
- res_blocks = result_prev->plane_res_b;

This last thing is part of the glk+ watermark formula as well.
But
I'm not 100% convinced that it's needed.

I simply can't find where this is documented. WAs 1125 and 1126, which
contain text that match this code exactly, are not applicable to GLK.
Which BSpec page and paragraph/section mentions this?

Post by Ville Syrjälä
One might assume that the the
non-decrasing latency values guarantee that the resulting watermarks
are also non-decreasing. But I've not actually done the math to see if
that's true.
Hmm. It might not actually be true on account of the 'memory latency
microseconds >= line time microseconds' check when selecting which
method to use to calculate the watermark. Not quite sure which way
that would make things go.
We also seem to be missing the res_lines handling here. But given
that we only did this for compressed fbs before I don't think this
patch is making things much worse by limiting this to pre-glk only.
The glk+ handling and res_lines fix could be done as a followup.

Post by Paulo Zanoni
}
if (INTEL_GEN(dev_priv) >= 11) {
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Rodrigo Vivi]

Post by Paulo Zanoni

Post by Ville Syrjälä

Post by Paulo Zanoni
BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.
---
drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------
------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index 67a4d0735291..18157c6ee126 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
struct drm_i915_private *dev_priv,
res_lines = div_round_up_fixed16(selected_result,
wp-

Post by Paulo Zanoni
plane_blocks_per_line);

- /* Display WA #1125: skl,bxt,kbl,glk */
- if (level == 0 && wp->rc_surface)
- res_blocks += fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+ if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {

IS_GEN9_BC || IS_BXT
would be a little easier to parse, me thinks.

I can do that, but what I really want is "DISPLAY_GEN(dev_priv) == 9".

work in progress...

btw...

DISPLAY_GEN(dev_priv) == 9 or simply DISPLAY(dev_priv, 9) ?

I'm play around here with:

#define DISPLAY(dev_priv, g) (!!((dev_priv)->info.display_mask & BIT(g-1)))
#define DISPLAY_RANGE(dev_priv, s, e) \
(!!((dev_priv)->info.display_mask & INTEL_GEN_MASK((s), (e))))

thoughts? comments? ideas?

Post by Paulo Zanoni
/me looks at Rodrigo

Post by Ville Syrjälä

Post by Paulo Zanoni
+ /* Display WA #1125: skl,bxt,kbl */
+ if (level == 0 && wp->rc_surface)
+ res_blocks +=
+ fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+
+ /* Display WA #1126: skl,bxt,kbl */
+ if (level >= 1 && level <= 7) {
+ if (wp->y_tiled) {
+ res_blocks +=
+ fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+ res_lines += wp->y_min_scanlines;
+ } else {
+ res_blocks++;
+ }
- /* Display WA #1126: skl,bxt,kbl,glk */
- if (level >= 1 && level <= 7) {
- if (wp->y_tiled) {
- res_blocks += fixed16_to_u32_round_up(
- wp-

Post by Paulo Zanoni
y_tile_minimum);

- res_lines += wp->y_min_scanlines;
- } else {
- res_blocks++;
+ /*
+ * Make sure result blocks for higher
latency levels are
+ * atleast as high as level below the
current level.
+ * Assumption in DDB algorithm
optimization for special
+ * cases. Also covers Display WA #1125 for
RC.
+ */
+ if (result_prev->plane_res_b > res_blocks)
+ res_blocks = result_prev-

Post by Paulo Zanoni
plane_res_b;

}
-
- /*
- * Make sure result blocks for higher latency
levels are atleast
- * as high as level below the current level.
- * Assumption in DDB algorithm optimization for
special cases.
- * Also covers Display WA #1125 for RC.
- */
- if (result_prev->plane_res_b > res_blocks)
- res_blocks = result_prev->plane_res_b;

This last thing is part of the glk+ watermark formula as well. But
I'm not 100% convinced that it's needed.

I simply can't find where this is documented. WAs 1125 and 1126, which
contain text that match this code exactly, are not applicable to GLK.
Which BSpec page and paragraph/section mentions this?

Post by Ville Syrjälä
One might assume that the the
non-decrasing latency values guarantee that the resulting watermarks
are also non-decreasing. But I've not actually done the math to see if
that's true.
Hmm. It might not actually be true on account of the 'memory latency
microseconds >= line time microseconds' check when selecting which
method to use to calculate the watermark. Not quite sure which way
that would make things go.
We also seem to be missing the res_lines handling here. But given
that we only did this for compressed fbs before I don't think this
patch is making things much worse by limiting this to pre-glk only.
The glk+ handling and res_lines fix could be done as a followup.

Post by Paulo Zanoni
}
if (INTEL_GEN(dev_priv) >= 11) {
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Paulo Zanoni]

Post by Rodrigo Vivi

Post by Paulo Zanoni

Post by Ville Syrjälä

Post by Paulo Zanoni
BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.
---
drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++---
----
------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index 67a4d0735291..18157c6ee126 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
struct drm_i915_private *dev_priv,
res_lines = div_round_up_fixed16(selected_result,
wp-

Post by Paulo Zanoni
plane_blocks_per_line);

- /* Display WA #1125: skl,bxt,kbl,glk */
- if (level == 0 && wp->rc_surface)
- res_blocks += fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+ if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {

IS_GEN9_BC || IS_BXT
would be a little easier to parse, me thinks.

I can do that, but what I really want is "DISPLAY_GEN(dev_priv) == 9".

work in progress...
btw...
DISPLAY_GEN(dev_priv) == 9 or simply DISPLAY(dev_priv, 9) ?

It should mimic the model we already use: INTEL_GEN(dev_priv) >= 9.

I would expect a macro called DISPLAY() to return *the* Display or to
simply display somewhere the thing I pass as an argument. Now
DISPLAY_GEN() sounds more like it returns the GEN of the DISPLAY (or
generates a Display).

Post by Rodrigo Vivi
#define DISPLAY(dev_priv, g) (!!((dev_priv)->info.display_mask & BIT(g-1)))
#define DISPLAY_RANGE(dev_priv, s, e) \
(!!((dev_priv)->info.display_mask &
INTEL_GEN_MASK((s), (e))))
thoughts? comments? ideas?

#define DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen)

Post by Rodrigo Vivi

Post by Paulo Zanoni
/me looks at Rodrigo

Post by Ville Syrjälä

Post by Paulo Zanoni
+ /* Display WA #1125: skl,bxt,kbl */
+ if (level == 0 && wp->rc_surface)
+ res_blocks +=
+ fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+
+ /* Display WA #1126: skl,bxt,kbl */
+ if (level >= 1 && level <= 7) {
+ if (wp->y_tiled) {
+ res_blocks +=
+ fixed16_to_u32_round_up(wp
-

Post by Paulo Zanoni
y_tile_minimum);

+ res_lines += wp-

Post by Paulo Zanoni
y_min_scanlines;

+ } else {
+ res_blocks++;
+ }
- /* Display WA #1126: skl,bxt,kbl,glk */
- if (level >= 1 && level <= 7) {
- if (wp->y_tiled) {
- res_blocks += fixed16_to_u32_round_up(
- wp-

Post by Paulo Zanoni
y_tile_minimum);

- res_lines += wp->y_min_scanlines;
- } else {
- res_blocks++;
+ /*
+ * Make sure result blocks for higher
latency levels are
+ * atleast as high as level below the
current level.
+ * Assumption in DDB algorithm
optimization for special
+ * cases. Also covers Display WA #1125
for
RC.
+ */
+ if (result_prev->plane_res_b >
res_blocks)
+ res_blocks = result_prev-

Post by Paulo Zanoni
plane_res_b;

}
-
- /*
- * Make sure result blocks for higher latency
levels are atleast
- * as high as level below the current level.
- * Assumption in DDB algorithm optimization
for
special cases.
- * Also covers Display WA #1125 for RC.
- */
- if (result_prev->plane_res_b > res_blocks)
- res_blocks = result_prev->plane_res_b;

This last thing is part of the glk+ watermark formula as well. But
I'm not 100% convinced that it's needed.

I simply can't find where this is documented. WAs 1125 and 1126, which
contain text that match this code exactly, are not applicable to GLK.
Which BSpec page and paragraph/section mentions this?

Post by Ville Syrjälä
One might assume that the the
non-decrasing latency values guarantee that the resulting
watermarks
are also non-decreasing. But I've not actually done the math to
see
if
that's true.
Hmm. It might not actually be true on account of the 'memory latency
microseconds >= line time microseconds' check when selecting which
method to use to calculate the watermark. Not quite sure which way
that would make things go.
We also seem to be missing the res_lines handling here. But given
that we only did this for compressed fbs before I don't think this
patch is making things much worse by limiting this to pre-glk only.
The glk+ handling and res_lines fix could be done as a followup.

Post by Paulo Zanoni
}
if (INTEL_GEN(dev_priv) >= 11) {
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Rodrigo Vivi]

Post by Paulo Zanoni

Post by Rodrigo Vivi

Post by Paulo Zanoni

Post by Ville Syrjälä

Post by Paulo Zanoni
BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.
---
drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++---
----
------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index 67a4d0735291..18157c6ee126 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
struct drm_i915_private *dev_priv,
res_lines = div_round_up_fixed16(selected_result,
wp-

Post by Paulo Zanoni
plane_blocks_per_line);

- /* Display WA #1125: skl,bxt,kbl,glk */
- if (level == 0 && wp->rc_surface)
- res_blocks += fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+ if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {

IS_GEN9_BC || IS_BXT
would be a little easier to parse, me thinks.

I can do that, but what I really want is "DISPLAY_GEN(dev_priv) == 9".

work in progress...
btw...
DISPLAY_GEN(dev_priv) == 9 or simply DISPLAY(dev_priv, 9) ?

It should mimic the model we already use: INTEL_GEN(dev_priv) >= 9.

there's a macro defined on gen we end up never using
IS_GEN(9, GEN_FOREVER) with the same effect of INTEL_GEN(dev_priv) >= 9

Should we just kill that or try to use more that instead of direct comparison?

The advantage seems to be the use of bitmasks...

Post by Paulo Zanoni
I would expect a macro called DISPLAY() to return *the* Display or to
simply display somewhere the thing I pass as an argument. Now
DISPLAY_GEN() sounds more like it returns the GEN of the DISPLAY (or
generates a Display).

what about IS_DISPLAY(dev_priv, 9) ?
and IS_DISPLAY_RANGE(dev_priv, 5, 9)

Post by Paulo Zanoni

Post by Rodrigo Vivi
#define DISPLAY(dev_priv, g) (!!((dev_priv)->info.display_mask & BIT(g-1)))
#define DISPLAY_RANGE(dev_priv, s, e) \
(!!((dev_priv)->info.display_mask &
INTEL_GEN_MASK((s), (e))))
thoughts? comments? ideas?

#define DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen)

Post by Rodrigo Vivi

Post by Paulo Zanoni
/me looks at Rodrigo

Post by Ville Syrjälä

Post by Paulo Zanoni
+ /* Display WA #1125: skl,bxt,kbl */
+ if (level == 0 && wp->rc_surface)
+ res_blocks +=
+ fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+
+ /* Display WA #1126: skl,bxt,kbl */
+ if (level >= 1 && level <= 7) {
+ if (wp->y_tiled) {
+ res_blocks +=
+ fixed16_to_u32_round_up(wp
-

Post by Paulo Zanoni
y_tile_minimum);

+ res_lines += wp-

Post by Paulo Zanoni
y_min_scanlines;

+ } else {
+ res_blocks++;
+ }
- /* Display WA #1126: skl,bxt,kbl,glk */
- if (level >= 1 && level <= 7) {
- if (wp->y_tiled) {
- res_blocks += fixed16_to_u32_round_up(
- wp-

Post by Paulo Zanoni
y_tile_minimum);

- res_lines += wp->y_min_scanlines;
- } else {
- res_blocks++;
+ /*
+ * Make sure result blocks for higher
latency levels are
+ * atleast as high as level below the
current level.
+ * Assumption in DDB algorithm
optimization for special
+ * cases. Also covers Display WA #1125
for
RC.
+ */
+ if (result_prev->plane_res_b >
res_blocks)
+ res_blocks = result_prev-

Post by Paulo Zanoni
plane_res_b;

}
-
- /*
- * Make sure result blocks for higher latency
levels are atleast
- * as high as level below the current level.
- * Assumption in DDB algorithm optimization
for
special cases.
- * Also covers Display WA #1125 for RC.
- */
- if (result_prev->plane_res_b > res_blocks)
- res_blocks = result_prev->plane_res_b;

This last thing is part of the glk+ watermark formula as well. But
I'm not 100% convinced that it's needed.

I simply can't find where this is documented. WAs 1125 and 1126, which
contain text that match this code exactly, are not applicable to GLK.
Which BSpec page and paragraph/section mentions this?

Post by Ville Syrjälä
One might assume that the the
non-decrasing latency values guarantee that the resulting watermarks
are also non-decreasing. But I've not actually done the math to
see
if
that's true.
Hmm. It might not actually be true on account of the 'memory latency
microseconds >= line time microseconds' check when selecting which
method to use to calculate the watermark. Not quite sure which way
that would make things go.
We also seem to be missing the res_lines handling here. But given
that we only did this for compressed fbs before I don't think this
patch is making things much worse by limiting this to pre-glk only.
The glk+ handling and res_lines fix could be done as a followup.

Post by Paulo Zanoni
}
if (INTEL_GEN(dev_priv) >= 11) {
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Jani Nikula]

Post by Rodrigo Vivi

Post by Paulo Zanoni

Post by Rodrigo Vivi

Post by Paulo Zanoni

Post by Ville Syrjälä

Post by Paulo Zanoni
BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.
---
drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++---
----
------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index 67a4d0735291..18157c6ee126 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
struct drm_i915_private *dev_priv,
res_lines = div_round_up_fixed16(selected_result,
wp-

Post by Paulo Zanoni
plane_blocks_per_line);

- /* Display WA #1125: skl,bxt,kbl,glk */
- if (level == 0 && wp->rc_surface)
- res_blocks += fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+ if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {

IS_GEN9_BC || IS_BXT
would be a little easier to parse, me thinks.

I can do that, but what I really want is "DISPLAY_GEN(dev_priv) == 9".

work in progress...
btw...
DISPLAY_GEN(dev_priv) == 9 or simply DISPLAY(dev_priv, 9) ?

It should mimic the model we already use: INTEL_GEN(dev_priv) >= 9.

there's a macro defined on gen we end up never using
IS_GEN(9, GEN_FOREVER) with the same effect of INTEL_GEN(dev_priv) >= 9
Should we just kill that or try to use more that instead of direct comparison?
The advantage seems to be the use of bitmasks...

Post by Paulo Zanoni
I would expect a macro called DISPLAY() to return *the* Display or to
simply display somewhere the thing I pass as an argument. Now
DISPLAY_GEN() sounds more like it returns the GEN of the DISPLAY (or
generates a Display).

what about IS_DISPLAY(dev_priv, 9) ?
and IS_DISPLAY_RANGE(dev_priv, 5, 9)

Perhaps IS_DISPLAY_GEN(dev_priv, start, end).

*However* the naming and composition of the macro is *much* less
important than what the code ends up looking. Does the display gen
adequately cover the differences between platforms ultimately?

For example, a clear counter indication is that you'll be hard pressed
to express the current HAS_GMCH_DISPLAY() in terms of display gen in a
way that doesn't have to check for VLV/CHV. I don't think you can avoid
IS_GEMINILAKE() either, you'll end up using display gen 10 in some
places but Geminilake in others, ultimately making the end result worse
than the starting point.

BR,
Jani.

Post by Rodrigo Vivi

Post by Paulo Zanoni

Post by Rodrigo Vivi
#define DISPLAY(dev_priv, g) (!!((dev_priv)->info.display_mask & BIT(g-1)))
#define DISPLAY_RANGE(dev_priv, s, e) \
(!!((dev_priv)->info.display_mask &
INTEL_GEN_MASK((s), (e))))
thoughts? comments? ideas?

#define DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen)

Post by Rodrigo Vivi

Post by Paulo Zanoni
/me looks at Rodrigo

Post by Ville Syrjälä

Post by Paulo Zanoni
+ /* Display WA #1125: skl,bxt,kbl */
+ if (level == 0 && wp->rc_surface)
+ res_blocks +=
+ fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+
+ /* Display WA #1126: skl,bxt,kbl */
+ if (level >= 1 && level <= 7) {
+ if (wp->y_tiled) {
+ res_blocks +=
+ fixed16_to_u32_round_up(wp
-

Post by Paulo Zanoni
y_tile_minimum);

+ res_lines += wp-

Post by Paulo Zanoni
y_min_scanlines;

+ } else {
+ res_blocks++;
+ }
- /* Display WA #1126: skl,bxt,kbl,glk */
- if (level >= 1 && level <= 7) {
- if (wp->y_tiled) {
- res_blocks += fixed16_to_u32_round_up(
- wp-

Post by Paulo Zanoni
y_tile_minimum);

- res_lines += wp->y_min_scanlines;
- } else {
- res_blocks++;
+ /*
+ * Make sure result blocks for higher
latency levels are
+ * atleast as high as level below the
current level.
+ * Assumption in DDB algorithm
optimization for special
+ * cases. Also covers Display WA #1125
for
RC.
+ */
+ if (result_prev->plane_res_b >
res_blocks)
+ res_blocks = result_prev-

Post by Paulo Zanoni
plane_res_b;

}
-
- /*
- * Make sure result blocks for higher latency
levels are atleast
- * as high as level below the current level.
- * Assumption in DDB algorithm optimization
for
special cases.
- * Also covers Display WA #1125 for RC.
- */
- if (result_prev->plane_res_b > res_blocks)
- res_blocks = result_prev->plane_res_b;

This last thing is part of the glk+ watermark formula as well. But
I'm not 100% convinced that it's needed.

I simply can't find where this is documented. WAs 1125 and 1126, which
contain text that match this code exactly, are not applicable to GLK.
Which BSpec page and paragraph/section mentions this?

Post by Ville Syrjälä
One might assume that the the
non-decrasing latency values guarantee that the resulting watermarks
are also non-decreasing. But I've not actually done the math to
see
if
that's true.
Hmm. It might not actually be true on account of the 'memory latency
microseconds >= line time microseconds' check when selecting which
method to use to calculate the watermark. Not quite sure which way
that would make things go.
We also seem to be missing the res_lines handling here. But given
that we only did this for compressed fbs before I don't think this
patch is making things much worse by limiting this to pre-glk only.
The glk+ handling and res_lines fix could be done as a followup.

Post by Paulo Zanoni
}
if (INTEL_GEN(dev_priv) >= 11) {
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Rodrigo Vivi]

Post by Jani Nikula

Post by Rodrigo Vivi

Post by Paulo Zanoni

Post by Rodrigo Vivi

Post by Paulo Zanoni

Post by Ville Syrjälä

Post by Paulo Zanoni
BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.
---
drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++---
----
------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index 67a4d0735291..18157c6ee126 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4696,28 +4696,31 @@ static int skl_compute_plane_wm(const
struct drm_i915_private *dev_priv,
res_lines = div_round_up_fixed16(selected_result,
wp-

Post by Paulo Zanoni
plane_blocks_per_line);

- /* Display WA #1125: skl,bxt,kbl,glk */
- if (level == 0 && wp->rc_surface)
- res_blocks += fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+ if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {

IS_GEN9_BC || IS_BXT
would be a little easier to parse, me thinks.

I can do that, but what I really want is "DISPLAY_GEN(dev_priv) == 9".

work in progress...
btw...
DISPLAY_GEN(dev_priv) == 9 or simply DISPLAY(dev_priv, 9) ?

It should mimic the model we already use: INTEL_GEN(dev_priv) >= 9.

there's a macro defined on gen we end up never using
IS_GEN(9, GEN_FOREVER) with the same effect of INTEL_GEN(dev_priv) >= 9
Should we just kill that or try to use more that instead of direct comparison?
The advantage seems to be the use of bitmasks...

Post by Paulo Zanoni
I would expect a macro called DISPLAY() to return *the* Display or to
simply display somewhere the thing I pass as an argument. Now
DISPLAY_GEN() sounds more like it returns the GEN of the DISPLAY (or
generates a Display).

what about IS_DISPLAY(dev_priv, 9) ?
and IS_DISPLAY_RANGE(dev_priv, 5, 9)

Perhaps IS_DISPLAY_GEN(dev_priv, start, end).
*However* the naming and composition of the macro is *much* less
important than what the code ends up looking.

I fully agree. For this reason I started with the already existent GEN checks.

Post by Jani Nikula
Does the display gen
adequately cover the differences between platforms ultimately?

This thought got me starting many different attempts and then
using git reset --hard HEAD
so many times this week.

No, this is not enough to cover all the needs. Geminilake right
now is the only exception case. We could have another case on internal
right now where we could use same approach to make it better.

But still the ugliest part can be the intersaction and gray areas with
GEM part.

Post by Jani Nikula
For example, a clear counter indication is that you'll be hard pressed
to express the current HAS_GMCH_DISPLAY() in terms of display gen in a
way that doesn't have to check for VLV/CHV.

Yeap, I don't think this display_gen solve this case. Although maybe
the range can help a bit to reduce the gmch_display checks.

Post by Jani Nikula
I don't think you can avoid
IS_GEMINILAKE() either, you'll end up using display gen 10 in some
places but Geminilake in others, ultimately making the end result worse
than the starting point.

Well... I decided to give a try starting for the glk case to see
how it gets.

https://github.com/vivijim/drm-intel/tree/display_gen

Could you please take a look and let me know what do you think?

This version is taking display_gen to extreme, but we could
have a reduced one with glk checks inside bxt and dsi functions.

well, either way one fact is that we already have a code that is mixed with
IS_GEN and platform codename checks. Even if we still have some cases
where platform codename checks is unavoidable maybe it is better if we
prefer gen or display gen over platform codename when possible.

At least it reduces the mixed cases and gets easier to add
new platforms.

Thanks,
Rodrigo.

Post by Jani Nikula
BR,
Jani.

Post by Rodrigo Vivi

Post by Paulo Zanoni

Post by Rodrigo Vivi
#define DISPLAY(dev_priv, g) (!!((dev_priv)->info.display_mask & BIT(g-1)))
#define DISPLAY_RANGE(dev_priv, s, e) \
(!!((dev_priv)->info.display_mask &
INTEL_GEN_MASK((s), (e))))
thoughts? comments? ideas?

#define DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen)

Post by Rodrigo Vivi

Post by Paulo Zanoni
/me looks at Rodrigo

Post by Ville Syrjälä

Post by Paulo Zanoni
+ /* Display WA #1125: skl,bxt,kbl */
+ if (level == 0 && wp->rc_surface)
+ res_blocks +=
+ fixed16_to_u32_round_up(wp-

Post by Paulo Zanoni
y_tile_minimum);

+
+ /* Display WA #1126: skl,bxt,kbl */
+ if (level >= 1 && level <= 7) {
+ if (wp->y_tiled) {
+ res_blocks +=
+ fixed16_to_u32_round_up(wp
-

Post by Paulo Zanoni
y_tile_minimum);

+ res_lines += wp-

Post by Paulo Zanoni
y_min_scanlines;

+ } else {
+ res_blocks++;
+ }
- /* Display WA #1126: skl,bxt,kbl,glk */
- if (level >= 1 && level <= 7) {
- if (wp->y_tiled) {
- res_blocks += fixed16_to_u32_round_up(
- wp-

Post by Paulo Zanoni
y_tile_minimum);

- res_lines += wp->y_min_scanlines;
- } else {
- res_blocks++;
+ /*
+ * Make sure result blocks for higher
latency levels are
+ * atleast as high as level below the
current level.
+ * Assumption in DDB algorithm
optimization for special
+ * cases. Also covers Display WA #1125
for
RC.
+ */
+ if (result_prev->plane_res_b >
res_blocks)
+ res_blocks = result_prev-

Post by Paulo Zanoni
plane_res_b;

}
-
- /*
- * Make sure result blocks for higher latency
levels are atleast
- * as high as level below the current level.
- * Assumption in DDB algorithm optimization
for
special cases.
- * Also covers Display WA #1125 for RC.
- */
- if (result_prev->plane_res_b > res_blocks)
- res_blocks = result_prev->plane_res_b;

This last thing is part of the glk+ watermark formula as well. But
I'm not 100% convinced that it's needed.

I simply can't find where this is documented. WAs 1125 and 1126, which
contain text that match this code exactly, are not applicable to GLK.
Which BSpec page and paragraph/section mentions this?

Post by Ville Syrjälä
One might assume that the the
non-decrasing latency values guarantee that the resulting watermarks
are also non-decreasing. But I've not actually done the math to
see
if
that's true.
Hmm. It might not actually be true on account of the 'memory latency
microseconds >= line time microseconds' check when selecting which
method to use to calculate the watermark. Not quite sure which way
that would make things go.
We also seem to be missing the res_lines handling here. But given
that we only did this for compressed fbs before I don't think this
patch is making things much worse by limiting this to pre-glk only.
The glk+ handling and res_lines fix could be done as a followup.

Post by Paulo Zanoni
}
if (INTEL_GEN(dev_priv) >= 11) {
--
2.14.4
_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Jani Nikula, Intel Open Source Graphics Center

[Paulo Zanoni]

BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.

v2: Change how we check for gen9 display platforms (Ville).

Cc: Matt Roper <***@intel.com>
Reviewed-by: Ville Syrjälä <***@linux.intel.com>
Signed-off-by: Paulo Zanoni <***@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9da8ff263d36..b5623a70c19c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4754,28 +4754,31 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
res_lines = div_round_up_fixed16(selected_result,
wp->plane_blocks_per_line);

- /* Display WA #1125: skl,bxt,kbl,glk */
- if (level == 0 && wp->rc_surface)
- res_blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
+ if (IS_GEN9_BC(dev_priv) || IS_BROXTON(dev_priv)) {
+ /* Display WA #1125: skl,bxt,kbl */
+ if (level == 0 && wp->rc_surface)
+ res_blocks +=
+ fixed16_to_u32_round_up(wp->y_tile_minimum);
+
+ /* Display WA #1126: skl,bxt,kbl */
+ if (level >= 1 && level <= 7) {
+ if (wp->y_tiled) {
+ res_blocks +=
+ fixed16_to_u32_round_up(wp->y_tile_minimum);
+ res_lines += wp->y_min_scanlines;
+ } else {
+ res_blocks++;
+ }

- /* Display WA #1126: skl,bxt,kbl,glk */
- if (level >= 1 && level <= 7) {
- if (wp->y_tiled) {
- res_blocks += fixed16_to_u32_round_up(
- wp->y_tile_minimum);
- res_lines += wp->y_min_scanlines;
- } else {
- res_blocks++;
+ /*
+ * Make sure result blocks for higher latency levels are
+ * atleast as high as level below the current level.
+ * Assumption in DDB algorithm optimization for special
+ * cases. Also covers Display WA #1125 for RC.
+ */
+ if (result_prev->plane_res_b > res_blocks)
+ res_blocks = result_prev->plane_res_b;
}
-
- /*
- * Make sure result blocks for higher latency levels are atleast
- * as high as level below the current level.
- * Assumption in DDB algorithm optimization for special cases.
- * Also covers Display WA #1125 for RC.
- */
- if (result_prev->plane_res_b > res_blocks)
- res_blocks = result_prev->plane_res_b;
}

if (INTEL_GEN(dev_priv) >= 11) {

[Patchwork]

== Series Details ==

Series: More watermarks improvements
URL : https://patchwork.freedesktop.org/series/51086/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5018 -> Patchwork_10532 =

== Summary - SUCCESS ==

No regressions found.

External URL: https://patchwork.freedesktop.org/api/1.0/series/51086/revisions/1/mbox/

== Known issues ==

Here are the changes found in Patchwork_10532 that come from known issues:

=== IGT changes ===

==== Issues hit ====

***@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
fi-byt-clapper: PASS -> FAIL (fdo#103191, fdo#107362)


==== Possible fixes ====

***@gem_cpu_reloc@basic:
fi-kbl-7560u: INCOMPLETE -> PASS

***@gem_exec_suspend@basic-s3:
fi-blb-e6850: INCOMPLETE (fdo#107718) -> PASS

***@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
fi-skl-6700hq: DMESG-WARN (fdo#105998) -> PASS

***@pm_rpm@module-reload:
fi-glk-j4005: DMESG-WARN (fdo#106000) -> PASS


fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (51 -> 45) ==

Missing (6): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-u


== Build changes ==

* Linux: CI_DRM_5018 -> Patchwork_10532

CI_DRM_5018: ae98bc614f3a2f29f3c48ed799d6fd863d200d3e @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4685: 78619fde4008424c472906041edb1d204e014f7c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10532: e0dd3129b095e596afd67cd2e7ae6df5e46ed707 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

e0dd3129b095 drm/i915: pass dev_priv instead of cstate to skl_compute_transition_wm()
f908dda817d4 drm/i915: add pipe_htotal to struct skl_wm_params
28175f55b9c2 drm/i915: make skl_needs_memory_bw_wa() take dev_priv instead of state
559e4b7abf44 drm/i915: reorganize the error message for invalid watermarks
ea0147852049 drm/i915: move ddb_blocks to be a watermark parameter
c22a33c74070 drm/i915: refactor skl_write_plane_wm()
1e5ceb113b99 drm/i915: simplify wm->is_planar assignment
ee3ca377b090 drm/i915: remove useless memset() for watermarks parameters
64810aa4560c drm/i915: fix handling of invisible planes in watermarks code
9a6a1e9b2d2c drm/i915: remove padding from struct skl_wm_level
f1e40c1fcdd8 drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10532/issues.html

[Patchwork]

== Series Details ==

Series: More watermarks improvements
URL : https://patchwork.freedesktop.org/series/51086/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5018_full -> Patchwork_10532_full =

== Summary - WARNING ==

Minor unknown changes coming with Patchwork_10532_full need to be verified
manually.

If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10532_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_10532_full:

=== IGT changes ===

==== Warnings ====

***@perf_pmu@rc6:
shard-kbl: SKIP -> PASS

***@pm_rc6_residency@rc6-accuracy:
shard-snb: SKIP -> PASS


== Known issues ==

Here are the changes found in Patchwork_10532_full that come from known issues:

=== IGT changes ===

==== Issues hit ====

***@gem_workarounds@suspend-resume:
shard-kbl: PASS -> INCOMPLETE (fdo#103665)

***@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
shard-hsw: PASS -> DMESG-WARN (fdo#107956)

***@kms_cursor_crc@cursor-128x128-suspend:
shard-glk: PASS -> FAIL (fdo#103232) +1

***@kms_cursor_crc@cursor-64x64-suspend:
shard-apl: PASS -> FAIL (fdo#103191, fdo#103232)

***@kms_cursor_crc@cursor-size-change:
shard-apl: PASS -> FAIL (fdo#103232)

***@kms_draw_crc@draw-method-xrgb2101010-render-untiled:
shard-skl: PASS -> FAIL (fdo#103184)

***@kms_flip@2x-flip-vs-expired-vblank:
shard-glk: PASS -> FAIL (fdo#105363)

***@kms_flip@plain-flip-ts-check:
shard-skl: NOTRUN -> FAIL (fdo#100368)

***@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
shard-glk: PASS -> FAIL (fdo#103167)

***@kms_plane_alpha_blend@pipe-a-alpha-transparant-fb:
shard-kbl: NOTRUN -> FAIL (fdo#108145)

***@kms_plane_multiple@atomic-pipe-c-tiling-y:
shard-glk: PASS -> FAIL (fdo#103166)
shard-apl: PASS -> FAIL (fdo#103166) +1

***@kms_setmode@basic:
shard-apl: PASS -> FAIL (fdo#99912)

***@kms_sysfs_edid_timing:
shard-kbl: NOTRUN -> FAIL (fdo#100047)

***@perf@blocking:
shard-hsw: PASS -> FAIL (fdo#102252)

***@pm_backlight@fade_with_suspend:
shard-skl: NOTRUN -> FAIL (fdo#107847)

***@pm_rps@reset:
shard-kbl: PASS -> FAIL (fdo#102250)

***@syncobj_wait@wait-all-for-submit-snapshot:
shard-snb: NOTRUN -> INCOMPLETE (fdo#105411)

***@syncobj_wait@wait-for-submit-complex:
shard-skl: NOTRUN -> INCOMPLETE (fdo#108490)


==== Possible fixes ====

***@gem_ppgtt@blt-vs-render-ctx0:
shard-kbl: INCOMPLETE (fdo#103665, fdo#106023) -> PASS

***@kms_busy@extended-pageflip-hang-newfb-render-c:
shard-glk: DMESG-WARN (fdo#107956) -> PASS

***@kms_ccs@pipe-b-crc-sprite-planes-basic:
shard-glk: FAIL (fdo#108145) -> PASS

***@kms_cursor_crc@cursor-64x64-onscreen:
shard-glk: FAIL (fdo#103232) -> PASS +1

***@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy:
shard-glk: FAIL (fdo#104873) -> PASS

***@kms_flip@flip-vs-expired-vblank:
shard-skl: FAIL (fdo#105363) -> PASS

***@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-shrfb-draw-mmap-wc:
shard-glk: FAIL (fdo#103167) -> PASS +4

***@kms_frontbuffer_tracking@fbc-shrfb-scaledprimary:
shard-glk: DMESG-WARN (fdo#105763, fdo#106538) -> PASS +2

***@kms_plane@plane-position-covered-pipe-a-planes:
shard-apl: FAIL (fdo#103166) -> PASS +1

***@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
shard-apl: FAIL (fdo#108145) -> PASS

***@kms_plane_multiple@atomic-pipe-b-tiling-x:
shard-glk: FAIL (fdo#103166) -> PASS

***@pm_rps@reset:
shard-glk: FAIL (fdo#102250) -> PASS


fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102250 https://bugs.freedesktop.org/show_bug.cgi?id=102250
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#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873
fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
fdo#107847 https://bugs.freedesktop.org/show_bug.cgi?id=107847
fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
fdo#108490 https://bugs.freedesktop.org/show_bug.cgi?id=108490
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (6 -> 6) ==

No changes in participating hosts


== Build changes ==

* Linux: CI_DRM_5018 -> Patchwork_10532

CI_DRM_5018: ae98bc614f3a2f29f3c48ed799d6fd863d200d3e @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4685: 78619fde4008424c472906041edb1d204e014f7c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10532: e0dd3129b095e596afd67cd2e7ae6df5e46ed707 @ 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_10532/shards.html

[Patchwork]

== Series Details ==

Series: More watermarks improvements (rev2)
URL : https://patchwork.freedesktop.org/series/51086/
State : failure

== Summary ==

Applying: drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
Applying: drm/i915: remove padding from struct skl_wm_level
Using index info to reconstruct a base tree...
M drivers/gpu/drm/i915/i915_drv.h
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: drm/i915: fix handling of invisible planes in watermarks code
Using index info to reconstruct a base tree...
M drivers/gpu/drm/i915/i915_drv.h
M drivers/gpu/drm/i915/intel_pm.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_pm.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_pm.c
Auto-merging drivers/gpu/drm/i915/i915_drv.h
error: Failed to merge in the changes.
Patch failed at 0003 drm/i915: fix handling of invisible planes in watermarks code
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".