On Thu, 2020-02-27 at 15:47 +0200, Ville Syrjälä wrote:
> 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.lisovskiy@xxxxxxxxx>
> > ---
> > .../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?
Damn.. thanks for spotting.
Stan
>
> >
> > 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
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
