| Message ID | 1475681598-12081-6-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: > 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@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 | 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); >
On Wed, Oct 05, 2016 at 06:44:04PM -0300, Paulo Zanoni wrote: > Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu: > > 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. Note that this is not PLANE_A, but setting up a shorthand local for const struct skl_plane_wm *p_wm = crtc_state->wm.skl.optimal.planes; -Chris
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]; 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; + + 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; } 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; } 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);
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. 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 | 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(-)