Re: [PATCH 2/3] drm/i915: Extract code required to calculate max qgv/psf gv point

Re: [PATCH 2/3] drm/i915: Extract code required to calculate max qgv/psf gv point

[Ville Syrjälä]

On Tue, Nov 28, 2023 at 10:37:53AM +0200, Stanislav Lisovskiy wrote:
> We need that in order to force disable SAGV in next patch.
> Also it is beneficial to separate that code, as in majority cases,
> when SAGV is enabled, we don't even need those calculations.
> Also we probably need to determine max PSF GV point as well, however
> currently we don't do that when we disable SAGV, which might be
> actually causing some issues in that case.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 82 ++++++++++++++++++++-----
>  1 file changed, 65 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 583cd2ebdf89..efd408e96e8a 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -805,6 +805,64 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
>  	return to_intel_bw_state(bw_state);
>  }
>  
> +static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
> +					 int num_active_planes)
> +{
> +	unsigned int max_bw_point = 0;
> +	unsigned int max_bw = 0;
> +	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
> +	int i;
> +
> +	for (i = 0; i < num_qgv_points; i++) {
> +		unsigned int idx;
> +		unsigned int max_data_rate;
> +
> +		if (DISPLAY_VER(i915) > 11)
> +			idx = tgl_max_bw_index(i915, num_active_planes, i);
> +		else
> +			idx = icl_max_bw_index(i915, num_active_planes, i);
> +
> +		if (idx >= ARRAY_SIZE(i915->display.bw.max))
> +			continue;
> +
> +		max_data_rate = i915->display.bw.max[idx].deratedbw[i];

Looks like that that part could be extracted to a helper
to be used by both codepaths (would be a natural counterpart
to adl_psf_bw()).

> +
> +		/*
> +		 * We need to know which qgv point gives us
> +		 * maximum bandwidth in order to disable SAGV
> +		 * if we find that we exceed SAGV block time
> +		 * with watermarks. By that moment we already
> +		 * have those, as it is calculated earlier in
> +		 * intel_atomic_check,
> +		 */
> +		if (max_data_rate > max_bw) {
> +			max_bw_point = i;
> +			max_bw = max_data_rate;
> +		}
> +	}
> +
> +	return max_bw_point;
> +}
> +
> +unsigned int icl_max_bw_psf_gv_point(struct drm_i915_private *i915)
> +{
> +	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
> +	unsigned int max_bw = 0;
> +	unsigned int max_bw_point = 0;
> +	int i;
> +
> +	for (i = 0; i < num_psf_gv_points; i++) {
> +		unsigned int max_data_rate = adl_psf_bw(i915, i);
> +
> +		if (max_data_rate > max_bw) {
> +			max_bw_point = i;
> +			max_bw = max_data_rate;
> +		}
> +	}
> +
> +	return max_bw_point;
> +}
> +
>  static int mtl_find_qgv_points(struct drm_i915_private *i915,
>  			       unsigned int data_rate,
>  			       unsigned int num_active_planes,
> @@ -882,8 +940,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
>  			       const struct intel_bw_state *old_bw_state,
>  			       struct intel_bw_state *new_bw_state)
>  {
> -	unsigned int max_bw_point = 0;
> -	unsigned int max_bw = 0;
>  	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
>  	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
>  	u16 psf_points = 0;
> @@ -909,18 +965,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
>  
>  		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
>  
> -		/*
> -		 * We need to know which qgv point gives us
> -		 * maximum bandwidth in order to disable SAGV
> -		 * if we find that we exceed SAGV block time
> -		 * with watermarks. By that moment we already
> -		 * have those, as it is calculated earlier in
> -		 * intel_atomic_check,
> -		 */
> -		if (max_data_rate > max_bw) {
> -			max_bw_point = i;
> -			max_bw = max_data_rate;
> -		}
>  		if (max_data_rate >= data_rate)
>  			qgv_points |= BIT(i);
>  
> @@ -964,9 +1008,13 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
>  	 * cause.
>  	 */
>  	if (!intel_can_enable_sagv(i915, new_bw_state)) {
> -		qgv_points = BIT(max_bw_point);
> -		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
> -			    max_bw_point);
> +		unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, num_active_planes);
> +		unsigned int max_bw_psf_gv_point = icl_max_bw_psf_gv_point(i915);
> +
> +		qgv_points = BIT(max_bw_qgv_point);
> +		psf_points = BIT(max_bw_psf_gv_point);

We didn't restrict the PSF here previously.

> +		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d PSF GV point %d\n",
> +			    max_bw_qgv_point, max_bw_psf_gv_point);
>  	}
>  
>  	/*
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/3] drm/i915: Extract code required to calculate max qgv/psf gv point

[Lisovskiy, Stanislav]

On Fri, Jan 12, 2024 at 07:35:24PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 28, 2023 at 10:37:53AM +0200, Stanislav Lisovskiy wrote:
> > We need that in order to force disable SAGV in next patch.
> > Also it is beneficial to separate that code, as in majority cases,
> > when SAGV is enabled, we don't even need those calculations.
> > Also we probably need to determine max PSF GV point as well, however
> > currently we don't do that when we disable SAGV, which might be
> > actually causing some issues in that case.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.c | 82 ++++++++++++++++++++-----
> >  1 file changed, 65 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index 583cd2ebdf89..efd408e96e8a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -805,6 +805,64 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
> >  	return to_intel_bw_state(bw_state);
> >  }
> >  
> > +static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
> > +					 int num_active_planes)
> > +{
> > +	unsigned int max_bw_point = 0;
> > +	unsigned int max_bw = 0;
> > +	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
> > +	int i;
> > +
> > +	for (i = 0; i < num_qgv_points; i++) {
> > +		unsigned int idx;
> > +		unsigned int max_data_rate;
> > +
> > +		if (DISPLAY_VER(i915) > 11)
> > +			idx = tgl_max_bw_index(i915, num_active_planes, i);
> > +		else
> > +			idx = icl_max_bw_index(i915, num_active_planes, i);
> > +
> > +		if (idx >= ARRAY_SIZE(i915->display.bw.max))
> > +			continue;
> > +
> > +		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
> 
> Looks like that that part could be extracted to a helper
> to be used by both codepaths (would be a natural counterpart
> to adl_psf_bw()).
> 
> > +
> > +		/*
> > +		 * We need to know which qgv point gives us
> > +		 * maximum bandwidth in order to disable SAGV
> > +		 * if we find that we exceed SAGV block time
> > +		 * with watermarks. By that moment we already
> > +		 * have those, as it is calculated earlier in
> > +		 * intel_atomic_check,
> > +		 */
> > +		if (max_data_rate > max_bw) {
> > +			max_bw_point = i;
> > +			max_bw = max_data_rate;
> > +		}
> > +	}
> > +
> > +	return max_bw_point;
> > +}
> > +
> > +unsigned int icl_max_bw_psf_gv_point(struct drm_i915_private *i915)
> > +{
> > +	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
> > +	unsigned int max_bw = 0;
> > +	unsigned int max_bw_point = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < num_psf_gv_points; i++) {
> > +		unsigned int max_data_rate = adl_psf_bw(i915, i);
> > +
> > +		if (max_data_rate > max_bw) {
> > +			max_bw_point = i;
> > +			max_bw = max_data_rate;
> > +		}
> > +	}
> > +
> > +	return max_bw_point;
> > +}
> > +
> >  static int mtl_find_qgv_points(struct drm_i915_private *i915,
> >  			       unsigned int data_rate,
> >  			       unsigned int num_active_planes,
> > @@ -882,8 +940,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
> >  			       const struct intel_bw_state *old_bw_state,
> >  			       struct intel_bw_state *new_bw_state)
> >  {
> > -	unsigned int max_bw_point = 0;
> > -	unsigned int max_bw = 0;
> >  	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
> >  	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
> >  	u16 psf_points = 0;
> > @@ -909,18 +965,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
> >  
> >  		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
> >  
> > -		/*
> > -		 * We need to know which qgv point gives us
> > -		 * maximum bandwidth in order to disable SAGV
> > -		 * if we find that we exceed SAGV block time
> > -		 * with watermarks. By that moment we already
> > -		 * have those, as it is calculated earlier in
> > -		 * intel_atomic_check,
> > -		 */
> > -		if (max_data_rate > max_bw) {
> > -			max_bw_point = i;
> > -			max_bw = max_data_rate;
> > -		}
> >  		if (max_data_rate >= data_rate)
> >  			qgv_points |= BIT(i);
> >  
> > @@ -964,9 +1008,13 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
> >  	 * cause.
> >  	 */
> >  	if (!intel_can_enable_sagv(i915, new_bw_state)) {
> > -		qgv_points = BIT(max_bw_point);
> > -		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
> > -			    max_bw_point);
> > +		unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, num_active_planes);
> > +		unsigned int max_bw_psf_gv_point = icl_max_bw_psf_gv_point(i915);
> > +
> > +		qgv_points = BIT(max_bw_qgv_point);
> > +		psf_points = BIT(max_bw_psf_gv_point);
> 
> We didn't restrict the PSF here previously.

Yep, but I really suspect we should. BSpec states that we should restrict all the GV points
except highest one, also that some PSF GV points aren't same or usable, depending on BW reqs.
So I would restrict that as well, in case if SAGV is off, just to be on safe side.

Stan

> 
> > +		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d PSF GV point %d\n",
> > +			    max_bw_qgv_point, max_bw_psf_gv_point);
> >  	}
> >  
> >  	/*
> > -- 
> > 2.37.3
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 2/3] drm/i915: Extract code required to calculate max qgv/psf gv point

[Ville Syrjälä]

On Wed, Jan 17, 2024 at 12:12:18PM +0200, Lisovskiy, Stanislav wrote:
> On Fri, Jan 12, 2024 at 07:35:24PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 28, 2023 at 10:37:53AM +0200, Stanislav Lisovskiy wrote:
> > > We need that in order to force disable SAGV in next patch.
> > > Also it is beneficial to separate that code, as in majority cases,
> > > when SAGV is enabled, we don't even need those calculations.
> > > Also we probably need to determine max PSF GV point as well, however
> > > currently we don't do that when we disable SAGV, which might be
> > > actually causing some issues in that case.
> > > 
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_bw.c | 82 ++++++++++++++++++++-----
> > >  1 file changed, 65 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > > index 583cd2ebdf89..efd408e96e8a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > > @@ -805,6 +805,64 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
> > >  	return to_intel_bw_state(bw_state);
> > >  }
> > >  
> > > +static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
> > > +					 int num_active_planes)
> > > +{
> > > +	unsigned int max_bw_point = 0;
> > > +	unsigned int max_bw = 0;
> > > +	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < num_qgv_points; i++) {
> > > +		unsigned int idx;
> > > +		unsigned int max_data_rate;
> > > +
> > > +		if (DISPLAY_VER(i915) > 11)
> > > +			idx = tgl_max_bw_index(i915, num_active_planes, i);
> > > +		else
> > > +			idx = icl_max_bw_index(i915, num_active_planes, i);
> > > +
> > > +		if (idx >= ARRAY_SIZE(i915->display.bw.max))
> > > +			continue;
> > > +
> > > +		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
> > 
> > Looks like that that part could be extracted to a helper
> > to be used by both codepaths (would be a natural counterpart
> > to adl_psf_bw()).
> > 
> > > +
> > > +		/*
> > > +		 * We need to know which qgv point gives us
> > > +		 * maximum bandwidth in order to disable SAGV
> > > +		 * if we find that we exceed SAGV block time
> > > +		 * with watermarks. By that moment we already
> > > +		 * have those, as it is calculated earlier in
> > > +		 * intel_atomic_check,
> > > +		 */
> > > +		if (max_data_rate > max_bw) {
> > > +			max_bw_point = i;
> > > +			max_bw = max_data_rate;
> > > +		}
> > > +	}
> > > +
> > > +	return max_bw_point;
> > > +}
> > > +
> > > +unsigned int icl_max_bw_psf_gv_point(struct drm_i915_private *i915)
> > > +{
> > > +	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
> > > +	unsigned int max_bw = 0;
> > > +	unsigned int max_bw_point = 0;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < num_psf_gv_points; i++) {
> > > +		unsigned int max_data_rate = adl_psf_bw(i915, i);
> > > +
> > > +		if (max_data_rate > max_bw) {
> > > +			max_bw_point = i;
> > > +			max_bw = max_data_rate;
> > > +		}
> > > +	}
> > > +
> > > +	return max_bw_point;
> > > +}
> > > +
> > >  static int mtl_find_qgv_points(struct drm_i915_private *i915,
> > >  			       unsigned int data_rate,
> > >  			       unsigned int num_active_planes,
> > > @@ -882,8 +940,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
> > >  			       const struct intel_bw_state *old_bw_state,
> > >  			       struct intel_bw_state *new_bw_state)
> > >  {
> > > -	unsigned int max_bw_point = 0;
> > > -	unsigned int max_bw = 0;
> > >  	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
> > >  	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
> > >  	u16 psf_points = 0;
> > > @@ -909,18 +965,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
> > >  
> > >  		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
> > >  
> > > -		/*
> > > -		 * We need to know which qgv point gives us
> > > -		 * maximum bandwidth in order to disable SAGV
> > > -		 * if we find that we exceed SAGV block time
> > > -		 * with watermarks. By that moment we already
> > > -		 * have those, as it is calculated earlier in
> > > -		 * intel_atomic_check,
> > > -		 */
> > > -		if (max_data_rate > max_bw) {
> > > -			max_bw_point = i;
> > > -			max_bw = max_data_rate;
> > > -		}
> > >  		if (max_data_rate >= data_rate)
> > >  			qgv_points |= BIT(i);
> > >  
> > > @@ -964,9 +1008,13 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
> > >  	 * cause.
> > >  	 */
> > >  	if (!intel_can_enable_sagv(i915, new_bw_state)) {
> > > -		qgv_points = BIT(max_bw_point);
> > > -		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
> > > -			    max_bw_point);
> > > +		unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, num_active_planes);
> > > +		unsigned int max_bw_psf_gv_point = icl_max_bw_psf_gv_point(i915);
> > > +
> > > +		qgv_points = BIT(max_bw_qgv_point);
> > > +		psf_points = BIT(max_bw_psf_gv_point);
> > 
> > We didn't restrict the PSF here previously.
> 
> Yep, but I really suspect we should. BSpec states that we should restrict all the GV points
> except highest one, also that some PSF GV points aren't same or usable, depending on BW reqs.
> So I would restrict that as well, in case if SAGV is off, just to be on safe side.

