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

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

[Lisovskiy, Stanislav]

Date: Mon, 14 Feb 2022 19:39:28 +0200
From: "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/6] drm/i915: Unconfuse pre-icl vs. icl+ intel_sagv_{pre,post}_plane_update()
User-Agent: Mutt/1.9.4 (2018-02-28)

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
> 

----- End forwarded message -----