| Message ID | 1458588418-31129-2-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 21, 2016 at 04:26:54PM -0300, Paulo Zanoni wrote: > We ignore ORIGIN_GTT because the hardware tracking can recognize GTT > writes and take care of them. We also ignore ORIGIN_FLIP because we > deal with flips without relying on the frontbuffer tracking > infrastructure. On the other hand, a flush is a flush and means we're > good to go, so we need to update busy_bits in order to reflect that, > even if we're not going to do anything else about it. > > How to reproduce the bug fixed by this patch: > - boot SKL up to the desktop environment > - stop the display manager > - run any of the igt/kms_frontbuffer_tracking/*fbc*onoff* subtests > - the tests will fail > > The steps above will create the right conditions for us to lose track > of busy_bits. If you, for example, run the full set of FBC tests, the > onoff subtests will succeed. > > Also notice that the "bug" is that we'll just keep FBC disabled on > cases where it could be enabled, so it's not something the users can > perceive, it just affects power consumption numbers on properly > configured machines. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Is this covered by your nasty igt test suite? Kernel side looks good, so with appropriate Testcase: tag added: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_fbc.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 2e571f5..b8ba79c 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -996,13 +996,13 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, > if (!fbc_supported(dev_priv)) > return; > > - if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP) > - return; > - > mutex_lock(&fbc->lock); > > fbc->busy_bits &= ~frontbuffer_bits; > > + if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP) > + goto out; > + > if (!fbc->busy_bits && fbc->enabled && > (frontbuffer_bits & intel_fbc_get_frontbuffer_bit(fbc))) { > if (fbc->active) > @@ -1011,6 +1011,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, > __intel_fbc_post_update(fbc->crtc); > } > > +out: > mutex_unlock(&fbc->lock); > } > > -- > 2.7.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Ter, 2016-03-22 às 12:13 +0100, Daniel Vetter escreveu: > On Mon, Mar 21, 2016 at 04:26:54PM -0300, Paulo Zanoni wrote: > > > > We ignore ORIGIN_GTT because the hardware tracking can recognize > > GTT > > writes and take care of them. We also ignore ORIGIN_FLIP because we > > deal with flips without relying on the frontbuffer tracking > > infrastructure. On the other hand, a flush is a flush and means > > we're > > good to go, so we need to update busy_bits in order to reflect > > that, > > even if we're not going to do anything else about it. > > > > How to reproduce the bug fixed by this patch: > > - boot SKL up to the desktop environment > > - stop the display manager > > - run any of the igt/kms_frontbuffer_tracking/*fbc*onoff* subtests > > - the tests will fail > > > > The steps above will create the right conditions for us to lose > > track > > of busy_bits. If you, for example, run the full set of FBC tests, > > the > > onoff subtests will succeed. > > > > Also notice that the "bug" is that we'll just keep FBC disabled on > > cases where it could be enabled, so it's not something the users > > can > > perceive, it just affects power consumption numbers on properly > > configured machines. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > Is this covered by your nasty igt test suite? Kernel side looks good, > so > with appropriate Testcase: tag added: I mentioned the tests in the middle of the commit message, but forgot the Testcase tags. I'll add them. > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Thanks! > > > > > --- > > drivers/gpu/drm/i915/intel_fbc.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > b/drivers/gpu/drm/i915/intel_fbc.c > > index 2e571f5..b8ba79c 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -996,13 +996,13 @@ void intel_fbc_flush(struct drm_i915_private > > *dev_priv, > > if (!fbc_supported(dev_priv)) > > return; > > > > - if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP) > > - return; > > - > > mutex_lock(&fbc->lock); > > > > fbc->busy_bits &= ~frontbuffer_bits; > > > > + if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP) > > + goto out; > > + > > if (!fbc->busy_bits && fbc->enabled && > > (frontbuffer_bits & > > intel_fbc_get_frontbuffer_bit(fbc))) { > > if (fbc->active) > > @@ -1011,6 +1011,7 @@ void intel_fbc_flush(struct drm_i915_private > > *dev_priv, > > __intel_fbc_post_update(fbc->crtc); > > } > > > > +out: > > mutex_unlock(&fbc->lock); > > } > > > > -- > > 2.7.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 2e571f5..b8ba79c 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -996,13 +996,13 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, if (!fbc_supported(dev_priv)) return; - if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP) - return; - mutex_lock(&fbc->lock); fbc->busy_bits &= ~frontbuffer_bits; + if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP) + goto out; + if (!fbc->busy_bits && fbc->enabled && (frontbuffer_bits & intel_fbc_get_frontbuffer_bit(fbc))) { if (fbc->active) @@ -1011,6 +1011,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, __intel_fbc_post_update(fbc->crtc); } +out: mutex_unlock(&fbc->lock); }
We ignore ORIGIN_GTT because the hardware tracking can recognize GTT writes and take care of them. We also ignore ORIGIN_FLIP because we deal with flips without relying on the frontbuffer tracking infrastructure. On the other hand, a flush is a flush and means we're good to go, so we need to update busy_bits in order to reflect that, even if we're not going to do anything else about it. How to reproduce the bug fixed by this patch: - boot SKL up to the desktop environment - stop the display manager - run any of the igt/kms_frontbuffer_tracking/*fbc*onoff* subtests - the tests will fail The steps above will create the right conditions for us to lose track of busy_bits. If you, for example, run the full set of FBC tests, the onoff subtests will succeed. Also notice that the "bug" is that we'll just keep FBC disabled on cases where it could be enabled, so it's not something the users can perceive, it just affects power consumption numbers on properly configured machines. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_fbc.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)