Pretty sure it's explicitly noted that PSF doesn't cause issues with
latency and hence doesn't need this.

In any case, a change like this has no business being in a patch
that's just supposed to refactor code.

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/3] drm/i915: Extract code required to calculate max qgv/psf gv point

[Lisovskiy, Stanislav]

On Wed, Jan 17, 2024 at 01:12:49PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 17, 2024 at 12:12:18PM +0200, Lisovskiy, Stanislav wrote:
> > On Fri, Jan 12, 2024 at 07:35:24PM +0200, Ville Syrjälä wrote:
> > > On Tue, Nov 28, 2023 at 10:37:53AM +0200, Stanislav Lisovskiy wrote:
> > > > We need that in order to force disable SAGV in next patch.
> > > > Also it is beneficial to separate that code, as in majority cases,
> > > > when SAGV is enabled, we don't even need those calculations.
> > > > Also we probably need to determine max PSF GV point as well, however
> > > > currently we don't do that when we disable SAGV, which might be
> > > > actually causing some issues in that case.
> > > > 
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_bw.c | 82 ++++++++++++++++++++-----
> > > >  1 file changed, 65 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > > > index 583cd2ebdf89..efd408e96e8a 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > > > @@ -805,6 +805,64 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
> > > >  	return to_intel_bw_state(bw_state);
> > > >  }
> > > >  
> > > > +static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
> > > > +					 int num_active_planes)
> > > > +{
> > > > +	unsigned int max_bw_point = 0;
> > > > +	unsigned int max_bw = 0;
> > > > +	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < num_qgv_points; i++) {
> > > > +		unsigned int idx;
> > > > +		unsigned int max_data_rate;
> > > > +
> > > > +		if (DISPLAY_VER(i915) > 11)
> > > > +			idx = tgl_max_bw_index(i915, num_active_planes, i);
> > > > +		else
> > > > +			idx = icl_max_bw_index(i915, num_active_planes, i);
> > > > +
> > > > +		if (idx >= ARRAY_SIZE(i915->display.bw.max))
> > > > +			continue;
> > > > +
> > > > +		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
> > > 
> > > Looks like that that part could be extracted to a helper
> > > to be used by both codepaths (would be a natural counterpart
> > > to adl_psf_bw()).
> > > 
> > > > +
> > > > +		/*
> > > > +		 * We need to know which qgv point gives us
> > > > +		 * maximum bandwidth in order to disable SAGV
> > > > +		 * if we find that we exceed SAGV block time
> > > > +		 * with watermarks. By that moment we already
> > > > +		 * have those, as it is calculated earlier in
> > > > +		 * intel_atomic_check,
> > > > +		 */
> > > > +		if (max_data_rate > max_bw) {
> > > > +			max_bw_point = i;
> > > > +			max_bw = max_data_rate;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return max_bw_point;
> > > > +}
> > > > +
> > > > +unsigned int icl_max_bw_psf_gv_point(struct drm_i915_private *i915)
> > > > +{
> > > > +	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
> > > > +	unsigned int max_bw = 0;
> > > > +	unsigned int max_bw_point = 0;
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < num_psf_gv_points; i++) {
> > > > +		unsigned int max_data_rate = adl_psf_bw(i915, i);
> > > > +
> > > > +		if (max_data_rate > max_bw) {
> > > > +			max_bw_point = i;
> > > > +			max_bw = max_data_rate;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return max_bw_point;
> > > > +}
> > > > +
> > > >  static int mtl_find_qgv_points(struct drm_i915_private *i915,
> > > >  			       unsigned int data_rate,
> > > >  			       unsigned int num_active_planes,
> > > > @@ -882,8 +940,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
> > > >  			       const struct intel_bw_state *old_bw_state,
> > > >  			       struct intel_bw_state *new_bw_state)
> > > >  {
> > > > -	unsigned int max_bw_point = 0;
> > > > -	unsigned int max_bw = 0;
> > > >  	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
> > > >  	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
> > > >  	u16 psf_points = 0;
> > > > @@ -909,18 +965,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
> > > >  
> > > >  		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
> > > >  
> > > > -		/*
> > > > -		 * We need to know which qgv point gives us
> > > > -		 * maximum bandwidth in order to disable SAGV
> > > > -		 * if we find that we exceed SAGV block time
> > > > -		 * with watermarks. By that moment we already
> > > > -		 * have those, as it is calculated earlier in
> > > > -		 * intel_atomic_check,
> > > > -		 */
> > > > -		if (max_data_rate > max_bw) {
> > > > -			max_bw_point = i;
> > > > -			max_bw = max_data_rate;
> > > > -		}
> > > >  		if (max_data_rate >= data_rate)
> > > >  			qgv_points |= BIT(i);
> > > >  
> > > > @@ -964,9 +1008,13 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
> > > >  	 * cause.
> > > >  	 */
> > > >  	if (!intel_can_enable_sagv(i915, new_bw_state)) {
> > > > -		qgv_points = BIT(max_bw_point);
> > > > -		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
> > > > -			    max_bw_point);
> > > > +		unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, num_active_planes);
> > > > +		unsigned int max_bw_psf_gv_point = icl_max_bw_psf_gv_point(i915);
> > > > +
> > > > +		qgv_points = BIT(max_bw_qgv_point);
> > > > +		psf_points = BIT(max_bw_psf_gv_point);
> > > 
> > > We didn't restrict the PSF here previously.
> > 
> > Yep, but I really suspect we should. BSpec states that we should restrict all the GV points
> > except highest one, also that some PSF GV points aren't same or usable, depending on BW reqs.
> > So I would restrict that as well, in case if SAGV is off, just to be on safe side.
> 
> Pretty sure it's explicitly noted that PSF doesn't cause issues with
> latency and hence doesn't need this.
> 
> In any case, a change like this has no business being in a patch
> that's just supposed to refactor code.

Ok, lets drop this, until clarified.

Stan

> 
> -- 
> Ville Syrjälä
> Intel

[PATCH 0/3] QGV/SAGV related fixes

[Stanislav Lisovskiy]

We have couple of customer issues, related to SAGV/QGV point
calculation. Those patches contain fixes plus some additional
debugs for those issues.

Stanislav Lisovskiy (3):
  drm/i915: Add meaningful traces for QGV point info error handling
  drm/i915: Extract code required to calculate max qgv/psf gv point
  drm/i915: Disable SAGV on bw init, to force QGV point recalculation

 drivers/gpu/drm/i915/display/intel_bw.c | 109 +++++++++++++++++-------
 drivers/gpu/drm/i915/display/intel_bw.h |   2 +
 drivers/gpu/drm/i915/soc/intel_dram.c   |   2 +
 3 files changed, 84 insertions(+), 29 deletions(-)

-- 
2.37.3

[PATCH 1/3] drm/i915: Add meaningful traces for QGV point info error handling

[Stanislav Lisovskiy]

For debug purposes we need those - error path won't flood the log,
however there has been already numerous cases, when due to lack
of debugs, we couldn't immediately tell what was the problem on
customer machine, which slowed down the investigation, requiring
to get access to target device and adding those traces manually.

v2: - Make the debug more generic and move it to intel_dram_detect
      (Gustavo Sousa)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 4 +++-
 drivers/gpu/drm/i915/soc/intel_dram.c   | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index 7f2a50b4f494..77886cc21211 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -290,8 +290,10 @@ static int icl_get_qgv_points(struct drm_i915_private *dev_priv,
 		struct intel_qgv_point *sp = &qi->points[i];
 
 		ret = intel_read_qgv_point_info(dev_priv, sp, i);
-		if (ret)
+		if (ret) {
+			drm_dbg_kms(&dev_priv->drm, "Could not read QGV %d info\n", i);
 			return ret;
+		}
 
 		drm_dbg_kms(&dev_priv->drm,
 			    "QGV %d: DCLK=%d tRP=%d tRDPRE=%d tRAS=%d tRCD=%d tRC=%d\n",
diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c b/drivers/gpu/drm/i915/soc/intel_dram.c
index 15492b69f698..e957be5bfb35 100644
--- a/drivers/gpu/drm/i915/soc/intel_dram.c
+++ b/drivers/gpu/drm/i915/soc/intel_dram.c
@@ -681,6 +681,8 @@ void intel_dram_detect(struct drm_i915_private *i915)
 	if (ret)
 		return;
 
+	drm_dbg_kms(&i915->drm, "Num qgv points %d\n", dram_info->num_qgv_points);
+
 	drm_dbg_kms(&i915->drm, "DRAM channels: %u\n", dram_info->num_channels);
 
 	drm_dbg_kms(&i915->drm, "Watermark level 0 adjustment needed: %s\n",
-- 
2.37.3

[PATCH 2/3] drm/i915: Extract code required to calculate max qgv/psf gv point

[Stanislav Lisovskiy]

We need that in order to force disable SAGV in next patch.
Also it is beneficial to separate that code, as in majority cases,
when SAGV is enabled, we don't even need those calculations.
Also we probably need to determine max PSF GV point as well, however
currently we don't do that when we disable SAGV, which might be
actually causing some issues in that case.

v2: - Introduce helper adl_qgv_bw(counterpart to adl_psf_bw)
      (Ville Syrjälä)
    - Don't restrict psf gv points for SAGV disable case
      (Ville Syrjälä)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 81 ++++++++++++++++---------
 1 file changed, 53 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index 77886cc21211..7baa1c13eccd 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -652,15 +652,31 @@ static unsigned int tgl_max_bw_index(struct drm_i915_private *dev_priv,
 	return 0;
 }
 
-static unsigned int adl_psf_bw(struct drm_i915_private *dev_priv,
+static unsigned int adl_psf_bw(struct drm_i915_private *i915,
 			       int psf_gv_point)
 {
 	const struct intel_bw_info *bi =
-			&dev_priv->display.bw.max[0];
+			&i915->display.bw.max[0];
 
 	return bi->psf_bw[psf_gv_point];
 }
 
+static unsigned int adl_qgv_bw(struct drm_i915_private *i915,
+			       int qgv_point, int num_active_planes)
+{
+	unsigned int idx;
+
+	if (DISPLAY_VER(i915) > 11)
+		idx = tgl_max_bw_index(i915, num_active_planes, qgv_point);
+	else
+		idx = icl_max_bw_index(i915, num_active_planes, qgv_point);
+
+	if (idx >= ARRAY_SIZE(i915->display.bw.max))
+		return 0;
+
+	return i915->display.bw.max[idx].deratedbw[qgv_point];
+}
+
 void intel_bw_init_hw(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_DISPLAY(dev_priv))
@@ -806,6 +822,36 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
 	return to_intel_bw_state(bw_state);
 }
 
+static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
+					 int num_active_planes)
+{
+	unsigned int max_bw_point = 0;
+	unsigned int max_bw = 0;
+	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
+	int i;
+
+	for (i = 0; i < num_qgv_points; i++) {
+		unsigned int max_data_rate;
+
+		max_data_rate = adl_qgv_bw(i915, i, num_active_planes);
+
+		/*
+		 * We need to know which qgv point gives us
+		 * maximum bandwidth in order to disable SAGV
+		 * if we find that we exceed SAGV block time
+		 * with watermarks. By that moment we already
+		 * have those, as it is calculated earlier in
+		 * intel_atomic_check,
+		 */
+		if (max_data_rate > max_bw) {
+			max_bw_point = i;
+			max_bw = max_data_rate;
+		}
+	}
+
+	return max_bw_point;
+}
+
 static int mtl_find_qgv_points(struct drm_i915_private *i915,
 			       unsigned int data_rate,
 			       unsigned int num_active_planes,
@@ -883,8 +929,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
 			       const struct intel_bw_state *old_bw_state,
 			       struct intel_bw_state *new_bw_state)
 {
-	unsigned int max_bw_point = 0;
-	unsigned int max_bw = 0;
 	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
 	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
 	u16 psf_points = 0;
@@ -897,31 +941,10 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
 		return ret;
 
 	for (i = 0; i < num_qgv_points; i++) {
-		unsigned int idx;
 		unsigned int max_data_rate;
 
-		if (DISPLAY_VER(i915) >= 12)
-			idx = tgl_max_bw_index(i915, num_active_planes, i);
-		else
-			idx = icl_max_bw_index(i915, num_active_planes, i);
-
-		if (idx >= ARRAY_SIZE(i915->display.bw.max))
-			continue;
-
-		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
+		max_data_rate = adl_qgv_bw(i915, i, num_active_planes);
 
-		/*
-		 * We need to know which qgv point gives us
-		 * maximum bandwidth in order to disable SAGV
-		 * if we find that we exceed SAGV block time
-		 * with watermarks. By that moment we already
-		 * have those, as it is calculated earlier in
-		 * intel_atomic_check,
-		 */
-		if (max_data_rate > max_bw) {
-			max_bw_point = i;
-			max_bw = max_data_rate;
-		}
 		if (max_data_rate >= data_rate)
 			qgv_points |= BIT(i);
 
@@ -965,9 +988,11 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
 	 * cause.
 	 */
 	if (!intel_can_enable_sagv(i915, new_bw_state)) {
-		qgv_points = BIT(max_bw_point);
+		unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, num_active_planes);
+
+		qgv_points = BIT(max_bw_qgv_point);
 		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
-			    max_bw_point);
+			    max_bw_qgv_point);
 	}
 
 	/*
-- 
2.37.3

[PATCH 3/3] drm/i915: Disable SAGV on bw init, to force QGV point recalculation

[Stanislav Lisovskiy]

Problem is that on some platforms, we do get QGV point mask in wrong
state on boot. However driver assumes it is set to 0
(i.e all points allowed), however in reality we might get them all restricted,
causing issues.
Lets disable SAGV initially to force proper QGV point state.
If more QGV points are available, driver will recalculate and update
those then after next commit.

v2: - Added trace to see which QGV/PSF GV point is used when SAGV is
      disabled.
v3: - Move force disable function to intel_bw_init in order to initialize
      bw state as well, so that hw/sw are immediately in sync after init.
v4: - Don't try sending PCode request, seems like it is not possible at
      intel_bw_init, however assigning bw->state to be restricted as if
      SAGV is off, still forces driveer to send PCode request anyway on
      next modeset, so the solution still works.
      However we still need to address the case, when no display is connected,
      which anyway requires much more changes.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_bw.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index 7baa1c13eccd..36a6304207ba 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -852,6 +852,27 @@ static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
 	return max_bw_point;
 }
 
+void icl_force_disable_sagv(struct drm_i915_private *i915, struct intel_bw_state *bw_state)
+{
+	unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, 0);
+	unsigned int qgv_points;
+	unsigned int psf_points;
+
+	qgv_points = BIT(max_bw_qgv_point);
+
+	/*
+	 * We don't restrict PSF GV points, when disabling SAGV
+	 */
+	psf_points = 0;
+
+	bw_state->qgv_points_mask = ~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
+				      ADLS_PCODE_REQ_PSF_PT(psf_points)) &
+				      icl_qgv_points_mask(i915);
+
+	drm_dbg_kms(&i915->drm, "Forcing SAGV disable: leaving QGV point %d\n",
+				max_bw_qgv_point);
+}
+
 static int mtl_find_qgv_points(struct drm_i915_private *i915,
 			       unsigned int data_rate,
 			       unsigned int num_active_planes,
@@ -1351,5 +1372,8 @@ int intel_bw_init(struct drm_i915_private *dev_priv)
 	intel_atomic_global_obj_init(dev_priv, &dev_priv->display.bw.obj,
 				     &state->base, &intel_bw_funcs);
 
+	if (DISPLAY_VER(dev_priv) < 14)
+		icl_force_disable_sagv(dev_priv, state);
+
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
index 59cb4fc5db76..243192fd4cae 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.h
+++ b/drivers/gpu/drm/i915/display/intel_bw.h
@@ -74,5 +74,7 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
 			    bool *need_cdclk_calc);
 int intel_bw_min_cdclk(struct drm_i915_private *i915,
 		       const struct intel_bw_state *bw_state);
+void icl_force_disable_sagv(struct drm_i915_private *dev_priv,
+			    struct intel_bw_state *bw_state);
 
 #endif /* __INTEL_BW_H__ */
-- 
2.37.3

✗ Fi.CI.CHECKPATCH: warning for QGV/SAGV related fixes (rev4)

[Patchwork]

== Series Details ==

Series: QGV/SAGV related fixes (rev4)
URL   : https://patchwork.freedesktop.org/series/126962/
State : warning

== Summary ==

Error: dim checkpatch failed
9d8849950f20 drm/i915: Add meaningful traces for QGV point info error handling
27f440447eae drm/i915: Extract code required to calculate max qgv/psf gv point
b8d3de777d4d drm/i915: Disable SAGV on bw init, to force QGV point recalculation
-:9: WARNING:COMMIT_LOG_LONG_LINE: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#9: 
(i.e all points allowed), however in reality we might get them all restricted,

-:54: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#54: FILE: drivers/gpu/drm/i915/display/intel_bw.c:873:
+	drm_dbg_kms(&i915->drm, "Forcing SAGV disable: leaving QGV point %d\n",
+				max_bw_qgv_point);

total: 0 errors, 1 warnings, 1 checks, 42 lines checked

✗ Fi.CI.SPARSE: warning for QGV/SAGV related fixes (rev4)

[Patchwork]

== Series Details ==

Series: QGV/SAGV related fixes (rev4)
URL   : https://patchwork.freedesktop.org/series/126962/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.
+./arch/x86/include/asm/bitops.h:116:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:147:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:149:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:153:26: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:155:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:155:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:173:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:175:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:179:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:181:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:181:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:185:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:187:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:191:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:194:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:194:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:236:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:238:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return'
+./drivers/gpu/drm/i915/intel_uncore.h:346:1: warning: trying to copy expression type 31
+./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return'

✗ Fi.CI.BAT: failure for QGV/SAGV related fixes (rev4)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 9287 bytes --]

