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

[Kumar, Mahesh]

Hi Chris,

Thanks for review.

On 7/30/2018 9:08 PM, Chris Wilson wrote:
> 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!
hmm, ddb_size will not go beyond u16 as we have only 1024 blocks and in 
future also I'm not expecting it to overflow u16.
I don't wanted to change already used data-types , anyway will clean 
this part :)
>
>> +       pipe_size = div_u64(ddb_size * pipe_width[for_pipe], total_width);
> So why did you store all pipe_width?
hmm agree, we don't really need to store the width for each pipe.
>
> And are not all the previous pipes stored in their respective cstate? So
> alloc->start = &previous_cstate->wm.skl.ddb->end;
Actually this function may not get called in fixed sequence each time 
for each CRTC, It will be called first for the crtc which got added 
first in state.
I don't want to shuffle the DDB allocation during each modeset as in 
most of the cases we will be only changing refresh-rate of the panel not 
the resolution (dynamic media refresh rate switching kind of scenarios)

thanks,
-Mahesh
>
> Looking at the earlier results seems far more robust.
> -Chris