| Message ID | 20180730141202.18021-1-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/skl: distribute DDB based on panel resolution | expand |
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@intel.com > Signed-off-by: Mahesh Kumar <mahesh1.kumar@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
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@intel.com >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@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
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); + pipe_size = div_u64(ddb_size * pipe_width[for_pipe], total_width); + alloc->start = ddb_size_before_pipe; alloc->end = alloc->start + pipe_size; } @@ -5259,7 +5288,7 @@ skl_ddb_add_affected_pipes(struct drm_atomic_state *state, bool *changed) * any other display updates race with this transaction, so we need * to grab the lock on *all* CRTC's. */ - if (intel_state->active_pipe_changes) { + if (intel_state->active_pipe_changes || intel_state->modeset) { realloc_pipes = ~0; intel_state->wm_results.dirty_pipes = ~0; }
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@intel.com Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 13 deletions(-)