Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu: > Now that we've make skl_wm_levels make a little more sense, we can > remove all of the redundant wm information. Up until now we'd been > storing two copies of all of the skl watermarks: one being the > skl_pipe_wm structs, the other being the global wm struct in > drm_i915_private containing the raw register values. This is > confusing > and problematic, since it means we're prone to accidentally letting > the > two copies go out of sync. So, get rid of all of the functions > responsible for computing the register values and just use a single > helper, skl_write_wm_level(), to convert and write the new watermarks > on > the fly. I like the direction of this patch too, but there are some small possible problems. See below. > > 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 | 2 - > drivers/gpu/drm/i915/intel_display.c | 14 ++- > drivers/gpu/drm/i915/intel_drv.h | 6 +- > drivers/gpu/drm/i915/intel_pm.c | 203 ++++++++++++------------- > ---------- > drivers/gpu/drm/i915/intel_sprite.c | 8 +- > 5 files changed, 88 insertions(+), 145 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index 0f97d43..63519ac 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1643,8 +1643,6 @@ struct skl_ddb_allocation { > struct skl_wm_values { > unsigned dirty_pipes; > struct skl_ddb_allocation ddb; > - uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; > - uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES]; > }; > > struct skl_wm_level { > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index dd15ae2..c580d3d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct > drm_plane *plane, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state- > >base.crtc); > struct drm_framebuffer *fb = plane_state->base.fb; > const struct skl_wm_values *wm = &dev_priv->wm.skl_results; > + const struct skl_plane_wm *p_wm = > + &crtc_state->wm.skl.optimal.planes[0]; I wish someone would do a patch to convert all these hardcoded values to PLANE_X, and convert "int"s to "enum plane"s everywhere. > int pipe = intel_crtc->pipe; > u32 plane_ctl; > unsigned int rotation = plane_state->base.rotation; > @@ -3414,7 +3416,7 @@ static void skylake_update_primary_plane(struct > drm_plane *plane, > intel_crtc->adjusted_y = src_y; > > if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base)) > - skl_write_plane_wm(intel_crtc, wm, 0); > + skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0); > > I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); > I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x); > @@ -3448,6 +3450,8 @@ static void > skylake_disable_primary_plane(struct drm_plane *primary, > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc- > >state); > + const struct skl_plane_wm *p_wm = &cstate- > >wm.skl.optimal.planes[0]; > int pipe = intel_crtc->pipe; > > /* > @@ -3455,7 +3459,8 @@ static void > skylake_disable_primary_plane(struct drm_plane *primary, > * plane's visiblity isn't actually changing neither is its > watermarks. > */ > if (!crtc->primary->state->visible) > - skl_write_plane_wm(intel_crtc, &dev_priv- > >wm.skl_results, 0); > + skl_write_plane_wm(intel_crtc, p_wm, > + &dev_priv->wm.skl_results.ddb, > 0); > > I915_WRITE(PLANE_CTL(pipe, 0), 0); > I915_WRITE(PLANE_SURF(pipe, 0), 0); > @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct > drm_crtc *crtc, u32 base, > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc- > >state); > const struct skl_wm_values *wm = &dev_priv->wm.skl_results; > + const struct skl_plane_wm *p_wm = > + &cstate->wm.skl.optimal.planes[PLANE_CURSOR]; > int pipe = intel_crtc->pipe; > uint32_t cntl = 0; > > if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & > drm_crtc_mask(crtc)) > - skl_write_cursor_wm(intel_crtc, wm); > + skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb); > > if (plane_state && plane_state->base.visible) { > cntl = MCURSOR_GAMMA_ENABLE; > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index d684f4f..958dc72 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const struct > skl_ddb_allocation *old, > bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state, > struct intel_crtc *intel_crtc); > void skl_write_cursor_wm(struct intel_crtc *intel_crtc, > - const struct skl_wm_values *wm); > + const struct skl_plane_wm *wm, > + const struct skl_ddb_allocation *ddb); > void skl_write_plane_wm(struct intel_crtc *intel_crtc, > - const struct skl_wm_values *wm, > + const struct skl_plane_wm *wm, > + const struct skl_ddb_allocation *ddb, > int plane); > uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state > *pipe_config); > bool ilk_disable_lp_wm(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index 250f12d..7708646 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(struct > drm_atomic_state *state) > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_atomic_state *intel_state = > to_intel_atomic_state(state); > struct drm_crtc *crtc; > + struct intel_crtc_state *cstate; > + struct skl_plane_wm *wm; > enum pipe pipe; > int level, plane; > > @@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct > drm_atomic_state *state) > /* Since we're now guaranteed to only have one active > CRTC... */ > pipe = ffs(intel_state->active_crtcs) - 1; > crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + cstate = intel_atomic_get_crtc_state(state, > to_intel_crtc(crtc)); > > if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE) > return false; > > for_each_plane(dev_priv, pipe, plane) { > + wm = &cstate->wm.skl.optimal.planes[plane]; > + > /* Skip this plane if it's not enabled */ > - if (intel_state->wm_results.plane[pipe][plane][0] == > 0) > + if (!wm->wm[0].plane_en) > continue; > > /* Find the highest enabled wm level for this plane > */ > - for (level = ilk_wm_max_level(dev); > - intel_state- > >wm_results.plane[pipe][plane][level] == 0; --level) > + for (level = ilk_wm_max_level(dev); !wm- > >wm[level].plane_en; > + --level) > { } > > /* > @@ -3777,67 +3782,6 @@ static int skl_build_pipe_wm(struct > intel_crtc_state *cstate, > return 0; > } > > -static void skl_compute_wm_results(struct drm_device *dev, > - struct skl_pipe_wm *p_wm, > - struct skl_wm_values *r, > - 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 (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 |= 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][i][level] = temp; > - } > - > - } > - > - 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 |= 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 |= 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; > -} > - > static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, > i915_reg_t reg, > const struct skl_ddb_entry *entry) > @@ -3848,8 +3792,22 @@ static void skl_ddb_entry_write(struct > drm_i915_private *dev_priv, > I915_WRITE(reg, 0); > } > > +static void skl_write_wm_level(struct drm_i915_private *dev_priv, > + i915_reg_t reg, > + const struct skl_wm_level *level) > +{ > + uint32_t val = 0; > + > + val |= level->plane_res_b; > + val |= level->plane_res_l << PLANE_WM_LINES_SHIFT; > + val |= level->plane_en; The line above seems wrong, you should check for plane_en and then set PLANE_WM_EN. IMHO it would even better if we completely zeroed the register in case plane_en is false, so we could have: uint32_t val = 0; if (level->plane_en) { val |= PLANE_WM_EN; val |= level->plane_res_b; val |= level->plane_res_l << PLANE_WM_LINES_SHIFT; } > + > + I915_WRITE(reg, val); > +} > + > void skl_write_plane_wm(struct intel_crtc *intel_crtc, > - const struct skl_wm_values *wm, > + const struct skl_plane_wm *wm, > + const struct skl_ddb_allocation *ddb, > int plane) > { > struct drm_crtc *crtc = &intel_crtc->base; > @@ -3859,19 +3817,21 @@ void skl_write_plane_wm(struct intel_crtc > *intel_crtc, > enum pipe pipe = intel_crtc->pipe; > > for (level = 0; level <= max_level; level++) { > - I915_WRITE(PLANE_WM(pipe, plane, level), > - wm->plane[pipe][plane][level]); > + skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane, > level), > + &wm->wm[level]); > } > - I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm- > >plane_trans[pipe][plane]); > + skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane), > + &wm->trans_wm); > > skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane), > - &wm->ddb.plane[pipe][plane]); > + &ddb->plane[pipe][plane]); > skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, > plane), > - &wm->ddb.y_plane[pipe][plane]); > + &ddb->y_plane[pipe][plane]); > } > > void skl_write_cursor_wm(struct intel_crtc *intel_crtc, > - const struct skl_wm_values *wm) > + const struct skl_plane_wm *wm, > + const struct skl_ddb_allocation *ddb) > { > struct drm_crtc *crtc = &intel_crtc->base; > struct drm_device *dev = crtc->dev; > @@ -3880,13 +3840,13 @@ void skl_write_cursor_wm(struct intel_crtc > *intel_crtc, > enum pipe pipe = intel_crtc->pipe; > > for (level = 0; level <= max_level; level++) { > - I915_WRITE(CUR_WM(pipe, level), > - wm->plane[pipe][PLANE_CURSOR][level]); > + skl_write_wm_level(dev_priv, CUR_WM(pipe, level), > + &wm->wm[level]); > } > - I915_WRITE(CUR_WM_TRANS(pipe), wm- > >plane_trans[pipe][PLANE_CURSOR]); > + skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm- > >trans_wm); > > skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe), > - &wm->ddb.plane[pipe][PLANE_CURSOR]); > + &ddb->plane[pipe][PLANE_CURSOR]); > } > > static inline bool skl_ddb_entries_overlap(const struct > skl_ddb_entry *a, > @@ -4064,11 +4024,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values > *dst, > struct skl_wm_values *src, > enum pipe pipe) > { > - memcpy(dst->plane[pipe], src->plane[pipe], > - sizeof(dst->plane[pipe])); > - memcpy(dst->plane_trans[pipe], src->plane_trans[pipe], > - sizeof(dst->plane_trans[pipe])); > - > memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe], > sizeof(dst->ddb.y_plane[pipe])); > memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe], > @@ -4117,7 +4072,6 @@ skl_compute_wm(struct drm_atomic_state *state) > * no suitable watermark values can be found. > */ > for_each_crtc_in_state(state, crtc, cstate, i) { > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_crtc_state *intel_cstate = > to_intel_crtc_state(cstate); > > @@ -4135,7 +4089,6 @@ skl_compute_wm(struct drm_atomic_state *state) > continue; > > intel_cstate->update_wm_pre = true; > - skl_compute_wm_results(crtc->dev, pipe_wm, results, > intel_crtc); > } > > return 0; > @@ -4169,9 +4122,11 @@ static void skl_update_wm(struct drm_crtc > *crtc) > int plane; > > for (plane = 0; plane < > intel_num_planes(intel_crtc); plane++) > - skl_write_plane_wm(intel_crtc, results, > plane); > + skl_write_plane_wm(intel_crtc, &pipe_wm- > >planes[plane], > + &results->ddb, plane); > > - skl_write_cursor_wm(intel_crtc, results); > + skl_write_cursor_wm(intel_crtc, &pipe_wm- > >planes[PLANE_CURSOR], > + &results->ddb); > } > > skl_copy_wm_for_pipe(hw_vals, results, pipe); > @@ -4256,28 +4211,13 @@ static void ilk_optimize_watermarks(struct > intel_crtc_state *cstate) > mutex_unlock(&dev_priv->wm.wm_mutex); > } > > -static void skl_pipe_wm_active_state(uint32_t val, > - struct skl_pipe_wm *active, > - bool is_transwm, > - int i, > - int level) > +static inline void skl_wm_level_from_reg_val(uint32_t val, > + struct skl_wm_level > *level) > { > - struct skl_plane_wm *plane_wm = &active->planes[i]; > - bool is_enabled = (val & PLANE_WM_EN) != 0; > - > - if (!is_transwm) { > - 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 { > - 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; > - } > + level->plane_en = val & PLANE_WM_EN; > + level->plane_res_b = val & PLANE_WM_BLOCKS_MASK; > + level->plane_res_l = (val & PLANE_WM_LINES_MASK) >> > + PLANE_WM_LINES_SHIFT; This also looks wrong, since PLANE_WM_LINES_MASK is 0x1f. We should do like the original code did: shifting before masking. > } > > static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) > @@ -4287,23 +4227,33 @@ static void skl_pipe_wm_get_hw_state(struct > drm_crtc *crtc) > struct skl_wm_values *hw = &dev_priv->wm.skl_hw; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_crtc_state *cstate = to_intel_crtc_state(crtc- > >state); > + struct intel_plane *intel_plane; > struct skl_pipe_wm *active = &cstate->wm.skl.optimal; > + struct skl_plane_wm *wm; > enum pipe pipe = intel_crtc->pipe; > - int level, i, max_level; > - uint32_t temp; > + int level, id, max_level = ilk_wm_max_level(dev); > + uint32_t val; > > - max_level = ilk_wm_max_level(dev); > + for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { > + id = skl_wm_plane_id(intel_plane); > + wm = &cstate->wm.skl.optimal.planes[id]; > > - for (level = 0; level <= max_level; level++) { > - for (i = 0; i < intel_num_planes(intel_crtc); i++) > - hw->plane[pipe][i][level] = > - I915_READ(PLANE_WM(pipe, i, > level)); > - hw->plane[pipe][PLANE_CURSOR][level] = > I915_READ(CUR_WM(pipe, level)); > - } > + for (level = 0; level <= max_level; level++) { > + if (id != PLANE_CURSOR) > + val = I915_READ(PLANE_WM(pipe, id, > level)); > + else > + val = I915_READ(CUR_WM(pipe, > level)); > + > + skl_wm_level_from_reg_val(val, &wm- > >wm[level]); > + } > > - for (i = 0; i < intel_num_planes(intel_crtc); i++) > - hw->plane_trans[pipe][i] = > I915_READ(PLANE_WM_TRANS(pipe, i)); > - hw->plane_trans[pipe][PLANE_CURSOR] = > I915_READ(CUR_WM_TRANS(pipe)); > + if (id != PLANE_CURSOR) > + val = I915_READ(PLANE_WM_TRANS(pipe, id)); > + else > + val = I915_READ(CUR_WM_TRANS(pipe)); > + > + skl_wm_level_from_reg_val(val, &wm->trans_wm); > + } > > if (!intel_crtc->active) > return; > @@ -4311,25 +4261,6 @@ static void skl_pipe_wm_get_hw_state(struct > drm_crtc *crtc) > hw->dirty_pipes |= drm_crtc_mask(crtc); > > active->linetime = I915_READ(PIPE_WM_LINETIME(pipe)); > - > - 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, i, level); > - } > - temp = hw->plane[pipe][PLANE_CURSOR][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, i, 0); > - } > - > - temp = hw->plane_trans[pipe][PLANE_CURSOR]; > - skl_pipe_wm_active_state(temp, active, true, i, 0); > - > - intel_crtc->wm.active.skl = *active; As far as I understood, we should still be setting intel_crtc- >wm.active.skl. Shouldn't we? If not, why? Now, due to the problems above, weren't you getting some WARNs about the hw state not matching what it should? In case not, maybe you could investigate why. > } > > void skl_wm_get_hw_state(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index 73a521f..0fb775b 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -208,6 +208,8 @@ skl_update_plane(struct drm_plane *drm_plane, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > const int pipe = intel_plane->pipe; > const int plane = intel_plane->plane + 1; > + const struct skl_plane_wm *p_wm = > + &crtc_state->wm.skl.optimal.planes[plane]; > u32 plane_ctl; > const struct drm_intel_sprite_colorkey *key = &plane_state- > >ckey; > u32 surf_addr = plane_state->main.offset; > @@ -232,7 +234,7 @@ skl_update_plane(struct drm_plane *drm_plane, > plane_ctl |= skl_plane_ctl_rotation(rotation); > > if (wm->dirty_pipes & drm_crtc_mask(crtc)) > - skl_write_plane_wm(intel_crtc, wm, plane); > + skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, > plane); > > if (key->flags) { > I915_WRITE(PLANE_KEYVAL(pipe, plane), key- > >min_value); > @@ -289,6 +291,7 @@ skl_disable_plane(struct drm_plane *dplane, > struct drm_crtc *crtc) > struct drm_device *dev = dplane->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_plane *intel_plane = to_intel_plane(dplane); > + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc- > >state); > const int pipe = intel_plane->pipe; > const int plane = intel_plane->plane + 1; > > @@ -298,7 +301,8 @@ skl_disable_plane(struct drm_plane *dplane, > struct drm_crtc *crtc) > */ > if (!dplane->state->visible) > skl_write_plane_wm(to_intel_crtc(crtc), > - &dev_priv->wm.skl_results, > plane); > + &cstate- > >wm.skl.optimal.planes[plane], > + &dev_priv->wm.skl_results.ddb, > plane); > > I915_WRITE(PLANE_CTL(pipe, plane), 0); > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel