2015-06-18 6:23 GMT-03:00 Daniel Vetter <daniel.vetter@xxxxxxxx>: > The current/old frontbuffer might still have gpu frontbuffer rendering > pending. But once flipped it won't have the corresponding frontbuffer > bits any more and hence the request retire function won't ever clear > the corresponding busy bits. The async flip tracking (with the > flip_prepare and flip_complete functions) already does this, but > somehow I've forgotten to do this for synchronous flips. > > Note that we don't track outstanding rendering of the new framebuffer > with busy_bits since all our plane update code waits for previous > rendering to complete before displaying a new buffer. Hence a new > buffer will never be busy. Steps to reproduce: - Boot your machine to the display manager - Kill it, returning to fbcon - Try to run any subtest from igt/tests/kms_frontbuffer_tracking: they all fail because FBC never gets enabled - Do any blt/render operation to the frontbuffer - Now the tests will succeed I also submitted a new test for this, that doesn't require the steps above, so we can also add: Testcase: igt/kms_frontbuffer_tracking/fbc-modesetfrombusy More below: > > v2: Drop the spurious inline Ville spotted. > > Reported-by: Paulo Zanoni <przanoni@xxxxxxxxx> > Cc: Paulo Zanoni <przanoni@xxxxxxxxx> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > --- > Paulo promised to also extend kms_frontbuffer_tracking with flip vs. > busy buffer tests. > --- > drivers/gpu/drm/i915/intel_drv.h | 18 ++---------------- > drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++ > 2 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index bcafefcf048b..b7c69460fb20 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev, > unsigned frontbuffer_bits); > void intel_frontbuffer_flush(struct drm_device *dev, > unsigned frontbuffer_bits); > -/** > - * intel_frontbuffer_flip - synchronous frontbuffer flip > - * @dev: DRM device > - * @frontbuffer_bits: frontbuffer plane tracking bits > - * > - * This function gets called after scheduling a flip on @obj. This is for > - * synchronous plane updates which will happen on the next vblank and which will > - * not get delayed by pending gpu rendering. > - * > - * Can be called without any locks held. > - */ > -static inline > +inline > void intel_frontbuffer_flip(struct drm_device *dev, > - unsigned frontbuffer_bits) > -{ > - intel_frontbuffer_flush(dev, frontbuffer_bits); > -} > + unsigned frontbuffer_bits); > > unsigned int intel_fb_align_height(struct drm_device *dev, > unsigned int height, > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c > index 57095f54c1f2..c3559fde25e5 100644 > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c > @@ -270,3 +270,29 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev, > > intel_frontbuffer_flush(dev, frontbuffer_bits); > } > + > +/** > + * intel_frontbuffer_flip - synchronous frontbuffer flip > + * @dev: DRM device > + * @frontbuffer_bits: frontbuffer plane tracking bits > + * > + * This function gets called after scheduling a flip on @obj. This is for > + * synchronous plane updates which will happen on the next vblank and which will > + * not get delayed by pending gpu rendering. > + * > + * Can be called without any locks held. > + */ > + > +void intel_frontbuffer_flip(struct drm_device *dev, > + unsigned frontbuffer_bits) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + mutex_lock(&dev_priv->fb_tracking.lock); > + dev_priv->fb_tracking.flip_bits |= frontbuffer_bits; I don't understand the purpose of the line above. Is this really necessary? It seems that flip_bits is only necessary for the asynchronous flip case, where someone schedules a flip then draws on the buffer, so we don't flush the buffer at flip_complete. We do unconditional flush at intel_frontbuffer_flip(), so it seems like we shouldn't be touching flip_bits here. I tested both this version and the version without the flip_bits change, and both appear to solve the problem I can reproduce, so for both: Tested-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> And, with the flip_bits problem explained or fixed: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > + /* Remove stale busy bits due to the old buffer. */ > + dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits; > + mutex_unlock(&dev_priv->fb_tracking.lock); > + > + intel_frontbuffer_flush(dev, frontbuffer_bits); > +} > -- > 2.1.0 > -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx