On Wed, Jun 27, 2018 at 01:45:04PM +0200, Maarten Lankhorst wrote: > Op 26-06-18 om 19:59 schreef Ville Syrjälä: > > On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote: > >> The only time we should start FBC is when we have waited a vblank > >> after the atomic update. > > What about front buffer tracking? Is this going to leave FBC permanently > > disabled unless there are flips/plane updates? > No, see intel_fbc_flush. If there's a race with frontbuffer tracking and page flip, > we will not enable FBC in intel_fbc_flush(), but in that case we would enable it from > intel_fbc_post_update(). I guess what I haven't quite figured out is why the pre_plane_update() even leaves fbc logically enabled. I would think we should just disable fbc there if anything important is going to change, and then re-enable at post_plane_update() after reallocating the cfb. > > Or the other way around, intel_fbc_post_update won't enable FBC if the fb is dirty, but > intel_fbc_flush() afterwards will. > > I think there are a few cases we need to consider: > > 1. plane update which doesn't need fbc disable > > 2. plane update which needs to disable fbc but can re-enable it after the flip > > has happended (eg. need to reallocate the cfb due to plane size/format change) > > 3. plane update which needs to disable fbc and can't re-enable it (eg. the new > > plane state no longer fbc compatible) > > 4. front buffer invalidate + flush > > > > HW nuke will handle case 1. Case 2 could do the fbc re-enable after the > > post-flip vblank wait. Case 3 would ideally let us move FBC to another > > plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc > > after the flush has happened. > I don't see how this code will break any case. :) OK, so I guess you just remove the delayed activation at flush/post_plane_update. So now it'll enable it immediately. Hmm, and if we just get a flush without the invalidate then we're going to just use the nuke msg register anyway. So I suppose this shouldn't cause much additional fbc on<->off ping pong. Actually, are we now effecitively forcing a synchronous vblank wait in pre_plane_update for every frame via the hw_deactivate() polling for fbc to stop? AFAICS with the re-enable now being immediate we're going to have to disable fbc every single time. I think the correct fix for that would involve a) not disabling fbc around flips when the hw flip nuke will suffice, and b) convert the "wait for fbc to disable" thing into a post_plane_update time assert (and verify whether the hw is actually happy with disabling both fbc and the plane at the same time). > > ~Maarten > > >> We've already forced a vblank wait by doing > >> wait_for_flip_done before intel_post_plane_update(), so we don't need > >> to wait a second time before enabling. > >> > >> Removing the worker simplifies the code and removes possible race > >> conditions, like happening in 103167. > >> > >> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > >> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167 > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > >> --- > >> drivers/gpu/drm/i915/i915_debugfs.c | 5 -- > >> drivers/gpu/drm/i915/i915_drv.h | 6 -- > >> drivers/gpu/drm/i915/intel_fbc.c | 96 +---------------------------- > >> 3 files changed, 1 insertion(+), 106 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > >> index c400f42a54ec..48a57c0636bf 100644 > >> --- a/drivers/gpu/drm/i915/i915_debugfs.c > >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c > >> @@ -1659,11 +1659,6 @@ static int i915_fbc_status(struct seq_file *m, void *unused) > >> else > >> seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason); > >> > >> - if (fbc->work.scheduled) > >> - seq_printf(m, "FBC worker scheduled on vblank %llu, now %llu\n", > >> - fbc->work.scheduled_vblank, > >> - drm_crtc_vblank_count(&fbc->crtc->base)); > >> - > >> if (intel_fbc_is_active(dev_priv)) { > >> u32 mask; > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >> index 328d4312c438..9645dcb30454 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -580,12 +580,6 @@ struct intel_fbc { > >> unsigned int gen9_wa_cfb_stride; > >> } params; > >> > >> - struct intel_fbc_work { > >> - bool scheduled; > >> - u64 scheduled_vblank; > >> - struct work_struct work; > >> - } work; > >> - > >> const char *no_fbc_reason; > >> }; > >> > >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > >> index 9f9ea0b5452f..01d1d2088f04 100644 > >> --- a/drivers/gpu/drm/i915/intel_fbc.c > >> +++ b/drivers/gpu/drm/i915/intel_fbc.c > >> @@ -399,89 +399,6 @@ bool intel_fbc_is_active(struct drm_i915_private *dev_priv) > >> return dev_priv->fbc.active; > >> } > >> > >> -static void intel_fbc_work_fn(struct work_struct *__work) > >> -{ > >> - struct drm_i915_private *dev_priv = > >> - container_of(__work, struct drm_i915_private, fbc.work.work); > >> - struct intel_fbc *fbc = &dev_priv->fbc; > >> - struct intel_fbc_work *work = &fbc->work; > >> - struct intel_crtc *crtc = fbc->crtc; > >> - struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[crtc->pipe]; > >> - > >> - if (drm_crtc_vblank_get(&crtc->base)) { > >> - /* CRTC is now off, leave FBC deactivated */ > >> - mutex_lock(&fbc->lock); > >> - work->scheduled = false; > >> - mutex_unlock(&fbc->lock); > >> - return; > >> - } > >> - > >> -retry: > >> - /* Delay the actual enabling to let pageflipping cease and the > >> - * display to settle before starting the compression. Note that > >> - * this delay also serves a second purpose: it allows for a > >> - * vblank to pass after disabling the FBC before we attempt > >> - * to modify the control registers. > >> - * > >> - * WaFbcWaitForVBlankBeforeEnable:ilk,snb > >> - * > >> - * It is also worth mentioning that since work->scheduled_vblank can be > >> - * updated multiple times by the other threads, hitting the timeout is > >> - * not an error condition. We'll just end up hitting the "goto retry" > >> - * case below. > >> - */ > >> - wait_event_timeout(vblank->queue, > >> - drm_crtc_vblank_count(&crtc->base) != work->scheduled_vblank, > >> - msecs_to_jiffies(50)); > >> - > >> - mutex_lock(&fbc->lock); > >> - > >> - /* Were we cancelled? */ > >> - if (!work->scheduled) > >> - goto out; > >> - > >> - /* Were we delayed again while this function was sleeping? */ > >> - if (drm_crtc_vblank_count(&crtc->base) == work->scheduled_vblank) { > >> - mutex_unlock(&fbc->lock); > >> - goto retry; > >> - } > >> - > >> - intel_fbc_hw_activate(dev_priv); > >> - > >> - work->scheduled = false; > >> - > >> -out: > >> - mutex_unlock(&fbc->lock); > >> - drm_crtc_vblank_put(&crtc->base); > >> -} > >> - > >> -static void intel_fbc_schedule_activation(struct intel_crtc *crtc) > >> -{ > >> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >> - struct intel_fbc *fbc = &dev_priv->fbc; > >> - struct intel_fbc_work *work = &fbc->work; > >> - > >> - WARN_ON(!mutex_is_locked(&fbc->lock)); > >> - if (WARN_ON(!fbc->enabled)) > >> - return; > >> - > >> - if (drm_crtc_vblank_get(&crtc->base)) { > >> - DRM_ERROR("vblank not available for FBC on pipe %c\n", > >> - pipe_name(crtc->pipe)); > >> - return; > >> - } > >> - > >> - /* It is useless to call intel_fbc_cancel_work() or cancel_work() in > >> - * this function since we're not releasing fbc.lock, so it won't have an > >> - * opportunity to grab it to discover that it was cancelled. So we just > >> - * update the expected jiffy count. */ > >> - work->scheduled = true; > >> - work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base); > >> - drm_crtc_vblank_put(&crtc->base); > >> - > >> - schedule_work(&work->work); > >> -} > >> - > >> static void intel_fbc_deactivate(struct drm_i915_private *dev_priv, > >> const char *reason) > >> { > >> @@ -489,11 +406,6 @@ static void intel_fbc_deactivate(struct drm_i915_private *dev_priv, > >> > >> WARN_ON(!mutex_is_locked(&fbc->lock)); > >> > >> - /* Calling cancel_work() here won't help due to the fact that the work > >> - * function grabs fbc->lock. Just set scheduled to false so the work > >> - * function can know it was cancelled. */ > >> - fbc->work.scheduled = false; > >> - > >> if (fbc->active) > >> intel_fbc_hw_deactivate(dev_priv); > >> > >> @@ -1005,7 +917,7 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc) > >> > >> if (!fbc->busy_bits) { > >> intel_fbc_deactivate(dev_priv, "FBC enabled (active or scheduled)"); > >> - intel_fbc_schedule_activation(crtc); > >> + intel_fbc_hw_activate(dev_priv); > >> } else > >> intel_fbc_deactivate(dev_priv, "frontbuffer write"); > >> } > >> @@ -1212,8 +1124,6 @@ void intel_fbc_disable(struct intel_crtc *crtc) > >> if (fbc->crtc == crtc) > >> __intel_fbc_disable(dev_priv); > >> mutex_unlock(&fbc->lock); > >> - > >> - cancel_work_sync(&fbc->work.work); > >> } > >> > >> /** > >> @@ -1235,8 +1145,6 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv) > >> __intel_fbc_disable(dev_priv); > >> } > >> mutex_unlock(&fbc->lock); > >> - > >> - cancel_work_sync(&fbc->work.work); > >> } > >> > >> static void intel_fbc_underrun_work_fn(struct work_struct *work) > >> @@ -1387,12 +1295,10 @@ void intel_fbc_init(struct drm_i915_private *dev_priv) > >> { > >> struct intel_fbc *fbc = &dev_priv->fbc; > >> > >> - INIT_WORK(&fbc->work.work, intel_fbc_work_fn); > >> INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn); > >> mutex_init(&fbc->lock); > >> fbc->enabled = false; > >> fbc->active = false; > >> - fbc->work.scheduled = false; > >> > >> if (need_fbc_vtd_wa(dev_priv)) > >> mkwrite_device_info(dev_priv)->has_fbc = false; > >> -- > >> 2.18.0 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
