[RFC PATCH 1/5] drm/i915: Always wait for flip_done
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Thu Aug 31 15:18:06 UTC 2017
Op 30-08-17 om 15:40 schreef Daniel Vetter:
> On Wed, Aug 30, 2017 at 02:54:28PM +0200, Maarten Lankhorst wrote:
>> Op 30-08-17 om 14:43 schreef Daniel Vetter:
>>> 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 at linux.intel.com>
>>>> ---
>>>> 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.
>> Erm, not sure what you mean here, isn't state->legacy_cursor_update mostly false, which means in most cases we'll wait?
> Yup. And the few cases where it's true should be handled by the
> fastpath/async plane update logic. Would allow us to ditch a few
> conditions all over the code.
> -Daniel
I've done some more tests, and it seems this patch is required.
Without this we may not always wait, and that could result in spurious failures of page flip with -EBUSY.
I'd love to hide this from userspace, but I don't think that's possible yet. It's best to always call
drm_atomic_helper_wait_for_flip_done() for now. It's a noop for legacy_cursor_update anyway.
commit->flip_done is always set early.
~Maarten
More information about the dri-devel
mailing list