[PATCH] drm/i915/bdw: Don't allow the FBC base to be 0

[PATCH] drm/i915/bdw: Don't allow the FBC base to be 0

[Ben Widawsky]

"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(-)

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

Re: [PATCH] drm/i915/bdw: Don't allow the FBC base to be 0

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/bdw: Don't allow the FBC base to be 0

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/bdw: Don't allow the FBC base to be 0

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/bdw: Don't allow the FBC base to be 0

[Ben Widawsky]

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

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/bdw: Don't allow the FBC base to be 0

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/bdw: Don't allow the FBC base to be 0

[Ben Widawsky]

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.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/bdw: Don't allow the FBC base to be 0

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/bdw: Don't allow the FBC base to be 0

[Damien Lespiau]

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.

-- 
Damien