| Message ID | 1406833700-3203-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 31 Jul 2014 12:08:20 -0700 Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > WA to skip the first page of stolen memory due to sporadic HW write on *CS Idle > > v2: Improve variable names and fix allocated size. > > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_stolen.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 21c025a..82035b0 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -289,7 +289,8 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) > int i915_gem_init_stolen(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - int bios_reserved = 0; > + int start_rsvd = 0; > + int end_rsvd = 0; > > #ifdef CONFIG_INTEL_IOMMU > if (intel_iommu_gfx_mapped && INTEL_INFO(dev)->gen < 8) { > @@ -308,15 +309,19 @@ int i915_gem_init_stolen(struct drm_device *dev) > DRM_DEBUG_KMS("found %zd bytes of stolen memory at %08lx\n", > dev_priv->gtt.stolen_size, dev_priv->mm.stolen_base); > > + /* WaSkipStolenMemoryFirstPage */ > + if (INTEL_INFO(dev)->gen >= 8) > + start_rsvd = 4096; > + > if (IS_VALLEYVIEW(dev)) > - bios_reserved = 1024*1024; /* top 1M on VLV/BYT */ > + end_rsvd = 1024*1024; /* top 1M on VLV/BYT */ > > - if (WARN_ON(bios_reserved > dev_priv->gtt.stolen_size)) > + if (WARN_ON((start_rsvd + end_rsvd) > dev_priv->gtt.stolen_size)) > return 0; > > /* Basic memrange allocator for stolen space */ > - drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size - > - bios_reserved); > + drm_mm_init(&dev_priv->mm.stolen, start_rsvd, > + dev_priv->gtt.stolen_size - start_rsvd - end_rsvd); > > return 0; > } Beyond the fastboot stuff Ville has already mentioned, the early allocation of the existing fb from stolen will prevent us from clobbering the currently displayed buffer with the contents of the ringbuffers and whatever else we allocate out of stolen at early boot. We might be able to avoid that by doing stolen allocations top down, or by reserving the displayed fb even if we can't allocate an obj for it, only freeing it after our first mode set. Can you file a bug or JIRA for that to make sure we don't lose track of the fastboot & boot corruption issues after this fix lands? Thanks,
On 01/08/2014 17:34, Jesse Barnes wrote: > On Thu, 31 Jul 2014 12:08:20 -0700 > Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > >> WA to skip the first page of stolen memory due to sporadic HW write on *CS Idle >> >> v2: Improve variable names and fix allocated size. >> >> Reviewed-by: Ben Widawsky <ben@bwidawsk.net> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem_stolen.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c >> index 21c025a..82035b0 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c >> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c >> @@ -289,7 +289,8 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) >> int i915_gem_init_stolen(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> - int bios_reserved = 0; >> + int start_rsvd = 0; >> + int end_rsvd = 0; >> >> #ifdef CONFIG_INTEL_IOMMU >> if (intel_iommu_gfx_mapped && INTEL_INFO(dev)->gen < 8) { >> @@ -308,15 +309,19 @@ int i915_gem_init_stolen(struct drm_device *dev) >> DRM_DEBUG_KMS("found %zd bytes of stolen memory at %08lx\n", >> dev_priv->gtt.stolen_size, dev_priv->mm.stolen_base); >> >> + /* WaSkipStolenMemoryFirstPage */ >> + if (INTEL_INFO(dev)->gen >= 8) >> + start_rsvd = 4096; >> + >> if (IS_VALLEYVIEW(dev)) >> - bios_reserved = 1024*1024; /* top 1M on VLV/BYT */ >> + end_rsvd = 1024*1024; /* top 1M on VLV/BYT */ >> >> - if (WARN_ON(bios_reserved > dev_priv->gtt.stolen_size)) >> + if (WARN_ON((start_rsvd + end_rsvd) > dev_priv->gtt.stolen_size)) >> return 0; >> >> /* Basic memrange allocator for stolen space */ >> - drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size - >> - bios_reserved); >> + drm_mm_init(&dev_priv->mm.stolen, start_rsvd, >> + dev_priv->gtt.stolen_size - start_rsvd - end_rsvd); >> >> return 0; >> } > > Beyond the fastboot stuff Ville has already mentioned, the early > allocation of the existing fb from stolen will prevent us from > clobbering the currently displayed buffer with the contents of the > ringbuffers and whatever else we allocate out of stolen at early boot. > > We might be able to avoid that by doing stolen allocations top down, or > by reserving the displayed fb even if we can't allocate an obj for it, > only freeing it after our first mode set. > > Can you file a bug or JIRA for that to make sure we don't lose track of > the fastboot & boot corruption issues after this fix lands? Reviving an old thread, Any particular reason why this patch is not merged to nightly? Is it known to cause any other regressions? regards Arun > > Thanks, >
On Tue, Feb 03, 2015 at 04:11:05PM +0000, Siluvery, Arun wrote: > On 01/08/2014 17:34, Jesse Barnes wrote: > > On Thu, 31 Jul 2014 12:08:20 -0700 > > Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > > >> WA to skip the first page of stolen memory due to sporadic HW write on *CS Idle > >> > >> v2: Improve variable names and fix allocated size. > >> > >> Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_gem_stolen.c | 15 ++++++++++----- > >> 1 file changed, 10 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > >> index 21c025a..82035b0 100644 > >> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > >> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > >> @@ -289,7 +289,8 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) > >> int i915_gem_init_stolen(struct drm_device *dev) > >> { > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> - int bios_reserved = 0; > >> + int start_rsvd = 0; > >> + int end_rsvd = 0; > >> > >> #ifdef CONFIG_INTEL_IOMMU > >> if (intel_iommu_gfx_mapped && INTEL_INFO(dev)->gen < 8) { > >> @@ -308,15 +309,19 @@ int i915_gem_init_stolen(struct drm_device *dev) > >> DRM_DEBUG_KMS("found %zd bytes of stolen memory at %08lx\n", > >> dev_priv->gtt.stolen_size, dev_priv->mm.stolen_base); > >> > >> + /* WaSkipStolenMemoryFirstPage */ > >> + if (INTEL_INFO(dev)->gen >= 8) > >> + start_rsvd = 4096; > >> + > >> if (IS_VALLEYVIEW(dev)) > >> - bios_reserved = 1024*1024; /* top 1M on VLV/BYT */ > >> + end_rsvd = 1024*1024; /* top 1M on VLV/BYT */ > >> > >> - if (WARN_ON(bios_reserved > dev_priv->gtt.stolen_size)) > >> + if (WARN_ON((start_rsvd + end_rsvd) > dev_priv->gtt.stolen_size)) > >> return 0; > >> > >> /* Basic memrange allocator for stolen space */ > >> - drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size - > >> - bios_reserved); > >> + drm_mm_init(&dev_priv->mm.stolen, start_rsvd, > >> + dev_priv->gtt.stolen_size - start_rsvd - end_rsvd); > >> > >> return 0; > >> } > > > > Beyond the fastboot stuff Ville has already mentioned, the early > > allocation of the existing fb from stolen will prevent us from > > clobbering the currently displayed buffer with the contents of the > > ringbuffers and whatever else we allocate out of stolen at early boot. > > > > We might be able to avoid that by doing stolen allocations top down, or > > by reserving the displayed fb even if we can't allocate an obj for it, > > only freeing it after our first mode set. > > > > Can you file a bug or JIRA for that to make sure we don't lose track of > > the fastboot & boot corruption issues after this fix lands? > > Reviving an old thread, > Any particular reason why this patch is not merged to nightly? > Is it known to cause any other regressions? It breaks the BIOS fb takeover like I said several times. If no one is willing to fix it properly I was thinking we might just try to do the BIOS fb takeover, and if it succeeded we do nothing else, otherwise we allocate an unused 1 page object to keep the rings/fbc buffer/etc. away from the first page. The first page corruption supposedly happens only when the CS is doing stuff, so if the CS corrupts the fbcon a bit it's no big deal. And since we don't accelerate the fbconf hooks the corruption shouldn't really happen under normal conditions anyway. You could see it while running some igts or something, but that's not a huge problem.
On Tue, Feb 03, 2015 at 07:40:21PM +0200, Ville Syrjälä wrote: > On Tue, Feb 03, 2015 at 04:11:05PM +0000, Siluvery, Arun wrote: > > On 01/08/2014 17:34, Jesse Barnes wrote: > > > On Thu, 31 Jul 2014 12:08:20 -0700 > > > Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > > > > >> WA to skip the first page of stolen memory due to sporadic HW write on *CS Idle > > >> > > >> v2: Improve variable names and fix allocated size. > > >> > > >> Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > > >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > >> --- > > >> drivers/gpu/drm/i915/i915_gem_stolen.c | 15 ++++++++++----- > > >> 1 file changed, 10 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > > >> index 21c025a..82035b0 100644 > > >> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > > >> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > > >> @@ -289,7 +289,8 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) > > >> int i915_gem_init_stolen(struct drm_device *dev) > > >> { > > >> struct drm_i915_private *dev_priv = dev->dev_private; > > >> - int bios_reserved = 0; > > >> + int start_rsvd = 0; > > >> + int end_rsvd = 0; > > >> > > >> #ifdef CONFIG_INTEL_IOMMU > > >> if (intel_iommu_gfx_mapped && INTEL_INFO(dev)->gen < 8) { > > >> @@ -308,15 +309,19 @@ int i915_gem_init_stolen(struct drm_device *dev) > > >> DRM_DEBUG_KMS("found %zd bytes of stolen memory at %08lx\n", > > >> dev_priv->gtt.stolen_size, dev_priv->mm.stolen_base); > > >> > > >> + /* WaSkipStolenMemoryFirstPage */ > > >> + if (INTEL_INFO(dev)->gen >= 8) > > >> + start_rsvd = 4096; > > >> + > > >> if (IS_VALLEYVIEW(dev)) > > >> - bios_reserved = 1024*1024; /* top 1M on VLV/BYT */ > > >> + end_rsvd = 1024*1024; /* top 1M on VLV/BYT */ > > >> > > >> - if (WARN_ON(bios_reserved > dev_priv->gtt.stolen_size)) > > >> + if (WARN_ON((start_rsvd + end_rsvd) > dev_priv->gtt.stolen_size)) > > >> return 0; > > >> > > >> /* Basic memrange allocator for stolen space */ > > >> - drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size - > > >> - bios_reserved); > > >> + drm_mm_init(&dev_priv->mm.stolen, start_rsvd, > > >> + dev_priv->gtt.stolen_size - start_rsvd - end_rsvd); > > >> > > >> return 0; > > >> } > > > > > > Beyond the fastboot stuff Ville has already mentioned, the early > > > allocation of the existing fb from stolen will prevent us from > > > clobbering the currently displayed buffer with the contents of the > > > ringbuffers and whatever else we allocate out of stolen at early boot. > > > > > > We might be able to avoid that by doing stolen allocations top down, or > > > by reserving the displayed fb even if we can't allocate an obj for it, > > > only freeing it after our first mode set. > > > > > > Can you file a bug or JIRA for that to make sure we don't lose track of > > > the fastboot & boot corruption issues after this fix lands? > > > > Reviving an old thread, > > Any particular reason why this patch is not merged to nightly? > > Is it known to cause any other regressions? > > It breaks the BIOS fb takeover like I said several times. > > If no one is willing to fix it properly I was thinking we might just > try to do the BIOS fb takeover, and if it succeeded we do nothing else, > otherwise we allocate an unused 1 page object to keep the rings/fbc > buffer/etc. away from the first page. > > The first page corruption supposedly happens only when the CS is doing > stuff, so if the CS corrupts the fbcon a bit it's no big deal. And > since we don't accelerate the fbconf hooks the corruption shouldn't > really happen under normal conditions anyway. You could see it while > running some igts or something, but that's not a huge problem. Without fbcon we still reconstruct the fb, but will free it on the first modeset when userspace provides a real framebuffer. So this approach doesn't work. Instead we need to teach the stolen allocation functions to respect the limit. We can't just restrict the entire drm_mm like in this patch since the preallocated stolen obj logic must keep on working. Otherwise we break fastboot. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 21c025a..82035b0 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -289,7 +289,8 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) int i915_gem_init_stolen(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - int bios_reserved = 0; + int start_rsvd = 0; + int end_rsvd = 0; #ifdef CONFIG_INTEL_IOMMU if (intel_iommu_gfx_mapped && INTEL_INFO(dev)->gen < 8) { @@ -308,15 +309,19 @@ int i915_gem_init_stolen(struct drm_device *dev) DRM_DEBUG_KMS("found %zd bytes of stolen memory at %08lx\n", dev_priv->gtt.stolen_size, dev_priv->mm.stolen_base); + /* WaSkipStolenMemoryFirstPage */ + if (INTEL_INFO(dev)->gen >= 8) + start_rsvd = 4096; + if (IS_VALLEYVIEW(dev)) - bios_reserved = 1024*1024; /* top 1M on VLV/BYT */ + end_rsvd = 1024*1024; /* top 1M on VLV/BYT */ - if (WARN_ON(bios_reserved > dev_priv->gtt.stolen_size)) + if (WARN_ON((start_rsvd + end_rsvd) > dev_priv->gtt.stolen_size)) return 0; /* Basic memrange allocator for stolen space */ - drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size - - bios_reserved); + drm_mm_init(&dev_priv->mm.stolen, start_rsvd, + dev_priv->gtt.stolen_size - start_rsvd - end_rsvd); return 0; }