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