| Message ID | 1439588061-18064-9-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 14, 2015 at 06:34:13PM -0300, Paulo Zanoni wrote: > The FBC hardware for these platforms doesn't have access to the > bios_reserved range, so it always assumes the maximum (8mb) is used. > So avoid this range while allocating. > > This solves a bunch of FIFO underruns that happen if you end up > putting the CFB in that memory range. On my machine, with 32mb of > stolen, I need a 2560x1440 mode for that. If the restriction applies only to FBC, apply it to only fbc. -Chris
Em Sáb, 2015-08-15 às 09:29 +0100, Chris Wilson escreveu: > On Fri, Aug 14, 2015 at 06:34:13PM -0300, Paulo Zanoni wrote: > > The FBC hardware for these platforms doesn't have access to the > > bios_reserved range, so it always assumes the maximum (8mb) is > > used. > > So avoid this range while allocating. > > > > This solves a bunch of FIFO underruns that happen if you end up > > putting the CFB in that memory range. On my machine, with 32mb of > > stolen, I need a 2560x1440 mode for that. > > If the restriction applies only to FBC, apply it to only fbc. That's what the patch is doing. The part where we set the unusual start/end is at intel_fbc.c:find_compression_threshold(). Everything else should be behaving just as before. Maybe you're being confused by the addition of the stolen_usable_size variable. > -Chris >
On Tue, Aug 18, 2015 at 09:49:34PM +0000, Zanoni, Paulo R wrote: > Em Sáb, 2015-08-15 às 09:29 +0100, Chris Wilson escreveu: > > On Fri, Aug 14, 2015 at 06:34:13PM -0300, Paulo Zanoni wrote: > > > The FBC hardware for these platforms doesn't have access to the > > > bios_reserved range, so it always assumes the maximum (8mb) is > > > used. > > > So avoid this range while allocating. > > > > > > This solves a bunch of FIFO underruns that happen if you end up > > > putting the CFB in that memory range. On my machine, with 32mb of > > > stolen, I need a 2560x1440 mode for that. > > > > If the restriction applies only to FBC, apply it to only fbc. > > That's what the patch is doing. The part where we set the unusual > start/end is at intel_fbc.c:find_compression_threshold(). Everything > else should be behaving just as before. > > Maybe you're being confused by the addition of the stolen_usable_size > variable. Hmm, I expected to see the constaint being passed into the search routine.a It looked like you were adjusting the stolen_size probably the root of my confusion. Anyway we have a quandary. You want to reserve stolen space, and I want to make sure as much of it gets used as possible (especially for long lived objects). What I have in mind is adding a priority to the search and allow us to evict anything of lower priority. We can use a page replacement algorithm even for the pinned fbcon (since only the GGTT itself is pinned and we can swap the pages underneath). This of course requires a callback for low priority evictable objects. I have 3 priorities in mind plus the evictable flag: USER, KERNEL, POWER (with FBC taking the highest priority of POWER). That will allow us to do the BIOS takeover and only if we run out of space force the copy. -Chris
2015-08-19 5:24 GMT-03:00 chris@chris-wilson.co.uk <chris@chris-wilson.co.uk>: > On Tue, Aug 18, 2015 at 09:49:34PM +0000, Zanoni, Paulo R wrote: >> Em Sáb, 2015-08-15 às 09:29 +0100, Chris Wilson escreveu: >> > On Fri, Aug 14, 2015 at 06:34:13PM -0300, Paulo Zanoni wrote: >> > > The FBC hardware for these platforms doesn't have access to the >> > > bios_reserved range, so it always assumes the maximum (8mb) is >> > > used. >> > > So avoid this range while allocating. >> > > >> > > This solves a bunch of FIFO underruns that happen if you end up >> > > putting the CFB in that memory range. On my machine, with 32mb of >> > > stolen, I need a 2560x1440 mode for that. >> > >> > If the restriction applies only to FBC, apply it to only fbc. >> >> That's what the patch is doing. The part where we set the unusual >> start/end is at intel_fbc.c:find_compression_threshold(). Everything >> else should be behaving just as before. >> >> Maybe you're being confused by the addition of the stolen_usable_size >> variable. > > Hmm, I expected to see the constaint being passed into the search > routine.a It looked like you were adjusting the stolen_size probably the > root of my confusion. > > Anyway we have a quandary. You want to reserve stolen space, and I want > to make sure as much of it gets used as possible (especially for long > lived objects). > > What I have in mind is adding a priority to the search and allow us to > evict anything of lower priority. We can use a page replacement > algorithm even for the pinned fbcon (since only the GGTT itself is > pinned and we can swap the pages underneath). This of course requires a > callback for low priority evictable objects. I have 3 priorities in mind > plus the evictable flag: USER, KERNEL, POWER (with FBC taking the > highest priority of POWER). That will allow us to do the BIOS takeover > and only if we run out of space force the copy. While I understand your idea, I just want to clarify one thing: it does not apply to _this_ patch, right? I suppose we're discussing about "[PATCH 15/16] Revert "drm/i915: Allocate fbcon from stolen memory" here. The memory avoided by this patch (08/16) can still be used by the other stolen memory users. For the fbcon patch, can't we adopt a simpler "if FBC is enabled by default on this platform, don't alloc fbcon from stolen"?. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e74a844..f44d101 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3159,6 +3159,10 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev) int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv, struct drm_mm_node *node, u64 size, unsigned alignment); +int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv, + struct drm_mm_node *node, u64 size, + unsigned alignment, u64 start, + u64 end); void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv, struct drm_mm_node *node); int i915_gem_init_stolen(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 8275007..96ebb98 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -341,6 +341,7 @@ struct i915_gtt { struct i915_address_space base; size_t stolen_size; /* Total size of stolen memory */ + size_t stolen_usable_size; /* Total size minus BIOS reserved */ u64 mappable_end; /* End offset that we can CPU map */ struct io_mapping *mappable; /* Mapping to our CPU mappable region */ phys_addr_t mappable_base; /* PA of our GMADR */ diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index a36cb95..824334d 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -42,9 +42,9 @@ * for is a boon. */ -int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv, - struct drm_mm_node *node, u64 size, - unsigned alignment) +int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv, + struct drm_mm_node *node, u64 size, + unsigned alignment, u64 start, u64 end) { int ret; @@ -52,13 +52,23 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv, return -ENODEV; mutex_lock(&dev_priv->mm.stolen_lock); - ret = drm_mm_insert_node(&dev_priv->mm.stolen, node, size, alignment, - DRM_MM_SEARCH_DEFAULT); + ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, node, size, + alignment, start, end, + DRM_MM_SEARCH_DEFAULT); mutex_unlock(&dev_priv->mm.stolen_lock); return ret; } +int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv, + struct drm_mm_node *node, u64 size, + unsigned alignment) +{ + return i915_gem_stolen_insert_node_in_range(dev_priv, node, size, + alignment, 0, + dev_priv->gtt.stolen_usable_size); +} + void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv, struct drm_mm_node *node) { @@ -352,9 +362,11 @@ int i915_gem_init_stolen(struct drm_device *dev) dev_priv->gtt.stolen_size >> 10, (dev_priv->gtt.stolen_size - reserved_total) >> 10); + dev_priv->gtt.stolen_usable_size = dev_priv->gtt.stolen_size - + reserved_total; + /* Basic memrange allocator for stolen space */ - drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size - - reserved_total); + drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_usable_size); return 0; } diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 28e569c..9fd7b74 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -570,6 +570,16 @@ static int find_compression_threshold(struct drm_i915_private *dev_priv, { int compression_threshold = 1; int ret; + u64 end; + + /* The FBC hardware for BDW/SKL doesn't have access to the stolen + * reserved range size, so it always assumes the maximum (8mb) is used. + * If we enable FBC using a CFB on that memory range we'll get FIFO + * underruns, even if that range is not reserved by the BIOS. */ + if (IS_BROADWELL(dev_priv) || IS_SKYLAKE(dev_priv)) + end = dev_priv->gtt.stolen_size - 8 * 1024 * 1024; + else + end = dev_priv->gtt.stolen_usable_size; /* HACK: This code depends on what we will do in *_enable_fbc. If that * code changes, this code needs to change as well. @@ -579,7 +589,8 @@ static int find_compression_threshold(struct drm_i915_private *dev_priv, */ /* Try to over-allocate to reduce reallocations and fragmentation. */ - ret = i915_gem_stolen_insert_node(dev_priv, node, size <<= 1, 4096); + ret = i915_gem_stolen_insert_node_in_range(dev_priv, node, size <<= 1, + 4096, 0, end); if (ret == 0) return compression_threshold; @@ -589,7 +600,8 @@ again: (fb_cpp == 2 && compression_threshold == 2)) return 0; - ret = i915_gem_stolen_insert_node(dev_priv, node, size >>= 1, 4096); + ret = i915_gem_stolen_insert_node_in_range(dev_priv, node, size >>= 1, + 4096, 0, end); if (ret && INTEL_INFO(dev_priv)->gen <= 4) { return 0; } else if (ret) {
The FBC hardware for these platforms doesn't have access to the bios_reserved range, so it always assumes the maximum (8mb) is used. So avoid this range while allocating. This solves a bunch of FIFO underruns that happen if you end up putting the CFB in that memory range. On my machine, with 32mb of stolen, I need a 2560x1440 mode for that. Testcase: igt/kms_frontbuffer_tracking/fbc-* (given the right setup) Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 4 ++++ drivers/gpu/drm/i915/i915_gem_gtt.h | 1 + drivers/gpu/drm/i915/i915_gem_stolen.c | 26 +++++++++++++++++++------- drivers/gpu/drm/i915/intel_fbc.c | 16 ++++++++++++++-- 4 files changed, 38 insertions(+), 9 deletions(-)