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 :)
> } 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".
>
> 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?
> + }
> +
> + 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.
> +
> + drm_crtc_vblank_put(crtc);
> + }
> +}
> +
> +
Two newlines :)
> /**
> * 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.
> + 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.
I'll wait for your answers before reaching any conclusions of what I
think should be done, since I may be misunderstanding something.
> bool post_enable_primary;
> unsigned update_sprite_watermarks;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
