On Fri, May 06, 2016 at 02:42:46PM -0400, Lyude Paul wrote:
> On Fri, 2016-05-06 at 14:12 -0400, Lyude Paul wrote:
> > On Thu, 2016-04-21 at 16:17 -0700, Matt Roper wrote:
> > >
> > > Now that we're properly pre-allocating the DDB during the atomic check
> > > phase and we trust that the allocation is appropriate, let's actually
> > > use the allocation computed and not duplicate that work during the
> > > commit phase.
> > >
> > > v2:
> > > - Significant rebasing now that we can use cached data rates and
> > > minimum block allocations to avoid grabbing additional plane states.
> > >
> > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 14 +--
> > > drivers/gpu/drm/i915/intel_pm.c | 224 +++++++++++---------------------
> > > --
> > > -
> > > 2 files changed, 67 insertions(+), 171 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index c970e3e..80d8f77 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13599,6 +13599,7 @@ static int intel_atomic_commit(struct drm_device
> > > *dev,
> > >
> > > drm_atomic_helper_swap_state(dev, state);
> > > dev_priv->wm.config = intel_state->wm_config;
> > > + dev_priv->wm.skl_results.ddb = intel_state->ddb;
> > > intel_shared_dpll_commit(state);
> > >
> > > if (intel_state->modeset) {
> > > @@ -13716,19 +13717,6 @@ static int intel_atomic_commit(struct drm_device
> > > *dev,
> > > intel_modeset_verify_crtc(crtc, old_crtc_state, crtc-
> > > >state);
> > > }
> > >
> > > - /*
> > > - * Temporary sanity check: make sure our pre-computed DDB matches
> > > the
> > > - * one we actually wind up programming.
> > > - *
> > > - * Not a great place to put this, but the easiest place we have
> > > access
> > > - * to both the pre-computed and final DDB's; we'll be removing this
> > > - * check in the next patch anyway.
> > > - */
> > > - WARN(IS_GEN9(dev) &&
> > > - memcmp(&intel_state->ddb, &dev_priv->wm.skl_results.ddb,
> > > - sizeof(intel_state->ddb)),
> > > - "Pre-computed DDB does not match final DDB!\n");
> > > -
> > > if (intel_state->modeset)
> > > intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index fca44f8..f28fd36 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -2849,7 +2849,6 @@ skl_wm_plane_id(const struct intel_plane *plane)
> > > static void
> > > skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> > > const struct intel_crtc_state *cstate,
> > > - struct intel_wm_config *config,
> > > struct skl_ddb_entry *alloc, /* out */
> > > int *num_active /* out */)
> > > {
> > > @@ -2857,24 +2856,22 @@ 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;
> > > - struct drm_crtc *crtc;
> > > unsigned int pipe_size, ddb_size;
> > > int nth_active_pipe;
> > > int pipe = to_intel_crtc(for_crtc)->pipe;
> > >
> > > - if (intel_state && intel_state->active_pipe_changes)
> > > - *num_active = hweight32(intel_state->active_crtcs);
> > > - else if (intel_state)
> > > - *num_active = hweight32(dev_priv->active_crtcs);
> > > - else
> > > - *num_active = config->num_pipes_active;
> > > -
> > > - if (!cstate->base.active) {
> > > + if (WARN_ON(!state) || !cstate->base.active) {
> > > alloc->start = 0;
> > > alloc->end = 0;
> > > + *num_active = hweight32(dev_priv->active_crtcs);
> > > return;
> > > }
> > >
> > > + if (intel_state->active_pipe_changes)
> > > + *num_active = hweight32(intel_state->active_crtcs);
> > > + else
> > > + *num_active = hweight32(dev_priv->active_crtcs);
> > > +
> > > if (IS_BROXTON(dev))
> > > ddb_size = BXT_DDB_SIZE;
> > > else
> > > @@ -2883,50 +2880,23 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device
> > > *dev,
> > > ddb_size -= 4; /* 4 blocks for bypass path allocation */
> > >
> > > /*
> > > - * FIXME: At the moment we may be called on either in-flight or
> > > fully
> > > - * committed cstate's. Once we fully move DDB allocation in the
> > > check
> > > - * phase, we'll only be called on in-flight states and the 'else'
> > > - * branch here will go away.
> > > - *
> > > - * The 'else' branch is slightly racy here, but it was racy to
> > > begin
> > > - * with; since it's going away soon, no effort is made to address
> > > that.
> > > + * 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 (state) {
> > > - /*
> > > - * 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 (!intel_state->active_pipe_changes) {
> > > - *alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> > > - 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;
> > > - alloc->end = alloc->start + pipe_size;
> > > - } else {
> > > - nth_active_pipe = 0;
> > > - for_each_crtc(dev, crtc) {
> > > - if (!to_intel_crtc(crtc)->active)
> > > - continue;
> > > -
> > > - if (crtc == for_crtc)
> > > - break;
> > > -
> > > - nth_active_pipe++;
> > > - }
> > > -
> > > - pipe_size = ddb_size / config->num_pipes_active;
> > > - alloc->start = nth_active_pipe * ddb_size /
> > > - config->num_pipes_active;
> > > - alloc->end = alloc->start + pipe_size;
> > > + if (!intel_state->active_pipe_changes) {
> > > + *alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> > So I finally figured out what's causing all of the valid wm calculations to
> > get
> > rejected, leading to blank screens etc. etc. The problem is this line right
> > here
> > where we assign *alloc, we never actually set dev_priv-
> > >wm.skl_hw.ddb.pipe[pipe]
> > to a value anywhere so we end up having bogus ddb entry sizes. If you change
> > it
> > to something like:
> >
> > *alloc = dev_priv->wm.skl_hw.ddb.plane[pipe][intel_crtc->plane];
> Whoops, I stand corrected on this, since this seems to cause a few issues with
> VT switching. We actually want something like this:
>
> if (!intel_state->active_pipe_changes &&
> dev_priv->wm.skl_hw.ddb.pipe[pipe].end != 0) {
> *alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> return;
> }
>
dev_priv->wm.skl_hw.ddb.pipe[pipe] does get assigned in skl_update_wm()
via a full structure copy:
/* store the new configuration */
dev_priv->wm.skl_hw = *results;
So if you start with a good value in skl_hw and then you only recompute
when the value changes, skl_hw will always have good values when you get
to the point you noted above. The problem is that if you start with a
bogus/unexpected value (based on the initial BIOS setting), then you've
got a garbage-in, garbage-out situation for every update that tries to
re-use the value. Only something that triggers a full recompute (i.e.,
a pipe changing active status) will fix the problem at that point.
I've been out of office for the last two weeks, but I'm finally back
today, so I'm incorporating the fixes for this problem into a new
version of the series and rebasing to the latest -nightly. I'll post my
updated patches shortly. Sorry for the delay getting these final
details worked out!
Matt
> >
> > We get the right values and everything starts working as expected. BTW: Things
> > seem to be pretty stable with this patchset after this fix, haven't tried it
> > on
> > any other platforms then skl yet though so I'll let you know if I run into any
> > more problems.
> >
> > >
> > > + 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;
> > > + alloc->end = alloc->start + pipe_size;
> > > }
> > >
> > > static unsigned int skl_cursor_allocation(int num_active)
> > > @@ -3025,62 +2995,33 @@ skl_get_total_relative_data_rate(struct
> > > intel_crtc_state *intel_cstate)
> > > struct drm_crtc *crtc = cstate->crtc;
> > > struct drm_device *dev = crtc->dev;
> > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > + const struct drm_plane *plane;
> > > const struct intel_plane *intel_plane;
> > > + struct drm_plane_state *pstate;
> > > unsigned int rate, total_data_rate = 0;
> > > int id;
> > > + int i;
> > > +
> > > + if (WARN_ON(!state))
> > > + return 0;
> > >
> > > /* Calculate and cache data rate for each plane */
> > > - /*
> > > - * FIXME: At the moment this function can be called on either an
> > > - * in-flight or a committed state object. If it's in-flight then
> > > we
> > > - * only want to re-calculate the plane data rate for planes that
> > > are
> > > - * part of the transaction (i.e., we don't want to grab any
> > > additional
> > > - * plane states if we don't have to). If we're operating on
> > > committed
> > > - * state, we'll just go ahead and recalculate the plane data rate
> > > for
> > > - * all planes.
> > > - *
> > > - * Once we finish moving our DDB allocation to the atomic check
> > > phase,
> > > - * we'll only be calling this function on in-flight state objects,
> > > so
> > > - * the 'else' branch here will go away.
> > > - */
> > > - if (state) {
> > > - struct drm_plane *plane;
> > > - struct drm_plane_state *pstate;
> > > - int i;
> > > -
> > > - for_each_plane_in_state(state, plane, pstate, i) {
> > > - intel_plane = to_intel_plane(plane);
> > > - id = skl_wm_plane_id(intel_plane);
> > > -
> > > - if (intel_plane->pipe != intel_crtc->pipe)
> > > - continue;
> > > -
> > > - /* packed/uv */
> > > - rate = skl_plane_relative_data_rate(intel_cstate,
> > > - pstate, 0);
> > > - intel_cstate->wm.skl.plane_data_rate[id] = rate;
> > > -
> > > - /* y-plane */
> > > - rate = skl_plane_relative_data_rate(intel_cstate,
> > > - pstate, 1);
> > > - intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> > > - }
> > > - } else {
> > > - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
> > > {
> > > - const struct drm_plane_state *pstate =
> > > - intel_plane->base.state;
> > > - int id = skl_wm_plane_id(intel_plane);
> > > + for_each_plane_in_state(state, plane, pstate, i) {
> > > + id = skl_wm_plane_id(to_intel_plane(plane));
> > > + intel_plane = to_intel_plane(plane);
> > >
> > > - /* packed/uv */
> > > - rate = skl_plane_relative_data_rate(intel_cstate,
> > > - pstate, 0);
> > > - intel_cstate->wm.skl.plane_data_rate[id] = rate;
> > > + if (intel_plane->pipe != intel_crtc->pipe)
> > > + continue;
> > >
> > > - /* y-plane */
> > > - rate = skl_plane_relative_data_rate(intel_cstate,
> > > - pstate, 1);
> > > - intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> > > - }
> > > + /* packed/uv */
> > > + rate = skl_plane_relative_data_rate(intel_cstate,
> > > + pstate, 0);
> > > + intel_cstate->wm.skl.plane_data_rate[id] = rate;
> > > +
> > > + /* y-plane */
> > > + rate = skl_plane_relative_data_rate(intel_cstate,
> > > + pstate, 1);
> > > + intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> > > }
> > >
> > > /* Calculate CRTC's total data rate from cached values */
> > > @@ -3104,8 +3045,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> > > struct drm_atomic_state *state = cstate->base.state;
> > > struct drm_crtc *crtc = cstate->base.crtc;
> > > struct drm_device *dev = crtc->dev;
> > > - struct drm_i915_private *dev_priv = to_i915(dev);
> > > - struct intel_wm_config *config = &dev_priv->wm.config;
> > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > struct intel_plane *intel_plane;
> > > struct drm_plane *plane;
> > > @@ -3119,6 +3058,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> > > int num_active;
> > > int id, i;
> > >
> > > + if (WARN_ON(!state))
> > > + return 0;
> > > +
> > > if (!cstate->base.active) {
> > > ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
> > > memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> > > @@ -3126,8 +3068,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> > > return 0;
> > > }
> > >
> > > - skl_ddb_get_pipe_allocation_limits(dev, cstate, config, alloc,
> > > - &num_active);
> > > + skl_ddb_get_pipe_allocation_limits(dev, cstate, alloc,
> > > &num_active);
> > > alloc_size = skl_ddb_entry_size(alloc);
> > > if (alloc_size == 0) {
> > > memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> > > @@ -3139,53 +3080,31 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> > > *cstate,
> > > ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> > >
> > > alloc_size -= cursor_blocks;
> > > - alloc->end -= cursor_blocks;
> > >
> > > /* 1. Allocate the mininum required blocks for each active plane */
> > > - /*
> > > - * TODO: Remove support for already-committed state once we
> > > - * only allocate DDB on in-flight states.
> > > - */
> > > - if (state) {
> > > - for_each_plane_in_state(state, plane, pstate, i) {
> > > - intel_plane = to_intel_plane(plane);
> > > - id = skl_wm_plane_id(intel_plane);
> > > -
> > > - if (intel_plane->pipe != pipe)
> > > - continue;
> > > -
> > > - if (!to_intel_plane_state(pstate)->visible) {
> > > - minimum[id] = 0;
> > > - y_minimum[id] = 0;
> > > - continue;
> > > - }
> > > - if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > > - minimum[id] = 0;
> > > - y_minimum[id] = 0;
> > > - continue;
> > > - }
> > > -
> > > - minimum[id] = 8;
> > > - if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
> > > - y_minimum[id] = 8;
> > > - else
> > > - y_minimum[id] = 0;
> > > - }
> > > - } else {
> > > - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
> > > {
> > > - struct drm_plane *plane = &intel_plane->base;
> > > - struct drm_framebuffer *fb = plane->state->fb;
> > > - int id = skl_wm_plane_id(intel_plane);
> > > -
> > > - if (!to_intel_plane_state(plane->state)->visible)
> > > - continue;
> > > + for_each_plane_in_state(state, plane, pstate, i) {
> > > + intel_plane = to_intel_plane(plane);
> > > + id = skl_wm_plane_id(intel_plane);
> > >
> > > - if (plane->type == DRM_PLANE_TYPE_CURSOR)
> > > - continue;
> > > + if (intel_plane->pipe != pipe)
> > > + continue;
> > >
> > > - minimum[id] = 8;
> > > - y_minimum[id] = (fb->pixel_format ==
> > > DRM_FORMAT_NV12)
> > > ? 8 : 0;
> > > + if (!to_intel_plane_state(pstate)->visible) {
> > > + minimum[id] = 0;
> > > + y_minimum[id] = 0;
> > > + continue;
> > > + }
> > > + if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > > + minimum[id] = 0;
> > > + y_minimum[id] = 0;
> > > + continue;
> > > }
> > > +
> > > + minimum[id] = 8;
> > > + if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
> > > + y_minimum[id] = 8;
> > > + else
> > > + y_minimum[id] = 0;
> > > }
> > >
> > > for (i = 0; i < PLANE_CURSOR; i++) {
> > > @@ -3736,7 +3655,6 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
> > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> > >
> > > - WARN_ON(skl_allocate_pipe_ddb(cstate, ddb) != 0);
> > > skl_build_pipe_wm(cstate, ddb, pipe_wm);
> > >
> > > if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
> > > @@ -3800,16 +3718,6 @@ static void skl_clear_wm(struct skl_wm_values
> > > *watermarks, enum pipe pipe)
> > > memset(watermarks->plane_trans[pipe],
> > > 0, sizeof(uint32_t) * I915_MAX_PLANES);
> > > watermarks->plane_trans[pipe][PLANE_CURSOR] = 0;
> > > -
> > > - /* Clear ddb entries for pipe */
> > > - memset(&watermarks->ddb.pipe[pipe], 0, sizeof(struct
> > > skl_ddb_entry));
> > > - memset(&watermarks->ddb.plane[pipe], 0,
> > > - sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> > > - memset(&watermarks->ddb.y_plane[pipe], 0,
> > > - sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> > > - memset(&watermarks->ddb.plane[pipe][PLANE_CURSOR], 0,
> > > - sizeof(struct skl_ddb_entry));
> > > -
> > > }
> > >
> > > static int
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
