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@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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx