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

[Zanoni, Paulo R]

Em Qua, 2015-10-21 às 18:31 +0100, chris at chris-wilson.co.uk escreveu:
> 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?

Sure. I guess I just got used to seeing 'nuke' on the specs and forgot
that people without the specs would not know what it means.


> > > 
> > > > +			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?

Will do.

> -Chris
>