Re: [PATCH v8 4/4] drm/i915/display: handle systems with duplicate qgv/psf gv points

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Mar 25, 2024 at 03:01:56PM +0200, Vinod Govindapillai wrote:
> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> 
> There could be multiple qgv and psf gv points with similar values
> In case if we need to set one such QGV or psf gv  point where there
> could be duplicate entries, we would have to select all those
> points. Otherwise pcode might reject the GV configuration. We do
> handle this when we set appropriate qgv and psf gv as part of
> intel_bw_atomic_check calls. But during the bw_init force disable
> QGV points phase, we need to select all those points corresponding
> to the maximum bw as well.
> 
> v1: - use the same treatment to qgv points as well (Vinod)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 844d2d9efeb4..20c67474154e 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -847,6 +847,8 @@ static unsigned int icl_max_bw_qgv_point_mask(struct drm_i915_private *i915,
>  		if (max_data_rate > max_bw) {
>  			max_bw_point_mask = BIT(i);
>  			max_bw = max_data_rate;
> +		} else if (max_data_rate == max_bw) {
> +			max_bw_point_mask |= BIT(i);
>  		}
>  	}
>  
> @@ -866,6 +868,8 @@ static unsigned int icl_max_bw_psf_gv_point_mask(struct drm_i915_private *i915)
>  		if (max_data_rate > max_bw) {
>  			max_bw_point_mask = BIT(i);
>  			max_bw = max_data_rate;
> +		} else if (max_data_rate == max_bw) {
> +			max_bw_point_mask |= BIT(i);

This doesn't seem entirely safe. What happens if we somehow
have two qgv points with the same bandwidth but different
uderlying clock/gear ratio/etc.?

While such behaviour may not seem entirely sensible, given
that we need to do this stuff at all, I don't think we can
assume any kind of sensible behaviour from pcode here.

So I think we will need to check that the qgv points
being used here are in fact 100% identical.

>  		}
>  	}
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel