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

[chris at chris-wilson.co.uk]

On Wed, Oct 21, 2015 at 05:08:42PM +0000, Zanoni, Paulo R wrote:
> 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.

Maybe intel_fbc_recompress(), that seems a little more obvious than nuke?

> > 
> > > +			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".

Sure.

> > 
> > 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.

I like this comment. Care to add it to the function?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre