| Message ID | 20161013105826.9710-2-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Qui, 2016-10-13 às 16:28 +0530, Kumar, Mahesh escreveu: > This patch make changes to use linetime latency instead of allocated > DDB size during plane watermark calculation in switch case, This is > required to implement new DDB allocation algorithm. > > In New Algorithm DDB is allocated based on WM values, because of > which > number of DDB blocks will not be available during WM calculation, > So this "linetime latency" is suggested by SV/HW team to use during > switch-case for WM blocks selection. > > Changes since v1: > - Rebase on top of Paulo's patch series > Changes since v2: > - Fix if-else condition (pointed by Maarten) > > Signed-off-by: "Kumar, Mahesh" <mahesh1.kumar@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index 7f1748a..098336d 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3616,10 +3616,14 @@ static int skl_compute_plane_wm(const struct > drm_i915_private *dev_priv, > fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) { > selected_result = max(method2, y_tile_minimum); > } else { > + uint32_t linetime_us = 0; > + > + linetime_us = DIV_ROUND_UP(width * 1000, > + skl_pipe_pixel_rate(cstate)); Can't we just call skl_compute_linetime_wm() here? I don't like having two pieces of the code computing the same thing. My last round of bug fixes included a fix for duplicated code that got out of sync after spec changes. > if ((cpp * cstate->base.adjusted_mode.crtc_htotal / > 512 < 1) && > (plane_bytes_per_line / 512 < 1)) > selected_result = method2; > - else if ((ddb_allocation / plane_blocks_per_line) >= > 1) > + else if (latency >= linetime_us) Still doesn't match the spec. The "ddb_allocation / planes_block_per_line" is still necessary. > selected_result = min(method1, method2); > else > selected_result = method1;
Hi, On Monday 31 October 2016 11:33 PM, Paulo Zanoni wrote: > Em Qui, 2016-10-13 às 16:28 +0530, Kumar, Mahesh escreveu: >> This patch make changes to use linetime latency instead of allocated >> DDB size during plane watermark calculation in switch case, This is >> required to implement new DDB allocation algorithm. >> >> In New Algorithm DDB is allocated based on WM values, because of >> which >> number of DDB blocks will not be available during WM calculation, >> So this "linetime latency" is suggested by SV/HW team to use during >> switch-case for WM blocks selection. >> >> Changes since v1: >> - Rebase on top of Paulo's patch series >> Changes since v2: >> - Fix if-else condition (pointed by Maarten) >> >> Signed-off-by: "Kumar, Mahesh" <mahesh1.kumar@intel.com> >> --- >> drivers/gpu/drm/i915/intel_pm.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c >> b/drivers/gpu/drm/i915/intel_pm.c >> index 7f1748a..098336d 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -3616,10 +3616,14 @@ static int skl_compute_plane_wm(const struct >> drm_i915_private *dev_priv, >> fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) { >> selected_result = max(method2, y_tile_minimum); >> } else { >> + uint32_t linetime_us = 0; >> + >> + linetime_us = DIV_ROUND_UP(width * 1000, >> + skl_pipe_pixel_rate(cstate)); > Can't we just call skl_compute_linetime_wm() here? I don't like having > two pieces of the code computing the same thing. My last round of bug > fixes included a fix for duplicated code that got out of sync after > spec changes. These two have different values in skl_compute_linetime_wm we multiply by 8, not here. > >> if ((cpp * cstate->base.adjusted_mode.crtc_htotal / >> 512 < 1) && >> (plane_bytes_per_line / 512 < 1)) >> selected_result = method2; >> - else if ((ddb_allocation / plane_blocks_per_line) >= >> 1) >> + else if (latency >= linetime_us) > Still doesn't match the spec. The "ddb_allocation / > planes_block_per_line" is still necessary. After New algorithm patch, we will not have access to ddb_allocation, as allocation will happen later. So we can't use ddb_allocation in our calculation, This check in Bspec is kept for the environment/OS where we don't allocate DDB dynamically. hence those OS will have access to ddb_allocation during WM calculation phase. thanks, Regards, -Mahesh > >> selected_result = min(method1, method2); >> else >> selected_result = method1;
Em Qua, 2016-11-09 às 20:28 +0530, Mahesh Kumar escreveu: > Hi, > > > On Monday 31 October 2016 11:33 PM, Paulo Zanoni wrote: > > > > Em Qui, 2016-10-13 às 16:28 +0530, Kumar, Mahesh escreveu: > > > > > > This patch make changes to use linetime latency instead of > > > allocated > > > DDB size during plane watermark calculation in switch case, This > > > is > > > required to implement new DDB allocation algorithm. > > > > > > In New Algorithm DDB is allocated based on WM values, because of > > > which > > > number of DDB blocks will not be available during WM calculation, > > > So this "linetime latency" is suggested by SV/HW team to use > > > during > > > switch-case for WM blocks selection. > > > > > > Changes since v1: > > > - Rebase on top of Paulo's patch series > > > Changes since v2: > > > - Fix if-else condition (pointed by Maarten) > > > > > > Signed-off-by: "Kumar, Mahesh" <mahesh1.kumar@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > > b/drivers/gpu/drm/i915/intel_pm.c > > > index 7f1748a..098336d 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3616,10 +3616,14 @@ static int skl_compute_plane_wm(const > > > struct > > > drm_i915_private *dev_priv, > > > fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) { > > > selected_result = max(method2, y_tile_minimum); > > > } else { > > > + uint32_t linetime_us = 0; > > > + > > > + linetime_us = DIV_ROUND_UP(width * 1000, > > > + skl_pipe_pixel_rate(cstate)); > > Can't we just call skl_compute_linetime_wm() here? I don't like > > having > > two pieces of the code computing the same thing. My last round of > > bug > > fixes included a fix for duplicated code that got out of sync after > > spec changes. > These two have different values in skl_compute_linetime_wm we > multiply > by 8, not here. You can move the *8 multiplication to the caller that needs it. > > > > > > > > > > if ((cpp * cstate- > > > >base.adjusted_mode.crtc_htotal / > > > 512 < 1) && > > > (plane_bytes_per_line / 512 < 1)) > > > selected_result = method2; > > > - else if ((ddb_allocation / > > > plane_blocks_per_line) >= > > > 1) > > > + else if (latency >= linetime_us) > > Still doesn't match the spec. The "ddb_allocation / > > planes_block_per_line" is still necessary. > After New algorithm patch, we will not have access to ddb_allocation, > as > allocation will happen later. > So we can't use ddb_allocation in our calculation, This check in > Bspec > is kept for the environment/OS where we don't allocate DDB > dynamically. > hence those OS will have access to ddb_allocation during WM > calculation > phase. > thanks, So the patch that implements the DDB allocation can remove the check when/if it gets merged. The code with only this patch does not use the new algorithm, so let's make everything works if we only have it applied. Bisectability is really important. > > Regards, > -Mahesh > > > > > > > > > > selected_result = min(method1, > > > method2); > > > else > > > selected_result = method1; >
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 7f1748a..098336d 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3616,10 +3616,14 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) { selected_result = max(method2, y_tile_minimum); } else { + uint32_t linetime_us = 0; + + linetime_us = DIV_ROUND_UP(width * 1000, + skl_pipe_pixel_rate(cstate)); if ((cpp * cstate->base.adjusted_mode.crtc_htotal / 512 < 1) && (plane_bytes_per_line / 512 < 1)) selected_result = method2; - else if ((ddb_allocation / plane_blocks_per_line) >= 1) + else if (latency >= linetime_us) selected_result = min(method1, method2); else selected_result = method1;
This patch make changes to use linetime latency instead of allocated DDB size during plane watermark calculation in switch case, This is required to implement new DDB allocation algorithm. In New Algorithm DDB is allocated based on WM values, because of which number of DDB blocks will not be available during WM calculation, So this "linetime latency" is suggested by SV/HW team to use during switch-case for WM blocks selection. Changes since v1: - Rebase on top of Paulo's patch series Changes since v2: - Fix if-else condition (pointed by Maarten) Signed-off-by: "Kumar, Mahesh" <mahesh1.kumar@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)