Re: [Intel-gfx] [PATCH v1] drm/i915: Use intel_plane_data_rate for min_cdclk calculation

[Lisovskiy, Stanislav]

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 
> > ---
> >  .../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(_state->uapi.src) >> 16;
> > +   src_h = drm_rect_height(_state->uapi.src) >> 16;
> > +   dst_w = drm_rect_width(_state->uapi.dst);
> > +   dst_h = drm_rect_height(_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 

Re: [Intel-gfx] [PATCH v1] drm/i915: Use intel_plane_data_rate for min_cdclk calculation

[Ville Syrjälä]

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 
> ---
>  .../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(_state->uapi.src) >> 16;
> + src_h = drm_rect_height(_state->uapi.src) >> 16;
> + dst_w = drm_rect_width(_state->uapi.dst);
> + dst_h = drm_rect_height(_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.

>  
>   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 

[Intel-gfx] [PATCH v1] drm/i915: Use intel_plane_data_rate for min_cdclk calculation

[Stanislav Lisovskiy]

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 
---
 .../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(_state->uapi.src) >> 16;
+   src_h = drm_rect_height(_state->uapi.src) >> 16;
+   dst_w = drm_rect_width(_state->uapi.dst);
+   dst_h = drm_rect_height(_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));
 }
 
 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;
 
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 =