On Thu, Oct 22, 2015 at 04:50:05PM +0200, Maarten Lankhorst wrote:
> Op 22-10-15 om 15:30 schreef Ville Syrjälä:
> > On Thu, Oct 22, 2015 at 01:56:28PM +0200, Maarten Lankhorst wrote:
> >> By handling this after the atomic helper waits for vblanks there will
> >> be one less wait for vblank in the atomic path.
> >>
> >> Also get rid of the double wait_for_vblank on broadwell, looks like
> >> it's a bug doing the vblank wait twice.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/intel_display.c | 28 +++-------------------------
> >> drivers/gpu/drm/i915/intel_drv.h | 1 -
> >> 2 files changed, 3 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 051a1e2b1c55..b8e1a5471bed 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4672,14 +4672,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> >> int pipe = intel_crtc->pipe;
> >>
> >> /*
> >> - * 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 (IS_BROADWELL(dev))
> >> - intel_wait_for_vblank(dev, pipe);
> > The right fix for this crap is to not use the flip done interrupt. But
> > apparently no one wants to fix that.
> This is fixed later on when converting page_flip to atomic. I've modified
> page_flip_finished to ignore flips when visible_changed happens on broadwell.
Sounds like another hack.
> > So now I'm worried that we now depend on the atomic helper doing
> > synchornous vblank waits here. Are we expecting those vblank waits to
> > remain there? I would have expected to get rid of all synchronois vblank
> > waits in plane codepaths (apart from ones for hw workarounds).
> In my unify page flip and atomic series I first add all post_plane_update and
> unpinning to unpin_work_fn.
>
> After that I have a patch to convert the last part of atomic after wait_for_vblanks
> to schedule unpin_work_fn, next I'll get more bold and zap the wait_for_vblanks and run it async.
> It depends on work->can_async_unpin, if it's false it has to wait for everything to finish before queueing the next flip,
> so it will finish before next one starts.
>
> > Also does the helper actually wait for vblanks when the plane gets
> > enabled w/o the framebuffer actually changing?
> afaict it does, as long as the crtc is enabled.a
Where is that code? All I see is a old_fb != new_fb check.
>
> I'm guessing that drm_atomic_helper_wait_for_vblanks should be fixed to look at crtc->state->active instead of crtc->state->enable..
>
> >> -
> >> - /*
> >> * FIXME IPS should be fine as long as one plane is
> >> * enabled, but in practice it seems to have problems
> >> * when going from primary only to sprite only and vice
> >> @@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >> struct drm_plane *plane;
> >>
> >> - if (atomic->wait_vblank)
> >> - intel_wait_for_vblank(dev, crtc->pipe);
> >> -
> >> intel_frontbuffer_flip(dev, atomic->fb_bits);
> >>
> >> if (atomic->disable_cxsr)
> >> @@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >> if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> >> intel_crtc->atomic.disable_cxsr = true;
> >> /* to potentially re-enable cxsr */
> >> - intel_crtc->atomic.wait_vblank = true;
> > What's there to make sure cxsr doesn't get enabled/disabled at the wrong
> > time now? I guess this is the same question as for the BDW w/a, ie. does
> > the plane getting enabled/disabled (or rather becoming
> > visible/invisible) always imply a vblank wait?
> crtc mutex lock and in the future atomic wm's..
> >> intel_crtc->atomic.update_wm_post = true;
> >> }
> >> } else if (turn_off) {
> >> intel_crtc->atomic.update_wm_post = true;
> >> /* must disable cxsr around plane enable/disable */
> >> if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> >> - if (is_crtc_enabled)
> >> - intel_crtc->atomic.wait_vblank = true;
> >> intel_crtc->atomic.disable_cxsr = true;
> >> }
> >> } else if (intel_wm_need_update(plane, plane_state)) {
> >> @@ -11716,21 +11702,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >> plane_state->rotation != BIT(DRM_ROTATE_0))
> >> intel_crtc->atomic.disable_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;
> >> -
> >> intel_crtc->atomic.update_fbc |= visible || mode_changed;
> >> break;
> >> case DRM_PLANE_TYPE_CURSOR:
> >> break;
> >> case DRM_PLANE_TYPE_OVERLAY:
> >> if (turn_off && !mode_changed) {
> >> - intel_crtc->atomic.wait_vblank = true;
> >> intel_crtc->atomic.update_sprite_watermarks |=
> >> 1 << i;
> >> }
> >> @@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev,
> >>
> >> if (put_domains)
> >> modeset_put_power_domains(dev_priv, put_domains);
> >> -
> >> - intel_post_plane_update(intel_crtc);
> >> }
> >>
> >> /* FIXME: add subpixel order */
> >>
> >> drm_atomic_helper_wait_for_vblanks(dev, state);
> >>
> >> + for_each_crtc_in_state(state, crtc, crtc_state, i)
> >> + intel_post_plane_update(to_intel_crtc(crtc));
> >> +
> >> mutex_lock(&dev->struct_mutex);
> >> drm_atomic_helper_cleanup_planes(dev, state);
> >> mutex_unlock(&dev->struct_mutex);
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 54a2c0da2ece..4b6bb1adfddd 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -522,7 +522,6 @@ struct intel_crtc_atomic_commit {
> >>
> >> /* Sleepable operations to perform after commit */
> >> unsigned fb_bits;
> >> - bool wait_vblank;
> >> bool update_fbc;
> >> bool post_enable_primary;
> >> unsigned update_sprite_watermarks;
> >> --
> >> 2.1.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
