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@xxxxxxxxxxxxxxx> >> --- >> 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_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; >> - } > 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. >> >> 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. >> + } >> + >> + 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)); > 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(crtc)); >> @@ -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. >> + 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. 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx