| Message ID | 1445349004-16409-4-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. > + else > + __intel_fbc_update(dev_priv); > } > } This becomes if (enabled && origin != ORIGIN_FLIP) intel_fbc_nuke(); else __intel_fbc_update(); It seems a little odd that anything is done if disabled, so care to elaborate that reason, and I presume there is an equally good comment before the context that explains why FLIP is special? -Chris
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@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 >
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@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
Em Qua, 2015-10-21 às 18:31 +0100, chris@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@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 >
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); + else + __intel_fbc_update(dev_priv); } }
There's no need to stop and restart FBC: a nuke should be fine. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_fbc.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)