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 04:11:27PM +0000, Govindapillai, Vinod wrote:
> Hi Ville,
> 
> On Mon, 2024-03-25 at 17:03 +0200, Ville Syrjälä wrote:
> > 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.
> 
> Main thing is we need to match the comparison what pcode is doing.. right? 
> We compare the deratedbw of different QGV points calculated using the rest of the information
> provided as part of qgv info. I assume pcode is also going to do the same kind of comparison or that
> is what I understood from one of the email conversation.
> 
> Do you want this clarified from pcode team?

If pcode is only checking the bandwidth then it might be
technically broken as then we can't be 100% sure we can
actually disable sagv. The only way that can work is if
pcode then never ever switches between two qgv points
that have provide the same bandwidth.

> 
> BR
> vinod
> 
> > 
> > >                 }
> > >         }
> > >  
> > > -- 
> > > 2.34.1
> > 
> 

-- 
Ville Syrjälä
Intel