Re: [PATCH] drm/i915: Block enabling FBC until flips have been completed

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: "Souza, Jose" <jose.souza@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/i915: Block enabling FBC until flips have been completed
Date: Fri, 13 Apr 2018 09:19:26 +0200	[thread overview]
Message-ID: <3ca87e4d-0f3a-fafa-27b4-8871afeb6f78@linux.intel.com> (raw)
In-Reply-To: <75b2f362926bba97b55c9cae5a341266937c893a.camel@intel.com>

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);
>>  	}
>>  

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx