Re: [Intel-gfx] [PATCH v2 08/13] drm/i915: Clean up skl+ vs. icl+ watermark computation
On Wed, Nov 21, 2018 at 09:05:33PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 20, 2018 at 02:44:34PM -0800, Matt Roper wrote:
> > On Wed, Nov 14, 2018 at 11:07:24PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä
...snip...
> > > +static int icl_build_plane_wm(struct skl_ddb_allocation *ddb,
> > > + struct skl_pipe_wm *pipe_wm,
> > > + struct intel_crtc_state *crtc_state,
> > > + const struct intel_plane_state *plane_state)
> > > +{
> > > + enum plane_id plane_id = to_intel_plane(plane_state->base.plane)->id;
> > > + int ret;
> > > +
> > > + /* Watermarks calculated in master */
> > > + if (plane_state->slave)
> > > + return 0;
> > > +
> > > + if (plane_state->linked_plane) {
> > > + const struct drm_framebuffer *fb = plane_state->base.fb;
> > > + enum plane_id y_plane_id = plane_state->linked_plane->id;
> > > +
> > > + WARN_ON(!fb->format->is_yuv ||
> > > + fb->format->num_planes == 1);
> > > +
> > > + ret = skl_build_plane_wm_single(ddb, crtc_state, plane_state,
> > > + y_plane_id, 0);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = skl_build_plane_wm_single(ddb, crtc_state, plane_state,
> > > + plane_id, 1);
> > > + if (ret)
> > > + return ret;
> > > + } else if (intel_wm_plane_visible(crtc_state, plane_state)) {
> >
> > Isn't a visibility test also relevant to the nv12 (master plane) case
> > above? I don't understand why we'd only test it for rgb planes.
>
> linked_plane!=NULL implies that the plane is visible (see
> icl_check_nv12_planes()). I should probably add another WARN_ON() for
> that.
Ah, okay. In that case, with or without the WARN_ON(),
Reviewed-by: Matt Roper
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 08/13] drm/i915: Clean up skl+ vs. icl+ watermark computation
On Tue, Nov 20, 2018 at 02:44:34PM -0800, Matt Roper wrote:
> On Wed, Nov 14, 2018 at 11:07:24PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > Make a cleaner split between the skl+ and icl+ ways of computing
> > watermarks. This way skl_build_pipe_wm() doesn't have to know any
> > of the gritty details of icl+ master/slave planes.
> >
> > We can also simplify a bunch of the lower level code by pulling
> > the plane visibility checks a bit higher up.
> >
> > Cc: Matt Roper
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 192 +---
> > 1 file changed, 103 insertions(+), 89 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 59c91ec11c60..a743e089ab7d 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4591,9 +4591,6 @@ 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))
> > - return 0;
> > -
> > /* only NV12 format has two planes */
> > if (plane_id == 1 && fb->format->format != DRM_FORMAT_NV12) {
> > DRM_DEBUG_KMS("Non NV12 format have single plane\n");
> > @@ -4707,9 +4704,6 @@ static int skl_compute_plane_wm(const struct
> > drm_i915_private *dev_priv,
> > if (latency == 0)
> > return level == 0 ? -EINVAL : 0;
> >
> > - if (!intel_wm_plane_visible(cstate, intel_pstate))
> > - return 0;
> > -
> > /* Display WA #1141: kbl,cfl */
> > if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
> > IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0)) &&
> > @@ -4832,21 +4826,16 @@ 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,
> > const struct intel_crtc_state *cstate,
> > const struct intel_plane_state *intel_pstate,
> > uint16_t ddb_blocks,
> > const struct skl_wm_params *wm_params,
> > - struct skl_plane_wm *wm,
> > struct skl_wm_level *levels)
> > {
> > int level, max_level = ilk_wm_max_level(dev_priv);
> > struct skl_wm_level *result_prev = [0];
> > int ret;
> >
> > - if (WARN_ON(!intel_pstate->base.fb))
> > - return -EINVAL;
> > -
> > for (level = 0; level <= max_level; level++) {
> > struct skl_wm_level *result = [level];
> >
> > @@ -4864,9 +4853,6 @@ skl_compute_wm_levels(const struct drm_i915_private
> > *dev_priv,
> > result_prev = result;
> > }
> >
> > - if (intel_pstate->base.fb->format->format == DRM_FORMAT_NV12)
> > - wm->is_planar = true;
> > -
> > return 0;
> > }
> >
> > @@ -4904,9 +4890,6 @@ static void skl_compute_transition_wm(const 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)
> > - return;
> > -
> > /* Transition WM are not recommended by HW team for GEN9 */
> > if (INTEL_GEN(dev_priv) <= 9)
> > return;
> > @@ -4955,97 +4938,134 @@ static void skl_compute_transition_wm(const struct
> > intel_crtc_state *cstate,
> > }
> > }
> >
> > -static int __skl_build_plane_wm_single(struct skl_ddb_allocation *ddb,
> > - struct skl_pipe_wm *pipe_wm,
> > - enum plane_id plane_id,
> > - const struct intel_crtc_state *cstate,
> > - const struct intel_plane_state *pstate,
> > - int color_plane)
> > -{
> > - struct drm_i915_private *dev_priv = to_i915(pstate->base.plane->dev);
> > - struct skl_plane_wm *wm = _wm->planes[plane_id];
> > - enum pipe pipe = to_intel_plane(pstate->base.plane)->pipe;
> > - struct skl_wm_params wm_params;
> > - uint16_t ddb_blocks = skl_ddb_entry_size(>plane[pipe][plane_id]);
> > - int ret;
> > -
> > - ret = skl_compute_plane_wm_params(dev_priv, cstate, pstate,
> > - _params, color_plane);
> > - if (ret)
> > - return ret;
> > -
> > - ret = skl_compute_wm_levels(dev_priv, ddb, cstate, pstate,
> > - ddb_blocks, _params, wm, wm->wm);
> > -
> > - if (ret)
> > - return ret;
> > -
> > - skl_compute_transition_wm(cstate, _params, wm, ddb_blocks);
> > -
> > - return 0;
> > -}
> > -
> > static int skl_build_plane_wm_single(struct skl_ddb_allocation *ddb,
> > -
Re: [Intel-gfx] [PATCH v2 08/13] drm/i915: Clean up skl+ vs. icl+ watermark computation
On Wed, Nov 14, 2018 at 11:07:24PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Make a cleaner split between the skl+ and icl+ ways of computing
> watermarks. This way skl_build_pipe_wm() doesn't have to know any
> of the gritty details of icl+ master/slave planes.
>
> We can also simplify a bunch of the lower level code by pulling
> the plane visibility checks a bit higher up.
>
> Cc: Matt Roper
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/intel_pm.c | 192 +---
> 1 file changed, 103 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 59c91ec11c60..a743e089ab7d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4591,9 +4591,6 @@ 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))
> - return 0;
> -
> /* only NV12 format has two planes */
> if (plane_id == 1 && fb->format->format != DRM_FORMAT_NV12) {
> DRM_DEBUG_KMS("Non NV12 format have single plane\n");
> @@ -4707,9 +4704,6 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
> if (latency == 0)
> return level == 0 ? -EINVAL : 0;
>
> - if (!intel_wm_plane_visible(cstate, intel_pstate))
> - return 0;
> -
> /* Display WA #1141: kbl,cfl */
> if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
> IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0)) &&
> @@ -4832,21 +4826,16 @@ 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,
> const struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate,
> uint16_t ddb_blocks,
> const struct skl_wm_params *wm_params,
> - struct skl_plane_wm *wm,
> struct skl_wm_level *levels)
> {
> int level, max_level = ilk_wm_max_level(dev_priv);
> struct skl_wm_level *result_prev = [0];
> int ret;
>
> - if (WARN_ON(!intel_pstate->base.fb))
> - return -EINVAL;
> -
> for (level = 0; level <= max_level; level++) {
> struct skl_wm_level *result = [level];
>
> @@ -4864,9 +4853,6 @@ skl_compute_wm_levels(const struct drm_i915_private
> *dev_priv,
> result_prev = result;
> }
>
> - if (intel_pstate->base.fb->format->format == DRM_FORMAT_NV12)
> - wm->is_planar = true;
> -
> return 0;
> }
>
> @@ -4904,9 +4890,6 @@ static void skl_compute_transition_wm(const 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)
> - return;
> -
> /* Transition WM are not recommended by HW team for GEN9 */
> if (INTEL_GEN(dev_priv) <= 9)
> return;
> @@ -4955,97 +4938,134 @@ static void skl_compute_transition_wm(const struct
> intel_crtc_state *cstate,
> }
> }
>
> -static int __skl_build_plane_wm_single(struct skl_ddb_allocation *ddb,
> -struct skl_pipe_wm *pipe_wm,
> -enum plane_id plane_id,
> -const struct intel_crtc_state *cstate,
> -const struct intel_plane_state *pstate,
> -int color_plane)
> -{
> - struct drm_i915_private *dev_priv = to_i915(pstate->base.plane->dev);
> - struct skl_plane_wm *wm = _wm->planes[plane_id];
> - enum pipe pipe = to_intel_plane(pstate->base.plane)->pipe;
> - struct skl_wm_params wm_params;
> - uint16_t ddb_blocks = skl_ddb_entry_size(>plane[pipe][plane_id]);
> - int ret;
> -
> - ret = skl_compute_plane_wm_params(dev_priv, cstate, pstate,
> - _params, color_plane);
> - if (ret)
> - return ret;
> -
> - ret = skl_compute_wm_levels(dev_priv, ddb, cstate, pstate,
> - ddb_blocks, _params, wm, wm->wm);
> -
> - if (ret)
> - return ret;
> -
> - skl_compute_transition_wm(cstate, _params, wm, ddb_blocks);
> -
> - return 0;
> -}
> -
> static int skl_build_plane_wm_single(struct skl_ddb_allocation *ddb,
> - struct skl_pipe_wm *pipe_wm,
> - const struct intel_crtc_state *cstate,
> - const struct intel_plane_state *pstate)
> +
[Intel-gfx] [PATCH v2 08/13] drm/i915: Clean up skl+ vs. icl+ watermark computation
From: Ville Syrjälä
Make a cleaner split between the skl+ and icl+ ways of computing
watermarks. This way skl_build_pipe_wm() doesn't have to know any
of the gritty details of icl+ master/slave planes.
We can also simplify a bunch of the lower level code by pulling
the plane visibility checks a bit higher up.
Cc: Matt Roper
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/intel_pm.c | 192 +---
1 file changed, 103 insertions(+), 89 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 59c91ec11c60..a743e089ab7d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4591,9 +4591,6 @@ 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))
- return 0;
-
/* only NV12 format has two planes */
if (plane_id == 1 && fb->format->format != DRM_FORMAT_NV12) {
DRM_DEBUG_KMS("Non NV12 format have single plane\n");
@@ -4707,9 +4704,6 @@ static int skl_compute_plane_wm(const struct
drm_i915_private *dev_priv,
if (latency == 0)
return level == 0 ? -EINVAL : 0;
- if (!intel_wm_plane_visible(cstate, intel_pstate))
- return 0;
-
/* Display WA #1141: kbl,cfl */
if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0)) &&
@@ -4832,21 +4826,16 @@ 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,
const struct intel_crtc_state *cstate,
const struct intel_plane_state *intel_pstate,
uint16_t ddb_blocks,
const struct skl_wm_params *wm_params,
- struct skl_plane_wm *wm,
struct skl_wm_level *levels)
{
int level, max_level = ilk_wm_max_level(dev_priv);
struct skl_wm_level *result_prev = [0];
int ret;
- if (WARN_ON(!intel_pstate->base.fb))
- return -EINVAL;
-
for (level = 0; level <= max_level; level++) {
struct skl_wm_level *result = [level];
@@ -4864,9 +4853,6 @@ skl_compute_wm_levels(const struct drm_i915_private
*dev_priv,
result_prev = result;
}
- if (intel_pstate->base.fb->format->format == DRM_FORMAT_NV12)
- wm->is_planar = true;
-
return 0;
}
@@ -4904,9 +4890,6 @@ static void skl_compute_transition_wm(const 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)
- return;
-
/* Transition WM are not recommended by HW team for GEN9 */
if (INTEL_GEN(dev_priv) <= 9)
return;
@@ -4955,97 +4938,134 @@ static void skl_compute_transition_wm(const struct
intel_crtc_state *cstate,
}
}
-static int __skl_build_plane_wm_single(struct skl_ddb_allocation *ddb,
- struct skl_pipe_wm *pipe_wm,
- enum plane_id plane_id,
- const struct intel_crtc_state *cstate,
- const struct intel_plane_state *pstate,
- int color_plane)
-{
- struct drm_i915_private *dev_priv = to_i915(pstate->base.plane->dev);
- struct skl_plane_wm *wm = _wm->planes[plane_id];
- enum pipe pipe = to_intel_plane(pstate->base.plane)->pipe;
- struct skl_wm_params wm_params;
- uint16_t ddb_blocks = skl_ddb_entry_size(>plane[pipe][plane_id]);
- int ret;
-
- ret = skl_compute_plane_wm_params(dev_priv, cstate, pstate,
- _params, color_plane);
- if (ret)
- return ret;
-
- ret = skl_compute_wm_levels(dev_priv, ddb, cstate, pstate,
- ddb_blocks, _params, wm, wm->wm);
-
- if (ret)
- return ret;
-
- skl_compute_transition_wm(cstate, _params, wm, ddb_blocks);
-
- return 0;
-}
-
static int skl_build_plane_wm_single(struct skl_ddb_allocation *ddb,
-struct skl_pipe_wm *pipe_wm,
-const struct intel_crtc_state *cstate,
-const struct intel_plane_state *pstate)
+struct intel_crtc_state *crtc_state,
+const struct intel_plane_state
*plane_state,
+
