| Message ID | 20240220093136.32181-3-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QGV/SAGV related fixes | expand |
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); > } > > /*
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); } /*
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(-)