Re: [Intel-gfx] [PATCH 4/6] drm/i915: Unconfuse pre-icl vs. icl+ intel_sagv_{pre, post}_plane_update()

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

On Mon, Feb 14, 2022 at 11:18:09AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> intel_sagv_{pre,post}_plane_update() can accidentally forget
> to bail out early on pre-icl and proceed down the icl+ codepath
> at the end of the function. Fortunately it'll bail out before
> it gets too far due to old_qgv_mask==new_qgv_mask==0 so no real
> bug here. But lets make the code less confusing anyway.
> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d8eb553ffad3..068870b17c43 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3806,8 +3806,9 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
>  	if (!new_bw_state)
>  		return;
>  
> -	if (DISPLAY_VER(dev_priv) < 11 && !intel_can_enable_sagv(dev_priv, new_bw_state)) {
> -		intel_disable_sagv(dev_priv);
> +	if (DISPLAY_VER(dev_priv) < 11) {
> +		if (!intel_can_enable_sagv(dev_priv, new_bw_state))
> +			intel_disable_sagv(dev_priv);
>  		return;
>  	}

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

Agree, I think that was my original code as well. However to be honest, don't like
the whole idea of splitting the code flow and bailing our prematurely for gen < 11 here.
Would be nice to have some unified approach, so that we have common main logic for all platforms,
like 
if (intel_bw_state_equals(new, old))
	return

intel_bw_state_apply_restrictions(...)  -> here we would add intel_enable/disable_sagv for gen <11 and
                                           qgv point restrictions for gen >= 11

Stan

>  
> @@ -3857,8 +3858,9 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state)
>  	if (!new_bw_state)
>  		return;
>  
> -	if (DISPLAY_VER(dev_priv) < 11 && intel_can_enable_sagv(dev_priv, new_bw_state)) {
> -		intel_enable_sagv(dev_priv);
> +	if (DISPLAY_VER(dev_priv) < 11) {
> +		if (intel_can_enable_sagv(dev_priv, new_bw_state))
> +			intel_enable_sagv(dev_priv);
>  		return;
>  	}
>  
> -- 
> 2.34.1
>