Re: [PATCH 1/3] drm/i915: Compute use_sagv_wm differently

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Tue, Dec 19, 2023 at 03:07:54PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> drm_atomic_check_only() gets upset if we try to add extra crtcs
> to any commit that isn't flagged with DRM_MODE_ATOMIC_ALLOW_MODESET.
> This conflicts with how SAGV watermarks work on pre-ADL as we
> need to manually switch over the SAGV watermarks before we can
> safely enable SAGV.
> 
> So in order to make SAGV usage possible we need to compute each
> pipe's use of SAGV watermarks as if there aren't any other
> active pipes. Ie. if the current pipe isn't the one blocking
> SAGV then we make it use the SAGV watermarks, even if some
> other pipe prevents SAGV from actually being used. Otherwise
> we could end up with a pipes using the normal watermarks (but
> not blocking SAGV), and some other pipe in parallel enabling
> SAGV, which would likely cause underruns.
> 
> The alternative approach of preventing SAGV usage until all
> pipes simultanously end up using SAGV watermarks would only
> really work if userspace always adds all pipes to every
> commits, which isn't the case typically.
> 
> The downside of this is that we will end up using the less
> optimal SAGV watermarks even if some other pipe prevents
> SAGV from actually being enabled. In which case the system
> won't achieve the minimum possible power consumption.

This is quite sad, that we have to do that, basically means
we might end up using SAGV watermarks, which use a bit more 
DBuf(taking into account SAGV related memory latency) just
because we can't sync properly with drm commit architecture
(as I understand the main problem is that crtc addition to the commit).
This is not catastrophical of course, but we are now basically
using the solution which we know for sure, that isn't optimal.

But as me personally and you probably as well, don't really 
have time resource to spend for solving it more efficiently,
lets use this as a solution.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_watermark.c | 38 ++++++++++++--------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 56588d6e24ae..9cee19cbe253 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -443,12 +443,35 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
>  
>  	for_each_new_intel_crtc_in_state(state, crtc,
>  					 new_crtc_state, i) {
> +		struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal;
> +
>  		new_bw_state = intel_atomic_get_bw_state(state);
>  		if (IS_ERR(new_bw_state))
>  			return PTR_ERR(new_bw_state);
>  
>  		old_bw_state = intel_atomic_get_old_bw_state(state);
>  
> +		/*
> +		 * We store use_sagv_wm in the crtc state rather than relying on
> +		 * that bw state since we have no convenient way to get at the
> +		 * latter from the plane commit hooks (especially in the legacy
> +		 * cursor case).
> +		 *
> +		 * drm_atomic_check_only() gets upset if we pull more crtcs
> +		 * into the state, so we have to calculate this based on the
> +		 * individual intel_crtc_can_enable_sagv() rather than
> +		 * the overall intel_crtc_can_enable_sagv(). Otherwise the
> +		 * crtcs not included in the commit would not switch to the
> +		 * SAGV watermarks when we are about to enable SAGV, and that
> +		 * would lead to underruns. This does mean extra power draw
> +		 * when only a subset of the crtcs are blocking SAGV as the
> +		 * other crtcs can't be allowed to use the more optimal
> +		 * normal (ie. non-SAGV) watermarks.
> +		 */
> +		pipe_wm->use_sagv_wm = !HAS_HW_SAGV_WM(i915) &&
> +			DISPLAY_VER(i915) >= 12 &&
> +			intel_crtc_can_enable_sagv(new_crtc_state);
> +
>  		if (intel_crtc_can_enable_sagv(new_crtc_state))
>  			new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
>  		else
> @@ -478,21 +501,6 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
>  			return ret;
>  	}
>  
> -	for_each_new_intel_crtc_in_state(state, crtc,
> -					 new_crtc_state, i) {
> -		struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal;
> -
> -		/*
> -		 * We store use_sagv_wm in the crtc state rather than relying on
> -		 * that bw state since we have no convenient way to get at the
> -		 * latter from the plane commit hooks (especially in the legacy
> -		 * cursor case)
> -		 */
> -		pipe_wm->use_sagv_wm = !HAS_HW_SAGV_WM(i915) &&
> -			DISPLAY_VER(i915) >= 12 &&
> -			intel_can_enable_sagv(i915, new_bw_state);
> -	}
> -
>  	return 0;
>  }
>  
> -- 
> 2.41.0
>