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@xxxxxxxxx > Signed-off-by: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx> > --- > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
