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

Op 05-10-16 om 22:33 schreef Paulo Zanoni:

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@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com

---

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.

Agreed, but I want to note that PLANE_CURSOR is always supposed to be the last member. Unless you have sprite planes covering the cursor, which doesn't ever happen.

~Maarten