| Message ID | 20180412160718.4618-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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() > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > 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. > 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. > + 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); > } >
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@linux.intel.com> >> 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); >> } >>
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; 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) { + 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); }
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. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> 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(-)