[Intel-gfx] [PATCH v4 3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.
Zanoni, Paulo R
paulo.r.zanoni at intel.com
Thu Feb 18 14:14:33 UTC 2016
Em Qui, 2016-02-18 às 14:22 +0100, Maarten Lankhorst escreveu:
> Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:
> > Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> > > Currently we perform our own wait in post_plane_update,
> > > but the atomic core performs another one in wait_for_vblanks.
> > > This means that 2 vblanks are done when a fb is changed,
> > > which is a bit overkill.
> > >
> > > Merge them by creating a helper function that takes a crtc mask
> > > for the planes to wait on.
> > >
> > > The broadwell vblank workaround may look gone entirely but this
> > > is
> > > not the case. pipe_config->wm_changed is set to true
> > > when any plane is turned on, which forces a vblank wait.
> > >
> > > Changes since v1:
> > > - Removing the double vblank wait on broadwell moved to its own
> > > commit.
> > > Changes since v2:
> > > - Move out POWER_DOMAIN_MODESET handling to its own commit.
> > > Changes since v3:
> > > - Do not wait for vblank on legacy cursor updates. (Ville)
> > > - Move broadwell vblank workaround comment to page_flip_finished.
> > > (Ville)
> > > Changes since v4:
> > > - Compile fix, legacy_cursor_flip -> *_update.
> > >
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.c
> > > om>
> > > ---
> > > drivers/gpu/drm/i915/intel_atomic.c | 1 +
> > > drivers/gpu/drm/i915/intel_display.c | 86
> > > +++++++++++++++++++++++++++---------
> > > drivers/gpu/drm/i915/intel_drv.h | 2 +-
> > > 3 files changed, 67 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > > b/drivers/gpu/drm/i915/intel_atomic.c
> > > index 4625f8a9ba12..8e579a8505ac 100644
> > > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > > @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc
> > > *crtc)
> > > crtc_state->disable_lp_wm = false;
> > > crtc_state->disable_cxsr = false;
> > > crtc_state->wm_changed = false;
> > > + crtc_state->fb_changed = false;
> > >
> > > return &crtc_state->base;
> > > }
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 804f2c6f260d..4d4dddc1f970 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4785,9 +4785,6 @@ static void intel_post_plane_update(struct
> > > intel_crtc *crtc)
> > > to_intel_crtc_state(crtc->base.state);
> > > struct drm_device *dev = crtc->base.dev;
> > >
> > > - if (atomic->wait_vblank)
> > > - intel_wait_for_vblank(dev, crtc->pipe);
> > > -
> > > intel_frontbuffer_flip(dev, atomic->fb_bits);
> > >
> > > crtc->wm.cxsr_allowed = true;
> > > @@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct
> > > intel_crtc *crtc)
> > > return true;
> > >
> > > /*
> > > + * BDW signals flip done immediately if the plane
> > > + * is disabled, even if the plane enable is already
> > > + * armed to occur at the next vblank :(
> > > + */
> > Having this comment here is just... weird. I think it removes a lot
> > of
> > the context that was present before.
> >
> > > +
> > > + /*
> > > * A DSPSURFLIVE check isn't enough in case the mmio and
> > > CS
> > > flips
> > > * used the same base address. In that case the mmio
> > > flip
> > > might
> > > * have completed, but the CS hasn't even executed the
> > > flip
> > > yet.
> > > @@ -11778,6 +11781,9 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > > if (!was_visible && !visible)
> > > return 0;
> > >
> > > + if (fb != old_plane_state->base.fb)
> > > + pipe_config->fb_changed = true;
> > > +
> > > turn_off = was_visible && (!visible || mode_changed);
> > > turn_on = visible && (!was_visible || mode_changed);
> > >
> > > @@ -11793,8 +11799,6 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > >
> > > /* must disable cxsr around plane enable/disable
> > > */
> > > if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> > > - if (is_crtc_enabled)
> > > - intel_crtc->atomic.wait_vblank =
> > > true;
> > > pipe_config->disable_cxsr = true;
> > > }
> > We could have killed the brackets here :)
> Indeed, will do so in next version.
> > > } else if (intel_wm_need_update(plane, plane_state)) {
> > > @@ -11810,14 +11814,6 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > > intel_crtc->atomic.post_enable_primary =
> > > turn_on;
> > > intel_crtc->atomic.update_fbc = true;
> > >
> > > - /*
> > > - * BDW signals flip done immediately if the
> > > plane
> > > - * is disabled, even if the plane enable is
> > > already
> > > - * armed to occur at the next vblank :(
> > > - */
> > > - if (turn_on && IS_BROADWELL(dev))
> > > - intel_crtc->atomic.wait_vblank = true;
> > > -
> > > break;
> > > case DRM_PLANE_TYPE_CURSOR:
> > > break;
> > > @@ -11831,12 +11827,10 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > > if (IS_IVYBRIDGE(dev) &&
> > > needs_scaling(to_intel_plane_state(plane_sta
> > > te))
> > > &&
> > > !needs_scaling(old_plane_state)) {
> > > - to_intel_crtc_state(crtc_state)-
> > > > disable_lp_wm = true;
> > > - } else if (turn_off && !mode_changed) {
> > > - intel_crtc->atomic.wait_vblank = true;
> > > + pipe_config->disable_lp_wm = true;
> > > + } else if (turn_off && !mode_changed)
> > > intel_crtc-
> > > >atomic.update_sprite_watermarks
> > > > =
> > > 1 << i;
> > > - }
> > Weird brackets here. Either kill on both sides of the if statement
> > (the
> > preferred way), or none.
> >
> > Also, shouldn't we introduce pipe_config->sprite_changed? What's
> > guaranteeing that we're going to definitely wait for a vblank
> > during
> > sprite updates? I like explicit dumb-proof code instead of
> > implications
> > such as "we're doing waits during sprite updates because whenever
> > we
> > update sprites, a specific unrelated variable that's used for a
> > different purpose gets set to true, and we check for this
> > variable".
> sprite_changed would be same as fb_changed + wm_changed, and
> update_sprite_watermarks gets removed in this series
> because nothing uses it.
Hmmm, right. For some reason, I was interpreting fb_changed as being
only valid for the primary fb, not for spr/cur. My bad. So fb_changed
means "if any of pri/cur/spr changed" I guess. Maybe planes_changed
could be a better name, or fbs_changed (plural), since we're talking
about more then one possible plane here. Not a requirement, just
throwing the idea.
> > >
> > > break;
> > > }
> > > @@ -13370,6 +13364,48 @@ static int
> > > intel_atomic_prepare_commit(struct drm_device *dev,
> > > return ret;
> > > }
> > >
> > > +static void intel_atomic_wait_for_vblanks(struct drm_device
> > > *dev,
> > > + struct
> > > drm_i915_private
> > > *dev_priv,
> > > + unsigned crtc_mask)
> > > +{
> > > + unsigned last_vblank_count[I915_MAX_PIPES];
> > > + enum pipe pipe;
> > > + int ret;
> > > +
> > > + if (!crtc_mask)
> > > + return;
> > > +
> > > + for_each_pipe(dev_priv, pipe) {
> > > + struct drm_crtc *crtc = dev_priv-
> > > > pipe_to_crtc_mapping[pipe];
> > > +
> > > + if (!((1 << pipe) & crtc_mask))
> > > + continue;
> > > +
> > > + ret = drm_crtc_vblank_get(crtc);
> > > + if (ret != 0) {
> > > + crtc_mask &= ~(1 << pipe);
> > > + continue;
> > Shouldn't we DRM_ERROR here?
> WARN_ON is enough, this shouldn't ever happen.
Even better :)
> > > + }
> > > +
> > > + last_vblank_count[pipe] =
> > > drm_crtc_vblank_count(crtc);
> > > + }
> > > +
> > > + for_each_pipe(dev_priv, pipe) {
> > > + struct drm_crtc *crtc = dev_priv-
> > > > pipe_to_crtc_mapping[pipe];
> > > +
> > > + if (!((1 << pipe) & crtc_mask))
> > > + continue;
> > > +
> > > + wait_event_timeout(dev->vblank[pipe].queue,
> > > + last_vblank_count[pipe] !=
> > > + drm_crtc_vblank_count(cr
> > > tc),
> > > + msecs_to_jiffies(50));
> > Also maybe DRM_ERROR in case we hit the timeout?
> >
> > I know the original code doesn't have this, but now that we're
> > doing or
> > own thing, we may scream in unexpected cases.
> I'll make it a WARN_ON, shouldn't happen.
> > > +
> > > + drm_crtc_vblank_put(crtc);
> > > + }
> > > +}
> > > +
> > > +
> > Two newlines :)
> Indeed!
> > > /**
> > > * intel_atomic_commit - commit validated state object
> > > * @dev: DRM device
> > > @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct
> > > drm_device *dev,
> > > int ret = 0, i;
> > > bool hw_check = intel_state->modeset;
> > > unsigned long put_domains[I915_MAX_PIPES] = {};
> > > + unsigned crtc_vblank_mask = 0;
> > >
> > > ret = intel_atomic_prepare_commit(dev, state, async);
> > > if (ret) {
> > > @@ -13470,8 +13507,9 @@ static int intel_atomic_commit(struct
> > > drm_device *dev,
> > > for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > struct intel_crtc *intel_crtc =
> > > to_intel_crtc(crtc);
> > > bool modeset = needs_modeset(crtc->state);
> > > - bool update_pipe = !modeset &&
> > > - to_intel_crtc_state(crtc->state)-
> > > > update_pipe;
> > > + struct intel_crtc_state *pipe_config =
> > > + to_intel_crtc_state(crtc->state);
> > > + bool update_pipe = !modeset && pipe_config-
> > > > update_pipe;
> > >
> > > if (modeset && crtc->state->active) {
> > > update_scanline_offset(to_intel_crtc(crt
> > > c));
> > > @@ -13488,14 +13526,20 @@ static int intel_atomic_commit(struct
> > > drm_device *dev,
> > > (crtc->state->planes_changed ||
> > > update_pipe))
> > > drm_atomic_helper_commit_planes_on_crtc(
> > > crtc
> > > _state);
> > >
> > > - intel_post_plane_update(intel_crtc);
> > > + if (pipe_config->base.active &&
> > > + (pipe_config->wm_changed || pipe_config-
> > > > disable_cxsr ||
> > > + pipe_config->fb_changed))
> > So the wm_changed is just for the BDW workaround + sprites? What
> > about
> > this disable_cxsr, why is it here? Also see my comment above about
> > sprite_changed. Maybe we need some comments here to explain the
> > complex
> > checks.
> No it's waiting for a vblank for post_plane_update so all optimized
> wm values
> can get written, it's not just for the BDW workaround.
> It just happens to be that the BDW w/a gets applied too as a side
> effect.
So maybe some comment regarding the BDW WA could be here.
What about disable_cxsr? Why is this here?
> > > + crtc_vblank_mask |= 1 << i;
> > > }
> > >
> > > /* FIXME: add subpixel order */
> > >
> > > - drm_atomic_helper_wait_for_vblanks(dev, state);
> > > + if (!state->legacy_cursor_update)
> > > + intel_atomic_wait_for_vblanks(dev, dev_priv,
> > > crtc_vblank_mask);
> > >
> > > for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > + intel_post_plane_update(to_intel_crtc(crtc));
> > > +
> > > if (put_domains[i])
> > > modeset_put_power_domains(dev_priv,
> > > put_domains[i]);
> > > }
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 335e6b24b0bc..e911c86f873b 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -379,6 +379,7 @@ struct intel_crtc_state {
> > > bool update_pipe; /* can a fast modeset be performed? */
> > > bool disable_cxsr;
> > > bool wm_changed; /* watermarks are updated */
> > > + bool fb_changed; /* fb on any of the planes is changed
> > > */
> > >
> > > /* Pipe source size (ie. panel fitter input size)
> > > * All planes will be positioned inside this space,
> > > @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
> > >
> > > /* Sleepable operations to perform after commit */
> > > unsigned fb_bits;
> > > - bool wait_vblank;
> > One of the things that I like about the code without this patch is
> > that
> > it's very explicit on when we need to wait for vblanks (except for
> > the
> > problem where we wait twice). The new code is not as easy to
> > read/understand as the old one. This comment is similar to the one
> > I
> > made in patch 6: I'm not sure if sacrificing readability is worth
> > it.
> >
> > I wonder that maybe the cleanest fix to the "we're waiting 2
> > vblanks"
> > problem is to just remove the call to
> > drm_atomic_helper_wait_for_vblanks(), which would be a first patch.
> > Then we'd have a second patch introducing
> > intel_atomic_wait_for_vblanks() for the "wait for all vblanks at
> > the
> > same time" optimization, and then a last commit possibly replacing
> > commit->wait_vblank for state->fb_changed. Just an idea, not a
> > request.
> There were cases in which the atomic helper would wait for vblanks
> which
> wouldn't trigger any of the other changes, so it can't be blindly
> removed. Mostly when
> updating a plane with a same sized fb.
Those could be specifically addressed on their own patches, then :)
> Wait for vblank is required for unpinning, it would be bad if the
> current fb is unpinned.
>
> > I'll wait for your answers before reaching any conclusions of what
> > I
> > think should be done, since I may be misunderstanding something.
> I want to call all post flip work scheduled later on so it happens
> after the next vblank.
> That will remove all confusion on when a vblank is required, because
> all post_plane_update
> and unpin will always wait until next vblank.
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list