Em Sex, 2016-10-07 Ã s 20:11 -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. > > Changes since v1: > - Style nitpicks > - Fix accidental usage of i vs. PLANE_CURSOR > - Split out skl_pipe_wm_active_state simplification into separate > patch > > Signed-off-by: Lyude <cpaul at redhat.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni at 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  | 207 +++++++++++++++++++-------- > ------------ >  3 files changed, 111 insertions(+), 108 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index e9d035ea..0287c93 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1649,9 +1649,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 cc5d5e9..4c2ebcd 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3670,67 +3670,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)); > + if (!intel_pstate) > + intel_pstate = to_intel_plane_state(plane->state); >  > - 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); > + WARN_ON(!intel_pstate->base.fb); >  > - /* > -  * 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); > + ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]); >  > - WARN_ON(!intel_pstate->base.fb); > - > - 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; >  } > @@ -3751,19 +3736,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, > @@ -3772,19 +3749,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; >  } >  > @@ -3794,50 +3785,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; > @@ -4278,35 +4275,37 @@ static void skl_pipe_wm_active_state(uint32_t > val, >  >  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; > + active->planes[i].wm[level].plane_en = > is_enabled; > + active->planes[i].wm[level].plane_res_b = > + val & PLANE_WM_BLOCKS_MASK; > + active->planes[i].wm[level].plane_res_l = > + (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; > + active- > >planes[PLANE_CURSOR].wm[level].plane_en = > + is_enabled; > + active- > >planes[PLANE_CURSOR].wm[level].plane_res_b = > + val & PLANE_WM_BLOCKS_MASK; > + active- > >planes[PLANE_CURSOR].wm[level].plane_res_l = > + (val >> PLANE_WM_LINES_SHIFT) & > + PLANE_WM_LINES_MASK; >  } >  } 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; > + active->planes[i].trans_wm.plane_en = > is_enabled; > + active->planes[i].trans_wm.plane_res_b = > + val & PLANE_WM_BLOCKS_MASK; > + active->planes[i].trans_wm.plane_res_l = > + (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; > + active- > >planes[PLANE_CURSOR].trans_wm.plane_en = > + is_enabled; > + active- > >planes[PLANE_CURSOR].trans_wm.plane_res_b = > + val & PLANE_WM_BLOCKS_MASK; > + active- > >planes[PLANE_CURSOR].trans_wm.plane_res_l = > + (val >> PLANE_WM_LINES_SHIFT) & > + PLANE_WM_LINES_MASK; >  } >  } >  }