Re: [Intel-gfx] [PATCH 4/6] drm/i915: Clean up pre-skl wm calling convention

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Jun 09, 2021 at 11:56:30AM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Just pass the full atomic state+crtc to the pre-skl watermark
> functions, and clean up the types/variable names around the area.
> 
> Note that having both .compute_pipe_wm() and .compute_intermediate_wm()
> is entirely redundant now. We could unify them to a single vfunc.
> But let's do this one step at a time.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  5 +-
>  drivers/gpu/drm/i915/i915_drv.h              |  6 +-
>  drivers/gpu/drm/i915/intel_pm.c              | 97 ++++++++++----------
>  3 files changed, 58 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 1615501685c9..62221243fca9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7288,12 +7288,13 @@ static int intel_crtc_atomic_check(struct intel_atomic_state *state,
>  	}
>  
>  	if (dev_priv->display.compute_pipe_wm) {
> -		ret = dev_priv->display.compute_pipe_wm(crtc_state);
> +		ret = dev_priv->display.compute_pipe_wm(state, crtc);
>  		if (ret) {
>  			drm_dbg_kms(&dev_priv->drm,
>  				    "Target pipe watermarks are invalid\n");
>  			return ret;
>  		}
> +
>  	}
>  
>  	if (dev_priv->display.compute_intermediate_wm) {
> @@ -7306,7 +7307,7 @@ static int intel_crtc_atomic_check(struct intel_atomic_state *state,
>  		 * old state and the new state.  We can program these
>  		 * immediately.
>  		 */
> -		ret = dev_priv->display.compute_intermediate_wm(crtc_state);
> +		ret = dev_priv->display.compute_intermediate_wm(state, crtc);
>  		if (ret) {
>  			drm_dbg_kms(&dev_priv->drm,
>  				    "No valid intermediate pipe watermarks are possible\n");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 38ff2fb89744..e9cf0eaad7f8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -270,8 +270,10 @@ struct drm_i915_display_funcs {
>  	int (*bw_calc_min_cdclk)(struct intel_atomic_state *state);
>  	int (*get_fifo_size)(struct drm_i915_private *dev_priv,
>  			     enum i9xx_plane_id i9xx_plane);
> -	int (*compute_pipe_wm)(struct intel_crtc_state *crtc_state);
> -	int (*compute_intermediate_wm)(struct intel_crtc_state *crtc_state);
> +	int (*compute_pipe_wm)(struct intel_atomic_state *state,
> +			       struct intel_crtc *crtc);
> +	int (*compute_intermediate_wm)(struct intel_atomic_state *state,
> +				       struct intel_crtc *crtc);
>  	void (*initial_watermarks)(struct intel_atomic_state *state,
>  				   struct intel_crtc *crtc);
>  	void (*atomic_update_watermarks)(struct intel_atomic_state *state,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7ce9537fa2c7..dd682c64daf0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1370,11 +1370,11 @@ static bool g4x_compute_fbc_en(const struct g4x_wm_state *wm_state,
>  	return true;
>  }
>  
> -static int g4x_compute_pipe_wm(struct intel_crtc_state *crtc_state)
> +static int g4x_compute_pipe_wm(struct intel_atomic_state *state,
> +			       struct intel_crtc *crtc)
>  {
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -	struct intel_atomic_state *state =
> -		to_intel_atomic_state(crtc_state->uapi.state);
> +	struct intel_crtc_state *crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
>  	struct g4x_wm_state *wm_state = &crtc_state->wm.g4x.optimal;
>  	int num_active_planes = hweight8(crtc_state->active_planes &
>  					 ~BIT(PLANE_CURSOR));
> @@ -1451,20 +1451,21 @@ static int g4x_compute_pipe_wm(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> -static int g4x_compute_intermediate_wm(struct intel_crtc_state *new_crtc_state)
> +static int g4x_compute_intermediate_wm(struct intel_atomic_state *state,
> +				       struct intel_crtc *crtc)
>  {
> -	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_crtc_state *new_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +	const struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(state, crtc);
>  	struct g4x_wm_state *intermediate = &new_crtc_state->wm.g4x.intermediate;
>  	const struct g4x_wm_state *optimal = &new_crtc_state->wm.g4x.optimal;
> -	struct intel_atomic_state *intel_state =
> -		to_intel_atomic_state(new_crtc_state->uapi.state);
> -	const struct intel_crtc_state *old_crtc_state =
> -		intel_atomic_get_old_crtc_state(intel_state, crtc);
>  	const struct g4x_wm_state *active = &old_crtc_state->wm.g4x.optimal;
>  	enum plane_id plane_id;
>  
> -	if (!new_crtc_state->hw.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi)) {
> +	if (!new_crtc_state->hw.active ||
> +	    drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi)) {
>  		*intermediate = *optimal;
>  
>  		intermediate->cxsr = false;
> @@ -1890,12 +1891,12 @@ static bool vlv_raw_crtc_wm_is_valid(const struct intel_crtc_state *crtc_state,
>  		vlv_raw_plane_wm_is_valid(crtc_state, PLANE_CURSOR, level);
>  }
>  
> -static int vlv_compute_pipe_wm(struct intel_crtc_state *crtc_state)
> +static int vlv_compute_pipe_wm(struct intel_atomic_state *state,
> +			       struct intel_crtc *crtc)
>  {
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct intel_atomic_state *state =
> -		to_intel_atomic_state(crtc_state->uapi.state);
> +	struct intel_crtc_state *crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
>  	struct vlv_wm_state *wm_state = &crtc_state->wm.vlv.optimal;
>  	const struct vlv_fifo_state *fifo_state =
>  		&crtc_state->wm.vlv.fifo_state;
> @@ -2095,19 +2096,20 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>  
>  #undef VLV_FIFO
>  
> -static int vlv_compute_intermediate_wm(struct intel_crtc_state *new_crtc_state)
> +static int vlv_compute_intermediate_wm(struct intel_atomic_state *state,
> +				       struct intel_crtc *crtc)
>  {
> -	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
> +	struct intel_crtc_state *new_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +	const struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(state, crtc);
>  	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
>  	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
> -	struct intel_atomic_state *intel_state =
> -		to_intel_atomic_state(new_crtc_state->uapi.state);
> -	const struct intel_crtc_state *old_crtc_state =
> -		intel_atomic_get_old_crtc_state(intel_state, crtc);
>  	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
>  	int level;
>  
> -	if (!new_crtc_state->hw.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi)) {
> +	if (!new_crtc_state->hw.active ||
> +	    drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi)) {
>  		*intermediate = *optimal;
>  
>  		intermediate->cxsr = false;
> @@ -3144,10 +3146,12 @@ static bool ilk_validate_pipe_wm(const struct drm_i915_private *dev_priv,
>  }
>  
>  /* Compute new watermarks for the pipe */
> -static int ilk_compute_pipe_wm(struct intel_crtc_state *crtc_state)
> +static int ilk_compute_pipe_wm(struct intel_atomic_state *state,
> +			       struct intel_crtc *crtc)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct intel_crtc_state *crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);

I didn't see much advantage on changing 1 to 2 arguments when you need the
previous one anyway.

But maybe I'm missing some other patch like the possible clean-up
mentioned at the commit message above...

anyway the code looks correct and no blocker here, so up to you:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>  	struct intel_pipe_wm *pipe_wm;
>  	struct intel_plane *plane;
>  	const struct intel_plane_state *plane_state;
> @@ -3220,16 +3224,16 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *crtc_state)
>   * state and the new state.  These can be programmed to the hardware
>   * immediately.
>   */
> -static int ilk_compute_intermediate_wm(struct intel_crtc_state *newstate)
> +static int ilk_compute_intermediate_wm(struct intel_atomic_state *state,
> +				       struct intel_crtc *crtc)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(newstate->uapi.crtc);
> -	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> -	struct intel_pipe_wm *a = &newstate->wm.ilk.intermediate;
> -	struct intel_atomic_state *intel_state =
> -		to_intel_atomic_state(newstate->uapi.state);
> -	const struct intel_crtc_state *oldstate =
> -		intel_atomic_get_old_crtc_state(intel_state, intel_crtc);
> -	const struct intel_pipe_wm *b = &oldstate->wm.ilk.optimal;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_crtc_state *new_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +	const struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(state, crtc);
> +	struct intel_pipe_wm *a = &new_crtc_state->wm.ilk.intermediate;
> +	const struct intel_pipe_wm *b = &old_crtc_state->wm.ilk.optimal;
>  	int level, max_level = ilk_wm_max_level(dev_priv);
>  
>  	/*
> @@ -3237,9 +3241,10 @@ static int ilk_compute_intermediate_wm(struct intel_crtc_state *newstate)
>  	 * currently active watermarks to get values that are safe both before
>  	 * and after the vblank.
>  	 */
> -	*a = newstate->wm.ilk.optimal;
> -	if (!newstate->hw.active || drm_atomic_crtc_needs_modeset(&newstate->uapi) ||
> -	    intel_state->skip_intermediate_wm)
> +	*a = new_crtc_state->wm.ilk.optimal;
> +	if (!new_crtc_state->hw.active ||
> +	    drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi) ||
> +	    state->skip_intermediate_wm)
>  		return 0;
>  
>  	a->pipe_enabled |= b->pipe_enabled;
> @@ -3270,8 +3275,8 @@ static int ilk_compute_intermediate_wm(struct intel_crtc_state *newstate)
>  	 * If our intermediate WM are identical to the final WM, then we can
>  	 * omit the post-vblank programming; only update if it's different.
>  	 */
> -	if (memcmp(a, &newstate->wm.ilk.optimal, sizeof(*a)) != 0)
> -		newstate->wm.need_postvbl_update = true;
> +	if (memcmp(a, &new_crtc_state->wm.ilk.optimal, sizeof(*a)) != 0)
> +		new_crtc_state->wm.need_postvbl_update = true;
>  
>  	return 0;
>  }
> @@ -3283,12 +3288,12 @@ static void ilk_merge_wm_level(struct drm_i915_private *dev_priv,
>  			       int level,
>  			       struct intel_wm_level *ret_wm)
>  {
> -	const struct intel_crtc *intel_crtc;
> +	const struct intel_crtc *crtc;
>  
>  	ret_wm->enable = true;
>  
> -	for_each_intel_crtc(&dev_priv->drm, intel_crtc) {
> -		const struct intel_pipe_wm *active = &intel_crtc->wm.active.ilk;
> +	for_each_intel_crtc(&dev_priv->drm, crtc) {
> +		const struct intel_pipe_wm *active = &crtc->wm.active.ilk;
>  		const struct intel_wm_level *wm = &active->wm[level];
>  
>  		if (!active->pipe_enabled)
> @@ -3388,7 +3393,7 @@ static void ilk_compute_wm_results(struct drm_i915_private *dev_priv,
>  				   enum intel_ddb_partitioning partitioning,
>  				   struct ilk_wm_values *results)
>  {
> -	struct intel_crtc *intel_crtc;
> +	struct intel_crtc *crtc;
>  	int level, wm_lp;
>  
>  	results->enable_fbc_wm = merged->fbc_wm_enabled;
> @@ -3433,9 +3438,9 @@ static void ilk_compute_wm_results(struct drm_i915_private *dev_priv,
>  	}
>  
>  	/* LP0 register values */
> -	for_each_intel_crtc(&dev_priv->drm, intel_crtc) {
> -		enum pipe pipe = intel_crtc->pipe;
> -		const struct intel_pipe_wm *pipe_wm = &intel_crtc->wm.active.ilk;
> +	for_each_intel_crtc(&dev_priv->drm, crtc) {
> +		enum pipe pipe = crtc->pipe;
> +		const struct intel_pipe_wm *pipe_wm = &crtc->wm.active.ilk;
>  		const struct intel_wm_level *r = &pipe_wm->wm[0];
>  
>  		if (drm_WARN_ON(&dev_priv->drm, !r->enable))
> -- 
> 2.31.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx