| Message ID | 20230416155417.174418-3-vinod.govindapillai@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prepare for MTL sagv config patches | expand |
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 > sp->t_rp = REG_FIELD_GET(MTL_TRP_MASK, val); > sp->t_rcd = REG_FIELD_GET(MTL_TRCD_MASK, val); > > -- > 2.34.1 >
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 > >
On Tue, Apr 18, 2023 at 12:47:33PM +0300, Govindapillai, Vinod wrote: > 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. Adding 500 here, essnetially means we get +1 to sp->dclk, whenever dclk has some reminder of that division which is >= 500. So kinda unclear: we are being instructed now to "floor" "rounded up" calculations! :)) So we are now doing something between flooring and DIV_ROUND_UP for sp->dclk * 16667 / 1000. What makes me think that there is some contradiction, or might be there is some hardware factor, which works with 500 granularity, so that it makes sense to round up only if delta is >= 500, otherwise floor. Stan > > 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 > > > >
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; sp->t_rp = REG_FIELD_GET(MTL_TRP_MASK, val); sp->t_rcd = REG_FIELD_GET(MTL_TRCD_MASK, val);
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(-)