On Thu, Feb 27, 2020 at 03:20:10PM +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ä)
>
> v3: - Again fix the division macro
> - Fix plane_pixel_rate to pixel_rate at intel_plane_pixel_rate
> callsites
>
> v4: - Renamed skl_plane_ratio function back(Ville Syrjälä)
>
> v5: - Don't precalculate plane pixel rate for invisible plane,
> check for visibility first, as in invisible case it will
> have dst_w and dst_h equal to zero, causing divide error.
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovs...@intel.com>
> ---
> .../gpu/drm/i915/display/intel_atomic_plane.c | 26 ++++++++++++++++++-
> .../gpu/drm/i915/display/intel_atomic_plane.h | 3 +++
> drivers/gpu/drm/i915/display/intel_sprite.c | 15 ++---------
> 3 files changed, 30 insertions(+), 14 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..956af022cf5b 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -133,15 +133,39 @@ 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);
> +
> + WARN_ON(!dst_w || !dst_h);
Pointless warn. You'll get a div by zero anyway.
> +
> + return DIV_ROUND_UP_ULL(mul_u32_u32(crtc_state->pixel_rate,
> + src_w * src_h),
Bogus alignment.
> + 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 pixel_rate;
>
> if (!plane_state->uapi.visible)
> return 0;
>
> + pixel_rate = intel_plane_pixel_rate(crtc_state, plane_state);
> +
> cpp = fb->format->cpp[0];
>
> /*
> @@ -153,7 +177,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 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..d16e17be26b0 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -355,9 +355,8 @@ 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 pixel_rate = intel_plane_pixel_rate(crtc_state, plane_state);
unsigned int?
>
> skl_plane_ratio(crtc_state, plane_state, &num, &den);
>
> @@ -365,17 +364,7 @@ static int skl_plane_min_cdclk(const struct
> intel_crtc_state *crtc_state,
> 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));
> + return DIV_ROUND_UP(pixel_rate * num, den);
> }
>
> static unsigned int
> --
> 2.24.1.485.gad05a3d8e5
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
