| Message ID | 1435672392-7329-6-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 30, 2015 at 10:53:09AM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > The problem with calling intel_fbc_update() at flush is that it fully > rechecks and recomputes the FBC state, and that includes reallocating > the CFB, which requires a struct_mutex lock that we don't always have. Actually, that is something we should fix with a stolen_mutex (will be needed elsewhere). > The lack of struct_mutex lock can be considered a regression from: > > commit dbef0f15b5c83231dacb214dbf9a6dba063ca21c > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > Date: Fri Feb 13 17:23:46 2015 -0200 > drm/i915: add frontbuffer tracking to FBC > > So introduce intel_fbc_stop() that doesn't unset fbc.crtc, then call > stop/enable at invalidate/flush. > > Notice that invalidate/flush is only called by the frontbuffer > tracking infrastrucutre, so it's safe to not do a full > intel_fbc_update() here. I think intel_fbc_update() is confusing. I can understand start/stop, but update to me is more akin to a flush. If it was called flush, then I would not expect it to enable or disable fbc or do any reallocations at all. static void intel_fbc_flush(struct drm_device *dev) { if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) { if (dev_priv->fbc.enabled) intel_fbc_stop(dev); intel_fbc_enable(&dev_priv->fbc.crtc->base); } } And now I look at enable/disable, start/stop and am confused as to what all the stages actually are. I presume that start/stop are the highest, and control the sw state. And that enable/disable are just hw interaction. And who sets fbc.enabled? start()? enable()? disable()? stop()? In confusion, -Chris
2015-06-30 11:34 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Tue, Jun 30, 2015 at 10:53:09AM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> The problem with calling intel_fbc_update() at flush is that it fully >> rechecks and recomputes the FBC state, and that includes reallocating >> the CFB, which requires a struct_mutex lock that we don't always have. > > Actually, that is something we should fix with a stolen_mutex (will be > needed elsewhere). > >> The lack of struct_mutex lock can be considered a regression from: >> >> commit dbef0f15b5c83231dacb214dbf9a6dba063ca21c >> Author: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Date: Fri Feb 13 17:23:46 2015 -0200 >> drm/i915: add frontbuffer tracking to FBC >> >> So introduce intel_fbc_stop() that doesn't unset fbc.crtc, then call >> stop/enable at invalidate/flush. >> >> Notice that invalidate/flush is only called by the frontbuffer >> tracking infrastrucutre, so it's safe to not do a full >> intel_fbc_update() here. > > I think intel_fbc_update() is confusing. I can understand start/stop, > but update to me is more akin to a flush. If it was called flush, then I > would not expect it to enable or disable fbc or do any reallocations at > all. > > static void intel_fbc_flush(struct drm_device *dev) > { > if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) { > if (dev_priv->fbc.enabled) > intel_fbc_stop(dev); > intel_fbc_enable(&dev_priv->fbc.crtc->base); > } > } > > And now I look at enable/disable, start/stop and am confused as to what > all the stages actually are. > > I presume that start/stop are the highest, and control the sw state. And > that enable/disable are just hw interaction. And who sets fbc.enabled? > start()? enable()? disable()? stop()? > > In confusion, I understand your concerns and I agree with you that this is confusing. I also agree that the addition of stop() makes things even worse. One of the problems is that intel_fbc_update() does "everything": it picks the CRTC, it can enable FBC, it can disable FBC, it can change the CRTC, etc. So we have: update(), enable(), disable(), flush() and invalidate(), and the patch added stop(). I had some patches that would move us to enable/disable (high level) activate/deactivate (low level), flush/invalidate (wrappers for activate/deactivate) and update (highest level). This would make the naming scheme similar to PSR. I wanted to merge the locking fixes first, but I can put everything on the same series if you want. Or leave this patch out of the "locking" series and add it to the next series... > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Tue, Jun 30, 2015 at 06:12:59PM -0300, Paulo Zanoni wrote: > 2015-06-30 11:34 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > I presume that start/stop are the highest, and control the sw state. And > > that enable/disable are just hw interaction. And who sets fbc.enabled? > > start()? enable()? disable()? stop()? > > > > In confusion, > > I understand your concerns and I agree with you that this is > confusing. I also agree that the addition of stop() makes things even > worse. One of the problems is that intel_fbc_update() does > "everything": it picks the CRTC, it can enable FBC, it can disable > FBC, it can change the CRTC, etc. So we have: update(), enable(), > disable(), flush() and invalidate(), and the patch added stop(). > > I had some patches that would move us to enable/disable (high level) > activate/deactivate (low level), flush/invalidate (wrappers for > activate/deactivate) and update (highest level). This would make the > naming scheme similar to PSR. I wanted to merge the locking fixes > first, but I can put everything on the same series if you want. Or > leave this patch out of the "locking" series and add it to the next > series... To keep it minimal, a quick outline comment telling me the layers and ordering would be of use right now to review the patches, and longer term to review the code. -Chris
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 316feb1..0a66814 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -422,7 +422,7 @@ static void intel_fbc_enable(struct drm_crtc *crtc) schedule_delayed_work(&work->work, msecs_to_jiffies(50)); } -static void __intel_fbc_disable(struct drm_device *dev) +static void intel_fbc_stop(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -434,6 +434,13 @@ static void __intel_fbc_disable(struct drm_device *dev) return; dev_priv->display.disable_fbc(dev); +} + +static void __intel_fbc_disable(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + intel_fbc_stop(dev); dev_priv->fbc.crtc = NULL; } @@ -753,7 +760,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits); if (dev_priv->fbc.busy_bits) - __intel_fbc_disable(dev); + intel_fbc_stop(dev); mutex_unlock(&dev_priv->fbc.lock); } @@ -770,8 +777,11 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, dev_priv->fbc.busy_bits &= ~frontbuffer_bits; - if (!dev_priv->fbc.busy_bits) - __intel_fbc_update(dev); + if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) { + if (dev_priv->fbc.enabled) + intel_fbc_stop(dev); + intel_fbc_enable(&dev_priv->fbc.crtc->base); + } out: mutex_unlock(&dev_priv->fbc.lock);