[Intel-gfx] [PATCH v2 14/14] drm/i915/fbc: Reallocate cfb if we need more of it

[Maarten Lankhorst]

Op 29-11-2019 om 12:37 schreef Ville Syrjälä:
> On Fri, Nov 29, 2019 at 09:48:45AM +0100, Maarten Lankhorst wrote:
>> Op 28-11-2019 om 16:59 schreef Ville Syrjälä:
>>> On Thu, Nov 28, 2019 at 04:48:04PM +0100, Maarten Lankhorst wrote:
>>>> Op 27-11-2019 om 21:12 schreef Ville Syrjala:
>>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>>
>>>>> The code assumes we can omit the cfb allocation once fbc
>>>>> has been enabled once. That's nonsense. Let's try to
>>>>> reallocate it if we need to.
>>>>>
>>>>> The code is still a mess, but maybe this is enough to get
>>>>> fbc going in some cases where it initially underallocates
>>>>> the cfb and there's no full modeset to fix it up.
>>>>>
>>>>> Cc: Daniel Drake <drake at endlessm.com>
>>>>> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>>>> Cc: Jian-Hong Pan <jian-hong at endlessm.com>
>>>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/display/intel_fbc.c | 22 +++++++++++++++-------
>>>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
>>>>> index c976698b0729..928059a5da80 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
>>>>> @@ -672,6 +672,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>>>>>  		cache->fence_id = -1;
>>>>>  }
>>>>>  
>>>>> +static bool intel_fbc_cfb_size_changed(struct drm_i915_private *dev_priv)
>>>>> +{
>>>>> +	struct intel_fbc *fbc = &dev_priv->fbc;
>>>>> +
>>>>> +	return intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) >
>>>>> +		fbc->compressed_fb.size * fbc->threshold;
>>>>> +}
>>>>> +
>>>>>  static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>>>>>  {
>>>>>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>>> @@ -757,8 +765,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>>>>>  	 * we didn't get any invalidate/deactivate calls, but this would require
>>>>>  	 * a lot of tracking just for a specific case. If we conclude it's an
>>>>>  	 * important case, we can implement it later. */
>>>>> -	if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) >
>>>>> -	    fbc->compressed_fb.size * fbc->threshold) {
>>>>> +	if (intel_fbc_cfb_size_changed(dev_priv)) {
>>>>>  		fbc->no_fbc_reason = "CFB requirements changed";
>>>>>  		return false;
>>>>>  	}
>>>>> @@ -1112,12 +1119,12 @@ void intel_fbc_enable(struct intel_crtc *crtc,
>>>>>  	mutex_lock(&fbc->lock);
>>>>>  
>>>>>  	if (fbc->crtc) {
>>>>> -		WARN_ON(fbc->crtc == crtc && !crtc_state->enable_fbc);
>>>>> -		goto out;
>>>>> -	}
>>>>> +		if (fbc->crtc != crtc ||
>>>>> +		    !intel_fbc_cfb_size_changed(dev_priv))
>>>>> +			goto out;
>>>>>  
>>>>> -	if (!crtc_state->enable_fbc)
>>>>> -		goto out;
>>>>> +		__intel_fbc_disable(dev_priv);
>>>>> +	}
>>>>>  
>>>>>  	WARN_ON(fbc->active);
>>>>>  
>>>>> @@ -1130,6 +1137,7 @@ void intel_fbc_enable(struct intel_crtc *crtc,
>>>>>  	if (intel_fbc_alloc_cfb(dev_priv,
>>>>>  				intel_fbc_calculate_cfb_size(dev_priv, cache),
>>>>>  				fb->format->cpp[0])) {
>>>>> +		cache->plane.visible = false;
>>>>>  		fbc->no_fbc_reason = "not enough stolen memory";
>>>>>  		goto out;
>>>>>  	}
>>>> Makes sense, unfortunately kms_cursor_legacy starts failing on this series. :(
>>>>
>>>> For 1-11, 14
>>>>
>>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>>
>>>> We should probably get rid of the FBC disable on frontbuffer disable as well. I had some patches but nothing upstream-worthy yet. :(
>>> How would we get rid of the disable there? By triggering nukes at some
>>> predefined interval? Doesn't sound all that great.
>> Not touching FBC on frontbuffer write at all, and forcing userspace to use the dirtyfb api. I think the whole implicit tracking should be removed.
> Perhaps. Not sure userspace is ready for that though.

We have to audit that DirtyFB is called on all gen9+ userspace, because FBC is only enabled by default on those platforms.

I know the modesetting ddx does, I believe xf86-video-intel as well. So it should be safe to do. We could hide the old behavior behind a kernel parameter for now for 1 or 2 releases,

so we can chicken out if needed.

> I guess the only long lasting frontbuffer invalidate is the
> one from set_domain. Everything else is bounded and so we
> know the flush is going to come in a somewhat timely manner.
> So for those cases I guess we could perhaps skip the invalidate.
>
> Hmm. Also looks like ORIGIN_GTT has been neutered and now
> we treat everyting as ORIGIN_CPU. That's maybe not so great.
> Should probably reinstate ORIGIN_GTT so we can actually benefit
> from the hw gtt tracking. Or we just try to kill that off as well.
HW tracking has been buggy for a long time, and is no longer available on current hw because of those bugs.
> Also I wonder where is the flush counterpart to the invalidate
> in i915_gem_object_prepare_write()?
>
Not sure.