== Series Details ==

Series: QGV/SAGV related fixes (rev4)
URL   : https://patchwork.freedesktop.org/series/126962/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14132 -> Patchwork_126962v4
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_126962v4 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_126962v4, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/index.html

Participating hosts (38 -> 37)
------------------------------

  Additional (1): bat-mtlp-8 
  Missing    (2): bat-dg2-9 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_126962v4:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@hangcheck:
    - bat-rpls-2:         [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14132/bat-rpls-2/igt@i915_selftest@live@hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-rpls-2/igt@i915_selftest@live@hangcheck.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@gem_contexts:
    - {bat-adls-6}:       [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14132/bat-adls-6/igt@i915_selftest@live@gem_contexts.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-adls-6/igt@i915_selftest@live@gem_contexts.html

  
Known issues
------------

  Here are the changes found in Patchwork_126962v4 that come from known issues:

### CI changes ###

#### Issues hit ####

  * boot:
    - bat-jsl-1:          [PASS][5] -> [FAIL][6] ([i915#8293])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14132/bat-jsl-1/boot.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-jsl-1/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - bat-mtlp-8:         NOTRUN -> [SKIP][7] ([i915#9318])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@debugfs_test@basic-hwmon.html

  * igt@gem_lmem_swapping@verify-random:
    - bat-mtlp-8:         NOTRUN -> [SKIP][8] ([i915#4613]) +3 other tests skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@gem_lmem_swapping@verify-random.html

  * igt@gem_mmap@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][9] ([i915#4083])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@gem_mmap@basic.html

  * igt@gem_mmap_gtt@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][10] ([i915#4077]) +2 other tests skip
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@gem_mmap_gtt@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][11] ([i915#4079]) +1 other test skip
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@gem_render_tiled_blits@basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-mtlp-8:         NOTRUN -> [SKIP][12] ([i915#6621])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@i915_pm_rps@basic-api.html

  * igt@i915_suspend@basic-s3-without-i915:
    - bat-mtlp-8:         NOTRUN -> [SKIP][13] ([i915#6645])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][14] ([i915#5190])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][15] ([i915#4212]) +8 other tests skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][16] ([i915#4213]) +1 other test skip
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][17] ([i915#3555] / [i915#3840] / [i915#9159])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-mtlp-8:         NOTRUN -> [SKIP][18] ([fdo#109285])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-mtlp-8:         NOTRUN -> [SKIP][19] ([i915#5274])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_pipe_crc_basic@suspend-read-crc@pipe-a-dp-1:
    - bat-dg2-8:          [PASS][20] -> [INCOMPLETE][21] ([i915#1982] / [i915#9280])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14132/bat-dg2-8/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-a-dp-1.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-dg2-8/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-a-dp-1.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-mtlp-8:         NOTRUN -> [SKIP][22] ([i915#3555] / [i915#8809])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-mtlp-8:         NOTRUN -> [SKIP][23] ([i915#3708] / [i915#4077]) +1 other test skip
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-fence-read:
    - bat-mtlp-8:         NOTRUN -> [SKIP][24] ([i915#3708]) +2 other tests skip
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-mtlp-8/igt@prime_vgem@basic-fence-read.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - {bat-rpls-3}:       [DMESG-WARN][25] ([i915#5591]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14132/bat-rpls-3/igt@i915_selftest@live@hangcheck.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/bat-rpls-3/igt@i915_selftest@live@hangcheck.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5591]: https://gitlab.freedesktop.org/drm/intel/issues/5591
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
  [i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
  [i915#8809]: https://gitlab.freedesktop.org/drm/intel/issues/8809
  [i915#9159]: https://gitlab.freedesktop.org/drm/intel/issues/9159
  [i915#9280]: https://gitlab.freedesktop.org/drm/intel/issues/9280
  [i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318
  [i915#9688]: https://gitlab.freedesktop.org/drm/intel/issues/9688


Build changes
-------------

  * Linux: CI_DRM_14132 -> Patchwork_126962v4

  CI-20190529: 20190529
  CI_DRM_14132: b42f47ca5fff1d04fb8eb02d64520b3f338a495d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7677: 57ed393a5b5d04e985f9950a7f1546fc95f4001e @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_126962v4: b42f47ca5fff1d04fb8eb02d64520b3f338a495d @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

40d294172de8 drm/i915: Disable SAGV on bw init, to force QGV point recalculation
41ddf262caf0 drm/i915: Extract code required to calculate max qgv/psf gv point
106beb007d6b drm/i915: Add meaningful traces for QGV point info error handling

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v4/index.html

[-- Attachment #2: Type: text/html, Size: 10584 bytes --]

Re: [PATCH 2/3] drm/i915: Extract code required to calculate max qgv/psf gv point

[Ville Syrjälä]

On Wed, Jan 17, 2024 at 05:57:17PM +0200, Stanislav Lisovskiy wrote:
> We need that in order to force disable SAGV in next patch.
> Also it is beneficial to separate that code, as in majority cases,
> when SAGV is enabled, we don't even need those calculations.
> Also we probably need to determine max PSF GV point as well, however
> currently we don't do that when we disable SAGV, which might be
> actually causing some issues in that case.
> 
> v2: - Introduce helper adl_qgv_bw(counterpart to adl_psf_bw)
>       (Ville Syrjälä)
>     - Don't restrict psf gv points for SAGV disable case
>       (Ville Syrjälä)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 81 ++++++++++++++++---------
>  1 file changed, 53 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 77886cc21211..7baa1c13eccd 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -652,15 +652,31 @@ static unsigned int tgl_max_bw_index(struct drm_i915_private *dev_priv,
>  	return 0;
>  }
>  
> -static unsigned int adl_psf_bw(struct drm_i915_private *dev_priv,
> +static unsigned int adl_psf_bw(struct drm_i915_private *i915,
>  			       int psf_gv_point)
>  {
>  	const struct intel_bw_info *bi =
> -			&dev_priv->display.bw.max[0];
> +			&i915->display.bw.max[0];

Unrelated change.

>  
>  	return bi->psf_bw[psf_gv_point];
>  }
>  
> +static unsigned int adl_qgv_bw(struct drm_i915_private *i915,

s/adl/icl/, or s/adl/intel/ perhaps.

> +			       int qgv_point, int num_active_planes)

Please keep the parameters in the same order as elsewhere.
It's hard enough trying to juggle multiple integer parameters
without intentionally confusing matters by swapping the order
randomly.

> +{
> +	unsigned int idx;
> +
> +	if (DISPLAY_VER(i915) > 11)

 >= 12

> +		idx = tgl_max_bw_index(i915, num_active_planes, qgv_point);
> +	else
> +		idx = icl_max_bw_index(i915, num_active_planes, qgv_point);
> +
> +	if (idx >= ARRAY_SIZE(i915->display.bw.max))
> +		return 0;
> +
> +	return i915->display.bw.max[idx].deratedbw[qgv_point];
> +}
> +
>  void intel_bw_init_hw(struct drm_i915_private *dev_priv)
>  {
>  	if (!HAS_DISPLAY(dev_priv))
> @@ -806,6 +822,36 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
>  	return to_intel_bw_state(bw_state);
>  }
>  
> +static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
> +					 int num_active_planes)
> +{
> +	unsigned int max_bw_point = 0;
> +	unsigned int max_bw = 0;
> +	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
> +	int i;
> +
> +	for (i = 0; i < num_qgv_points; i++) {
> +		unsigned int max_data_rate;
> +
> +		max_data_rate = adl_qgv_bw(i915, i, num_active_planes);
> +
> +		/*
> +		 * We need to know which qgv point gives us
> +		 * maximum bandwidth in order to disable SAGV
> +		 * if we find that we exceed SAGV block time
> +		 * with watermarks. By that moment we already
> +		 * have those, as it is calculated earlier in
> +		 * intel_atomic_check,
> +		 */
> +		if (max_data_rate > max_bw) {
> +			max_bw_point = i;
> +			max_bw = max_data_rate;
> +		}
> +	}
> +
> +	return max_bw_point;
> +}
> +
>  static int mtl_find_qgv_points(struct drm_i915_private *i915,
>  			       unsigned int data_rate,
>  			       unsigned int num_active_planes,
> @@ -883,8 +929,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
>  			       const struct intel_bw_state *old_bw_state,
>  			       struct intel_bw_state *new_bw_state)
>  {
> -	unsigned int max_bw_point = 0;
> -	unsigned int max_bw = 0;
>  	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
>  	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
>  	u16 psf_points = 0;
> @@ -897,31 +941,10 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
>  		return ret;
>  
>  	for (i = 0; i < num_qgv_points; i++) {
> -		unsigned int idx;
>  		unsigned int max_data_rate;
>  
> -		if (DISPLAY_VER(i915) >= 12)
> -			idx = tgl_max_bw_index(i915, num_active_planes, i);
> -		else
> -			idx = icl_max_bw_index(i915, num_active_planes, i);
> -
> -		if (idx >= ARRAY_SIZE(i915->display.bw.max))
> -			continue;
> -
> -		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
> +		max_data_rate = adl_qgv_bw(i915, i, num_active_planes);
>  
> -		/*
> -		 * We need to know which qgv point gives us
> -		 * maximum bandwidth in order to disable SAGV
> -		 * if we find that we exceed SAGV block time
> -		 * with watermarks. By that moment we already
> -		 * have those, as it is calculated earlier in
> -		 * intel_atomic_check,
> -		 */
> -		if (max_data_rate > max_bw) {
> -			max_bw_point = i;
> -			max_bw = max_data_rate;
> -		}
>  		if (max_data_rate >= data_rate)
>  			qgv_points |= BIT(i);
>  
> @@ -965,9 +988,11 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
>  	 * cause.
>  	 */
>  	if (!intel_can_enable_sagv(i915, new_bw_state)) {
> -		qgv_points = BIT(max_bw_point);
> +		unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, num_active_planes);
> +
> +		qgv_points = BIT(max_bw_qgv_point);
>  		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
> -			    max_bw_point);
> +			    max_bw_qgv_point);
>  	}
>  
>  	/*
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/3] drm/i915: Disable SAGV on bw init, to force QGV point recalculation

[Ville Syrjälä]

On Wed, Jan 17, 2024 at 05:57:18PM +0200, Stanislav Lisovskiy wrote:
> Problem is that on some platforms, we do get QGV point mask in wrong
> state on boot. However driver assumes it is set to 0
> (i.e all points allowed), however in reality we might get them all restricted,
> causing issues.
> Lets disable SAGV initially to force proper QGV point state.
> If more QGV points are available, driver will recalculate and update
> those then after next commit.
> 
> v2: - Added trace to see which QGV/PSF GV point is used when SAGV is
>       disabled.
> v3: - Move force disable function to intel_bw_init in order to initialize
>       bw state as well, so that hw/sw are immediately in sync after init.
> v4: - Don't try sending PCode request, seems like it is not possible at
>       intel_bw_init, however assigning bw->state to be restricted as if
>       SAGV is off, still forces driveer to send PCode request anyway on
>       next modeset, so the solution still works.
>       However we still need to address the case, when no display is connected,
>       which anyway requires much more changes.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_bw.h |  2 ++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 7baa1c13eccd..36a6304207ba 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -852,6 +852,27 @@ static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
>  	return max_bw_point;
>  }
>  
> +void icl_force_disable_sagv(struct drm_i915_private *i915, struct intel_bw_state *bw_state)
> +{
> +	unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, 0);
> +	unsigned int qgv_points;
> +	unsigned int psf_points;
> +
> +	qgv_points = BIT(max_bw_qgv_point);
> +
> +	/*
> +	 * We don't restrict PSF GV points, when disabling SAGV
> +	 */
> +	psf_points = 0;

Using 0 looks very wrong here. Since we have no idea how much
bandwidth the display is consuming at this time we should
restrict this to the max psf gv point as well.

> +
> +	bw_state->qgv_points_mask = ~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
> +				      ADLS_PCODE_REQ_PSF_PT(psf_points)) &
> +				      icl_qgv_points_mask(i915);
> +
> +	drm_dbg_kms(&i915->drm, "Forcing SAGV disable: leaving QGV point %d\n",
> +				max_bw_qgv_point);

You didn't actually poke the hardware to disable anything.

> +}
> +
>  static int mtl_find_qgv_points(struct drm_i915_private *i915,
>  			       unsigned int data_rate,
>  			       unsigned int num_active_planes,
> @@ -1351,5 +1372,8 @@ int intel_bw_init(struct drm_i915_private *dev_priv)
>  	intel_atomic_global_obj_init(dev_priv, &dev_priv->display.bw.obj,
>  				     &state->base, &intel_bw_funcs);
>  
> +	if (DISPLAY_VER(dev_priv) < 14)

Should be some kind of range check to avoid putting garbage in there on
old platforms that don't support QGV.

> +		icl_force_disable_sagv(dev_priv, state);
> +
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> index 59cb4fc5db76..243192fd4cae 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.h
> +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> @@ -74,5 +74,7 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
>  			    bool *need_cdclk_calc);
>  int intel_bw_min_cdclk(struct drm_i915_private *i915,
>  		       const struct intel_bw_state *bw_state);
> +void icl_force_disable_sagv(struct drm_i915_private *dev_priv,
> +			    struct intel_bw_state *bw_state);

Why?

>  
>  #endif /* __INTEL_BW_H__ */
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/3] drm/i915: Disable SAGV on bw init, to force QGV point recalculation

[Lisovskiy, Stanislav]

On Thu, Jan 18, 2024 at 10:35:56AM +0200, Ville Syrjälä wrote:
> On Wed, Jan 17, 2024 at 05:57:18PM +0200, Stanislav Lisovskiy wrote:
> > Problem is that on some platforms, we do get QGV point mask in wrong
> > state on boot. However driver assumes it is set to 0
> > (i.e all points allowed), however in reality we might get them all restricted,
> > causing issues.
> > Lets disable SAGV initially to force proper QGV point state.
> > If more QGV points are available, driver will recalculate and update
> > those then after next commit.
> > 
> > v2: - Added trace to see which QGV/PSF GV point is used when SAGV is
> >       disabled.
> > v3: - Move force disable function to intel_bw_init in order to initialize
> >       bw state as well, so that hw/sw are immediately in sync after init.
> > v4: - Don't try sending PCode request, seems like it is not possible at
> >       intel_bw_init, however assigning bw->state to be restricted as if
> >       SAGV is off, still forces driveer to send PCode request anyway on
> >       next modeset, so the solution still works.
> >       However we still need to address the case, when no display is connected,
> >       which anyway requires much more changes.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.c | 24 ++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_bw.h |  2 ++
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index 7baa1c13eccd..36a6304207ba 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -852,6 +852,27 @@ static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
> >  	return max_bw_point;
> >  }
> >  
> > +void icl_force_disable_sagv(struct drm_i915_private *i915, struct intel_bw_state *bw_state)
> > +{
> > +	unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, 0);
> > +	unsigned int qgv_points;
> > +	unsigned int psf_points;
> > +
> > +	qgv_points = BIT(max_bw_qgv_point);
> > +
> > +	/*
> > +	 * We don't restrict PSF GV points, when disabling SAGV
> > +	 */
> > +	psf_points = 0;
> 
> Using 0 looks very wrong here. Since we have no idea how much
> bandwidth the display is consuming at this time we should
> restrict this to the max psf gv point as well.

Didn't we just agree that we are not restricting to max PSF GV
point, in the last revision?..

"
> Yep, but I really suspect we should. BSpec states that we should restrict all the GV points
> except highest one, also that some PSF GV points aren't same or usable, depending on BW reqs.
> So I would restrict that as well, in case if SAGV is off, just to be on safe side.

Pretty sure it's explicitly noted that PSF doesn't cause issues with
latency and hence doesn't need this.

In any case, a change like this has no business being in a patch
that's just supposed to refactor code.
"



> 
> > +
> > +	bw_state->qgv_points_mask = ~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
> > +				      ADLS_PCODE_REQ_PSF_PT(psf_points)) &
> > +				      icl_qgv_points_mask(i915);
> > +
> > +	drm_dbg_kms(&i915->drm, "Forcing SAGV disable: leaving QGV point %d\n",
> > +				max_bw_qgv_point);
> 
> You didn't actually poke the hardware to disable anything.

I know, problem is that PCode request doesn't work at this stage.
Need to figure out why, but apparently it seems a bit too early.
PCode just rejects that request.

However that still works, because if more QGV points are available, driver
will send a new request anyway on next modeset.

Stan

> 
> > +}
> > +
> >  static int mtl_find_qgv_points(struct drm_i915_private *i915,
> >  			       unsigned int data_rate,
> >  			       unsigned int num_active_planes,
> > @@ -1351,5 +1372,8 @@ int intel_bw_init(struct drm_i915_private *dev_priv)
> >  	intel_atomic_global_obj_init(dev_priv, &dev_priv->display.bw.obj,
> >  				     &state->base, &intel_bw_funcs);
> >  
> > +	if (DISPLAY_VER(dev_priv) < 14)
> 
> Should be some kind of range check to avoid putting garbage in there on
> old platforms that don't support QGV.
> 
> > +		icl_force_disable_sagv(dev_priv, state);
> > +
> >  	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > index 59cb4fc5db76..243192fd4cae 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > @@ -74,5 +74,7 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
> >  			    bool *need_cdclk_calc);
> >  int intel_bw_min_cdclk(struct drm_i915_private *i915,
> >  		       const struct intel_bw_state *bw_state);
> > +void icl_force_disable_sagv(struct drm_i915_private *dev_priv,
> > +			    struct intel_bw_state *bw_state);
> 
> Why?
> 
> >  
> >  #endif /* __INTEL_BW_H__ */
> > -- 
> > 2.37.3
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 3/3] drm/i915: Disable SAGV on bw init, to force QGV point recalculation

[Ville Syrjälä]

On Thu, Jan 18, 2024 at 10:50:30AM +0200, Lisovskiy, Stanislav wrote:
> On Thu, Jan 18, 2024 at 10:35:56AM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 17, 2024 at 05:57:18PM +0200, Stanislav Lisovskiy wrote:
> > > Problem is that on some platforms, we do get QGV point mask in wrong
> > > state on boot. However driver assumes it is set to 0
> > > (i.e all points allowed), however in reality we might get them all restricted,
> > > causing issues.
> > > Lets disable SAGV initially to force proper QGV point state.
> > > If more QGV points are available, driver will recalculate and update
> > > those then after next commit.
> > > 
> > > v2: - Added trace to see which QGV/PSF GV point is used when SAGV is
> > >       disabled.
> > > v3: - Move force disable function to intel_bw_init in order to initialize
> > >       bw state as well, so that hw/sw are immediately in sync after init.
> > > v4: - Don't try sending PCode request, seems like it is not possible at
> > >       intel_bw_init, however assigning bw->state to be restricted as if
> > >       SAGV is off, still forces driveer to send PCode request anyway on
> > >       next modeset, so the solution still works.
> > >       However we still need to address the case, when no display is connected,
> > >       which anyway requires much more changes.
> > > 
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_bw.c | 24 ++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_bw.h |  2 ++
> > >  2 files changed, 26 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > > index 7baa1c13eccd..36a6304207ba 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > > @@ -852,6 +852,27 @@ static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
> > >  	return max_bw_point;
> > >  }
> > >  
> > > +void icl_force_disable_sagv(struct drm_i915_private *i915, struct intel_bw_state *bw_state)
> > > +{
> > > +	unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, 0);
> > > +	unsigned int qgv_points;
> > > +	unsigned int psf_points;
> > > +
> > > +	qgv_points = BIT(max_bw_qgv_point);
> > > +
> > > +	/*
> > > +	 * We don't restrict PSF GV points, when disabling SAGV
> > > +	 */
> > > +	psf_points = 0;
> > 
> > Using 0 looks very wrong here. Since we have no idea how much
> > bandwidth the display is consuming at this time we should
> > restrict this to the max psf gv point as well.
> 
> Didn't we just agree that we are not restricting to max PSF GV
> point, in the last revision?..

That was about restricting PSF GV points during normal SAGV disable,
which is all about the SAGV latency and nothing to do with bandwidth.

> 
> "
> > Yep, but I really suspect we should. BSpec states that we should restrict all the GV points
> > except highest one, also that some PSF GV points aren't same or usable, depending on BW reqs.
> > So I would restrict that as well, in case if SAGV is off, just to be on safe side.
> 
> Pretty sure it's explicitly noted that PSF doesn't cause issues with
> latency and hence doesn't need this.
> 
> In any case, a change like this has no business being in a patch
> that's just supposed to refactor code.
> "
> 
> 
> 
> > 
> > > +
> > > +	bw_state->qgv_points_mask = ~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
> > > +				      ADLS_PCODE_REQ_PSF_PT(psf_points)) &
> > > +				      icl_qgv_points_mask(i915);
> > > +
> > > +	drm_dbg_kms(&i915->drm, "Forcing SAGV disable: leaving QGV point %d\n",
> > > +				max_bw_qgv_point);
> > 
> > You didn't actually poke the hardware to disable anything.
> 
> I know, problem is that PCode request doesn't work at this stage.

Probably because you passed in garbage in the PSF mask. Please
check what pcode is reporting back to you.

> Need to figure out why, but apparently it seems a bit too early.
> PCode just rejects that request.
> 
> However that still works, because if more QGV points are available, driver
> will send a new request anyway on next modeset.

And what happens if the first commit needs to disable SAGV?

> 
> Stan
> 
> > 
> > > +}
> > > +
> > >  static int mtl_find_qgv_points(struct drm_i915_private *i915,
> > >  			       unsigned int data_rate,
> > >  			       unsigned int num_active_planes,
> > > @@ -1351,5 +1372,8 @@ int intel_bw_init(struct drm_i915_private *dev_priv)
> > >  	intel_atomic_global_obj_init(dev_priv, &dev_priv->display.bw.obj,
> > >  				     &state->base, &intel_bw_funcs);
> > >  
> > > +	if (DISPLAY_VER(dev_priv) < 14)
> > 
> > Should be some kind of range check to avoid putting garbage in there on
> > old platforms that don't support QGV.
> > 
> > > +		icl_force_disable_sagv(dev_priv, state);
> > > +
> > >  	return 0;
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > > index 59cb4fc5db76..243192fd4cae 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > > @@ -74,5 +74,7 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
> > >  			    bool *need_cdclk_calc);
> > >  int intel_bw_min_cdclk(struct drm_i915_private *i915,
> > >  		       const struct intel_bw_state *bw_state);
> > > +void icl_force_disable_sagv(struct drm_i915_private *dev_priv,
> > > +			    struct intel_bw_state *bw_state);
> > 
> > Why?
> > 
> > >  
> > >  #endif /* __INTEL_BW_H__ */
> > > -- 
> > > 2.37.3
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/3] drm/i915: Disable SAGV on bw init, to force QGV point recalculation

[Lisovskiy, Stanislav]

On Thu, Jan 18, 2024 at 11:07:23AM +0200, Ville Syrjälä wrote:
> On Thu, Jan 18, 2024 at 10:50:30AM +0200, Lisovskiy, Stanislav wrote:
> > On Thu, Jan 18, 2024 at 10:35:56AM +0200, Ville Syrjälä wrote:
> > > On Wed, Jan 17, 2024 at 05:57:18PM +0200, Stanislav Lisovskiy wrote:
> > > > Problem is that on some platforms, we do get QGV point mask in wrong
> > > > state on boot. However driver assumes it is set to 0
> > > > (i.e all points allowed), however in reality we might get them all restricted,
> > > > causing issues.
> > > > Lets disable SAGV initially to force proper QGV point state.
> > > > If more QGV points are available, driver will recalculate and update
> > > > those then after next commit.
> > > > 
> > > > v2: - Added trace to see which QGV/PSF GV point is used when SAGV is
> > > >       disabled.
> > > > v3: - Move force disable function to intel_bw_init in order to initialize
> > > >       bw state as well, so that hw/sw are immediately in sync after init.
> > > > v4: - Don't try sending PCode request, seems like it is not possible at
> > > >       intel_bw_init, however assigning bw->state to be restricted as if
> > > >       SAGV is off, still forces driveer to send PCode request anyway on
> > > >       next modeset, so the solution still works.
> > > >       However we still need to address the case, when no display is connected,
> > > >       which anyway requires much more changes.
> > > > 
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_bw.c | 24 ++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/display/intel_bw.h |  2 ++
> > > >  2 files changed, 26 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > > > index 7baa1c13eccd..36a6304207ba 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > > > @@ -852,6 +852,27 @@ static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
> > > >  	return max_bw_point;
> > > >  }
> > > >  
> > > > +void icl_force_disable_sagv(struct drm_i915_private *i915, struct intel_bw_state *bw_state)
> > > > +{
> > > > +	unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, 0);
> > > > +	unsigned int qgv_points;
> > > > +	unsigned int psf_points;
> > > > +
> > > > +	qgv_points = BIT(max_bw_qgv_point);
> > > > +
> > > > +	/*
> > > > +	 * We don't restrict PSF GV points, when disabling SAGV
> > > > +	 */
> > > > +	psf_points = 0;
> > > 
> > > Using 0 looks very wrong here. Since we have no idea how much
> > > bandwidth the display is consuming at this time we should
> > > restrict this to the max psf gv point as well.
> > 
> > Didn't we just agree that we are not restricting to max PSF GV
> > point, in the last revision?..
> 
> That was about restricting PSF GV points during normal SAGV disable,
> which is all about the SAGV latency and nothing to do with bandwidth.

Ah, right. In normal case we are disabling SAGV because of the latency.

> 
> > 
> > "
> > > Yep, but I really suspect we should. BSpec states that we should restrict all the GV points
> > > except highest one, also that some PSF GV points aren't same or usable, depending on BW reqs.
> > > So I would restrict that as well, in case if SAGV is off, just to be on safe side.
> > 
> > Pretty sure it's explicitly noted that PSF doesn't cause issues with
> > latency and hence doesn't need this.
> > 
> > In any case, a change like this has no business being in a patch
> > that's just supposed to refactor code.
> > "
> > 
> > 
> > 
> > > 
> > > > +
> > > > +	bw_state->qgv_points_mask = ~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
> > > > +				      ADLS_PCODE_REQ_PSF_PT(psf_points)) &
> > > > +				      icl_qgv_points_mask(i915);
> > > > +
> > > > +	drm_dbg_kms(&i915->drm, "Forcing SAGV disable: leaving QGV point %d\n",
> > > > +				max_bw_qgv_point);
> > > 
> > > You didn't actually poke the hardware to disable anything.
> > 
> > I know, problem is that PCode request doesn't work at this stage.
> 
> Probably because you passed in garbage in the PSF mask. Please
> check what pcode is reporting back to you.

Previously it was choosing max qgv/psf qgv point. 
It was printing that actually, unfortunately CI run logs are gone by now.

> 
> > Need to figure out why, but apparently it seems a bit too early.
> > PCode just rejects that request.
> > 
> > However that still works, because if more QGV points are available, driver
> > will send a new request anyway on next modeset.
> 
> And what happens if the first commit needs to disable SAGV?

Right, then we don't do anything, hw stays not in sync. 


Stan

> 
> > 
> > Stan
> > 
> > > 
> > > > +}
> > > > +
> > > >  static int mtl_find_qgv_points(struct drm_i915_private *i915,
> > > >  			       unsigned int data_rate,
> > > >  			       unsigned int num_active_planes,
> > > > @@ -1351,5 +1372,8 @@ int intel_bw_init(struct drm_i915_private *dev_priv)
> > > >  	intel_atomic_global_obj_init(dev_priv, &dev_priv->display.bw.obj,
> > > >  				     &state->base, &intel_bw_funcs);
> > > >  
> > > > +	if (DISPLAY_VER(dev_priv) < 14)
> > > 
> > > Should be some kind of range check to avoid putting garbage in there on
> > > old platforms that don't support QGV.
> > > 
> > > > +		icl_force_disable_sagv(dev_priv, state);
> > > > +
> > > >  	return 0;
> > > >  }
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > index 59cb4fc5db76..243192fd4cae 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > @@ -74,5 +74,7 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
> > > >  			    bool *need_cdclk_calc);
> > > >  int intel_bw_min_cdclk(struct drm_i915_private *i915,
> > > >  		       const struct intel_bw_state *bw_state);
> > > > +void icl_force_disable_sagv(struct drm_i915_private *dev_priv,
> > > > +			    struct intel_bw_state *bw_state);
> > > 
> > > Why?
> > > 
> > > >  
> > > >  #endif /* __INTEL_BW_H__ */
> > > > -- 
> > > > 2.37.3
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 1/3] drm/i915: Add meaningful traces for QGV point info error handling

[Gustavo Sousa]

Quoting Stanislav Lisovskiy (2024-01-17 12:57:16-03:00)
>For debug purposes we need those - error path won't flood the log,
>however there has been already numerous cases, when due to lack
>of debugs, we couldn't immediately tell what was the problem on
>customer machine, which slowed down the investigation, requiring
>to get access to target device and adding those traces manually.
>
>v2: - Make the debug more generic and move it to intel_dram_detect
>      (Gustavo Sousa)
>
>Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_bw.c | 4 +++-
> drivers/gpu/drm/i915/soc/intel_dram.c   | 2 ++
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
>index 7f2a50b4f494..77886cc21211 100644
>--- a/drivers/gpu/drm/i915/display/intel_bw.c
>+++ b/drivers/gpu/drm/i915/display/intel_bw.c
>@@ -290,8 +290,10 @@ static int icl_get_qgv_points(struct drm_i915_private *dev_priv,
>                 struct intel_qgv_point *sp = &qi->points[i];
> 
>                 ret = intel_read_qgv_point_info(dev_priv, sp, i);
>-                if (ret)
>+                if (ret) {
>+                        drm_dbg_kms(&dev_priv->drm, "Could not read QGV %d info\n", i);
>                         return ret;
>+                }
> 
>                 drm_dbg_kms(&dev_priv->drm,
>                             "QGV %d: DCLK=%d tRP=%d tRDPRE=%d tRAS=%d tRCD=%d tRC=%d\n",
>diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c b/drivers/gpu/drm/i915/soc/intel_dram.c
>index 15492b69f698..e957be5bfb35 100644
>--- a/drivers/gpu/drm/i915/soc/intel_dram.c
>+++ b/drivers/gpu/drm/i915/soc/intel_dram.c
>@@ -681,6 +681,8 @@ void intel_dram_detect(struct drm_i915_private *i915)
>         if (ret)
>                 return;
> 
>+        drm_dbg_kms(&i915->drm, "Num qgv points %d\n", dram_info->num_qgv_points);
>+

Could we use %u, since num_qgv_points is unsigned?

Aside from that,

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>         drm_dbg_kms(&i915->drm, "DRAM channels: %u\n", dram_info->num_channels);
> 
>         drm_dbg_kms(&i915->drm, "Watermark level 0 adjustment needed: %s\n",
>-- 
>2.37.3
>

✗ Fi.CI.CHECKPATCH: warning for QGV/SAGV related fixes (rev5)

[Patchwork]

== Series Details ==

Series: QGV/SAGV related fixes (rev5)
URL   : https://patchwork.freedesktop.org/series/126962/
State : warning

== Summary ==

Error: dim checkpatch failed
2b101b19c57e drm/i915: Add meaningful traces for QGV point info error handling
642d40ddd8ca drm/i915: Extract code required to calculate max qgv/psf gv point
cfe55d16c546 drm/i915: Disable SAGV on bw init, to force QGV point recalculation
-:9: WARNING:COMMIT_LOG_LONG_LINE: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#9: 
(i.e all points allowed), however in reality we might get them all restricted,

-:54: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#54: FILE: drivers/gpu/drm/i915/display/intel_bw.c:873:
+	drm_dbg_kms(&i915->drm, "Forcing SAGV disable: leaving QGV point %d\n",
+				max_bw_qgv_point);

total: 0 errors, 1 warnings, 1 checks, 42 lines checked

✗ Fi.CI.SPARSE: warning for QGV/SAGV related fixes (rev5)

[Patchwork]

== Series Details ==

Series: QGV/SAGV related fixes (rev5)
URL   : https://patchwork.freedesktop.org/series/126962/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.
+./arch/x86/include/asm/bitops.h:116:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:147:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:149:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:153:26: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:155:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:155:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:173:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:175:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:179:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:181:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:181:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:185:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:187:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:191:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:194:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:194:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:236:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:238:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return'
+./drivers/gpu/drm/i915/intel_uncore.h:346:1: warning: trying to copy expression type 31
+./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return'

✗ Fi.CI.BAT: failure for QGV/SAGV related fixes (rev5)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 8527 bytes --]

== Series Details ==

Series: QGV/SAGV related fixes (rev5)
URL   : https://patchwork.freedesktop.org/series/126962/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14139 -> Patchwork_126962v5
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_126962v5 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_126962v5, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/index.html

Participating hosts (37 -> 35)
------------------------------

  Additional (2): bat-kbl-2 bat-mtlp-8 
  Missing    (4): bat-dg2-9 fi-bsw-n3050 fi-snb-2520m fi-pnv-d510 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_126962v5:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_suspend@basic-s0@smem:
    - fi-kbl-7567u:       [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14139/fi-kbl-7567u/igt@gem_exec_suspend@basic-s0@smem.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/fi-kbl-7567u/igt@gem_exec_suspend@basic-s0@smem.html

  
Known issues
------------

  Here are the changes found in Patchwork_126962v5 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - bat-mtlp-8:         NOTRUN -> [SKIP][3] ([i915#9318])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@debugfs_test@basic-hwmon.html

  * igt@fbdev@info:
    - bat-kbl-2:          NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#1849])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-kbl-2/igt@fbdev@info.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-kbl-2:          NOTRUN -> [SKIP][5] ([fdo#109271]) +36 other tests skip
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-kbl-2/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_lmem_swapping@verify-random:
    - bat-mtlp-8:         NOTRUN -> [SKIP][6] ([i915#4613]) +3 other tests skip
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@gem_lmem_swapping@verify-random.html

  * igt@gem_mmap@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][7] ([i915#4083])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@gem_mmap@basic.html

  * igt@gem_mmap_gtt@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][8] ([i915#4077]) +2 other tests skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@gem_mmap_gtt@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][9] ([i915#4079]) +1 other test skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@gem_render_tiled_blits@basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-mtlp-8:         NOTRUN -> [SKIP][10] ([i915#6621])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@i915_pm_rps@basic-api.html

  * igt@i915_suspend@basic-s3-without-i915:
    - bat-mtlp-8:         NOTRUN -> [SKIP][11] ([i915#6645])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][12] ([i915#5190])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][13] ([i915#4212]) +8 other tests skip
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][14] ([i915#4213]) +1 other test skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][15] ([i915#3555] / [i915#3840] / [i915#9159])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-mtlp-8:         NOTRUN -> [SKIP][16] ([fdo#109285])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-mtlp-8:         NOTRUN -> [SKIP][17] ([i915#5274])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-mtlp-8:         NOTRUN -> [SKIP][18] ([i915#3555] / [i915#8809])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-mtlp-8:         NOTRUN -> [SKIP][19] ([i915#3708] / [i915#4077]) +1 other test skip
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-fence-read:
    - bat-mtlp-8:         NOTRUN -> [SKIP][20] ([i915#3708]) +2 other tests skip
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-mtlp-8/igt@prime_vgem@basic-fence-read.html

  
#### Possible fixes ####

  * igt@kms_pipe_crc_basic@suspend-read-crc@pipe-a-hdmi-a-1:
    - bat-rpls-2:         [ABORT][21] ([i915#10117]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14139/bat-rpls-2/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-a-hdmi-a-1.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/bat-rpls-2/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-a-hdmi-a-1.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [i915#10117]: https://gitlab.freedesktop.org/drm/intel/issues/10117
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
  [i915#8809]: https://gitlab.freedesktop.org/drm/intel/issues/8809
  [i915#9159]: https://gitlab.freedesktop.org/drm/intel/issues/9159
  [i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318
  [i915#9688]: https://gitlab.freedesktop.org/drm/intel/issues/9688


Build changes
-------------

  * Linux: CI_DRM_14139 -> Patchwork_126962v5

  CI-20190529: 20190529
  CI_DRM_14139: 776dc1588b4b329dba41ded2db883fbc1bf77950 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7683: 7683
  Patchwork_126962v5: 776dc1588b4b329dba41ded2db883fbc1bf77950 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

7a7dfa8ec26c drm/i915: Disable SAGV on bw init, to force QGV point recalculation
2296b75bf860 drm/i915: Extract code required to calculate max qgv/psf gv point
96d8f65fc32b drm/i915: Add meaningful traces for QGV point info error handling

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126962v5/index.html

[-- Attachment #2: Type: text/html, Size: 9860 bytes --]

Re: [PATCH 3/3] drm/i915: Disable SAGV on bw init, to force QGV point recalculation

[Lisovskiy, Stanislav]

On Thu, Jan 18, 2024 at 11:07:23AM +0200, Ville Syrjälä wrote:
> On Thu, Jan 18, 2024 at 10:50:30AM +0200, Lisovskiy, Stanislav wrote:
> > On Thu, Jan 18, 2024 at 10:35:56AM +0200, Ville Syrjälä wrote:
> > > On Wed, Jan 17, 2024 at 05:57:18PM +0200, Stanislav Lisovskiy wrote:
> > > > Problem is that on some platforms, we do get QGV point mask in wrong
> > > > state on boot. However driver assumes it is set to 0
> > > > (i.e all points allowed), however in reality we might get them all restricted,
> > > > causing issues.
> > > > Lets disable SAGV initially to force proper QGV point state.
> > > > If more QGV points are available, driver will recalculate and update
> > > > those then after next commit.
> > > > 
> > > > v2: - Added trace to see which QGV/PSF GV point is used when SAGV is
> > > >       disabled.
> > > > v3: - Move force disable function to intel_bw_init in order to initialize
> > > >       bw state as well, so that hw/sw are immediately in sync after init.
> > > > v4: - Don't try sending PCode request, seems like it is not possible at
> > > >       intel_bw_init, however assigning bw->state to be restricted as if
> > > >       SAGV is off, still forces driveer to send PCode request anyway on
> > > >       next modeset, so the solution still works.
> > > >       However we still need to address the case, when no display is connected,
> > > >       which anyway requires much more changes.
> > > > 
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_bw.c | 24 ++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/display/intel_bw.h |  2 ++
> > > >  2 files changed, 26 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > > > index 7baa1c13eccd..36a6304207ba 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > > > @@ -852,6 +852,27 @@ static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
> > > >  	return max_bw_point;
> > > >  }
> > > >  
> > > > +void icl_force_disable_sagv(struct drm_i915_private *i915, struct intel_bw_state *bw_state)
> > > > +{
> > > > +	unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, 0);
> > > > +	unsigned int qgv_points;
> > > > +	unsigned int psf_points;
> > > > +
> > > > +	qgv_points = BIT(max_bw_qgv_point);
> > > > +
> > > > +	/*
> > > > +	 * We don't restrict PSF GV points, when disabling SAGV
> > > > +	 */
> > > > +	psf_points = 0;
> > > 
> > > Using 0 looks very wrong here. Since we have no idea how much
> > > bandwidth the display is consuming at this time we should
> > > restrict this to the max psf gv point as well.
> > 
> > Didn't we just agree that we are not restricting to max PSF GV
> > point, in the last revision?..
> 
> That was about restricting PSF GV points during normal SAGV disable,
> which is all about the SAGV latency and nothing to do with bandwidth.
> 
> > 
> > "
> > > Yep, but I really suspect we should. BSpec states that we should restrict all the GV points
> > > except highest one, also that some PSF GV points aren't same or usable, depending on BW reqs.
> > > So I would restrict that as well, in case if SAGV is off, just to be on safe side.
> > 
> > Pretty sure it's explicitly noted that PSF doesn't cause issues with
> > latency and hence doesn't need this.
> > 
> > In any case, a change like this has no business being in a patch
> > that's just supposed to refactor code.
> > "
> > 
> > 
> > 
> > > 
> > > > +
> > > > +	bw_state->qgv_points_mask = ~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
> > > > +				      ADLS_PCODE_REQ_PSF_PT(psf_points)) &
> > > > +				      icl_qgv_points_mask(i915);
> > > > +
> > > > +	drm_dbg_kms(&i915->drm, "Forcing SAGV disable: leaving QGV point %d\n",
> > > > +				max_bw_qgv_point);
> > > 
> > > You didn't actually poke the hardware to disable anything.
> > 
> > I know, problem is that PCode request doesn't work at this stage.
> 
> Probably because you passed in garbage in the PSF mask. Please
> check what pcode is reporting back to you.


[   34.910602] i915 0000:00:02.0: [drm:icl_get_qgv_points.constprop.0 [i915]] QGV 0: DCLK=2134 tRP=15 tRDPRE=8 tRAS=35 tRCD=15 tRC=50
[   34.911288] i915 0000:00:02.0: [drm:icl_get_qgv_points.constprop.0 [i915]] QGV 1: DCLK=2934 tRP=21 tRDPRE=12 tRAS=47 tRCD=21 tRC=68
[   34.911996] i915 0000:00:02.0: [drm:icl_get_qgv_points.constprop.0 [i915]] QGV 2: DCLK=3201 tRP=22 tRDPRE=12 tRAS=52 tRCD=22 tRC=74
[   34.912699] i915 0000:00:02.0: [drm:icl_get_qgv_points.constprop.0 [i915]] QGV 3: DCLK=2668 tRP=19 tRDPRE=10 tRAS=43 tRCD=19 tRC=62
[   34.913434] i915 0000:00:02.0: [drm:icl_get_qgv_points.constprop.0 [i915]] PSF GV 0: CLK=32
[   34.914133] i915 0000:00:02.0: [drm:icl_get_qgv_points.constprop.0 [i915]] PSF GV 1: CLK=48
[   34.914914] i915 0000:00:02.0: [drm:icl_get_qgv_points.constprop.0 [i915]] PSF GV 2: CLK=48
[   34.915646] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW0 / QGV 0: num_planes=0 deratedbw=5532 peakbw: 17072
[   34.916343] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW0 / QGV 1: num_planes=0 deratedbw=6907 peakbw: 23472
[   34.917042] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW0 / QGV 2: num_planes=0 deratedbw=7449 peakbw: 25608
[   34.917739] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW0 / QGV 3: num_planes=0 deratedbw=6505 peakbw: 21344
[   34.920646] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW1 / QGV 0: num_planes=1 deratedbw=6112 peakbw: 17072
[   34.921342] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW1 / QGV 1: num_planes=1 deratedbw=7959 peakbw: 23472
[   34.922058] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW1 / QGV 2: num_planes=1 deratedbw=8626 peakbw: 25608
[   34.922767] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW1 / QGV 3: num_planes=1 deratedbw=7384 peakbw: 21344
[   34.925604] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW2 / QGV 0: num_planes=0 deratedbw=6451 peakbw: 17072
[   34.926314] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW2 / QGV 1: num_planes=0 deratedbw=8615 peakbw: 23472
[   34.927033] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW2 / QGV 2: num_planes=0 deratedbw=9365 peakbw: 25608
[   34.927834] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW2 / QGV 3: num_planes=0 deratedbw=7919 peakbw: 21344
[   34.930874] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW3 / QGV 0: num_planes=0 deratedbw=6635 peakbw: 17072
[   34.931653] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW3 / QGV 1: num_planes=0 deratedbw=8985 peakbw: 23472
[   34.932385] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW3 / QGV 2: num_planes=0 deratedbw=9784 peakbw: 25608
[   34.933095] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW3 / QGV 3: num_planes=0 deratedbw=8216 peakbw: 21344
[   34.935904] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW4 / QGV 0: num_planes=0 deratedbw=6730 peakbw: 17072
[   34.936635] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW4 / QGV 1: num_planes=0 deratedbw=9183 peakbw: 23472
[   34.937327] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW4 / QGV 2: num_planes=0 deratedbw=10008 peakbw: 25608
[   34.938038] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW4 / QGV 3: num_planes=0 deratedbw=8374 peakbw: 21344
[   34.940977] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW5 / QGV 0: num_planes=0 deratedbw=6779 peakbw: 17072
[   34.941700] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW5 / QGV 1: num_planes=0 deratedbw=9284 peakbw: 23472
[   34.942409] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW5 / QGV 2: num_planes=0 deratedbw=10124 peakbw: 25608
[   34.943148] i915 0000:00:02.0: [drm:tgl_get_bw_info.isra.0 [i915]] BW5 / QGV 3: num_planes=0 deratedbw=8455 peakbw: 21344
[   35.009240] i915 0000:00:02.0: [drm:intel_bw_init [i915]] Forcing SAGV disable: leaving QGV point 2 PSF GV point 1
[   35.011574] i915 0000:00:02.0: [drm:skl_pcode_request [i915]] PCODE timeout, retrying with preemption disabled
[   35.063185] i915 0000:00:02.0: [drm] *ERROR* Failed to disable qgv points (ffffff92) points: 0x50b
[   35.066268] i915 0000:00:02.0: [drm:intel_bw_init [i915]] Restricting QGV points failed: ffffff92

Got the traces, finally apparently both PSF/QGV masks look correct:

For SAGV we leave only QGV point 2 which provides the highest BW.
For PSF we leave point 1, which seems legitimate, however seems like PCode rejects it(0x02, bits 0..1 mean request rejected).

I have still a suspicion that PCode might be not ready at that stage when intel_bw_init is called.
Wondering why..

Stan


> 
> > Need to figure out why, but apparently it seems a bit too early.
> > PCode just rejects that request.
> > 
> > However that still works, because if more QGV points are available, driver
> > will send a new request anyway on next modeset.
> 
> And what happens if the first commit needs to disable SAGV?
> 
> > 
> > Stan
> > 
> > > 
> > > > +}
> > > > +
> > > >  static int mtl_find_qgv_points(struct drm_i915_private *i915,
> > > >  			       unsigned int data_rate,
> > > >  			       unsigned int num_active_planes,
> > > > @@ -1351,5 +1372,8 @@ int intel_bw_init(struct drm_i915_private *dev_priv)
> > > >  	intel_atomic_global_obj_init(dev_priv, &dev_priv->display.bw.obj,
> > > >  				     &state->base, &intel_bw_funcs);
> > > >  
> > > > +	if (DISPLAY_VER(dev_priv) < 14)
> > > 
> > > Should be some kind of range check to avoid putting garbage in there on
> > > old platforms that don't support QGV.
> > > 
> > > > +		icl_force_disable_sagv(dev_priv, state);
> > > > +
> > > >  	return 0;
> > > >  }
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > index 59cb4fc5db76..243192fd4cae 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > > > @@ -74,5 +74,7 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
> > > >  			    bool *need_cdclk_calc);
> > > >  int intel_bw_min_cdclk(struct drm_i915_private *i915,
> > > >  		       const struct intel_bw_state *bw_state);
> > > > +void icl_force_disable_sagv(struct drm_i915_private *dev_priv,
> > > > +			    struct intel_bw_state *bw_state);
> > > 
> > > Why?
> > > 
> > > >  
> > > >  #endif /* __INTEL_BW_H__ */
> > > > -- 
> > > > 2.37.3
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

[PATCH 2/3] drm/i915: Extract code required to calculate max qgv/psf gv point

[Stanislav Lisovskiy]

We need that in order to force disable SAGV in next patch.
Also it is beneficial to separate that code, as in majority cases,
when SAGV is enabled, we don't even need those calculations.
Also we probably need to determine max PSF GV point as well, however
currently we don't do that when we disable SAGV, which might be
actually causing some issues in that case.

v2: - Introduce helper adl_qgv_bw(counterpart to adl_psf_bw)
      (Ville Syrjälä)
    - Don't restrict psf gv points for SAGV disable case
      (Ville Syrjälä)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 81 ++++++++++++++++---------
 1 file changed, 53 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index 77886cc21211..7baa1c13eccd 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -652,15 +652,31 @@ static unsigned int tgl_max_bw_index(struct drm_i915_private *dev_priv,
 	return 0;
 }
 
-static unsigned int adl_psf_bw(struct drm_i915_private *dev_priv,
+static unsigned int adl_psf_bw(struct drm_i915_private *i915,
 			       int psf_gv_point)
 {
 	const struct intel_bw_info *bi =
-			&dev_priv->display.bw.max[0];
+			&i915->display.bw.max[0];
 
 	return bi->psf_bw[psf_gv_point];
 }
 
+static unsigned int adl_qgv_bw(struct drm_i915_private *i915,
+			       int qgv_point, int num_active_planes)
+{
+	unsigned int idx;
+
+	if (DISPLAY_VER(i915) > 11)
+		idx = tgl_max_bw_index(i915, num_active_planes, qgv_point);
+	else
+		idx = icl_max_bw_index(i915, num_active_planes, qgv_point);
+
+	if (idx >= ARRAY_SIZE(i915->display.bw.max))
+		return 0;
+
+	return i915->display.bw.max[idx].deratedbw[qgv_point];
+}
+
 void intel_bw_init_hw(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_DISPLAY(dev_priv))
@@ -806,6 +822,36 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
 	return to_intel_bw_state(bw_state);
 }
 
+static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
+					 int num_active_planes)
+{
+	unsigned int max_bw_point = 0;
+	unsigned int max_bw = 0;
+	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
+	int i;
+
+	for (i = 0; i < num_qgv_points; i++) {
+		unsigned int max_data_rate;
+
+		max_data_rate = adl_qgv_bw(i915, i, num_active_planes);
+
+		/*
+		 * We need to know which qgv point gives us
+		 * maximum bandwidth in order to disable SAGV
+		 * if we find that we exceed SAGV block time
+		 * with watermarks. By that moment we already
+		 * have those, as it is calculated earlier in
+		 * intel_atomic_check,
+		 */
+		if (max_data_rate > max_bw) {
+			max_bw_point = i;
+			max_bw = max_data_rate;
+		}
+	}
+
+	return max_bw_point;
+}
+
 static int mtl_find_qgv_points(struct drm_i915_private *i915,
 			       unsigned int data_rate,
 			       unsigned int num_active_planes,
@@ -883,8 +929,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
 			       const struct intel_bw_state *old_bw_state,
 			       struct intel_bw_state *new_bw_state)
 {
-	unsigned int max_bw_point = 0;
-	unsigned int max_bw = 0;
 	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
 	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
 	u16 psf_points = 0;
@@ -897,31 +941,10 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
 		return ret;
 
 	for (i = 0; i < num_qgv_points; i++) {
-		unsigned int idx;
 		unsigned int max_data_rate;
 
-		if (DISPLAY_VER(i915) >= 12)
-			idx = tgl_max_bw_index(i915, num_active_planes, i);
-		else
-			idx = icl_max_bw_index(i915, num_active_planes, i);
-
-		if (idx >= ARRAY_SIZE(i915->display.bw.max))
-			continue;
-
-		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
+		max_data_rate = adl_qgv_bw(i915, i, num_active_planes);
 
-		/*
-		 * We need to know which qgv point gives us
-		 * maximum bandwidth in order to disable SAGV
-		 * if we find that we exceed SAGV block time
-		 * with watermarks. By that moment we already
-		 * have those, as it is calculated earlier in
-		 * intel_atomic_check,
-		 */
-		if (max_data_rate > max_bw) {
-			max_bw_point = i;
-			max_bw = max_data_rate;
-		}
 		if (max_data_rate >= data_rate)
 			qgv_points |= BIT(i);
 
@@ -965,9 +988,11 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
 	 * cause.
 	 */
 	if (!intel_can_enable_sagv(i915, new_bw_state)) {
-		qgv_points = BIT(max_bw_point);
+		unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, num_active_planes);
+
+		qgv_points = BIT(max_bw_qgv_point);
 		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
-			    max_bw_point);
+			    max_bw_qgv_point);
 	}
 
 	/*
-- 
2.37.3

[PATCH 2/3] drm/i915: Extract code required to calculate max qgv/psf gv point

[Stanislav Lisovskiy]

We need that in order to force disable SAGV in next patch.
Also it is beneficial to separate that code, as in majority cases,
when SAGV is enabled, we don't even need those calculations.
Also we probably need to determine max PSF GV point as well, however
currently we don't do that when we disable SAGV, which might be
actually causing some issues in that case.

v2: - Introduce helper adl_qgv_bw(counterpart to adl_psf_bw)
      (Ville Syrjälä)
    - Don't restrict psf gv points for SAGV disable case
      (Ville Syrjälä)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 81 ++++++++++++++++---------
 1 file changed, 53 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index 77886cc21211..7baa1c13eccd 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -652,15 +652,31 @@ static unsigned int tgl_max_bw_index(struct drm_i915_private *dev_priv,
 	return 0;
 }
 
-static unsigned int adl_psf_bw(struct drm_i915_private *dev_priv,
+static unsigned int adl_psf_bw(struct drm_i915_private *i915,
 			       int psf_gv_point)
 {
 	const struct intel_bw_info *bi =
-			&dev_priv->display.bw.max[0];
+			&i915->display.bw.max[0];
 
 	return bi->psf_bw[psf_gv_point];
 }
 
+static unsigned int adl_qgv_bw(struct drm_i915_private *i915,
+			       int qgv_point, int num_active_planes)
+{
+	unsigned int idx;
+
+	if (DISPLAY_VER(i915) > 11)
+		idx = tgl_max_bw_index(i915, num_active_planes, qgv_point);
+	else
+		idx = icl_max_bw_index(i915, num_active_planes, qgv_point);
+
+	if (idx >= ARRAY_SIZE(i915->display.bw.max))
+		return 0;
+
+	return i915->display.bw.max[idx].deratedbw[qgv_point];
+}
+
 void intel_bw_init_hw(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_DISPLAY(dev_priv))
@@ -806,6 +822,36 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
 	return to_intel_bw_state(bw_state);
 }
 
+static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
+					 int num_active_planes)
+{
+	unsigned int max_bw_point = 0;
+	unsigned int max_bw = 0;
+	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
+	int i;
+
+	for (i = 0; i < num_qgv_points; i++) {
+		unsigned int max_data_rate;
+
+		max_data_rate = adl_qgv_bw(i915, i, num_active_planes);
+
+		/*
+		 * We need to know which qgv point gives us
+		 * maximum bandwidth in order to disable SAGV
+		 * if we find that we exceed SAGV block time
+		 * with watermarks. By that moment we already
+		 * have those, as it is calculated earlier in
+		 * intel_atomic_check,
+		 */
+		if (max_data_rate > max_bw) {
+			max_bw_point = i;
+			max_bw = max_data_rate;
+		}
+	}
+
+	return max_bw_point;
+}
+
 static int mtl_find_qgv_points(struct drm_i915_private *i915,
 			       unsigned int data_rate,
 			       unsigned int num_active_planes,
@@ -883,8 +929,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
 			       const struct intel_bw_state *old_bw_state,
 			       struct intel_bw_state *new_bw_state)
 {
-	unsigned int max_bw_point = 0;
-	unsigned int max_bw = 0;
 	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
 	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
 	u16 psf_points = 0;
@@ -897,31 +941,10 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
 		return ret;
 
 	for (i = 0; i < num_qgv_points; i++) {
-		unsigned int idx;
 		unsigned int max_data_rate;
 
-		if (DISPLAY_VER(i915) >= 12)
-			idx = tgl_max_bw_index(i915, num_active_planes, i);
-		else
-			idx = icl_max_bw_index(i915, num_active_planes, i);
-
-		if (idx >= ARRAY_SIZE(i915->display.bw.max))
-			continue;
-
-		max_data_rate = i915->display.bw.max[idx].deratedbw[i];
+		max_data_rate = adl_qgv_bw(i915, i, num_active_planes);
 
-		/*
-		 * We need to know which qgv point gives us
-		 * maximum bandwidth in order to disable SAGV
-		 * if we find that we exceed SAGV block time
-		 * with watermarks. By that moment we already
-		 * have those, as it is calculated earlier in
-		 * intel_atomic_check,
-		 */
-		if (max_data_rate > max_bw) {
-			max_bw_point = i;
-			max_bw = max_data_rate;
-		}
 		if (max_data_rate >= data_rate)
 			qgv_points |= BIT(i);
 
@@ -965,9 +988,11 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
 	 * cause.
 	 */
 	if (!intel_can_enable_sagv(i915, new_bw_state)) {
-		qgv_points = BIT(max_bw_point);
+		unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, num_active_planes);
+
+		qgv_points = BIT(max_bw_qgv_point);
 		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
-			    max_bw_point);
+			    max_bw_qgv_point);
 	}
 
 	/*
-- 
2.37.3

Re: [PATCH 2/3] drm/i915: Extract code required to calculate max qgv/psf gv point

[Govindapillai, Vinod]

Hi Stan.


On Tue, 2024-02-20 at 11:31 +0200, Stanislav Lisovskiy wrote:
> We need that in order to force disable SAGV in next patch.
> Also it is beneficial to separate that code, as in majority cases,
> when SAGV is enabled, we don't even need those calculations.
> Also we probably need to determine max PSF GV point as well, however
> currently we don't do that when we disable SAGV, which might be
> actually causing some issues in that case.
> 
> v2: - Introduce helper adl_qgv_bw(counterpart to adl_psf_bw)
>       (Ville Syrjälä)
>     - Don't restrict psf gv points for SAGV disable case
>       (Ville Syrjälä)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 81 ++++++++++++++++---------
>  1 file changed, 53 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 77886cc21211..7baa1c13eccd 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -652,15 +652,31 @@ static unsigned int tgl_max_bw_index(struct drm_i915_private *dev_priv,
>         return 0;
>  }
>  
> -static unsigned int adl_psf_bw(struct drm_i915_private *dev_priv,
> +static unsigned int adl_psf_bw(struct drm_i915_private *i915,
>                                int psf_gv_point)
>  {
>         const struct intel_bw_info *bi =
> -                       &dev_priv->display.bw.max[0];
> +                       &i915->display.bw.max[0];
>  
>         return bi->psf_bw[psf_gv_point];
>  }
This is probably not related to this patch..

>  
> +static unsigned int adl_qgv_bw(struct drm_i915_private *i915,
> +                              int qgv_point, int num_active_planes)
In the next patch you are chaging the order of these parameters and calling with adl_qgv_bw(i915,
num_active_planes, i). As you are adding this functions in this patch, I think you should fix the
position of parameters in this patch itself and next patch call this normally

Also about the naming of this function, should this be icl_* as this is called from icl_* functions?
> +{
> +       unsigned int idx;
> +
> +       if (DISPLAY_VER(i915) > 11)
This is just fine.. but just for the sake of easy readability, wonder DISPLAY_VER(i915) >= 12 is
better as TGL is display version 12.

BR
vinod

> +               idx = tgl_max_bw_index(i915, num_active_planes, qgv_point);
> +       else
> +               idx = icl_max_bw_index(i915, num_active_planes, qgv_point);
> +
> +       if (idx >= ARRAY_SIZE(i915->display.bw.max))
> +               return 0;
> +
> +       return i915->display.bw.max[idx].deratedbw[qgv_point];
> +}
> +
>  void intel_bw_init_hw(struct drm_i915_private *dev_priv)
>  {
>         if (!HAS_DISPLAY(dev_priv))
> @@ -806,6 +822,36 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
>         return to_intel_bw_state(bw_state);
>  }
>  
> +static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
> +                                        int num_active_planes)
> +{
> +       unsigned int max_bw_point = 0;
> +       unsigned int max_bw = 0;
> +       unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
> +       int i;
> +
> +       for (i = 0; i < num_qgv_points; i++) {
> +               unsigned int max_data_rate;
> +
> +               max_data_rate = adl_qgv_bw(i915, i, num_active_planes);
> +
> +               /*
> +                * We need to know which qgv point gives us
> +                * maximum bandwidth in order to disable SAGV
> +                * if we find that we exceed SAGV block time
> +                * with watermarks. By that moment we already
> +                * have those, as it is calculated earlier in
> +                * intel_atomic_check,
> +                */
> +               if (max_data_rate > max_bw) {
> +                       max_bw_point = i;
> +                       max_bw = max_data_rate;
> +               }
> +       }
> +
> +       return max_bw_point;
> +}
> +
>  static int mtl_find_qgv_points(struct drm_i915_private *i915,
>                                unsigned int data_rate,
>                                unsigned int num_active_planes,
> @@ -883,8 +929,6 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
>                                const struct intel_bw_state *old_bw_state,
>                                struct intel_bw_state *new_bw_state)
>  {
> -       unsigned int max_bw_point = 0;
> -       unsigned int max_bw = 0;
>         unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
>         unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
>         u16 psf_points = 0;
> @@ -897,31 +941,10 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
>                 return ret;
>  
>         for (i = 0; i < num_qgv_points; i++) {
> -               unsigned int idx;
>                 unsigned int max_data_rate;
>  
> -               if (DISPLAY_VER(i915) >= 12)
> -                       idx = tgl_max_bw_index(i915, num_active_planes, i);
> -               else
> -                       idx = icl_max_bw_index(i915, num_active_planes, i);
> -
> -               if (idx >= ARRAY_SIZE(i915->display.bw.max))
> -                       continue;
> -
> -               max_data_rate = i915->display.bw.max[idx].deratedbw[i];
> +               max_data_rate = adl_qgv_bw(i915, i, num_active_planes);
>  
> -               /*
> -                * We need to know which qgv point gives us
> -                * maximum bandwidth in order to disable SAGV
> -                * if we find that we exceed SAGV block time
> -                * with watermarks. By that moment we already
> -                * have those, as it is calculated earlier in
> -                * intel_atomic_check,
> -                */
> -               if (max_data_rate > max_bw) {
> -                       max_bw_point = i;
> -                       max_bw = max_data_rate;
> -               }
>                 if (max_data_rate >= data_rate)
>                         qgv_points |= BIT(i);
>  
> @@ -965,9 +988,11 @@ static int icl_find_qgv_points(struct drm_i915_private *i915,
>          * cause.
>          */
>         if (!intel_can_enable_sagv(i915, new_bw_state)) {
> -               qgv_points = BIT(max_bw_point);
> +               unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, num_active_planes);
> +
> +               qgv_points = BIT(max_bw_qgv_point);
>                 drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
> -                           max_bw_point);
> +                           max_bw_qgv_point);
>         }
>  
>         /*