| Message ID | 20180406205349.8949-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting José Roberto de Souza (2018-04-06 21:53:49) > A simple page flip can cause the CFB required size to increase and > if it is bigger than the currently allocated CFB it needs to be > resized to activate FBC again. I would have expected the answer to be to plug into atomic. During the prepare phase, you evaluate the planes for compatibility with FBC, and record what actions you plan to take in the commit (including allocating the next slab of stolen if required). Everything now goes through the same atomic prepare/commit, so it should be possible to eliminate all guess work. Hopefully Maarten can either explain it better or why it shouldn't be done that at all. ;) -Chris
On Fri, 06 Apr 2018, José Roberto de Souza <jose.souza@intel.com> wrote: > A simple page flip can cause the CFB required size to increase and > if it is bigger than the currently allocated CFB it needs to be > resized to activate FBC again. > > Until now this case was not being handled but CI is starting to > get some of this errors. > > So here it will free the old CFB and a try to allocate the required > CFB in the schedule activation work, it will happen after one vblank > so is guarantee that FBC was completed disabled and is not using CFB. > > Also in case that there is no enough stolen memory to allocate the > new CFB it will try 3 times per full modeset as the CFB requirement > could be reduced in the next non-full modeset. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105683 > --- > drivers/gpu/drm/i915/i915_drv.h | 3 +++ > drivers/gpu/drm/i915/intel_fbc.c | 46 +++++++++++++++++++++----------- > 2 files changed, 33 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 5373b171bb96..4ce19b45f67d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -566,6 +566,9 @@ struct intel_fbc { > } work; > > const char *no_fbc_reason; > + > + bool cfb_try_resize; > + u8 cfb_resize_tries_left; > }; > > /* > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 573b034a02fd..7d77936db3ec 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -41,6 +41,9 @@ > #include "intel_drv.h" > #include "i915_drv.h" > > +static void __intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv); > +static int intel_fbc_alloc_cfb(struct intel_crtc *crtc); > + > static inline bool fbc_supported(struct drm_i915_private *dev_priv) > { > return HAS_FBC(dev_priv); > @@ -446,6 +449,15 @@ static void intel_fbc_work_fn(struct work_struct *__work) > goto retry; > } > > + if (fbc->cfb_try_resize && fbc->cfb_resize_tries_left) { > + __intel_fbc_cleanup_cfb(dev_priv); > + if (intel_fbc_alloc_cfb(crtc)) { > + fbc->no_fbc_reason = "not enough stolen memory"; > + fbc->cfb_resize_tries_left--; > + goto out; > + } > + } > + > intel_fbc_hw_activate(dev_priv); > > work->scheduled = false; > @@ -850,22 +862,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > return false; > } > > - /* It is possible for the required CFB size change without a > - * crtc->disable + crtc->enable since it is possible to change the > - * stride without triggering a full modeset. Since we try to > - * over-allocate the CFB, there's a chance we may keep FBC enabled even > - * if this happens, but if we exceed the current CFB size we'll have to > - * disable FBC. Notice that it would be possible to disable FBC, wait > - * for a frame, free the stolen node, then try to reenable FBC in case > - * we didn't get any invalidate/deactivate calls, but this would require > - * a lot of tracking just for a specific case. If we conclude it's an > - * important case, we can implement it later. */ > - if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > - fbc->compressed_fb.size * fbc->threshold) { > - fbc->no_fbc_reason = "CFB requirements changed"; > - return false; > - } > - > /* > * Work around a problem on GEN9+ HW, where enabling FBC on a plane > * having a Y offset that isn't divisible by 4 causes FIFO underrun > @@ -877,6 +873,23 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > return false; > } > > + /* It is possible for the required CFB size change without a > + * crtc->disable + crtc->enable since it is possible to change the > + * stride without triggering a full modeset. Since we try to > + * over-allocate the CFB, there's a chance we may keep FBC enabled even > + * if this happens, but if we exceed the current CFB size we'll have to > + * resize CFB. > + */ > + if (!drm_mm_node_allocated(&fbc->compressed_fb) || > + (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > + fbc->compressed_fb.size * fbc->threshold)) { > + fbc->cfb_try_resize = true; > + DRM_DEBUG_KMS("CFB requirements have changed, activation \ > + work will try to resize it"); Either the string entirely on one line, or the string split to two, but please never line continuation within the string. BR, Jani. > + } else { > + fbc->cfb_try_resize = false; > + } > + > return true; > } > > @@ -1208,6 +1221,7 @@ void intel_fbc_enable(struct intel_crtc *crtc, > > fbc->enabled = true; > fbc->crtc = crtc; > + fbc->cfb_resize_tries_left = 3; > out: > mutex_unlock(&fbc->lock); > }
On Fri, 2018-04-06 at 22:03 +0100, Chris Wilson wrote: > Quoting José Roberto de Souza (2018-04-06 21:53:49) > > A simple page flip can cause the CFB required size to increase and > > if it is bigger than the currently allocated CFB it needs to be > > resized to activate FBC again. > > I would have expected the answer to be to plug into atomic. During > the > prepare phase, you evaluate the planes for compatibility with FBC, > and > record what actions you plan to take in the commit (including > allocating > the next slab of stolen if required). Everything now goes through the > same atomic prepare/commit, so it should be possible to eliminate all > guess work. > > Hopefully Maarten can either explain it better or why it shouldn't be > done that at all. ;) > -Chris Hi Chris You mean check if FBC needs to resize CFB inside intel_crtc_compute_config() if so set state->mode_changed=true, to cause a full modeset? This way intel_fbc_disable() and intel_fbc_enable() will be called. Yeah looks better than current solution, do you have any objections Paulo? Thanks
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5373b171bb96..4ce19b45f67d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -566,6 +566,9 @@ struct intel_fbc { } work; const char *no_fbc_reason; + + bool cfb_try_resize; + u8 cfb_resize_tries_left; }; /* diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 573b034a02fd..7d77936db3ec 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -41,6 +41,9 @@ #include "intel_drv.h" #include "i915_drv.h" +static void __intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv); +static int intel_fbc_alloc_cfb(struct intel_crtc *crtc); + static inline bool fbc_supported(struct drm_i915_private *dev_priv) { return HAS_FBC(dev_priv); @@ -446,6 +449,15 @@ static void intel_fbc_work_fn(struct work_struct *__work) goto retry; } + if (fbc->cfb_try_resize && fbc->cfb_resize_tries_left) { + __intel_fbc_cleanup_cfb(dev_priv); + if (intel_fbc_alloc_cfb(crtc)) { + fbc->no_fbc_reason = "not enough stolen memory"; + fbc->cfb_resize_tries_left--; + goto out; + } + } + intel_fbc_hw_activate(dev_priv); work->scheduled = false; @@ -850,22 +862,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) return false; } - /* It is possible for the required CFB size change without a - * crtc->disable + crtc->enable since it is possible to change the - * stride without triggering a full modeset. Since we try to - * over-allocate the CFB, there's a chance we may keep FBC enabled even - * if this happens, but if we exceed the current CFB size we'll have to - * disable FBC. Notice that it would be possible to disable FBC, wait - * for a frame, free the stolen node, then try to reenable FBC in case - * we didn't get any invalidate/deactivate calls, but this would require - * a lot of tracking just for a specific case. If we conclude it's an - * important case, we can implement it later. */ - if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > - fbc->compressed_fb.size * fbc->threshold) { - fbc->no_fbc_reason = "CFB requirements changed"; - return false; - } - /* * Work around a problem on GEN9+ HW, where enabling FBC on a plane * having a Y offset that isn't divisible by 4 causes FIFO underrun @@ -877,6 +873,23 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) return false; } + /* It is possible for the required CFB size change without a + * crtc->disable + crtc->enable since it is possible to change the + * stride without triggering a full modeset. Since we try to + * over-allocate the CFB, there's a chance we may keep FBC enabled even + * if this happens, but if we exceed the current CFB size we'll have to + * resize CFB. + */ + if (!drm_mm_node_allocated(&fbc->compressed_fb) || + (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > + fbc->compressed_fb.size * fbc->threshold)) { + fbc->cfb_try_resize = true; + DRM_DEBUG_KMS("CFB requirements have changed, activation \ + work will try to resize it"); + } else { + fbc->cfb_try_resize = false; + } + return true; } @@ -1208,6 +1221,7 @@ void intel_fbc_enable(struct intel_crtc *crtc, fbc->enabled = true; fbc->crtc = crtc; + fbc->cfb_resize_tries_left = 3; out: mutex_unlock(&fbc->lock); }
A simple page flip can cause the CFB required size to increase and if it is bigger than the currently allocated CFB it needs to be resized to activate FBC again. Until now this case was not being handled but CI is starting to get some of this errors. So here it will free the old CFB and a try to allocate the required CFB in the schedule activation work, it will happen after one vblank so is guarantee that FBC was completed disabled and is not using CFB. Also in case that there is no enough stolen memory to allocate the new CFB it will try 3 times per full modeset as the CFB requirement could be reduced in the next non-full modeset. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105683 --- drivers/gpu/drm/i915/i915_drv.h | 3 +++ drivers/gpu/drm/i915/intel_fbc.c | 46 +++++++++++++++++++++----------- 2 files changed, 33 insertions(+), 16 deletions(-)