| Message ID | 20240405113533.338553-3-vinod.govindapillai@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QGV/SAGV related fixes | expand |
On Fri, Apr 05, 2024 at 02:35:29PM +0300, Vinod Govindapillai wrote: > From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > 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ä) > v3: - Update icl_max_bw_qgv_point_mask to return max qgv point > mask (Vinod) > v4: - Minor changes in icl_find_qgv_points (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 | 80 +++++++++++++++---------- > 1 file changed, 50 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c > index 77886cc21211..c00094e5f11c 100644 > --- a/drivers/gpu/drm/i915/display/intel_bw.c > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > @@ -661,6 +661,22 @@ static unsigned int adl_psf_bw(struct drm_i915_private *dev_priv, > return bi->psf_bw[psf_gv_point]; > } > > +static unsigned int icl_qgv_bw(struct drm_i915_private *i915, > + int num_active_planes, int qgv_point) > +{ > + unsigned int idx; > + > + if (DISPLAY_VER(i915) >= 12) > + 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,35 @@ 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_mask(struct drm_i915_private *i915, > + int num_active_planes) > +{ > + unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points; > + unsigned int max_bw_point_mask = 0; > + unsigned int max_bw = 0; > + int i; > + > + for (i = 0; i < num_qgv_points; i++) { > + unsigned int max_data_rate = > + icl_qgv_bw(i915, num_active_planes, i); > + > + /* > + * 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_mask = BIT(i); > + max_bw = max_data_rate; > + } > + } > + > + return max_bw_point_mask; > +} > + Wondering, why we just don't call it "max_bw_point", of course "mask" could be applied to single point as well, however in most cases it still kind of implies that there are few of those, however we always find a single one here. Stan > static int mtl_find_qgv_points(struct drm_i915_private *i915, > unsigned int data_rate, > unsigned int num_active_planes, > @@ -883,8 +928,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 +940,8 @@ 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]; > - > - /* > - * 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; > - } > + unsigned int max_data_rate = icl_qgv_bw(i915, > + num_active_planes, i); > if (max_data_rate >= data_rate) > qgv_points |= BIT(i); > > @@ -965,9 +985,9 @@ 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); > - drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n", > - max_bw_point); > + qgv_points = icl_max_bw_qgv_point_mask(i915, num_active_planes); > + drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point mask 0x%x\n", > + qgv_points); > } > > /* > -- > 2.34.1 >
On Mon, 2024-04-08 at 11:32 +0300, Lisovskiy, Stanislav wrote: > On Fri, Apr 05, 2024 at 02:35:29PM +0300, Vinod Govindapillai wrote: > > From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > 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ä) > > v3: - Update icl_max_bw_qgv_point_mask to return max qgv point > > mask (Vinod) > > v4: - Minor changes in icl_find_qgv_points (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 | 80 +++++++++++++++---------- > > 1 file changed, 50 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c > > index 77886cc21211..c00094e5f11c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_bw.c > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > > @@ -661,6 +661,22 @@ static unsigned int adl_psf_bw(struct drm_i915_private *dev_priv, > > return bi->psf_bw[psf_gv_point]; > > } > > > > +static unsigned int icl_qgv_bw(struct drm_i915_private *i915, > > + int num_active_planes, int qgv_point) > > +{ > > + unsigned int idx; > > + > > + if (DISPLAY_VER(i915) >= 12) > > + 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,35 @@ 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_mask(struct drm_i915_private *i915, > > + int num_active_planes) > > +{ > > + unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points; > > + unsigned int max_bw_point_mask = 0; > > + unsigned int max_bw = 0; > > + int i; > > + > > + for (i = 0; i < num_qgv_points; i++) { > > + unsigned int max_data_rate = > > + icl_qgv_bw(i915, num_active_planes, i); > > + > > + /* > > + * 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_mask = BIT(i); > > + max_bw = max_data_rate; > > + } > > + } > > + > > + return max_bw_point_mask; > > +} > > + > > Wondering, why we just don't call it "max_bw_point", of course "mask" > could be applied to single point as well, however in most cases it still > kind of implies that there are few of those, however we always find > a single one here. Okay.. For QGV max point, indeed we are finding point with max bw. So the name can be changed. I think it can be changed before merging instead of floating another version - well if we these patches can be RB-ed Vinod > > Stan > > > static int mtl_find_qgv_points(struct drm_i915_private *i915, > > unsigned int data_rate, > > unsigned int num_active_planes, > > @@ -883,8 +928,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 +940,8 @@ 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]; > > - > > - /* > > - * 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; > > - } > > + unsigned int max_data_rate = icl_qgv_bw(i915, > > + num_active_planes, i); > > if (max_data_rate >= data_rate) > > qgv_points |= BIT(i); > > > > @@ -965,9 +985,9 @@ 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); > > - drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n", > > - max_bw_point); > > + qgv_points = icl_max_bw_qgv_point_mask(i915, num_active_planes); > > + drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point mask 0x%x\n", > > + qgv_points); > > } > > > > /* > > -- > > 2.34.1 > >
diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c index 77886cc21211..c00094e5f11c 100644 --- a/drivers/gpu/drm/i915/display/intel_bw.c +++ b/drivers/gpu/drm/i915/display/intel_bw.c @@ -661,6 +661,22 @@ static unsigned int adl_psf_bw(struct drm_i915_private *dev_priv, return bi->psf_bw[psf_gv_point]; } +static unsigned int icl_qgv_bw(struct drm_i915_private *i915, + int num_active_planes, int qgv_point) +{ + unsigned int idx; + + if (DISPLAY_VER(i915) >= 12) + 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,35 @@ 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_mask(struct drm_i915_private *i915, + int num_active_planes) +{ + unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points; + unsigned int max_bw_point_mask = 0; + unsigned int max_bw = 0; + int i; + + for (i = 0; i < num_qgv_points; i++) { + unsigned int max_data_rate = + icl_qgv_bw(i915, num_active_planes, i); + + /* + * 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_mask = BIT(i); + max_bw = max_data_rate; + } + } + + return max_bw_point_mask; +} + static int mtl_find_qgv_points(struct drm_i915_private *i915, unsigned int data_rate, unsigned int num_active_planes, @@ -883,8 +928,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 +940,8 @@ 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]; - - /* - * 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; - } + unsigned int max_data_rate = icl_qgv_bw(i915, + num_active_planes, i); if (max_data_rate >= data_rate) qgv_points |= BIT(i); @@ -965,9 +985,9 @@ 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); - drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n", - max_bw_point); + qgv_points = icl_max_bw_qgv_point_mask(i915, num_active_planes); + drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point mask 0x%x\n", + qgv_points); } /*