On Wed, Dec 14, 2016 at 05:55:39PM -0200, Paulo Zanoni wrote: > Don't even tell the mm allocator to handle the first page of stolen on > the affected platforms. 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). > v3: drm_mm_init() takes size instead of end as argument (Ville). > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605 > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_gem_gtt.h | 1 + > drivers/gpu/drm/i915/i915_gem_stolen.c | 34 +++++++++++++--------------------- > drivers/gpu/drm/i915/intel_fbc.c | 6 +++--- > 3 files changed, 17 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index 8965bbb..aefc968 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -316,6 +316,7 @@ struct i915_ggtt { > struct io_mapping mappable; /* Mapping to our CPU mappable region */ > /* Stolen memory is segmented in hardware with different portions * offlimits to certain functions. * * The drm_mm is initialised to the total accessible range, as found * from the PCI config. On Broadwell+, this is further restricted to * avoid the first page! The upper end of stolen memory is reserved for * hardware functions (such as intermediate encodings and compression) * and similarly removed from the accessible range. */ > size_t stolen_size; /* Total size of stolen memory */ > + size_t stolen_usable_start; /* First page may be unusable. */ > size_t stolen_usable_size; /* Total size minus BIOS reserved */ > size_t stolen_reserved_base; > size_t stolen_reserved_size; Aside: Why are these hw values using size_t? > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index b1c8897..56d7e0b 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, > @@ -74,10 +68,11 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv, > unsigned alignment) > { > struct i915_ggtt *ggtt = &dev_priv->ggtt; > + u64 start = ggtt->stolen_usable_start; > + u64 end = start + ggtt->stolen_usable_size; > > return i915_gem_stolen_insert_node_in_range(dev_priv, node, size, > - alignment, 0, > - ggtt->stolen_usable_size); ? The range is already restricted, you cannot get anything before stolen_usable_start, or anything after usable_size. Just pass 0, U64_MAX. > + alignment, start, end); > } > > void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv, > @@ -489,20 +484,17 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) > ggtt->stolen_size >> 10, > (ggtt->stolen_size - reserved_total) >> 10); > > - ggtt->stolen_usable_size = ggtt->stolen_size - reserved_total; > + ggtt->stolen_usable_start = 0; > + /* WaSkipStolenMemoryFirstPage:bdw+ */ > + if (INTEL_GEN(dev_priv) >= 8) > + ggtt->stolen_usable_start = 4096; > > - /* > - * 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); > + ggtt->stolen_usable_size = ggtt->stolen_size - reserved_total - > + ggtt->stolen_usable_start; > + > + /* Basic memrange allocator for stolen space. */ > + drm_mm_init(&dev_priv->mm.stolen, ggtt->stolen_usable_start, > + ggtt->stolen_usable_size); > > return 0; > } > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 62f215b..534fcb3c 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -528,7 +528,7 @@ static int find_compression_threshold(struct drm_i915_private *dev_priv, > struct i915_ggtt *ggtt = &dev_priv->ggtt; > int compression_threshold = 1; > int ret; > - u64 end; > + u64 start = ggtt->stolen_usable_start, 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. > @@ -538,7 +538,7 @@ static int find_compression_threshold(struct drm_i915_private *dev_priv, > IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) > end = ggtt->stolen_size - 8 * 1024 * 1024; > else > - end = ggtt->stolen_usable_size; > + end = start + ggtt->stolen_usable_size; end = U64_MAX; No need to set start. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
