[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 17:02:54 UTC 2016
Em Qui, 2016-02-18 às 15:46 +0100, Maarten Lankhorst escreveu:
> Op 18-02-16 om 15:14 schreef Zanoni, Paulo R:
> > 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.int
> > > > > el.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.
I noticed that you fix the brackets on patch 4/8 by removing the "else"
part. So feel free to keep this chunk like this, so you won't need to
resend patch 4.
> > > >
> > > > 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.
> planes_changed is already used, it means that any plane_state is
> being updated for this crtc.
> fbs_changed could work though, most other *_changed are plural.
> > > > >
> > > > > 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_coun
> > > > > t(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_c
> > > > > rtc(
> > > > > 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?
> That's what matches the current code, see the calc_changes hunk which
> had a vblank_wait = true.
Hmm, right. I missed that, sorry.
> > > > > + 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 :)
> It would break things though.
Ok, let's discard the idea then.
>
> I think what might make the most sense is adding a inline function
> for needs_vblank,
> with comments why certain flags require a vblank wait.
That could be good.
I have no further questions/requests for this patch. In the end, it
seems the changes needed are small :). I'll wait for the next version.
> > > 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