| Message ID | 1398995274-1317-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 01, 2014 at 06:47:54PM -0700, Ben Widawsky wrote: > "Restriction : > The offset must be greater than 4K bytes, avoiding the first 4KB of > stolen memory." Isn't this a more generic issue that we must avoid the first 4k? If so I think we should just reserve the first 4k permanently at driver init time. > > Since it looks like we currently allocate an overlay out of stolen > before we get the compressed framebuffer, I believe this is not > currently an issue which fixes anything. We simply want to make the code > as future-proof, and as clear as possible, by match the spec. > > Cc: Art Runyan <arthur.j.runyan@intel.com> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > 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 62ef55b..65016b0 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -107,19 +107,24 @@ static int i915_setup_compression(struct drm_device *dev, int size) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_mm_node *compressed_fb, *uninitialized_var(compressed_llb); > + unsigned long start = 0; > int ret; > > compressed_fb = kzalloc(sizeof(*compressed_fb), GFP_KERNEL); > if (!compressed_fb) > goto err_llb; > > + if (IS_BROADWELL(dev)) > + start = 0x1000; > + > /* Try to over-allocate to reduce reallocations and fragmentation */ > - ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_fb, > - size <<= 1, 4096, DRM_MM_SEARCH_DEFAULT); > + ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, compressed_fb, > + size <<= 1, 4096, start, dev_priv->gtt.stolen_size, > + DRM_MM_SEARCH_DEFAULT); > if (ret) > - ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_fb, > - size >>= 1, 4096, > - DRM_MM_SEARCH_DEFAULT); > + ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, compressed_fb, > + size >>= 1, 4096, start, dev_priv->gtt.stolen_size, > + DRM_MM_SEARCH_DEFAULT); > if (ret) > goto err_llb; > > -- > 1.9.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, May 02, 2014 at 11:19:27AM +0300, Ville Syrjälä wrote: > On Thu, May 01, 2014 at 06:47:54PM -0700, Ben Widawsky wrote: > > "Restriction : > > The offset must be greater than 4K bytes, avoiding the first 4KB of > > stolen memory." > > Isn't this a more generic issue that we must avoid the first 4k? If so > I think we should just reserve the first 4k permanently at driver init > time. What? On many machines the vga framebuffer is allocated from offset 0. I think some explanation is in order. -Chris
On Fri, May 02, 2014 at 09:38:11AM +0100, Chris Wilson wrote: > On Fri, May 02, 2014 at 11:19:27AM +0300, Ville Syrjälä wrote: > > On Thu, May 01, 2014 at 06:47:54PM -0700, Ben Widawsky wrote: > > > "Restriction : > > > The offset must be greater than 4K bytes, avoiding the first 4KB of > > > stolen memory." > > > > Isn't this a more generic issue that we must avoid the first 4k? If so > > I think we should just reserve the first 4k permanently at driver init > > time. > > What? On many machines the vga framebuffer is allocated from offset 0. I > think some explanation is in order. WaSkipStolenMemoryFirstPage
On Fri, May 02, 2014 at 04:00:25PM +0300, Ville Syrjälä wrote: > On Fri, May 02, 2014 at 09:38:11AM +0100, Chris Wilson wrote: > > On Fri, May 02, 2014 at 11:19:27AM +0300, Ville Syrjälä wrote: > > > On Thu, May 01, 2014 at 06:47:54PM -0700, Ben Widawsky wrote: > > > > "Restriction : > > > > The offset must be greater than 4K bytes, avoiding the first 4KB of > > > > stolen memory." > > > > > > Isn't this a more generic issue that we must avoid the first 4k? If so > > > I think we should just reserve the first 4k permanently at driver init > > > time. Is anyone opposed to this plan? Realistically it won't make a difference. > > > > What? On many machines the vga framebuffer is allocated from offset 0. I > > think some explanation is in order. The chopped off part of the commit message explained it (if I understood your point). > > WaSkipStolenMemoryFirstPage > > -- > Ville Syrjälä > Intel OTC
On Fri, May 02, 2014 at 10:00:01AM -0700, Ben Widawsky wrote: > On Fri, May 02, 2014 at 04:00:25PM +0300, Ville Syrjälä wrote: > > On Fri, May 02, 2014 at 09:38:11AM +0100, Chris Wilson wrote: > > > On Fri, May 02, 2014 at 11:19:27AM +0300, Ville Syrjälä wrote: > > > > On Thu, May 01, 2014 at 06:47:54PM -0700, Ben Widawsky wrote: > > > > > "Restriction : > > > > > The offset must be greater than 4K bytes, avoiding the first 4KB of > > > > > stolen memory." > > > > > > > > Isn't this a more generic issue that we must avoid the first 4k? If so > > > > I think we should just reserve the first 4k permanently at driver init > > > > time. > > Is anyone opposed to this plan? Realistically it won't make a > difference. > > > > > > > What? On many machines the vga framebuffer is allocated from offset 0. I > > > think some explanation is in order. > > The chopped off part of the commit message explained it (if I understood > your point). How do we handle the inherited fb if it starts at offset 0? -Chris
On Fri, May 02, 2014 at 09:35:20PM +0100, Chris Wilson wrote: > On Fri, May 02, 2014 at 10:00:01AM -0700, Ben Widawsky wrote: > > On Fri, May 02, 2014 at 04:00:25PM +0300, Ville Syrjälä wrote: > > > On Fri, May 02, 2014 at 09:38:11AM +0100, Chris Wilson wrote: > > > > On Fri, May 02, 2014 at 11:19:27AM +0300, Ville Syrjälä wrote: > > > > > On Thu, May 01, 2014 at 06:47:54PM -0700, Ben Widawsky wrote: > > > > > > "Restriction : > > > > > > The offset must be greater than 4K bytes, avoiding the first 4KB of > > > > > > stolen memory." > > > > > > > > > > Isn't this a more generic issue that we must avoid the first 4k? If so > > > > > I think we should just reserve the first 4k permanently at driver init > > > > > time. > > > > Is anyone opposed to this plan? Realistically it won't make a > > difference. > > > > > > > > > > What? On many machines the vga framebuffer is allocated from offset 0. I > > > > think some explanation is in order. > > > > The chopped off part of the commit message explained it (if I understood > > your point). > > How do we handle the inherited fb if it starts at offset 0? > -Chris > I must be missing something important. The FBC buffer is the only one requiring a non-zero offset from the base of stolen memory.
On Fri, May 02, 2014 at 06:48:52PM -0700, Ben Widawsky wrote: > On Fri, May 02, 2014 at 09:35:20PM +0100, Chris Wilson wrote: > > On Fri, May 02, 2014 at 10:00:01AM -0700, Ben Widawsky wrote: > > > On Fri, May 02, 2014 at 04:00:25PM +0300, Ville Syrjälä wrote: > > > > On Fri, May 02, 2014 at 09:38:11AM +0100, Chris Wilson wrote: > > > > > On Fri, May 02, 2014 at 11:19:27AM +0300, Ville Syrjälä wrote: > > > > > > On Thu, May 01, 2014 at 06:47:54PM -0700, Ben Widawsky wrote: > > > > > > > "Restriction : > > > > > > > The offset must be greater than 4K bytes, avoiding the first 4KB of > > > > > > > stolen memory." > > > > > > > > > > > > Isn't this a more generic issue that we must avoid the first 4k? If so > > > > > > I think we should just reserve the first 4k permanently at driver init > > > > > > time. > > > > > > Is anyone opposed to this plan? Realistically it won't make a > > > difference. > > > > > > > > > > > > > What? On many machines the vga framebuffer is allocated from offset 0. I > > > > > think some explanation is in order. > > > > > > The chopped off part of the commit message explained it (if I understood > > > your point). > > > > How do we handle the inherited fb if it starts at offset 0? > > -Chris > > > > I must be missing something important. The FBC buffer is the only one > requiring a non-zero offset from the base of stolen memory. Nothing important can be placed there since the CS apparently writes there without anyone explicitly telling it to do so. That's what I gathered from the hsd anyway. I don't have a real answer for the inherited config thing. Should we just not inherit in that case? One would hope that the BIOS doesn't put the FB at offset 0, but this being the BIOS I'm pretty sure it does. I guess we could always make a copy and flip, but it would be nice if we didn't have to. Or maybe just copy the first page somewhere else and set up the PTE to compensate? Though that might violate some assumption we have about stolen being always contiguous... On a related note our FB takeover code is anywya rather optimistic since it blindly assumes that the ggtt offset matches the stolen offet. We never actually look at the PTEs to confirm that.
On Thu, May 15, 2014 at 05:05:05PM +0300, Ville Syrjälä wrote: > > I must be missing something important. The FBC buffer is the only one > > requiring a non-zero offset from the base of stolen memory. > > Nothing important can be placed there since the CS apparently writes > there without anyone explicitly telling it to do so. That's what I > gathered from the hsd anyway. FWIW, that's what Art said in person as well a couple of weeks back. So, indeed, we need to reserve the first page of stolen on BDW.
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 62ef55b..65016b0 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -107,19 +107,24 @@ static int i915_setup_compression(struct drm_device *dev, int size) { struct drm_i915_private *dev_priv = dev->dev_private; struct drm_mm_node *compressed_fb, *uninitialized_var(compressed_llb); + unsigned long start = 0; int ret; compressed_fb = kzalloc(sizeof(*compressed_fb), GFP_KERNEL); if (!compressed_fb) goto err_llb; + if (IS_BROADWELL(dev)) + start = 0x1000; + /* Try to over-allocate to reduce reallocations and fragmentation */ - ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_fb, - size <<= 1, 4096, DRM_MM_SEARCH_DEFAULT); + ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, compressed_fb, + size <<= 1, 4096, start, dev_priv->gtt.stolen_size, + DRM_MM_SEARCH_DEFAULT); if (ret) - ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_fb, - size >>= 1, 4096, - DRM_MM_SEARCH_DEFAULT); + ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, compressed_fb, + size >>= 1, 4096, start, dev_priv->gtt.stolen_size, + DRM_MM_SEARCH_DEFAULT); if (ret) goto err_llb;
"Restriction : The offset must be greater than 4K bytes, avoiding the first 4KB of stolen memory." Since it looks like we currently allocate an overlay out of stolen before we get the compressed framebuffer, I believe this is not currently an issue which fixes anything. We simply want to make the code as future-proof, and as clear as possible, by match the spec. Cc: Art Runyan <arthur.j.runyan@intel.com> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_stolen.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)