Re: [Intel-gfx] [PATCH v1 2/4] drm/i915: update the QGV point frequency calculations

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Tue, 2023-04-18 at 12:25 +0300, Lisovskiy, Stanislav wrote:
> On Sun, Apr 16, 2023 at 06:54:15PM +0300, Vinod Govindapillai wrote:
> > From MTL onwwards, pcode locks the QGV point based on peak BW of
> > the intended QGV point passed by the driver. So the peak BW
> > calculation must match the value expected by the pcode. Update
> > the calculations as per the Bspec.
> > 
> > Bspec: 64636
> > 
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index 5fa599b04ca5..57f8204162dd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -179,7 +179,7 @@ static int mtl_read_qgv_point_info(struct drm_i915_private *dev_priv,
> >         val2 = intel_uncore_read(&dev_priv->uncore,
> >                                  MTL_MEM_SS_INFO_QGV_POINT_HIGH(point));
> >         dclk = REG_FIELD_GET(MTL_DCLK_MASK, val);
> > -       sp->dclk = DIV_ROUND_UP((16667 * dclk), 1000);
> > +       sp->dclk = (16667 * dclk + 500) / 1000;
> 
> Hmm, wonder does it at least partly now intersects with what I'm doing in 
> https://patchwork.freedesktop.org/series/114982/
> 
> I remember we were discussing if this "+500" is actually also rounding up
> itself.
> 
> The thing is that the way how rounding up is done for instance in DIV_ROUND_UP
> also, if you check, if you lets say want to divide n by d, however you want to round
> up the result, you add n = n + (d - 1) and then divide by d. This is how DIV_ROUND_UP works.
> That effectively means that if n would be anything more than m*d, result would be not m,
> but m + 1(note flooring would give m)
> 
> Adding 500, when dividing by 1000 is also rouding up, however it is a bit weaker.
> In example above that would mean, if we want to divide n by d, we first add n = n + d / 2
> and then divide by d.
> That effectively means that if n would be anything more than m*d + 500, result would not m,
> but again m + 1(again note, that true flooeing would have given m, not m + 1)
> 
> So it is still rounding up, but just being weaker/less precise though.
> 
> If we would want to truly floor that division, we would want to get m, but not m + 1 from
> above examples, which means that we should just divide n / d, without adding anything.
> So in my opinion, if we want to floor (16667 * dclk / 1000) result - it should not have
> both "DIV_ROUND_UP" and " + 500" things - thats what I've done in series which also was touching
> this code as well.
> 
> I think it would be nice to raise issue and clarify from HW team, if it was initial intention,
> because adding + 500 is clearly doing rounding up as well, but it is just now on +-500(d/2)
> granularity now,
> while DIV_ROUND_UP worked with +-1 granularity. However both things are essentially "rounding up".
> So in that case I would really want to challenge or clarify, what is written in BSpec.
> 
> Stan

Yes. Not much explanation about the addition of 500. I just blindly followed what was in that Bspec.
Yes ideally div round_up is (n + d -1) / d. So what is the point of having 500 if the purpose is a
rounding up unless it is accounting for some "other" factor. Anyway it is nice to get the
clarification.

So the "other" factor I assume is that pcode is using this formula to calculate QGV point peak BW.
So in MTL as we pass this peak BW to pcode for compare and select the QGV point, driver and pcode
calculations need to match. 

BR
Vinod
> 
> >         sp->t_rp = REG_FIELD_GET(MTL_TRP_MASK, val);
> >         sp->t_rcd = REG_FIELD_GET(MTL_TRCD_MASK, val);
> >  
> > -- 
> > 2.34.1
> >