[Intel-gfx] [PATCH v2] drm/i915: Use intel_plane_data_rate for min_cdclk calculation
Lisovskiy, Stanislav
stanislav.lisovskiy at intel.com
Fri Feb 21 14:38:01 UTC 2020
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 at 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
>
>
More information about the Intel-gfx
mailing list