| Message ID | 20200220152347.2530-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] drm/i915: Use intel_plane_data_rate for min_cdclk calculation | expand |
On Thu, Feb 20, 2020 at 05:23:47PM +0200, Stanislav Lisovskiy wrote: > There seems to be a bit of confusing redundancy in a way, how > plane data rate/min cdclk are calculated. > In fact both min cdclk, pixel rate and plane data rate are all > part of the same formula as per BSpec. > > However currently we have intel_plane_data_rate, which is used > to calculate plane data rate and which is also used in bandwidth > calculations. However for calculating min_cdclk we have another > piece of code, doing almost same calculation, but a bit differently > and in a different place. However as both are actually part of same > formula, probably would be wise to use plane data rate calculations > as a basis anyway, thus avoiding code duplication and possible bugs > related to this. > > Another thing is that I've noticed that during min_cdclk calculations > we account for plane scaling, while for plane data rate, we don't. > crtc->pixel_rate seems to account only for pipe ratio, however it is > clearly stated in BSpec that plane data rate also need to account > plane ratio as well. > > So what this commit does is: > - Adds a plane ratio calculation to intel_plane_data_rate > - Removes redundant calculations from skl_plane_min_cdclk which is > used for gen9+ and now uses intel_plane_data_rate as a basis from > there as well. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > .../gpu/drm/i915/display/intel_atomic_plane.c | 16 ++++++- > drivers/gpu/drm/i915/display/intel_sprite.c | 46 +++++++++++-------- > 2 files changed, 41 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index c86d7a35c816..702dfa14d112 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -133,15 +133,27 @@ intel_plane_destroy_state(struct drm_plane *plane, > kfree(plane_state); > } > > + > + > unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, > const struct intel_plane_state *plane_state) > { > const struct drm_framebuffer *fb = plane_state->hw.fb; > unsigned int cpp; > + unsigned int src_w, src_h, dst_w, dst_h; > > if (!plane_state->uapi.visible) > return 0; > > + src_w = drm_rect_width(&plane_state->uapi.src) >> 16; > + src_h = drm_rect_height(&plane_state->uapi.src) >> 16; > + dst_w = drm_rect_width(&plane_state->uapi.dst); > + dst_h = drm_rect_height(&plane_state->uapi.dst); > + > + /* Downscaling limits the maximum pixel rate */ > + dst_w = min(src_w, dst_w); > + dst_h = min(src_h, dst_h); > + > cpp = fb->format->cpp[0]; > > /* > @@ -153,7 +165,9 @@ unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, > if (fb->format->is_yuv && fb->format->num_planes > 1) > cpp *= 4; > > - return cpp * crtc_state->pixel_rate; > + return DIV64_U64_ROUND_UP(mul_u32_u32(cpp * crtc_state->pixel_rate, > + src_w * src_h), > + mul_u32_u32(dst_w, dst_h)); You don't need a 64bit divisor for this. > } > > int intel_plane_calc_min_cdclk(struct intel_atomic_state *state, > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c > index 7abeefe8dce5..75afb78ff1b0 100644 > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > @@ -330,24 +330,34 @@ bool icl_is_hdr_plane(struct drm_i915_private *dev_priv, enum plane_id plane_id) > } > > static void > -skl_plane_ratio(const struct intel_crtc_state *crtc_state, > - const struct intel_plane_state *plane_state, > - unsigned int *num, unsigned int *den) > +skl_plane_bpp_constraints(const struct intel_crtc_state *crtc_state, > + const struct intel_plane_state *plane_state, > + unsigned int *num, unsigned int *den) > { > struct drm_i915_private *dev_priv = to_i915(plane_state->uapi.plane->dev); > const struct drm_framebuffer *fb = plane_state->hw.fb; > + unsigned int cpp = fb->format->cpp[0]; > + > + /* > + * Based on HSD#:1408715493 > + * NV12 cpp == 4, P010 cpp == 8 > + * > + * FIXME what is the logic behind this? > + */ > + if (fb->format->is_yuv && fb->format->num_planes > 1) > + cpp *= 4; This is ugly. I think we need a plane pixel rate instead of abusing the data rate as the pixel rate like this. > > if (fb->format->cpp[0] == 8) { > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { > *num = 10; > - *den = 8; > + *den = 8 * cpp; > } else { > *num = 9; > - *den = 8; > + *den = 8 * cpp; > } > } else { > *num = 1; > - *den = 1; > + *den = cpp; > } > } > > @@ -355,27 +365,23 @@ static int skl_plane_min_cdclk(const struct intel_crtc_state *crtc_state, > const struct intel_plane_state *plane_state) > { > struct drm_i915_private *dev_priv = to_i915(plane_state->uapi.plane->dev); > - unsigned int pixel_rate = crtc_state->pixel_rate; > - unsigned int src_w, src_h, dst_w, dst_h; > unsigned int num, den; > + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); > > - skl_plane_ratio(crtc_state, plane_state, &num, &den); > + skl_plane_bpp_constraints(crtc_state, plane_state, &num, &den); > > /* two pixels per clock on glk+ */ > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > den *= 2; > > - src_w = drm_rect_width(&plane_state->uapi.src) >> 16; > - src_h = drm_rect_height(&plane_state->uapi.src) >> 16; > - dst_w = drm_rect_width(&plane_state->uapi.dst); > - dst_h = drm_rect_height(&plane_state->uapi.dst); > - > - /* Downscaling limits the maximum pixel rate */ > - dst_w = min(src_w, dst_w); > - dst_h = min(src_h, dst_h); > - > - return DIV64_U64_ROUND_UP(mul_u32_u32(pixel_rate * num, src_w * src_h), > - mul_u32_u32(den, dst_w * dst_h)); > + /* > + * intel_atomic_check_planes has already been called by this > + * time in intel_atomic_check, so use calculated plane > + * data rate as a basis, in order not to have duplicate code. > + * According to BSpec, plane data rate is anyway used as > + * a basis for this calculation. > + */ > + return DIV64_U64_ROUND_UP(crtc_state->data_rate[plane->id] * num, den); > } > > static unsigned int > -- > 2.24.1.485.gad05a3d8e5
On Thu, 2020-02-20 at 17:43 +0200, Ville Syrjälä wrote: > On Thu, Feb 20, 2020 at 05:23:47PM +0200, Stanislav Lisovskiy wrote: > > There seems to be a bit of confusing redundancy in a way, how > > plane data rate/min cdclk are calculated. > > In fact both min cdclk, pixel rate and plane data rate are all > > part of the same formula as per BSpec. > > > > However currently we have intel_plane_data_rate, which is used > > to calculate plane data rate and which is also used in bandwidth > > calculations. However for calculating min_cdclk we have another > > piece of code, doing almost same calculation, but a bit differently > > and in a different place. However as both are actually part of same > > formula, probably would be wise to use plane data rate calculations > > as a basis anyway, thus avoiding code duplication and possible bugs > > related to this. > > > > Another thing is that I've noticed that during min_cdclk > > calculations > > we account for plane scaling, while for plane data rate, we don't. > > crtc->pixel_rate seems to account only for pipe ratio, however it > > is > > clearly stated in BSpec that plane data rate also need to account > > plane ratio as well. > > > > So what this commit does is: > > - Adds a plane ratio calculation to intel_plane_data_rate > > - Removes redundant calculations from skl_plane_min_cdclk which is > > used for gen9+ and now uses intel_plane_data_rate as a basis from > > there as well. > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > .../gpu/drm/i915/display/intel_atomic_plane.c | 16 ++++++- > > drivers/gpu/drm/i915/display/intel_sprite.c | 46 +++++++++++-- > > ------ > > 2 files changed, 41 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > index c86d7a35c816..702dfa14d112 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > @@ -133,15 +133,27 @@ intel_plane_destroy_state(struct drm_plane > > *plane, > > kfree(plane_state); > > } > > > > + > > + > > unsigned int intel_plane_data_rate(const struct intel_crtc_state > > *crtc_state, > > const struct intel_plane_state > > *plane_state) > > { > > const struct drm_framebuffer *fb = plane_state->hw.fb; > > unsigned int cpp; > > + unsigned int src_w, src_h, dst_w, dst_h; > > > > if (!plane_state->uapi.visible) > > return 0; > > > > + src_w = drm_rect_width(&plane_state->uapi.src) >> 16; > > + src_h = drm_rect_height(&plane_state->uapi.src) >> 16; > > + dst_w = drm_rect_width(&plane_state->uapi.dst); > > + dst_h = drm_rect_height(&plane_state->uapi.dst); > > + > > + /* Downscaling limits the maximum pixel rate */ > > + dst_w = min(src_w, dst_w); > > + dst_h = min(src_h, dst_h); > > + > > cpp = fb->format->cpp[0]; > > > > /* > > @@ -153,7 +165,9 @@ unsigned int intel_plane_data_rate(const struct > > intel_crtc_state *crtc_state, > > if (fb->format->is_yuv && fb->format->num_planes > 1) > > cpp *= 4; > > > > - return cpp * crtc_state->pixel_rate; > > + return DIV64_U64_ROUND_UP(mul_u32_u32(cpp * crtc_state- > > >pixel_rate, > > + src_w * src_h), > > + mul_u32_u32(dst_w, dst_h)); > > You don't need a 64bit divisor for this. > > > } > > > > int intel_plane_calc_min_cdclk(struct intel_atomic_state *state, > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c > > b/drivers/gpu/drm/i915/display/intel_sprite.c > > index 7abeefe8dce5..75afb78ff1b0 100644 > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > > @@ -330,24 +330,34 @@ bool icl_is_hdr_plane(struct drm_i915_private > > *dev_priv, enum plane_id plane_id) > > } > > > > static void > > -skl_plane_ratio(const struct intel_crtc_state *crtc_state, > > - const struct intel_plane_state *plane_state, > > - unsigned int *num, unsigned int *den) > > +skl_plane_bpp_constraints(const struct intel_crtc_state > > *crtc_state, > > + const struct intel_plane_state *plane_state, > > + unsigned int *num, unsigned int *den) > > { > > struct drm_i915_private *dev_priv = to_i915(plane_state- > > >uapi.plane->dev); > > const struct drm_framebuffer *fb = plane_state->hw.fb; > > + unsigned int cpp = fb->format->cpp[0]; > > + > > + /* > > + * Based on HSD#:1408715493 > > + * NV12 cpp == 4, P010 cpp == 8 > > + * > > + * FIXME what is the logic behind this? > > + */ > > + if (fb->format->is_yuv && fb->format->num_planes > 1) > > + cpp *= 4; > > This is ugly. I think we need a plane pixel rate instead of > abusing the data rate as the pixel rate like this. Yeah, agree, but that is all because of this HSD#-something workaround, which is preventing to use plane_data_rate in a formula "as is". Was almost sure that this part is questionable. Probably the best way would be to extract some function like you say to calculate per-plane pixel rate, which would be used in both intel_plane_data_rate and skl_calc_min_cdclk. The thing what I just aim to eliminate is a duplication on this matter. I mean if we really calculate it as per BSpec it should be Plane data rate = Pixel rate * bpp * plane scale ratio * pipe scale ratio For recent Gens Pixel rate = Min CDCLK / 2, so all of those are obviously part of same formula, same should be in the code. Stan > > > > > if (fb->format->cpp[0] == 8) { > > if (INTEL_GEN(dev_priv) >= 10 || > > IS_GEMINILAKE(dev_priv)) { > > *num = 10; > > - *den = 8; > > + *den = 8 * cpp; > > } else { > > *num = 9; > > - *den = 8; > > + *den = 8 * cpp; > > } > > } else { > > *num = 1; > > - *den = 1; > > + *den = cpp; > > } > > } > > > > @@ -355,27 +365,23 @@ static int skl_plane_min_cdclk(const struct > > intel_crtc_state *crtc_state, > > const struct intel_plane_state > > *plane_state) > > { > > struct drm_i915_private *dev_priv = to_i915(plane_state- > > >uapi.plane->dev); > > - unsigned int pixel_rate = crtc_state->pixel_rate; > > - unsigned int src_w, src_h, dst_w, dst_h; > > unsigned int num, den; > > + struct intel_plane *plane = to_intel_plane(plane_state- > > >uapi.plane); > > > > - skl_plane_ratio(crtc_state, plane_state, &num, &den); > > + skl_plane_bpp_constraints(crtc_state, plane_state, &num, &den); > > > > /* two pixels per clock on glk+ */ > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > den *= 2; > > > > - src_w = drm_rect_width(&plane_state->uapi.src) >> 16; > > - src_h = drm_rect_height(&plane_state->uapi.src) >> 16; > > - dst_w = drm_rect_width(&plane_state->uapi.dst); > > - dst_h = drm_rect_height(&plane_state->uapi.dst); > > - > > - /* Downscaling limits the maximum pixel rate */ > > - dst_w = min(src_w, dst_w); > > - dst_h = min(src_h, dst_h); > > - > > - return DIV64_U64_ROUND_UP(mul_u32_u32(pixel_rate * num, src_w * > > src_h), > > - mul_u32_u32(den, dst_w * dst_h)); > > + /* > > + * intel_atomic_check_planes has already been called by this > > + * time in intel_atomic_check, so use calculated plane > > + * data rate as a basis, in order not to have duplicate code. > > + * According to BSpec, plane data rate is anyway used as > > + * a basis for this calculation. > > + */ > > + return DIV64_U64_ROUND_UP(crtc_state->data_rate[plane->id] * > > num, den); > > } > > > > static unsigned int > > -- > > 2.24.1.485.gad05a3d8e5 > >
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index c86d7a35c816..702dfa14d112 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -133,15 +133,27 @@ intel_plane_destroy_state(struct drm_plane *plane, kfree(plane_state); } + + unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state) { const struct drm_framebuffer *fb = plane_state->hw.fb; unsigned int cpp; + unsigned int src_w, src_h, dst_w, dst_h; if (!plane_state->uapi.visible) return 0; + src_w = drm_rect_width(&plane_state->uapi.src) >> 16; + src_h = drm_rect_height(&plane_state->uapi.src) >> 16; + dst_w = drm_rect_width(&plane_state->uapi.dst); + dst_h = drm_rect_height(&plane_state->uapi.dst); + + /* Downscaling limits the maximum pixel rate */ + dst_w = min(src_w, dst_w); + dst_h = min(src_h, dst_h); + cpp = fb->format->cpp[0]; /* @@ -153,7 +165,9 @@ unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, if (fb->format->is_yuv && fb->format->num_planes > 1) cpp *= 4; - return cpp * crtc_state->pixel_rate; + return DIV64_U64_ROUND_UP(mul_u32_u32(cpp * crtc_state->pixel_rate, + src_w * src_h), + mul_u32_u32(dst_w, dst_h)); } int intel_plane_calc_min_cdclk(struct intel_atomic_state *state, diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c index 7abeefe8dce5..75afb78ff1b0 100644 --- a/drivers/gpu/drm/i915/display/intel_sprite.c +++ b/drivers/gpu/drm/i915/display/intel_sprite.c @@ -330,24 +330,34 @@ bool icl_is_hdr_plane(struct drm_i915_private *dev_priv, enum plane_id plane_id) } static void -skl_plane_ratio(const struct intel_crtc_state *crtc_state, - const struct intel_plane_state *plane_state, - unsigned int *num, unsigned int *den) +skl_plane_bpp_constraints(const struct intel_crtc_state *crtc_state, + const struct intel_plane_state *plane_state, + unsigned int *num, unsigned int *den) { struct drm_i915_private *dev_priv = to_i915(plane_state->uapi.plane->dev); const struct drm_framebuffer *fb = plane_state->hw.fb; + unsigned int cpp = fb->format->cpp[0]; + + /* + * Based on HSD#:1408715493 + * NV12 cpp == 4, P010 cpp == 8 + * + * FIXME what is the logic behind this? + */ + if (fb->format->is_yuv && fb->format->num_planes > 1) + cpp *= 4; if (fb->format->cpp[0] == 8) { if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { *num = 10; - *den = 8; + *den = 8 * cpp; } else { *num = 9; - *den = 8; + *den = 8 * cpp; } } else { *num = 1; - *den = 1; + *den = cpp; } } @@ -355,27 +365,23 @@ static int skl_plane_min_cdclk(const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state) { struct drm_i915_private *dev_priv = to_i915(plane_state->uapi.plane->dev); - unsigned int pixel_rate = crtc_state->pixel_rate; - unsigned int src_w, src_h, dst_w, dst_h; unsigned int num, den; + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); - skl_plane_ratio(crtc_state, plane_state, &num, &den); + skl_plane_bpp_constraints(crtc_state, plane_state, &num, &den); /* two pixels per clock on glk+ */ if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) den *= 2; - src_w = drm_rect_width(&plane_state->uapi.src) >> 16; - src_h = drm_rect_height(&plane_state->uapi.src) >> 16; - dst_w = drm_rect_width(&plane_state->uapi.dst); - dst_h = drm_rect_height(&plane_state->uapi.dst); - - /* Downscaling limits the maximum pixel rate */ - dst_w = min(src_w, dst_w); - dst_h = min(src_h, dst_h); - - return DIV64_U64_ROUND_UP(mul_u32_u32(pixel_rate * num, src_w * src_h), - mul_u32_u32(den, dst_w * dst_h)); + /* + * intel_atomic_check_planes has already been called by this + * time in intel_atomic_check, so use calculated plane + * data rate as a basis, in order not to have duplicate code. + * According to BSpec, plane data rate is anyway used as + * a basis for this calculation. + */ + return DIV64_U64_ROUND_UP(crtc_state->data_rate[plane->id] * num, den); } static unsigned int
There seems to be a bit of confusing redundancy in a way, how plane data rate/min cdclk are calculated. In fact both min cdclk, pixel rate and plane data rate are all part of the same formula as per BSpec. However currently we have intel_plane_data_rate, which is used to calculate plane data rate and which is also used in bandwidth calculations. However for calculating min_cdclk we have another piece of code, doing almost same calculation, but a bit differently and in a different place. However as both are actually part of same formula, probably would be wise to use plane data rate calculations as a basis anyway, thus avoiding code duplication and possible bugs related to this. Another thing is that I've noticed that during min_cdclk calculations we account for plane scaling, while for plane data rate, we don't. crtc->pixel_rate seems to account only for pipe ratio, however it is clearly stated in BSpec that plane data rate also need to account plane ratio as well. So what this commit does is: - Adds a plane ratio calculation to intel_plane_data_rate - Removes redundant calculations from skl_plane_min_cdclk which is used for gen9+ and now uses intel_plane_data_rate as a basis from there as well. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- .../gpu/drm/i915/display/intel_atomic_plane.c | 16 ++++++- drivers/gpu/drm/i915/display/intel_sprite.c | 46 +++++++++++-------- 2 files changed, 41 insertions(+), 21 deletions(-)