| Message ID | 20240325130156.205726-5-vinod.govindapillai@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QGV/SAGV related fixes | expand |
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. > } > } > > -- > 2.34.1
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? BR vinod > > > } > > } > > > > -- > > 2.34.1 >
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 > > >
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); } }