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.lankho...@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);
>> }
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
