[Intel-gfx] [PATCH v2 2/9] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v3.
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Feb 9 10:42:24 UTC 2016
On Tue, Feb 09, 2016 at 11:27:44AM +0100, Maarten Lankhorst wrote:
> Hey,
>
> Op 13-01-16 om 13:58 schreef Ville Syrjälä:
> > On Mon, Jan 11, 2016 at 01:27:42PM +0100, Maarten Lankhorst wrote:
> >> 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.
> > Still NAK, because you just removed the comment entirely.
> I may have removed the comment but there will always be an unconditional wait because of the wm changes.
> In some future commit I will rework do_intel_finish_page_flip to deal with atomic, and in that function the comment
> will be useful again.
The comment is the spec here, so it should be kept. Actually what I
really want is to stop using the flip done interrupt entirely since
it's arguably broken, at which point the comment should problably be
moved to somewhere else (interrupt setup code, flip completion code,
etc.). But removing the comment would be bad since then people might
not understand why we do it the way we do.
> >> 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.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_atomic.c | 1 +
> >> drivers/gpu/drm/i915/intel_display.c | 80 ++++++++++++++++++++++++++----------
> >> drivers/gpu/drm/i915/intel_drv.h | 2 +-
> >> 3 files changed, 61 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> >> index 9682d94af710..ba9a57f33c43 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >> @@ -98,6 +98,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> >> crtc_state->disable_cxsr = false;
> >> crtc_state->wm_changed = false;
> >> crtc_state->wm.need_postvbl_update = 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 2aa1c5367625..eac73f005a70 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4796,9 +4796,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;
> >> @@ -11872,6 +11869,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;
> > Isn't that going to slow down cursor updates once again?
> Very likely.. Shall I add a if (!state->legacy_cursor_update) to intel_atomic_wait_for_vblanks ?
Not sure. Still wishing we could have just had the proper fps>=vrefresh
support fron the start.
> >> +
> >> turn_off = was_visible && (!visible || mode_changed);
> >> turn_on = visible && (!was_visible || mode_changed);
> >>
> >> @@ -11887,8 +11887,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;
> >> }
> >> } else if (intel_wm_need_update(plane, plane_state)) {
> >> @@ -11940,14 +11938,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >> plane_state->rotation != BIT(DRM_ROTATE_0))
> >> intel_crtc->atomic.disable_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;
> >> -
> >> intel_crtc->atomic.update_fbc |= visible || mode_changed;
> >> break;
> >> case DRM_PLANE_TYPE_CURSOR:
> >> @@ -11962,12 +11952,10 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >> if (IS_IVYBRIDGE(dev) &&
> >> needs_scaling(to_intel_plane_state(plane_state)) &&
> >> !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;
> >> - }
> >>
> >> break;
> >> }
> >> @@ -13509,6 +13497,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;
> >> + }
> >> +
> >> + 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(crtc),
> >> + msecs_to_jiffies(50));
> >> +
> >> + drm_crtc_vblank_put(crtc);
> >> + }
> >> +}
> >> +
> >> +
> >> /**
> >> * intel_atomic_commit - commit validated state object
> >> * @dev: DRM device
> >> @@ -13537,6 +13567,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) {
> >> @@ -13608,8 +13639,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(crtc));
> >> @@ -13623,12 +13655,18 @@ 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))
> >> + crtc_vblank_mask |= 1 << i;
> >> }
> >>
> >> /* FIXME: add subpixel order */
> >>
> >> - drm_atomic_helper_wait_for_vblanks(dev, state);
> >> + 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));
> >>
> >> /*
> >> * Now that the vblank has passed, we can go ahead and program the
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index bdfe4035e074..8940aa4cf50c 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -385,6 +385,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,
> >> @@ -570,7 +571,6 @@ struct intel_crtc_atomic_commit {
> >>
> >> /* Sleepable operations to perform after commit */
> >> unsigned fb_bits;
> >> - bool wait_vblank;
> >> bool update_fbc;
> >> bool post_enable_primary;
> >> unsigned update_sprite_watermarks;
> >> --
> >> 2.1.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list