| Message ID | 1474578035-424-7-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/09/2016 22:00, Paulo Zanoni wrote: > The confusing thing is that plane_blocks_per_line is listed as part of > the method 2 calculation but is also used for other things. We > calculated it in two different places and different ways: one inside > skl_wm_method2() and the other inside skl_compute_plane_wm(). The > skl_wm_method2() implementation is the one that matches the > specification. > > With this patch we fix the skl_compute_plane_wm() calculation and just > pass it as a parameter to skl_wm_method2(). We also take care to not > modify the value of plane_bytes_per_line since we're going to rely on > it having a correct value in later patches. > > This should affect the watermarks for Linear and Y-tiled. > > From my analysis, it looks like the two plane_blocks_per_line > variables got out of sync on 0fda65680e92, but we can't really say > that commit was a regression, it looks like just an incomplete fix. > There's always the possibility that 0fda65680e92 matched our > specification at that time, and then later the specification changed. Yes, it wouldn't be a regression since before that commit changing tiling with page flips did not work at all. Whether or not calculation was fully correct at the time I am not sure. The formulas changed at least twice after the initial implementation so both are possible. Regards, Tvrtko > v2: Try to add a "Fixes" tag (Maarten). > > Fixes: 0fda65680e92 ("drm/i915/skl: Update watermarks for Y tiling") > Cc: stable@vger.kernel.org > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Reviewed-by: Lyude <cpaul@redhat.com> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++------------------------ > 1 file changed, 15 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index a7f5f7f..a6ae7b7 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3494,30 +3494,14 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp, uint32_t latenc > } > > static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal, > - uint32_t horiz_pixels, uint8_t cpp, > - uint64_t tiling, uint32_t latency, > - uint32_t y_min_scanlines) > + uint32_t latency, uint32_t plane_blocks_per_line) > { > uint32_t ret; > - uint32_t plane_bytes_per_line, plane_blocks_per_line; > uint32_t wm_intermediate_val; > > if (latency == 0) > return UINT_MAX; > > - plane_bytes_per_line = horiz_pixels * cpp; > - > - if (tiling == I915_FORMAT_MOD_Y_TILED || > - tiling == I915_FORMAT_MOD_Yf_TILED) { > - plane_bytes_per_line *= y_min_scanlines; > - plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512); > - plane_blocks_per_line /= y_min_scanlines; > - } else if (tiling == DRM_FORMAT_MOD_NONE) { > - plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512) + 1; > - } else { > - plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512); > - } > - > wm_intermediate_val = latency * pixel_rate; > ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) * > plane_blocks_per_line; > @@ -3606,17 +3590,24 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > y_min_scanlines = 4; > } > > + plane_bytes_per_line = width * cpp; > + if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || > + fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) { > + plane_blocks_per_line = > + DIV_ROUND_UP(plane_bytes_per_line * y_min_scanlines, 512); > + plane_blocks_per_line /= y_min_scanlines; > + } else if (fb->modifier[0] == DRM_FORMAT_MOD_NONE) { > + plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512) > + + 1; > + } else { > + plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512); > + } > + > method1 = skl_wm_method1(plane_pixel_rate, cpp, latency); > method2 = skl_wm_method2(plane_pixel_rate, > cstate->base.adjusted_mode.crtc_htotal, > - width, > - cpp, > - fb->modifier[0], > latency, > - y_min_scanlines); > - > - plane_bytes_per_line = width * cpp; > - plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512); > + plane_blocks_per_line); > > if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || > fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a7f5f7f..a6ae7b7 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3494,30 +3494,14 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp, uint32_t latenc } static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal, - uint32_t horiz_pixels, uint8_t cpp, - uint64_t tiling, uint32_t latency, - uint32_t y_min_scanlines) + uint32_t latency, uint32_t plane_blocks_per_line) { uint32_t ret; - uint32_t plane_bytes_per_line, plane_blocks_per_line; uint32_t wm_intermediate_val; if (latency == 0) return UINT_MAX; - plane_bytes_per_line = horiz_pixels * cpp; - - if (tiling == I915_FORMAT_MOD_Y_TILED || - tiling == I915_FORMAT_MOD_Yf_TILED) { - plane_bytes_per_line *= y_min_scanlines; - plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512); - plane_blocks_per_line /= y_min_scanlines; - } else if (tiling == DRM_FORMAT_MOD_NONE) { - plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512) + 1; - } else { - plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512); - } - wm_intermediate_val = latency * pixel_rate; ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) * plane_blocks_per_line; @@ -3606,17 +3590,24 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, y_min_scanlines = 4; } + plane_bytes_per_line = width * cpp; + if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || + fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) { + plane_blocks_per_line = + DIV_ROUND_UP(plane_bytes_per_line * y_min_scanlines, 512); + plane_blocks_per_line /= y_min_scanlines; + } else if (fb->modifier[0] == DRM_FORMAT_MOD_NONE) { + plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512) + + 1; + } else { + plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512); + } + method1 = skl_wm_method1(plane_pixel_rate, cpp, latency); method2 = skl_wm_method2(plane_pixel_rate, cstate->base.adjusted_mode.crtc_htotal, - width, - cpp, - fb->modifier[0], latency, - y_min_scanlines); - - plane_bytes_per_line = width * cpp; - plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512); + plane_blocks_per_line); if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
The confusing thing is that plane_blocks_per_line is listed as part of the method 2 calculation but is also used for other things. We calculated it in two different places and different ways: one inside skl_wm_method2() and the other inside skl_compute_plane_wm(). The skl_wm_method2() implementation is the one that matches the specification. With this patch we fix the skl_compute_plane_wm() calculation and just pass it as a parameter to skl_wm_method2(). We also take care to not modify the value of plane_bytes_per_line since we're going to rely on it having a correct value in later patches. This should affect the watermarks for Linear and Y-tiled. From my analysis, it looks like the two plane_blocks_per_line variables got out of sync on 0fda65680e92, but we can't really say that commit was a regression, it looks like just an incomplete fix. There's always the possibility that 0fda65680e92 matched our specification at that time, and then later the specification changed. v2: Try to add a "Fixes" tag (Maarten). Fixes: 0fda65680e92 ("drm/i915/skl: Update watermarks for Y tiling") Cc: stable@vger.kernel.org Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Lyude <cpaul@redhat.com> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-)