On Wed, Aug 30, 2017 at 02:17:48PM +0200, Maarten Lankhorst wrote:
> The next commit removes the wait for flip_done in in
> drm_atomic_helper_commit_cleanup_done, but we need it for the tests
> to pass. Instead of using complicated vblank tracking which ends
> up being ignored anyway, call the correct atomic helper. :)
>
> RFC because I'm not completely sure what we want to do with the vblank waiting,
> I think for now this patch is the right way to go until we decide because it
> preserves the status quo when drm_crtc_commit was introduced.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_display.c | 83 +++---------------------------------
> 2 files changed, 8 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cbbafbfb0a55..de19621864a9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -707,8 +707,7 @@ struct drm_i915_display_funcs {
> struct drm_atomic_state *old_state);
> void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
> struct drm_atomic_state *old_state);
> - void (*update_crtcs)(struct drm_atomic_state *state,
> - unsigned int *crtc_vblank_mask);
> + void (*update_crtcs)(struct drm_atomic_state *state);
> void (*audio_codec_enable)(struct drm_connector *connector,
> struct intel_encoder *encoder,
> const struct drm_display_mode *adjusted_mode);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 52c73b4dabaa..3f3cb96aa11e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12114,73 +12114,10 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
> return dev->driver->get_vblank_counter(dev, crtc->pipe);
> }
>
> -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 intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> - pipe);
> -
> - if (!((1 << pipe) & crtc_mask))
> - continue;
> -
> - ret = drm_crtc_vblank_get(&crtc->base);
> - if (WARN_ON(ret != 0)) {
> - crtc_mask &= ~(1 << pipe);
> - continue;
> - }
> -
> - last_vblank_count[pipe] = drm_crtc_vblank_count(&crtc->base);
> - }
> -
> - for_each_pipe(dev_priv, pipe) {
> - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> - pipe);
> - long lret;
> -
> - if (!((1 << pipe) & crtc_mask))
> - continue;
> -
> - lret = wait_event_timeout(dev->vblank[pipe].queue,
> - last_vblank_count[pipe] !=
> - drm_crtc_vblank_count(&crtc->base),
> - msecs_to_jiffies(50));
> -
> - WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe));
> -
> - drm_crtc_vblank_put(&crtc->base);
> - }
> -}
> -
> -static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
> -{
> - /* fb updated, need to unpin old fb */
> - if (crtc_state->fb_changed)
> - return true;
> -
> - /* wm changes, need vblank before final wm's */
> - if (crtc_state->update_wm_post)
> - return true;
> -
> - if (crtc_state->wm.need_postvbl_update)
> - return true;
> -
> - return false;
> -}
> -
> static void intel_update_crtc(struct drm_crtc *crtc,
> struct drm_atomic_state *state,
> struct drm_crtc_state *old_crtc_state,
> - struct drm_crtc_state *new_crtc_state,
> - unsigned int *crtc_vblank_mask)
> + struct drm_crtc_state *new_crtc_state)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -12203,13 +12140,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
> }
>
> drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
> -
> - if (needs_vblank_wait(pipe_config))
> - *crtc_vblank_mask |= drm_crtc_mask(crtc);
> }
>
> -static void intel_update_crtcs(struct drm_atomic_state *state,
> - unsigned int *crtc_vblank_mask)
> +static void intel_update_crtcs(struct drm_atomic_state *state)
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> @@ -12220,12 +12153,11 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
> continue;
>
> intel_update_crtc(crtc, state, old_crtc_state,
> - new_crtc_state, crtc_vblank_mask);
> + new_crtc_state);
> }
> }
>
> -static void skl_update_crtcs(struct drm_atomic_state *state,
> - unsigned int *crtc_vblank_mask)
> +static void skl_update_crtcs(struct drm_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> @@ -12284,7 +12216,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> vbl_wait = true;
>
> intel_update_crtc(crtc, state, old_crtc_state,
> - new_crtc_state, crtc_vblank_mask);
> + new_crtc_state);
>
> if (vbl_wait)
> intel_wait_for_vblank(dev_priv, pipe);
> @@ -12346,7 +12278,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> struct intel_crtc_state *intel_cstate;
> bool hw_check = intel_state->modeset;
> u64 put_domains[I915_MAX_PIPES] = {};
> - unsigned crtc_vblank_mask = 0;
> int i;
>
> intel_atomic_commit_fence_wait(intel_state);
> @@ -12438,7 +12369,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> pm_qos_update_request(&dev_priv->pm_qos_atomic, 0);
>
> /* Now enable the clocks, plane, pipe, and connectors that we set up. */
> - dev_priv->display.update_crtcs(state, &crtc_vblank_mask);
> + dev_priv->display.update_crtcs(state);
>
> /* FIXME: We should call drm_atomic_helper_commit_hw_done() here
> * already, but still need the state for the delayed optimization. To
> @@ -12450,7 +12381,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> * we don't need out special handling any more.
> */
> if (!state->legacy_cursor_update)
I think this if is dead code, since either we use the fastpath in
intel_legacy_cursor_update, which means no atomic commit. Or we don't, and
then we clear this hack (on most platforms at least) since it just wreaks
complete havoc.
We should probably do this logic in the async helpers, i.e. if async
helpers reject a cursor update, clear state->legacy_cursor_update.
Since I think we don't necessarily need this patch in this series, I think
it'd be better in a separate series to straighten out the async_update
story. Which would then include moving i915 over to the core support.
-Daniel
> - intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
> + drm_atomic_helper_wait_for_flip_done(dev, state);
>
> /*
> * Now that the vblank has passed, we can go ahead and program the
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
