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@xxxxxxxxx>
> > ---
> > .../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
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
