Date: Mon, 14 Feb 2022 19:39:28 +0200 From: "Lisovskiy, Stanislav" <[email protected]> To: Ville Syrjala <[email protected]> Cc: [email protected] 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ä <[email protected]> > > 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 <[email protected]> > Signed-off-by: Ville Syrjälä <[email protected]> > --- > 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 <[email protected]> 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 -----
