Re: [Intel-gfx] [PATCH 6/6] drm/i915: Pimp icl+ sagv pre/post update

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

On Mon, Feb 14, 2022 at 12:27:30PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 14, 2022 at 12:00:11PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Feb 14, 2022 at 11:18:11AM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Add some debugs on what exactly we're doing to the QGV point mask
> > > in the icl+ sagv pre/post plane update hooks. Currently we're just
> > > guessing.
> > > 
> > > 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 | 37 ++++++++++++++++-----------------
> > >  1 file changed, 18 insertions(+), 19 deletions(-)
> > 
> > Weird I think, I had those debugs initially. Definitely remember
> > there was something similar. Was it kinda removed later?
> 
> Can't immediately see any such debugs being added or removed
> by any commit.

Definitely was there, either during restriction itself or in
intel_bw_atomic_check. Don't remember, if it was excessive debugs
someone was complaining or someother reason, why it was removed.

Stan

> 
> > 
> > Stan
> > 
> > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> >  
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 8b70cdc3b58b..5d1f1a9988bb 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3818,26 +3818,22 @@ static void icl_sagv_pre_plane_update(struct intel_atomic_state *state)
> > >  		intel_atomic_get_old_bw_state(state);
> > >  	const struct intel_bw_state *new_bw_state =
> > >  		intel_atomic_get_new_bw_state(state);
> > > -	u32 new_mask;
> > > +	u32 old_mask, new_mask;
> > >  
> > >  	if (!new_bw_state)
> > >  		return;
> > >  
> > > -	/*
> > > -	 * Nothing to mask
> > > -	 */
> > > -	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
> > > -		return;
> > > -
> > > +	old_mask = old_bw_state->qgv_points_mask;
> > >  	new_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
> > >  
> > > -	/*
> > > -	 * If new mask is zero - means there is nothing to mask,
> > > -	 * we can only unmask, which should be done in unmask.
> > > -	 */
> > > -	if (!new_mask)
> > > +	if (old_mask == new_mask)
> > >  		return;
> > >  
> > > +	WARN_ON(!new_bw_state->base.changed);
> > > +
> > > +	drm_dbg_kms(&dev_priv->drm, "Restricting QGV points: 0x%x -> 0x%x\n",
> > > +		    old_mask, new_mask);
> > > +
> > >  	/*
> > >  	 * Restrict required qgv points before updating the configuration.
> > >  	 * According to BSpec we can't mask and unmask qgv points at the same
> > > @@ -3854,19 +3850,22 @@ static void icl_sagv_post_plane_update(struct intel_atomic_state *state)
> > >  		intel_atomic_get_old_bw_state(state);
> > >  	const struct intel_bw_state *new_bw_state =
> > >  		intel_atomic_get_new_bw_state(state);
> > > -	u32 new_mask = 0;
> > > +	u32 old_mask, new_mask;
> > >  
> > >  	if (!new_bw_state)
> > >  		return;
> > >  
> > > -	/*
> > > -	 * Nothing to unmask
> > > -	 */
> > > -	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
> > > -		return;
> > > -
> > > +	old_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
> > >  	new_mask = new_bw_state->qgv_points_mask;
> > >  
> > > +	if (old_mask == new_mask)
> > > +		return;
> > > +
> > > +	WARN_ON(!new_bw_state->base.changed);
> > > +
> > > +	drm_dbg_kms(&dev_priv->drm, "Relaxing QGV points: 0x%x -> 0x%x\n",
> > > +		    old_mask, new_mask);
> > > +
> > >  	/*
> > >  	 * Allow required qgv points after updating the configuration.
> > >  	 * According to BSpec we can't mask and unmask qgv points at the same
> > > -- 
> > > 2.34.1
> > > 
> 
> -- 
> Ville Syrjälä
> Intel