Re: [PATCH v1.1 1/2] drm/i915: Allow preservation of watermarks, v2.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Mar 02, 2016 at 12:38:06PM +0100, Maarten Lankhorst wrote:
> As Paulo has noted we can help bisectability by separating computing
> watermarks on a noop in 2 separate commits.
> 
> This patch no longer clears the crtc watermark state, but recalculates
> it completely. Regardless whether a level is used the full values for
> each level are calculated. If a level is invalid wm[level].enable is
> unset.
> 
> Changes since v1:
> - Only call ilk_validate_wm_level when level <= usable_level.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

After thinking a bit more on this I think we need to do this
differently.

What should happen is each plane's watermarks are computed entirely
indpendently, then we would collect them up when coming up with the
optimal wms, limiting the resulting set to the max usable level,
leaving all higher levels at 0. This part is what was lost with the
current method, and it can even result in totally invalid watermark
values ending up merged into the final result.

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 61dcac9dea37..4c21b98e5a5c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2300,7 +2300,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  	struct intel_plane_state *pristate = NULL;
>  	struct intel_plane_state *sprstate = NULL;
>  	struct intel_plane_state *curstate = NULL;
> -	int level, max_level = ilk_wm_max_level(dev);
> +	int level, max_level = ilk_wm_max_level(dev), usable_level;
>  	struct ilk_wm_maximums max;
>  
>  	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> @@ -2308,7 +2308,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  		return PTR_ERR(cstate);
>  
>  	pipe_wm = &cstate->wm.optimal.ilk;
> -	memset(pipe_wm, 0, sizeof(*pipe_wm));
>  
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>  		ps = drm_atomic_get_plane_state(state,
> @@ -2330,13 +2329,15 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
>  		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
>  
> +	usable_level = max_level;
> +
>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
>  	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
> -		max_level = 1;
> +		usable_level = 1;
>  
>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
>  	if (pipe_wm->sprites_scaled)
> -		max_level = 0;
> +		usable_level = 0;
>  
>  	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
>  			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
> @@ -2350,20 +2351,22 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  	ilk_compute_wm_reg_maximums(dev, 1, &max);
>  
>  	for (level = 1; level <= max_level; level++) {
> -		struct intel_wm_level wm = {};
> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
>  
>  		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
> -				     pristate, sprstate, curstate, &wm);
> +				     pristate, sprstate, curstate, wm);
>  
>  		/*
>  		 * Disable any watermark level that exceeds the
>  		 * register maximums since such watermarks are
>  		 * always invalid.
>  		 */
> -		if (!ilk_validate_wm_level(level, &max, &wm))
> -			break;
> -
> -		pipe_wm->wm[level] = wm;
> +		if (level > usable_level) {
> +			wm->enable = false;
> +		} else if (!ilk_validate_wm_level(level, &max, wm)) {
> +			wm->enable = false;
> +			usable_level = level;
> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.1.0
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx