| Message ID | 1475681598-12081-5-git-send-email-cpaul@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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; > }
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
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_state(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; } 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; } } @@ -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); } 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; }
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. 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(-)