| Message ID | 20200220160856.19888-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: Use intel_plane_data_rate for min_cdclk calculation | expand |
On Thu, Feb 20, 2020 at 06:08:56PM +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. > > v2: - Don't use 64 division if not needed(Ville Syrjälä) > - Now use intel_plane_pixel_rate as a basis for calculations both > at intel_plane_data_rate and skl_plane_min_cdclk(Ville Syrjälä) > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > .../gpu/drm/i915/display/intel_atomic_plane.c | 22 +++++++++++++++- > .../gpu/drm/i915/display/intel_atomic_plane.h | 3 +++ > drivers/gpu/drm/i915/display/intel_sprite.c | 26 +++++++------------ > 3 files changed, 33 insertions(+), 18 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..3bd7ea9bf1b4 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -133,11 +133,31 @@ intel_plane_destroy_state(struct drm_plane *plane, > kfree(plane_state); > } > > +unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state, > + const struct intel_plane_state *plane_state) > +{ > + unsigned int src_w, src_h, dst_w, dst_h; > + > + 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 DIV_ROUND_UP(mul_u32_u32(crtc_state->pixel_rate, Wrong macro for 64/32->32 division. > + src_w * src_h), > + mul_u32_u32(dst_w, dst_h)); And the divisor shouldn't be a u64. > +} > + > 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 plane_pixel_rate = intel_plane_pixel_rate(crtc_state, plane_state); Just 'pixel_rate' should do. We know the rest from the fact that this is a plane function. Also I'd put this first so the declaration block looks at least a bit less messy. > > if (!plane_state->uapi.visible) > return 0; > @@ -153,7 +173,7 @@ 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 mul_u32_u32(plane_pixel_rate, cpp); We're not returning a u64. > } > > int intel_plane_calc_min_cdclk(struct intel_atomic_state *state, > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > index 2bcf15e34728..a6bbf42bae1f 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > @@ -18,6 +18,9 @@ struct intel_plane_state; > > extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; > > +unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state, > + const struct intel_plane_state *plane_state); > + > unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, > const struct intel_plane_state *plane_state); > void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state, > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c > index 7abeefe8dce5..4fa3081e2074 100644 > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > @@ -330,9 +330,9 @@ 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) Bogus rename. > { > struct drm_i915_private *dev_priv = to_i915(plane_state->uapi.plane->dev); > const struct drm_framebuffer *fb = plane_state->hw.fb; > @@ -355,27 +355,19 @@ 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; > + int plane_pixel_rate = intel_plane_pixel_rate(crtc_state, plane_state); same comments as for the other call site. > > - 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)); > + /* > + * Plane pixel rate is a pipe pixel rate * plane ratio * pipe ratio > + */ Should be obvious. > + return DIV64_U64_ROUND_UP(plane_pixel_rate * num, den); > } > > static unsigned int > -- > 2.24.1.485.gad05a3d8e5
On Fri, 2020-02-21 at 16:04 +0200, Ville Syrjälä wrote: > On Thu, Feb 20, 2020 at 06:08:56PM +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. > > > > v2: - Don't use 64 division if not needed(Ville Syrjälä) > > - Now use intel_plane_pixel_rate as a basis for calculations > > both > > at intel_plane_data_rate and skl_plane_min_cdclk(Ville > > Syrjälä) > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > .../gpu/drm/i915/display/intel_atomic_plane.c | 22 > > +++++++++++++++- > > .../gpu/drm/i915/display/intel_atomic_plane.h | 3 +++ > > drivers/gpu/drm/i915/display/intel_sprite.c | 26 +++++++------ > > ------ > > 3 files changed, 33 insertions(+), 18 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..3bd7ea9bf1b4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > @@ -133,11 +133,31 @@ intel_plane_destroy_state(struct drm_plane > > *plane, > > kfree(plane_state); > > } > > > > +unsigned int intel_plane_pixel_rate(const struct intel_crtc_state > > *crtc_state, > > + const struct intel_plane_state > > *plane_state) > > +{ > > + unsigned int src_w, src_h, dst_w, dst_h; > > + > > + 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 DIV_ROUND_UP(mul_u32_u32(crtc_state->pixel_rate, > > Wrong macro for 64/32->32 division. Yes, in fact we should use 64 macro here still. As I understand pixel rate is stored in kHz so for instance for pixel rate 172800 * 4K * 4K we already overflowing u32. Was just a bit confused with prev comment :) > > > + src_w * src_h), > > + mul_u32_u32(dst_w, dst_h)); > > And the divisor shouldn't be a u64. Agree divisor is not, however divident is 64. > > > +} > > + > > 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 plane_pixel_rate = > > intel_plane_pixel_rate(crtc_state, plane_state); > > Just 'pixel_rate' should do. We know the rest from the fact that this > is a plane function. Also I'd put this first so the declaration block > looks at least a bit less messy. > > > > > if (!plane_state->uapi.visible) > > return 0; > > @@ -153,7 +173,7 @@ 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 mul_u32_u32(plane_pixel_rate, cpp); > > We're not returning a u64. > > > } > > > > int intel_plane_calc_min_cdclk(struct intel_atomic_state *state, > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > > index 2bcf15e34728..a6bbf42bae1f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > > @@ -18,6 +18,9 @@ struct intel_plane_state; > > > > extern const struct drm_plane_helper_funcs > > intel_plane_helper_funcs; > > > > +unsigned int intel_plane_pixel_rate(const struct intel_crtc_state > > *crtc_state, > > + const struct intel_plane_state > > *plane_state); > > + > > unsigned int intel_plane_data_rate(const struct intel_crtc_state > > *crtc_state, > > const struct intel_plane_state > > *plane_state); > > void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state > > *plane_state, > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c > > b/drivers/gpu/drm/i915/display/intel_sprite.c > > index 7abeefe8dce5..4fa3081e2074 100644 > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > > @@ -330,9 +330,9 @@ 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) > > Bogus rename. Well, I guess you agree, that this function is not returning plane_ratio either :) Was just wondering if it has to be named somewhat differently. Stan > > > { > > struct drm_i915_private *dev_priv = to_i915(plane_state- > > >uapi.plane->dev); > > const struct drm_framebuffer *fb = plane_state->hw.fb; > > @@ -355,27 +355,19 @@ 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; > > + int plane_pixel_rate = intel_plane_pixel_rate(crtc_state, > > plane_state); > > same comments as for the other call site. > > > > > - 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)); > > + /* > > + * Plane pixel rate is a pipe pixel rate * plane ratio * pipe > > ratio > > + */ > > Should be obvious. > > > + return DIV64_U64_ROUND_UP(plane_pixel_rate * num, den); > > } > > > > static unsigned int > > -- > > 2.24.1.485.gad05a3d8e5 > >
On Fri, Feb 21, 2020 at 02:38:01PM +0000, Lisovskiy, Stanislav wrote: > On Fri, 2020-02-21 at 16:04 +0200, Ville Syrjälä wrote: > > On Thu, Feb 20, 2020 at 06:08:56PM +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. > > > > > > v2: - Don't use 64 division if not needed(Ville Syrjälä) > > > - Now use intel_plane_pixel_rate as a basis for calculations > > > both > > > at intel_plane_data_rate and skl_plane_min_cdclk(Ville > > > Syrjälä) > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > .../gpu/drm/i915/display/intel_atomic_plane.c | 22 > > > +++++++++++++++- > > > .../gpu/drm/i915/display/intel_atomic_plane.h | 3 +++ > > > drivers/gpu/drm/i915/display/intel_sprite.c | 26 +++++++------ > > > ------ > > > 3 files changed, 33 insertions(+), 18 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..3bd7ea9bf1b4 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > @@ -133,11 +133,31 @@ intel_plane_destroy_state(struct drm_plane > > > *plane, > > > kfree(plane_state); > > > } > > > > > > +unsigned int intel_plane_pixel_rate(const struct intel_crtc_state > > > *crtc_state, > > > + const struct intel_plane_state > > > *plane_state) > > > +{ > > > + unsigned int src_w, src_h, dst_w, dst_h; > > > + > > > + 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 DIV_ROUND_UP(mul_u32_u32(crtc_state->pixel_rate, > > > > Wrong macro for 64/32->32 division. > > Yes, in fact we should use 64 macro here still. > As I understand pixel rate is stored in kHz so for instance > for pixel rate 172800 * 4K * 4K we already overflowing u32. > Was just a bit confused with prev comment :) > > > > > > + src_w * src_h), > > > + mul_u32_u32(dst_w, dst_h)); > > > > And the divisor shouldn't be a u64. > > Agree divisor is not, however divident is 64. > > > > > +} > > > + > > > 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 plane_pixel_rate = > > > intel_plane_pixel_rate(crtc_state, plane_state); > > > > Just 'pixel_rate' should do. We know the rest from the fact that this > > is a plane function. Also I'd put this first so the declaration block > > looks at least a bit less messy. > > > > > > > > if (!plane_state->uapi.visible) > > > return 0; > > > @@ -153,7 +173,7 @@ 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 mul_u32_u32(plane_pixel_rate, cpp); > > > > We're not returning a u64. > > > > > } > > > > > > int intel_plane_calc_min_cdclk(struct intel_atomic_state *state, > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h > > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > > > index 2bcf15e34728..a6bbf42bae1f 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > > > @@ -18,6 +18,9 @@ struct intel_plane_state; > > > > > > extern const struct drm_plane_helper_funcs > > > intel_plane_helper_funcs; > > > > > > +unsigned int intel_plane_pixel_rate(const struct intel_crtc_state > > > *crtc_state, > > > + const struct intel_plane_state > > > *plane_state); > > > + > > > unsigned int intel_plane_data_rate(const struct intel_crtc_state > > > *crtc_state, > > > const struct intel_plane_state > > > *plane_state); > > > void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state > > > *plane_state, > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c > > > b/drivers/gpu/drm/i915/display/intel_sprite.c > > > index 7abeefe8dce5..4fa3081e2074 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > > > @@ -330,9 +330,9 @@ 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) > > > > Bogus rename. > > Well, I guess you agree, that this function is not returning > plane_ratio either :) Was just wondering if it has to be named somewhat > differently. It returns the plane ratio excluding the downscaling component. So seems good enough to me. Or at least I can't immediately think of anything particularly better.
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index c86d7a35c816..3bd7ea9bf1b4 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -133,11 +133,31 @@ intel_plane_destroy_state(struct drm_plane *plane, kfree(plane_state); } +unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state, + const struct intel_plane_state *plane_state) +{ + unsigned int src_w, src_h, dst_w, dst_h; + + 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 DIV_ROUND_UP(mul_u32_u32(crtc_state->pixel_rate, + src_w * src_h), + mul_u32_u32(dst_w, dst_h)); +} + 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 plane_pixel_rate = intel_plane_pixel_rate(crtc_state, plane_state); if (!plane_state->uapi.visible) return 0; @@ -153,7 +173,7 @@ 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 mul_u32_u32(plane_pixel_rate, cpp); } int intel_plane_calc_min_cdclk(struct intel_atomic_state *state, diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h index 2bcf15e34728..a6bbf42bae1f 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h @@ -18,6 +18,9 @@ struct intel_plane_state; extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; +unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state, + const struct intel_plane_state *plane_state); + unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state); void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state, diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c index 7abeefe8dce5..4fa3081e2074 100644 --- a/drivers/gpu/drm/i915/display/intel_sprite.c +++ b/drivers/gpu/drm/i915/display/intel_sprite.c @@ -330,9 +330,9 @@ 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; @@ -355,27 +355,19 @@ 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; + int plane_pixel_rate = intel_plane_pixel_rate(crtc_state, plane_state); - 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)); + /* + * Plane pixel rate is a pipe pixel rate * plane ratio * pipe ratio + */ + return DIV64_U64_ROUND_UP(plane_pixel_rate * 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. v2: - Don't use 64 division if not needed(Ville Syrjälä) - Now use intel_plane_pixel_rate as a basis for calculations both at intel_plane_data_rate and skl_plane_min_cdclk(Ville Syrjälä) Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- .../gpu/drm/i915/display/intel_atomic_plane.c | 22 +++++++++++++++- .../gpu/drm/i915/display/intel_atomic_plane.h | 3 +++ drivers/gpu/drm/i915/display/intel_sprite.c | 26 +++++++------------ 3 files changed, 33 insertions(+), 18 deletions(-)