On Mon, May 13, 2024 at 11:52:08PM +0300, Jani Nikula wrote:
> On Fri, 10 May 2024, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > Having the plane WM/DDB regitster write functions in skl_watermarks.c
> > is rather annoying when trying to implement DSB based plane updates.
> > Move them into the respective files that handle all other plane
> > register writes. Less places where I need to worry about the DSB
> > vs. MMIO decisions.
> >
> > The downside is that we spread the wm struct details a bit further
> > afield. But if that becomes too annoying we can probably abstract
> > things a bit more with a few extra functions.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> [snip]
>
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.h b/drivers/gpu/drm/i915/display/skl_universal_plane.h
> > index e92e00c01b29..8eb4521ee851 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.h
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.h
> > @@ -12,6 +12,8 @@ struct drm_i915_private;
> > struct intel_crtc;
> > struct intel_initial_plane_config;
> > struct intel_plane_state;
> > +struct skl_ddb_entry;
> > +struct skl_wm_level;
> >
> > enum pipe;
> > enum plane_id;
> > @@ -35,4 +37,7 @@ bool icl_is_nv12_y_plane(struct drm_i915_private *dev_priv,
> > u8 icl_hdr_plane_mask(void);
> > bool icl_is_hdr_plane(struct drm_i915_private *dev_priv, enum plane_id plane_id);
> >
> > +u32 skl_plane_ddb_reg_val(const struct skl_ddb_entry *entry);
> > +u32 skl_plane_wm_reg_val(const struct skl_wm_level *level);
>
> Yeah, I don't much like interfaces that return register values for
> registers that aren't even known... but let's see how this pans out. It
> does what it says on the box.
Yeah, I was mulling over whether to just define the register bits
separately for the cursor registers as well and have its own versions
of these. Might be what I'll end up doing.
I think there are also still some other PSR related plane registers
that are defined in a non-standard way, so those might need similar
treatment as well.
>
> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
Thanks. Pushed the lot.
>
> > +
> > #endif
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index 1daceb8ef9de..2064f72da675 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -1396,7 +1396,7 @@ skl_total_relative_data_rate(const struct intel_crtc_state *crtc_state)
> > return data_rate;
> > }
> >
> > -static const struct skl_wm_level *
> > +const struct skl_wm_level *
> > skl_plane_wm_level(const struct skl_pipe_wm *pipe_wm,
> > enum plane_id plane_id,
> > int level)
> > @@ -1409,7 +1409,7 @@ skl_plane_wm_level(const struct skl_pipe_wm *pipe_wm,
> > return &wm->wm[level];
> > }
> >
> > -static const struct skl_wm_level *
> > +const struct skl_wm_level *
> > skl_plane_trans_wm(const struct skl_pipe_wm *pipe_wm,
> > enum plane_id plane_id)
> > {
> > @@ -2365,97 +2365,6 @@ static int skl_build_pipe_wm(struct intel_atomic_state *state,
> > return skl_wm_check_vblank(crtc_state);
> > }
> >
> > -static u32 skl_plane_ddb_reg_val(const struct skl_ddb_entry *entry)
> > -{
> > - if (!entry->end)
> > - return 0;
> > -
> > - return PLANE_BUF_END(entry->end - 1) |
> > - PLANE_BUF_START(entry->start);
> > -}
> > -
> > -static u32 skl_plane_wm_reg_val(const struct skl_wm_level *level)
> > -{
> > - u32 val = 0;
> > -
> > - if (level->enable)
> > - val |= PLANE_WM_EN;
> > - if (level->ignore_lines)
> > - val |= PLANE_WM_IGNORE_LINES;
> > - val |= REG_FIELD_PREP(PLANE_WM_BLOCKS_MASK, level->blocks);
> > - val |= REG_FIELD_PREP(PLANE_WM_LINES_MASK, level->lines);
> > -
> > - return val;
> > -}
> > -
> > -void skl_write_plane_wm(struct intel_plane *plane,
> > - const struct intel_crtc_state *crtc_state)
> > -{
> > - struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > - enum plane_id plane_id = plane->id;
> > - enum pipe pipe = plane->pipe;
> > - const struct skl_pipe_wm *pipe_wm = &crtc_state->wm.skl.optimal;
> > - const struct skl_ddb_entry *ddb =
> > - &crtc_state->wm.skl.plane_ddb[plane_id];
> > - const struct skl_ddb_entry *ddb_y =
> > - &crtc_state->wm.skl.plane_ddb_y[plane_id];
> > - int level;
> > -
> > - for (level = 0; level < i915->display.wm.num_levels; level++)
> > - intel_de_write_fw(i915, PLANE_WM(pipe, plane_id, level),
> > - skl_plane_wm_reg_val(skl_plane_wm_level(pipe_wm, plane_id, level)));
> > -
> > - intel_de_write_fw(i915, PLANE_WM_TRANS(pipe, plane_id),
> > - skl_plane_wm_reg_val(skl_plane_trans_wm(pipe_wm, plane_id)));
> > -
> > - if (HAS_HW_SAGV_WM(i915)) {
> > - const struct skl_plane_wm *wm = &pipe_wm->planes[plane_id];
> > -
> > - intel_de_write_fw(i915, PLANE_WM_SAGV(pipe, plane_id),
> > - skl_plane_wm_reg_val(&wm->sagv.wm0));
> > - intel_de_write_fw(i915, PLANE_WM_SAGV_TRANS(pipe, plane_id),
> > - skl_plane_wm_reg_val(&wm->sagv.trans_wm));
> > - }
> > -
> > - intel_de_write_fw(i915, PLANE_BUF_CFG(pipe, plane_id),
> > - skl_plane_ddb_reg_val(ddb));
> > -
> > - if (DISPLAY_VER(i915) < 11)
> > - intel_de_write_fw(i915, PLANE_NV12_BUF_CFG(pipe, plane_id),
> > - skl_plane_ddb_reg_val(ddb_y));
> > -}
> > -
> > -void skl_write_cursor_wm(struct intel_plane *plane,
> > - const struct intel_crtc_state *crtc_state)
> > -{
> > - struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > - enum plane_id plane_id = plane->id;
> > - enum pipe pipe = plane->pipe;
> > - const struct skl_pipe_wm *pipe_wm = &crtc_state->wm.skl.optimal;
> > - const struct skl_ddb_entry *ddb =
> > - &crtc_state->wm.skl.plane_ddb[plane_id];
> > - int level;
> > -
> > - for (level = 0; level < i915->display.wm.num_levels; level++)
> > - intel_de_write_fw(i915, CUR_WM(pipe, level),
> > - skl_plane_wm_reg_val(skl_plane_wm_level(pipe_wm, plane_id, level)));
> > -
> > - intel_de_write_fw(i915, CUR_WM_TRANS(pipe),
> > - skl_plane_wm_reg_val(skl_plane_trans_wm(pipe_wm, plane_id)));
> > -
> > - if (HAS_HW_SAGV_WM(i915)) {
> > - const struct skl_plane_wm *wm = &pipe_wm->planes[plane_id];
> > -
> > - intel_de_write_fw(i915, CUR_WM_SAGV(pipe),
> > - skl_plane_wm_reg_val(&wm->sagv.wm0));
> > - intel_de_write_fw(i915, CUR_WM_SAGV_TRANS(pipe),
> > - skl_plane_wm_reg_val(&wm->sagv.trans_wm));
> > - }
> > -
> > - intel_de_write_fw(i915, CUR_BUF_CFG(pipe),
> > - skl_plane_ddb_reg_val(ddb));
> > -}
> > -
> > static bool skl_wm_level_equals(const struct skl_wm_level *l1,
> > const struct skl_wm_level *l2)
> > {
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h
> > index 91f92c0e706e..78b121941237 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.h
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.h
> > @@ -18,6 +18,8 @@ struct intel_bw_state;
> > struct intel_crtc;
> > struct intel_crtc_state;
> > struct intel_plane;
> > +struct skl_pipe_wm;
> > +struct skl_wm_level;
> >
> > u8 intel_enabled_dbuf_slices_mask(struct drm_i915_private *i915);
> >
> > @@ -30,11 +32,6 @@ bool intel_has_sagv(struct drm_i915_private *i915);
> > u32 skl_ddb_dbuf_slice_mask(struct drm_i915_private *i915,
> > const struct skl_ddb_entry *entry);
> >
> > -void skl_write_plane_wm(struct intel_plane *plane,
> > - const struct intel_crtc_state *crtc_state);
> > -void skl_write_cursor_wm(struct intel_plane *plane,
> > - const struct intel_crtc_state *crtc_state);
> > -
> > bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb,
> > const struct skl_ddb_entry *entries,
> > int num_entries, int ignore_idx);
> > @@ -51,6 +48,12 @@ unsigned int skl_watermark_max_latency(struct drm_i915_private *i915,
> > int initial_wm_level);
> > void skl_wm_init(struct drm_i915_private *i915);
> >
> > +const struct skl_wm_level *skl_plane_wm_level(const struct skl_pipe_wm *pipe_wm,
> > + enum plane_id plane_id,
> > + int level);
> > +const struct skl_wm_level *skl_plane_trans_wm(const struct skl_pipe_wm *pipe_wm,
> > + enum plane_id plane_id);
> > +
> > struct intel_dbuf_state {
> > struct intel_global_state base;
>
> --
> Jani Nikula, Intel
--
Ville Syrjälä
Intel
