Re: [PATCH 16/16] drm/i915: Handle SKL+ WM/DDB registers next to all other plane registers
On Mon, May 13, 2024 at 11:52:08PM +0300, Jani Nikula wrote:
> On Fri, 10 May 2024, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > 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ä
>
> [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
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[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 = _state->wm.skl.optimal;
> > - const struct skl_ddb_entry *ddb =
> > - _state->wm.skl.plane_ddb[plane_id];
> > - const struct skl_ddb_entry *ddb_y =
> > - _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 = _wm->planes[plane_id];
> > -
>
Re: [PATCH 16/16] drm/i915: Handle SKL+ WM/DDB registers next to all other plane registers
On Fri, 10 May 2024, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 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ä
[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.
Reviewed-by: Jani Nikula
> +
> #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[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 = _state->wm.skl.optimal;
> - const struct skl_ddb_entry *ddb =
> - _state->wm.skl.plane_ddb[plane_id];
> - const struct skl_ddb_entry *ddb_y =
> - _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 = _wm->planes[plane_id];
> -
> - intel_de_write_fw(i915, PLANE_WM_SAGV(pipe, plane_id),
> - skl_plane_wm_reg_val(>sagv.wm0));
> - intel_de_write_fw(i915, PLANE_WM_SAGV_TRANS(pipe, plane_id),
> - skl_plane_wm_reg_val(>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));
> -}
>
[PATCH 16/16] drm/i915: Handle SKL+ WM/DDB registers next to all other plane registers
From: Ville Syrjälä
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ä
---
drivers/gpu/drm/i915/display/intel_cursor.c | 32 +++
.../drm/i915/display/skl_universal_plane.c| 60
.../drm/i915/display/skl_universal_plane.h| 5 +
drivers/gpu/drm/i915/display/skl_watermark.c | 95 +--
drivers/gpu/drm/i915/display/skl_watermark.h | 13 ++-
5 files changed, 107 insertions(+), 98 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
b/drivers/gpu/drm/i915/display/intel_cursor.c
index d2b459634732..3ecab15d1431 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -24,6 +24,7 @@
#include "intel_psr.h"
#include "intel_psr_regs.h"
#include "intel_vblank.h"
+#include "skl_universal_plane.h"
#include "skl_watermark.h"
#include "gem/i915_gem_object.h"
@@ -556,6 +557,37 @@ static void i9xx_cursor_update_sel_fetch_arm(struct
intel_plane *plane,
}
}
+static 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 = _state->wm.skl.optimal;
+ const struct skl_ddb_entry *ddb =
+ _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 = _wm->planes[plane_id];
+
+ intel_de_write_fw(i915, CUR_WM_SAGV(pipe),
+ skl_plane_wm_reg_val(>sagv.wm0));
+ intel_de_write_fw(i915, CUR_WM_SAGV_TRANS(pipe),
+ skl_plane_wm_reg_val(>sagv.trans_wm));
+ }
+
+ intel_de_write_fw(i915, CUR_BUF_CFG(pipe),
+ skl_plane_ddb_reg_val(ddb));
+}
+
/* TODO: split into noarm+arm pair */
static void i9xx_cursor_update_arm(struct intel_plane *plane,
const struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index ab560820bb23..a9914cb31631 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -617,6 +617,66 @@ static u32 skl_plane_stride(const struct intel_plane_state
*plane_state,
return stride / skl_plane_stride_mult(fb, color_plane, rotation);
}
+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);
+}
+
+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;
+}
+
+static 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 = _state->wm.skl.optimal;
+ const struct skl_ddb_entry *ddb =
+ _state->wm.skl.plane_ddb[plane_id];
+ const struct skl_ddb_entry *ddb_y =
+ _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
