Op 12-04-18 om 21:41 schreef Souza, Jose: > On Thu, 2018-04-12 at 18:07 +0200, Maarten Lankhorst wrote: >> There is a small race window in which FBC can be enabled after >> pre_plane_update is called, but before the page flip has been >> queued or completed. > I don't think there is such window, intel_fbc_deactivate() that is > called from intel_fbc_pre_update() will set fbc->work.scheduled = > false; so the FBC will not be enabled in intel_fbc_work_fn() Yeah, intel_fbc_pre_update deactivates it, but intel_fbc_flush() can re-enable it. :) >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167 >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_fbc.c | 35 +++++++++++------------------- >> ----- >> 2 files changed, 12 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index a0b8db3db141..2e2f24c2db9e 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -492,6 +492,7 @@ struct intel_fbc { >> >> bool enabled; >> bool active; >> + bool flip_pending; >> >> bool underrun_detected; >> struct work_struct underrun_work; >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c >> b/drivers/gpu/drm/i915/intel_fbc.c >> index b431b6733cc1..4770dd7dad5c 100644 >> --- a/drivers/gpu/drm/i915/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/intel_fbc.c >> @@ -924,13 +924,6 @@ static void intel_fbc_get_reg_params(struct >> intel_crtc *crtc, >> 32 * fbc->threshold) >> * 8; >> } >> >> -static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params >> *params1, >> - struct intel_fbc_reg_params >> *params2) >> -{ >> - /* We can use this since intel_fbc_get_reg_params() does a >> memset. */ >> - return memcmp(params1, params2, sizeof(*params1)) == 0; >> -} >> - >> void intel_fbc_pre_update(struct intel_crtc *crtc, >> struct intel_crtc_state *crtc_state, >> struct intel_plane_state *plane_state) >> @@ -952,6 +945,7 @@ void intel_fbc_pre_update(struct intel_crtc >> *crtc, >> if (!fbc->enabled || fbc->crtc != crtc) >> goto unlock; >> >> + fbc->flip_pending = true; > Also this is not a good name, other actions can cause this function to > be executed other than a flip. Well, for FBC purposes it's a flip, but I am also ok with renaming it to update_pending? :) >> intel_fbc_update_state_cache(crtc, crtc_state, plane_state); >> >> deactivate: >> @@ -988,13 +982,15 @@ static void __intel_fbc_post_update(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_reg_params old_params; >> >> WARN_ON(!mutex_is_locked(&fbc->lock)); >> >> if (!fbc->enabled || fbc->crtc != crtc) >> return; >> >> + fbc->flip_pending = false; >> + WARN_ON(fbc->active); >> + >> if (!i915_modparams.enable_fbc) { >> intel_fbc_deactivate(dev_priv, "disabled at runtime >> per module param"); >> __intel_fbc_disable(dev_priv); >> @@ -1002,25 +998,16 @@ static void __intel_fbc_post_update(struct >> intel_crtc *crtc) >> return; >> } >> >> - if (!intel_fbc_can_activate(crtc)) { >> - WARN_ON(fbc->active); >> + if (!intel_fbc_can_activate(crtc)) >> return; >> - } >> >> - old_params = fbc->params; >> intel_fbc_get_reg_params(crtc, &fbc->params); >> >> - /* If the scanout has not changed, don't modify the FBC >> settings. >> - * Note that we make the fundamental assumption that the fb- >>> obj >> - * cannot be unpinned (and have its GTT offset and fence >> revoked) >> - * without first being decoupled from the scanout and FBC >> disabled. >> - */ >> - if (fbc->active && >> - intel_fbc_reg_params_equal(&old_params, &fbc->params)) >> - return; >> - >> - intel_fbc_deactivate(dev_priv, "FBC enabled (active or >> scheduled)"); >> - intel_fbc_schedule_activation(crtc); >> + if (!fbc->busy_bits) { > I guess this 'if' the line that is fixing the issue. I think that's not necessarily the case for these tests. I don't know if this fixes the bug, as the dirtyfb is called after the atomic update completed. I just noted that after pre_plane_update, you could sneak in a dirtyfb and then fbc could be activated too early. That's the hole I've been trying to close. But I closed it the other way around too just in case. :) >> + intel_fbc_deactivate(dev_priv, "FBC enabled (active >> or scheduled)"); >> + intel_fbc_schedule_activation(crtc); >> + } else >> + intel_fbc_deactivate(dev_priv, "frontbuffer write"); >> } >> >> void intel_fbc_post_update(struct intel_crtc *crtc) >> @@ -1085,7 +1072,7 @@ void intel_fbc_flush(struct drm_i915_private >> *dev_priv, >> (frontbuffer_bits & intel_fbc_get_frontbuffer_bit(fbc))) >> { >> if (fbc->active) >> intel_fbc_recompress(dev_priv); >> - else >> + else if (!fbc->flip_pending) >> __intel_fbc_post_update(fbc->crtc); >> } >> _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
