[Intel-gfx] [PATCH 03/18] drm/i915: only nuke FBC when a drawing operation triggers a flush

[Zanoni, Paulo R]

Em Ter, 2015-10-20 às 16:59 +0100, Chris Wilson escreveu:
> On Tue, Oct 20, 2015 at 11:49:49AM -0200, Paulo Zanoni wrote:
> > There's no need to stop and restart FBC: a nuke should be fine.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index 9477379..b9cfd16 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -1088,8 +1088,10 @@ void intel_fbc_flush(struct drm_i915_private
> > *dev_priv,
> >  		if (origin == ORIGIN_FLIP) {
> >  			__intel_fbc_update(dev_priv);
> >  		} else {
> > -			__intel_fbc_disable(dev_priv);
> > -			__intel_fbc_update(dev_priv);
> > +			if (dev_priv->fbc.enabled)
> > +				intel_fbc_nuke(dev_priv);
> 
> Ok, what does nuke actually do? From the name, I would expect FBC to
> be
> left in an unusable state.

As far as I understand, it triggers a full recompression of the CFB. It
should be equivalent to disable+reenable.

> 
> > +			else
> > +				__intel_fbc_update(dev_priv);
> >  		}
> >  	}
> 
> This becomes
> 
> if (enabled && origin != ORIGIN_FLIP)
>   intel_fbc_nuke();
> else
>   __intel_fbc_update();

Now I see this code could definitely have been made simpler... Fixing
this here would require me to redo many of the next patches. I hope you
accept patch 19/18 as a possible "fix".

> 
> It seems a little odd that anything is done if disabled, so care to
> elaborate that reason

When we're drawing on the frontbuffer we may get an invalidate() call
first, which will trigger an FBC deactivation. Then later we'll get a
flush() and will have to reenable. Sometimes we may just get the
flush() without the previous invalidate(), and for this case a nuke is
the easiest thing to do. That's all just the normal frontbuffer
tracking mechanism.


> , and I presume there is an equally good comment
> before the context that explains why FLIP is special?

It's just that we ignore flushes() for the FLIP case if FBC is active
due to the hardware tracking, which automatically does a nuke. There's
a check for this earlier on this function, which you can't see on this
diff context but you can see on patch 02/18. So if origin is FLIP, and
FBC is active, we return early.

> -Chris
>