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. >> 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 <[email protected]> >> --- >> 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 ? >> + >> 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 >> [email protected] >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
