[Intel-gfx] [PATCH] drm/i915/skl: distribute DDB based on panel resolution

[Chris Wilson]

Quoting Mahesh Kumar (2018-07-30 15:12:02)
> We distribute DDB equally among all pipes irrespective of display
> buffer requirement of each pipe. This leads to a situation where high
> resolution y-tiled display can not be enabled with 2 low resolution
> displays.
> 
> Main contributing factor for DDB requirement is width of the display.
> This patch make changes to distribute ddb based on display width.
> So display with higher width will get bigger chunk of DDB.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107113
> Cc: raviraj.p.sitaram at intel.com
> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7312ecb73415..e092f0deb93d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3814,8 +3814,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>         struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>         struct drm_i915_private *dev_priv = to_i915(dev);
>         struct drm_crtc *for_crtc = cstate->base.crtc;
> +       enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
> +       const struct drm_crtc_state *crtc_state;
> +       const struct drm_crtc *crtc;
> +       u32 pipe_width[I915_MAX_PIPES] = {0};
> +       u32 total_width = 0, width_before_pipe = 0;
>         unsigned int pipe_size, ddb_size;
> -       int nth_active_pipe;
> +       u16 ddb_size_before_pipe;
> +       u32 i;
>  
>         if (WARN_ON(!state) || !cstate->base.active) {
>                 alloc->start = 0;
> @@ -3833,14 +3839,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>                                       *num_active, ddb);
>  
>         /*
> -        * If the state doesn't change the active CRTC's, then there's
> -        * no need to recalculate; the existing pipe allocation limits
> -        * should remain unchanged.  Note that we're safe from racing
> -        * commits since any racing commit that changes the active CRTC
> -        * list would need to grab _all_ crtc locks, including the one
> -        * we currently hold.
> +        * If the state doesn't change the active CRTC's or there is no
> +        * modeset request, then there's no need to recalculate;
> +        * the existing pipe allocation limits should remain unchanged.
> +        * Note that we're safe from racing commits since any racing commit
> +        * that changes the active CRTC list or do modeset would need to
> +        * grab _all_ crtc locks, including the one we currently hold.
>          */
> -       if (!intel_state->active_pipe_changes) {
> +       if (!intel_state->active_pipe_changes && !intel_state->modeset) {
>                 /*
>                  * alloc may be cleared by clear_intel_crtc_state,
>                  * copy from old state to be sure
> @@ -3849,10 +3855,33 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>                 return;
>         }
>  
> -       nth_active_pipe = hweight32(intel_state->active_crtcs &
> -                                   (drm_crtc_mask(for_crtc) - 1));
> -       pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> -       alloc->start = nth_active_pipe * ddb_size / *num_active;
> +       /*
> +        * Watermark/ddb requirement highly depends upon width of the
> +        * framebuffer, So instead of allocating DDB equally among pipes
> +        * distribute DDB based on resolution/width of the display.
> +        */
> +       for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +               const struct drm_display_mode *adjusted_mode;
> +               int hdisplay, vdisplay;
> +               enum pipe pipe;
> +
> +               if (!crtc_state->enable)
> +                       continue;
> +
> +               pipe = to_intel_crtc(crtc)->pipe;
> +               adjusted_mode = &crtc_state->adjusted_mode;
> +               drm_mode_get_hv_timing(adjusted_mode, &hdisplay, &vdisplay);
> +               pipe_width[pipe] = hdisplay;
> +               total_width += pipe_width[pipe];
> +
> +               if (pipe < for_pipe)
> +                       width_before_pipe += pipe_width[pipe];
> +       }
> +
> +       ddb_size_before_pipe = div_u64(ddb_size * width_before_pipe,
> +                                      total_width);

ddb_size is unsigned int (u32)
width_before_pipe is u32
ddb_size_before_pipe is u16

That mismash of types is itself perplexing, but u32*u16 is only u32, you
need to cast it to u64 to avoid the overflow: i.e.
	div_u64(mul_u32_u32(ddb_size, width_before_pipe),
	        total_width);

But ddb_size_before_pipe obviously need to be the same type as ddb_size,
and if u16 is good enough, then you do not need a 64b divide!

> +       pipe_size = div_u64(ddb_size * pipe_width[for_pipe], total_width);

So why did you store all pipe_width?

And are not all the previous pipes stored in their respective cstate? So
alloc->start = &previous_cstate->wm.skl.ddb->end;

Looking at the earlier results seems far more robust.
-Chris