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@xxxxxxxxx>
> > > ---
> > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
