[Intel-gfx] [PATCH v4 3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Thu Feb 18 14:46:02 UTC 2016
Op 18-02-16 om 15:14 schreef Zanoni, Paulo R:
> 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 at 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.
planes_changed is already used, it means that any plane_state is being updated for this crtc.
fbs_changed could work though, most other *_changed are plural.
>>>>
>>>> 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?
That's what matches the current code, see the calc_changes hunk which had a vblank_wait = true.
>>>> + 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 :)
It would break things though.
I think what might make the most sense is adding a inline function for needs_vblank,
with comments why certain flags require a vblank wait.
>> 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list