Re: [PATCH 16/16] drm/i915: Handle SKL+ WM/DDB registers next to all other plane registers

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 10 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> 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@linux.intel.com>

[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 <jani.nikula@intel.com>

> +
>  #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