| Message ID | 1481727338-9901-2-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 14, 2016 at 12:55:38PM -0200, Paulo Zanoni wrote: > Reserve the first page of stolen memory right after initializing the > mm allocator. This means that we won't inherit the FB in case the BIOS > decides to put it at the start of stolen. But the BIOS should not be > putting it at the start of stolen since it's going to get corrupted. I > suppose the bug here is that some pixels at the very top of the screen > will be corrupted, so it's not exactly easy to notice. > > We have confirmation that the first page of stolen does actually get > corrupted, so I really think we should do this in order to avoid any > possible future headaches, even if that means losing BIOS framebuffer > inheritance. Let's not use the HW in a way it's not supposed to be > used. > > v2: don't even put the first page on the mm (Chris). > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605 > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Mental note: need to add to the list of test cases (insert_in_range > range of drm_mm). Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (code, I'll take your word that w/a accurately surmises the situation and is the blessed strategy) -Chris
On Wed, Dec 14, 2016 at 12:55:38PM -0200, Paulo Zanoni wrote: > Reserve the first page of stolen memory right after initializing the > mm allocator. This means that we won't inherit the FB in case the BIOS > decides to put it at the start of stolen. But the BIOS should not be > putting it at the start of stolen since it's going to get corrupted. I > suppose the bug here is that some pixels at the very top of the screen > will be corrupted, so it's not exactly easy to notice. > > We have confirmation that the first page of stolen does actually get > corrupted, so I really think we should do this in order to avoid any > possible future headaches, even if that means losing BIOS framebuffer > inheritance. Let's not use the HW in a way it's not supposed to be > used. > > v2: don't even put the first page on the mm (Chris). > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605 > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_stolen.c | 27 ++++++++------------------- > 1 file changed, 8 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index b1c8897..7435540 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -54,12 +54,6 @@ int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv, > if (!drm_mm_initialized(&dev_priv->mm.stolen)) > return -ENODEV; > > - /* See the comment at the drm_mm_init() call for more about this check. > - * WaSkipStolenMemoryFirstPage:bdw+ (incomplete) > - */ > - if (start < 4096 && INTEL_GEN(dev_priv) >= 8) > - start = 4096; > - > mutex_lock(&dev_priv->mm.stolen_lock); > ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, node, size, > alignment, start, end, > @@ -410,7 +404,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) > { > struct i915_ggtt *ggtt = &dev_priv->ggtt; > unsigned long reserved_total, reserved_base = 0, reserved_size; > - unsigned long stolen_top; > + unsigned long stolen_start = 0, stolen_top; > > mutex_init(&dev_priv->mm.stolen_lock); > > @@ -491,18 +485,13 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) > > ggtt->stolen_usable_size = ggtt->stolen_size - reserved_total; > > - /* > - * Basic memrange allocator for stolen space. > - * > - * TODO: Notice that some platforms require us to not use the first page > - * of the stolen memory but their BIOSes may still put the framebuffer > - * on the first page. So we don't reserve this page for now because of > - * that. Our current solution is to just prevent new nodes from being > - * inserted on the first page - see the check we have at > - * i915_gem_stolen_insert_node_in_range(). We may want to fix the fbcon > - * problem later. > - */ > - drm_mm_init(&dev_priv->mm.stolen, 0, ggtt->stolen_usable_size); > + /* WaSkipStolenMemoryFirstPage:bdw+ */ > + if (INTEL_GEN(dev_priv) >= 8) > + stolen_start = 4096; > + > + /* Basic memrange allocator for stolen space. */ > + drm_mm_init(&dev_priv->mm.stolen, stolen_start, > + ggtt->stolen_usable_size); Don't you need to reduce the size by 4k as well?
On Wed, Dec 14, 2016 at 06:39:08PM +0200, Ville Syrjälä wrote: > On Wed, Dec 14, 2016 at 12:55:38PM -0200, Paulo Zanoni wrote: > > Reserve the first page of stolen memory right after initializing the > > mm allocator. This means that we won't inherit the FB in case the BIOS > > decides to put it at the start of stolen. But the BIOS should not be > > putting it at the start of stolen since it's going to get corrupted. I > > suppose the bug here is that some pixels at the very top of the screen > > will be corrupted, so it's not exactly easy to notice. > > > > We have confirmation that the first page of stolen does actually get > > corrupted, so I really think we should do this in order to avoid any > > possible future headaches, even if that means losing BIOS framebuffer > > inheritance. Let's not use the HW in a way it's not supposed to be > > used. > > > > v2: don't even put the first page on the mm (Chris). > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605 > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem_stolen.c | 27 ++++++++------------------- > > 1 file changed, 8 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > > index b1c8897..7435540 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > > @@ -54,12 +54,6 @@ int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv, > > if (!drm_mm_initialized(&dev_priv->mm.stolen)) > > return -ENODEV; > > > > - /* See the comment at the drm_mm_init() call for more about this check. > > - * WaSkipStolenMemoryFirstPage:bdw+ (incomplete) > > - */ > > - if (start < 4096 && INTEL_GEN(dev_priv) >= 8) > > - start = 4096; > > - > > mutex_lock(&dev_priv->mm.stolen_lock); > > ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, node, size, > > alignment, start, end, > > @@ -410,7 +404,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) > > { > > struct i915_ggtt *ggtt = &dev_priv->ggtt; > > unsigned long reserved_total, reserved_base = 0, reserved_size; > > - unsigned long stolen_top; > > + unsigned long stolen_start = 0, stolen_top; > > > > mutex_init(&dev_priv->mm.stolen_lock); > > > > @@ -491,18 +485,13 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) > > > > ggtt->stolen_usable_size = ggtt->stolen_size - reserved_total; > > > > - /* > > - * Basic memrange allocator for stolen space. > > - * > > - * TODO: Notice that some platforms require us to not use the first page > > - * of the stolen memory but their BIOSes may still put the framebuffer > > - * on the first page. So we don't reserve this page for now because of > > - * that. Our current solution is to just prevent new nodes from being > > - * inserted on the first page - see the check we have at > > - * i915_gem_stolen_insert_node_in_range(). We may want to fix the fbcon > > - * problem later. > > - */ > > - drm_mm_init(&dev_priv->mm.stolen, 0, ggtt->stolen_usable_size); > > + /* WaSkipStolenMemoryFirstPage:bdw+ */ > > + if (INTEL_GEN(dev_priv) >= 8) > > + stolen_start = 4096; > > + > > + /* Basic memrange allocator for stolen space. */ > > + drm_mm_init(&dev_priv->mm.stolen, stolen_start, > > + ggtt->stolen_usable_size); > > Don't you need to reduce the size by 4k as well? Sigh. Yes. No more review for me. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index b1c8897..7435540 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -54,12 +54,6 @@ int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv, if (!drm_mm_initialized(&dev_priv->mm.stolen)) return -ENODEV; - /* See the comment at the drm_mm_init() call for more about this check. - * WaSkipStolenMemoryFirstPage:bdw+ (incomplete) - */ - if (start < 4096 && INTEL_GEN(dev_priv) >= 8) - start = 4096; - mutex_lock(&dev_priv->mm.stolen_lock); ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, node, size, alignment, start, end, @@ -410,7 +404,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) { struct i915_ggtt *ggtt = &dev_priv->ggtt; unsigned long reserved_total, reserved_base = 0, reserved_size; - unsigned long stolen_top; + unsigned long stolen_start = 0, stolen_top; mutex_init(&dev_priv->mm.stolen_lock); @@ -491,18 +485,13 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) ggtt->stolen_usable_size = ggtt->stolen_size - reserved_total; - /* - * Basic memrange allocator for stolen space. - * - * TODO: Notice that some platforms require us to not use the first page - * of the stolen memory but their BIOSes may still put the framebuffer - * on the first page. So we don't reserve this page for now because of - * that. Our current solution is to just prevent new nodes from being - * inserted on the first page - see the check we have at - * i915_gem_stolen_insert_node_in_range(). We may want to fix the fbcon - * problem later. - */ - drm_mm_init(&dev_priv->mm.stolen, 0, ggtt->stolen_usable_size); + /* WaSkipStolenMemoryFirstPage:bdw+ */ + if (INTEL_GEN(dev_priv) >= 8) + stolen_start = 4096; + + /* Basic memrange allocator for stolen space. */ + drm_mm_init(&dev_priv->mm.stolen, stolen_start, + ggtt->stolen_usable_size); return 0; }
Reserve the first page of stolen memory right after initializing the mm allocator. This means that we won't inherit the FB in case the BIOS decides to put it at the start of stolen. But the BIOS should not be putting it at the start of stolen since it's going to get corrupted. I suppose the bug here is that some pixels at the very top of the screen will be corrupted, so it's not exactly easy to notice. We have confirmation that the first page of stolen does actually get corrupted, so I really think we should do this in order to avoid any possible future headaches, even if that means losing BIOS framebuffer inheritance. Let's not use the HW in a way it's not supposed to be used. v2: don't even put the first page on the mm (Chris). Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605 Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_gem_stolen.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)