Re: [Intel-gfx] [PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane

From: Paulo Zanoni
Date: Wed Oct 05 2016 - 16:33:30 EST

Next message: Jitendra Khasdev: "[PATCH] staging: speakup: Replaced obsolete simple_strtoul"
Previous message: MickaÃl SalaÃn: "Re: [RFC v2 00/10] Landlock LSM: Unprivileged sandboxing"
In reply to: Lyude: "[PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane"
Next in thread: Maarten Lankhorst: "Re: [Intel-gfx] [PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Qua, 2016-10-05 Ãs 11:33 -0400, Lyude escreveu:
> Having skl_wm_level contain all of the watermarks for each plane is
> annoying since it prevents us from having any sort of object to
> represent a single watermark level, something we take advantage of in
> the next commit to cut down on all of the copy paste code in here.

I'd like to start my review pointing that I really like this patch. I
agree the current form is annoying.

See below for some details.

>
> Signed-off-by: Lyude <cpaul@xxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Ville SyrjÃlÃ <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> Âdrivers/gpu/drm/i915/i915_drv.hÂÂ|ÂÂÂ6 +-
> Âdrivers/gpu/drm/i915/intel_drv.h |ÂÂÂ6 +-
> Âdrivers/gpu/drm/i915/intel_pm.cÂÂ| 208 +++++++++++++++++----------
> ------------
> Â3 files changed, 100 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index d26e5999..0f97d43 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1648,9 +1648,9 @@ struct skl_wm_values {
> Â};
> Â
> Âstruct skl_wm_level {
> - bool plane_en[I915_MAX_PLANES];
> - uint16_t plane_res_b[I915_MAX_PLANES];
> - uint8_t plane_res_l[I915_MAX_PLANES];
> + bool plane_en;
> + uint16_t plane_res_b;
> + uint8_t plane_res_l;
> Â};
> Â
> Â/*
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 35ba282..d684f4f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -468,9 +468,13 @@ struct intel_pipe_wm {
> Â bool sprites_scaled;
> Â};
> Â
> -struct skl_pipe_wm {
> +struct skl_plane_wm {
> Â struct skl_wm_level wm[8];
> Â struct skl_wm_level trans_wm;
> +};
> +
> +struct skl_pipe_wm {
> + struct skl_plane_wm planes[I915_MAX_PLANES];
> Â uint32_t linetime;
> Â};
> Â
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index af96888..250f12d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3668,67 +3668,52 @@ static int
> Âskl_compute_wm_level(const struct drm_i915_private *dev_priv,
> Â ÂÂÂÂÂstruct skl_ddb_allocation *ddb,
> Â ÂÂÂÂÂstruct intel_crtc_state *cstate,
> + ÂÂÂÂÂstruct intel_plane *intel_plane,
> Â ÂÂÂÂÂint level,
> Â ÂÂÂÂÂstruct skl_wm_level *result)
> Â{
> Â struct drm_atomic_state *state = cstate->base.state;
> Â struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> >base.crtc);
> - struct drm_plane *plane;
> - struct intel_plane *intel_plane;
> - struct intel_plane_state *intel_pstate;
> + struct drm_plane *plane = &intel_plane->base;
> + struct intel_plane_state *intel_pstate = NULL;
> Â uint16_t ddb_blocks;
> Â enum pipe pipe = intel_crtc->pipe;
> Â int ret;
> + int i = skl_wm_plane_id(intel_plane);
> +
> + if (state)
> + intel_pstate =
> + intel_atomic_get_existing_plane_state(state,
> + ÂÂÂÂÂÂintel_
> plane);
> Â
> Â /*
> - Â* We'll only calculate watermarks for planes that are
> actually
> - Â* enabled, so make sure all other planes are set as
> disabled.
> + Â* Note: If we start supporting multiple pending atomic
> commits against
> + Â* the same planes/CRTC's in the future, plane->state will
> no longer be
> + Â* the correct pre-state to use for the calculations here
> and we'll
> + Â* need to change where we get the 'unchanged' plane data
> from.
> + Â*
> + Â* For now this is fine because we only allow one queued
> commit against
> + Â* a CRTC.ÂÂEven if the plane isn't modified by this
> transaction and we
> + Â* don't have a plane lock, we still have the CRTC's lock,
> so we know
> + Â* that no other transactions are racing with us to update
> it.
> Â Â*/
> - memset(result, 0, sizeof(*result));
> -
> - for_each_intel_plane_mask(&dev_priv->drm,
> - ÂÂintel_plane,
> - ÂÂcstate->base.plane_mask) {
> - int i = skl_wm_plane_id(intel_plane);
> -
> - plane = &intel_plane->base;
> - intel_pstate = NULL;
> - if (state)
> - intel_pstate =
> - intel_atomic_get_existing_plane_stat
> e(state,
> - ÂÂÂÂ
> ÂÂintel_plane);
> + if (!intel_pstate)
> + intel_pstate = to_intel_plane_state(plane->state);
> Â
> - /*
> - Â* Note: If we start supporting multiple pending
> atomic commits
> - Â* against the same planes/CRTC's in the future,
> plane->state
> - Â* will no longer be the correct pre-state to use
> for the
> - Â* calculations here and we'll need to change where
> we get the
> - Â* 'unchanged' plane data from.
> - Â*
> - Â* For now this is fine because we only allow one
> queued commit
> - Â* against a CRTC.ÂÂEven if the plane isn't modified
> by this
> - Â* transaction and we don't have a plane lock, we
> still have
> - Â* the CRTC's lock, so we know that no other
> transactions are
> - Â* racing with us to update it.
> - Â*/
> - if (!intel_pstate)
> - intel_pstate = to_intel_plane_state(plane-
> >state);
> + WARN_ON(!intel_pstate->base.fb);
> Â
> - WARN_ON(!intel_pstate->base.fb);
> + ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
> Â
> - ddb_blocks = skl_ddb_entry_size(&ddb-
> >plane[pipe][i]);
> -
> - ret = skl_compute_plane_wm(dev_priv,
> - ÂÂÂcstate,
> - ÂÂÂintel_pstate,
> - ÂÂÂddb_blocks,
> - ÂÂÂlevel,
> - ÂÂÂ&result->plane_res_b[i],
> - ÂÂÂ&result->plane_res_l[i],
> - ÂÂÂ&result->plane_en[i]);
> - if (ret)
> - return ret;
> - }
> + ret = skl_compute_plane_wm(dev_priv,
> + ÂÂÂcstate,
> + ÂÂÂintel_pstate,
> + ÂÂÂddb_blocks,
> + ÂÂÂlevel,
> + ÂÂÂ&result->plane_res_b,
> + ÂÂÂ&result->plane_res_l,
> + ÂÂÂ&result->plane_en);
> + if (ret)
> + return ret;
> Â
> Â return 0;
> Â}
> @@ -3749,19 +3734,11 @@ skl_compute_linetime_wm(struct
> intel_crtc_state *cstate)
> Âstatic void skl_compute_transition_wm(struct intel_crtc_state
> *cstate,
> Â ÂÂÂÂÂÂstruct skl_wm_level *trans_wm
> /* out */)
> Â{
> - struct drm_crtc *crtc = cstate->base.crtc;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_plane *intel_plane;
> -
> Â if (!cstate->base.active)
> Â return;
> Â
> Â /* Until we know more, just disable transition WMs */
> - for_each_intel_plane_on_crtc(crtc->dev, intel_crtc,
> intel_plane) {
> - int i = skl_wm_plane_id(intel_plane);
> -
> - trans_wm->plane_en[i] = false;
> - }
> + trans_wm->plane_en = false;
> Â}
> Â
> Âstatic int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> @@ -3770,19 +3747,33 @@ static int skl_build_pipe_wm(struct
> intel_crtc_state *cstate,
> Â{
> Â struct drm_device *dev = cstate->base.crtc->dev;
> Â const struct drm_i915_private *dev_priv = to_i915(dev);
> + struct intel_plane *intel_plane;
> + struct skl_plane_wm *wm;
> Â int level, max_level = ilk_wm_max_level(dev);
> Â int ret;
> Â
> - for (level = 0; level <= max_level; level++) {
> - ret = skl_compute_wm_level(dev_priv, ddb, cstate,
> - ÂÂÂlevel, &pipe_wm-
> >wm[level]);
> - if (ret)
> - return ret;
> + /*
> + Â* We'll only calculate watermarks for planes that are
> actually
> + Â* enabled, so make sure all other planes are set as
> disabled.
> + Â*/
> + memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes));
> +
> + for_each_intel_plane_mask(&dev_priv->drm,
> + ÂÂintel_plane,
> + ÂÂcstate->base.plane_mask) {
> + wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)];
> +
> + for (level = 0; level <= max_level; level++) {
> + ret = skl_compute_wm_level(dev_priv, ddb,
> cstate,
> + ÂÂÂintel_plane,
> level,
> + ÂÂÂ&wm->wm[level]);
> + if (ret)
> + return ret;
> + }
> + skl_compute_transition_wm(cstate, &wm->trans_wm);
> Â }
> Â pipe_wm->linetime = skl_compute_linetime_wm(cstate);
> Â
> - skl_compute_transition_wm(cstate, &pipe_wm->trans_wm);
> -
> Â return 0;
> Â}
> Â
> @@ -3792,50 +3783,56 @@ static void skl_compute_wm_results(struct
> drm_device *dev,
> Â ÂÂÂstruct intel_crtc *intel_crtc)
> Â{
> Â int level, max_level = ilk_wm_max_level(dev);
> + struct skl_plane_wm *plane_wm;
> Â enum pipe pipe = intel_crtc->pipe;
> Â uint32_t temp;
> Â int i;
> Â
> - for (level = 0; level <= max_level; level++) {
> - for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> + for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> + plane_wm = &p_wm->planes[i];
> +
> + for (level = 0; level <= max_level; level++) {
> Â temp = 0;
> Â
> - temp |= p_wm->wm[level].plane_res_l[i] <<
> + temp |= plane_wm->wm[level].plane_res_l <<
> Â PLANE_WM_LINES_SHIFT;
> - temp |= p_wm->wm[level].plane_res_b[i];
> - if (p_wm->wm[level].plane_en[i])
> + temp |= plane_wm->wm[level].plane_res_b;
> + if (plane_wm->wm[level].plane_en)
> Â temp |= PLANE_WM_EN;
> Â
> Â r->plane[pipe][i][level] = temp;
> Â }
> Â
> - temp = 0;
> -
> - temp |= p_wm->wm[level].plane_res_l[PLANE_CURSOR] <<
> PLANE_WM_LINES_SHIFT;
> - temp |= p_wm->wm[level].plane_res_b[PLANE_CURSOR];
> + }
> Â
> - if (p_wm->wm[level].plane_en[PLANE_CURSOR])
> + for (level = 0; level <= max_level; level++) {
> + plane_wm = &p_wm->planes[PLANE_CURSOR];
> + temp = 0;
> + temp |= plane_wm->wm[level].plane_res_l <<
> PLANE_WM_LINES_SHIFT;
> + temp |= plane_wm->wm[level].plane_res_b;
> + if (plane_wm->wm[level].plane_en)
> Â temp |= PLANE_WM_EN;
> Â
> Â r->plane[pipe][PLANE_CURSOR][level] = temp;
> -
> Â }
> Â
> Â /* transition WMs */
> Â for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> + plane_wm = &p_wm->planes[i];
> Â temp = 0;
> - temp |= p_wm->trans_wm.plane_res_l[i] <<
> PLANE_WM_LINES_SHIFT;
> - temp |= p_wm->trans_wm.plane_res_b[i];
> - if (p_wm->trans_wm.plane_en[i])
> + temp |= plane_wm->trans_wm.plane_res_l <<
> PLANE_WM_LINES_SHIFT;
> + temp |= plane_wm->trans_wm.plane_res_b;
> + if (plane_wm->trans_wm.plane_en)
> Â temp |= PLANE_WM_EN;
> Â
> Â r->plane_trans[pipe][i] = temp;
> Â }
> Â
> + plane_wm = &p_wm->planes[PLANE_CURSOR];
> Â temp = 0;
> - temp |= p_wm->trans_wm.plane_res_l[PLANE_CURSOR] <<
> PLANE_WM_LINES_SHIFT;
> - temp |= p_wm->trans_wm.plane_res_b[PLANE_CURSOR];
> - if (p_wm->trans_wm.plane_en[PLANE_CURSOR])
> + temp |= plane_wm->trans_wm.plane_res_l <<
> PLANE_WM_LINES_SHIFT;
> + temp |= plane_wm->trans_wm.plane_res_b;
> + if (plane_wm->trans_wm.plane_en)
> Â temp |= PLANE_WM_EN;
> Â
> Â r->plane_trans[pipe][PLANE_CURSOR] = temp;
> @@ -4262,44 +4259,24 @@ static void ilk_optimize_watermarks(struct
> intel_crtc_state *cstate)
> Âstatic void skl_pipe_wm_active_state(uint32_t val,
> Â ÂÂÂÂÂstruct skl_pipe_wm *active,
> Â ÂÂÂÂÂbool is_transwm,
> - ÂÂÂÂÂbool is_cursor,
> Â ÂÂÂÂÂint i,
> Â ÂÂÂÂÂint level)
> Â{
> + struct skl_plane_wm *plane_wm = &active->planes[i];
> Â bool is_enabled = (val & PLANE_WM_EN) != 0;
> Â
> Â if (!is_transwm) {
> - if (!is_cursor) {
> - active->wm[level].plane_en[i] = is_enabled;
> - active->wm[level].plane_res_b[i] =
> - val & PLANE_WM_BLOCKS_MASK;
> - active->wm[level].plane_res_l[i] =
> - (val >>
> PLANE_WM_LINES_SHIFT) &
> - PLANE_WM_LINES_MASK;
> - } else {
> - active->wm[level].plane_en[PLANE_CURSOR] =
> is_enabled;
> - active->wm[level].plane_res_b[PLANE_CURSOR]
> =
> - val & PLANE_WM_BLOCKS_MASK;
> - active->wm[level].plane_res_l[PLANE_CURSOR]
> =
> - (val >>
> PLANE_WM_LINES_SHIFT) &
> - PLANE_WM_LINES_MASK;
> - }
> + plane_wm->wm[level].plane_en = is_enabled;
> + plane_wm->wm[level].plane_res_b = val &
> PLANE_WM_BLOCKS_MASK;
> + plane_wm->wm[level].plane_res_l =
> + (val >> PLANE_WM_LINES_SHIFT) &
> + PLANE_WM_LINES_MASK;

Nitpick: you can join the two lines above and still stay under 80
columns.

> Â } else {
> - if (!is_cursor) {
> - active->trans_wm.plane_en[i] = is_enabled;
> - active->trans_wm.plane_res_b[i] =
> - val & PLANE_WM_BLOCKS_MASK;
> - active->trans_wm.plane_res_l[i] =
> - (val >>
> PLANE_WM_LINES_SHIFT) &
> - PLANE_WM_LINES_MASK;
> - } else {
> - active->trans_wm.plane_en[PLANE_CURSOR] =
> is_enabled;
> - active->trans_wm.plane_res_b[PLANE_CURSOR] =
> - val & PLANE_WM_BLOCKS_MASK;
> - active->trans_wm.plane_res_l[PLANE_CURSOR] =
> - (val >>
> PLANE_WM_LINES_SHIFT) &
> - PLANE_WM_LINES_MASK;
> - }
> + plane_wm->trans_wm.plane_en = is_enabled;
> + plane_wm->trans_wm.plane_res_b = val &
> PLANE_WM_BLOCKS_MASK;
> + plane_wm->trans_wm.plane_res_l =
> + (val >> PLANE_WM_LINES_SHIFT) &
> + PLANE_WM_LINES_MASK;

Same here.

> Â }
> Â}
> Â
> @@ -4338,20 +4315,19 @@ static void skl_pipe_wm_get_hw_state(struct
> drm_crtc *crtc)
> Â for (level = 0; level <= max_level; level++) {
> Â for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> Â temp = hw->plane[pipe][i][level];
> - skl_pipe_wm_active_state(temp, active,
> false,
> - false, i, level);
> + skl_pipe_wm_active_state(temp, active,
> false, i, level);
> Â }
> Â temp = hw->plane[pipe][PLANE_CURSOR][level];
> - skl_pipe_wm_active_state(temp, active, false, true,
> i, level);
> + skl_pipe_wm_active_state(temp, active, false, i,
> level);

While this is not wrong today, history shows that the number of planes
increases over time, so we may at some point in the future add PLANE_D
and more, so the code will become wrong. Just pass PLANE_CURSOR instead
of "i" here and below. Also, this simplification could have been a
separate patch.

Everything else looks correct, so if you fix the detail above I'll
provide a r-b tag.

> Â }
> Â
> Â for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> Â temp = hw->plane_trans[pipe][i];
> - skl_pipe_wm_active_state(temp, active, true, false,
> i, 0);
> + skl_pipe_wm_active_state(temp, active, true, i, 0);
> Â }
> Â
> Â temp = hw->plane_trans[pipe][PLANE_CURSOR];
> - skl_pipe_wm_active_state(temp, active, true, true, i, 0);
> + skl_pipe_wm_active_state(temp, active, true, i, 0);
> Â
> Â intel_crtc->wm.active.skl = *active;
> Â}

Next message: Jitendra Khasdev: "[PATCH] staging: speakup: Replaced obsolete simple_strtoul"
Previous message: MickaÃl SalaÃn: "Re: [RFC v2 00/10] Landlock LSM: Unprivileged sandboxing"
In reply to: Lyude: "[PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane"
Next in thread: Maarten Lankhorst: "Re: [Intel-gfx] [PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]