[PATCH] drm/i915: fix stolen bios_reserved checks

[PATCH] drm/i915: fix stolen bios_reserved checks

[Paulo Zanoni]

I started digging this when I noticed that the BDW code was just
reserving 1mb by coincidence since it was reading reserved fields.
Then I noticed we didn't have any values set for SNB and earlier, and
that the HSW sizes were wrong. After that, I noticed that the reserved
area has a specific start, and may not exactly end where the stolen
memory ends. I also noticed the base pointer can be zero. So I decided
to just write a single patch fixing everything instead of 20 patches
that would be much harder to review.

This patch may solve random stolen memory corruption/problems on
almost all platforms. Notice that since this is always dealing with
the top of the stolen memory, the problems are not so easy to
reproduce - especially since FBC is still disabled by default.

One of the major differences of this patch is that we now look at both
the size and base address. By only looking at the size we were
assuming that the bios reserved area was always at the very top of
stolen, which is not always true: I have a HSW machine that falls into
this category.

After we merge the patch series that allows user space to allocate
stolen memory we'll be able to write IGT tests that maybe catch the
bugs fixed by this patch.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 156 ++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_reg.h        |  19 ++--
 2 files changed, 157 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index ed682a9..cc8b26a 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -186,11 +186,103 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
 	drm_mm_takedown(&dev_priv->mm.stolen);
 }
 
+static void gen6_get_bios_reserved(struct drm_i915_private *dev_priv,
+				   unsigned long *base, unsigned long *size)
+{
+	uint32_t reg_val = I915_READ(GEN6_BIOS_RESERVED);
+
+	*base = reg_val & GEN6_BIOS_RESERVED_ADDR_MASK;
+
+	switch (reg_val & GEN6_BIOS_RESERVED_SIZE_MASK) {
+	case GEN6_BIOS_RESERVED_1M:
+		*size = 1024 * 1024;
+		break;
+	case GEN6_BIOS_RESERVED_512K:
+		*size = 512 * 1024;
+		break;
+	case GEN6_BIOS_RESERVED_256K:
+		*size = 256 * 1024;
+		break;
+	case GEN6_BIOS_RESERVED_128K:
+		*size = 128 * 1024;
+		break;
+	default:
+		*size = 1024 * 1024;
+		MISSING_CASE(reg_val & GEN6_BIOS_RESERVED_SIZE_MASK);
+	}
+}
+
+static void gen7_get_bios_reserved(struct drm_i915_private *dev_priv,
+				   unsigned long *base, unsigned long *size)
+{
+	uint32_t reg_val = I915_READ(GEN6_BIOS_RESERVED);
+
+	*base = reg_val & GEN7_BIOS_RESERVED_ADDR_MASK;
+
+	switch (reg_val & GEN7_BIOS_RESERVED_SIZE_MASK) {
+	case GEN7_BIOS_RESERVED_1M:
+		*size = 1024 * 1024;
+		break;
+	case GEN7_BIOS_RESERVED_256K:
+		*size = 256 * 1024;
+		break;
+	default:
+		*size = 1024 * 1024;
+		MISSING_CASE(reg_val & GEN7_BIOS_RESERVED_SIZE_MASK);
+	}
+}
+
+static void gen8_get_bios_reserved(struct drm_i915_private *dev_priv,
+				   unsigned long *base, unsigned long *size)
+{
+	uint32_t reg_val = I915_READ(GEN6_BIOS_RESERVED);
+
+	*base = reg_val & GEN6_BIOS_RESERVED_ADDR_MASK;
+
+	switch (reg_val & GEN8_BIOS_RESERVED_SIZE_MASK) {
+	case GEN8_BIOS_RESERVED_1M:
+		*size = 1024 * 1024;
+		break;
+	case GEN8_BIOS_RESERVED_2M:
+		*size = 2 * 1024 * 1024;
+		break;
+	case GEN8_BIOS_RESERVED_4M:
+		*size = 4 * 1024 * 1024;
+		break;
+	case GEN8_BIOS_RESERVED_8M:
+		*size = 8 * 1024 * 1024;
+		break;
+	default:
+		*size = 8 * 1024 * 1024;
+		MISSING_CASE(reg_val & GEN8_BIOS_RESERVED_SIZE_MASK);
+	}
+}
+
+static void bdw_get_bios_reserved(struct drm_i915_private *dev_priv,
+				  unsigned long *base, unsigned long *size)
+{
+	uint32_t reg_val = I915_READ(GEN6_BIOS_RESERVED);
+	unsigned long stolen_top;
+
+	stolen_top = dev_priv->mm.stolen_base + dev_priv->gtt.stolen_size;
+
+	*base = reg_val & GEN6_BIOS_RESERVED_ADDR_MASK;
+
+	/* On these platforms, the register doesn't have a size field, so the
+	 * size is the distance between the base and the top of the stolen
+	 * memory. We also have the genuine case where base is zero and there's
+	 * nothing reserved. */
+	if (*base == 0)
+		*size = 0;
+	else
+		*size = stolen_top - *base;
+}
+
 int i915_gem_init_stolen(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 tmp;
-	int bios_reserved = 0;
+	unsigned long bios_rsvd_total, bios_rsvd_base, bios_rsvd_size;
+	unsigned long stolen_top;
 
 	mutex_init(&dev_priv->mm.stolen_lock);
 
@@ -211,23 +303,61 @@ 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);
 
-	if (INTEL_INFO(dev)->gen >= 8) {
-		tmp = I915_READ(GEN7_BIOS_RESERVED);
-		tmp >>= GEN8_BIOS_RESERVED_SHIFT;
-		tmp &= GEN8_BIOS_RESERVED_MASK;
-		bios_reserved = (1024*1024) << tmp;
-	} else if (IS_GEN7(dev)) {
-		tmp = I915_READ(GEN7_BIOS_RESERVED);
-		bios_reserved = tmp & GEN7_BIOS_RESERVED_256K ?
-			256*1024 : 1024*1024;
+	stolen_top = dev_priv->mm.stolen_base + dev_priv->gtt.stolen_size;
+
+	switch (INTEL_INFO(dev_priv)->gen) {
+	case 2:
+	case 3:
+	case 4:
+	case 5:
+		/* Assume the gen6 maximum for the older platforms. */
+		bios_rsvd_size = 1024 * 1024;
+		bios_rsvd_base = stolen_top - bios_rsvd_size;
+		break;
+	case 6:
+		gen6_get_bios_reserved(dev_priv, &bios_rsvd_base,
+				       &bios_rsvd_size);
+		break;
+	case 7:
+		if (IS_HASWELL(dev_priv))
+			gen6_get_bios_reserved(dev_priv, &bios_rsvd_base,
+					       &bios_rsvd_size);
+		else
+			gen7_get_bios_reserved(dev_priv, &bios_rsvd_base,
+					       &bios_rsvd_size);
+		break;
+	default:
+		if (IS_BROADWELL(dev_priv) || IS_SKYLAKE(dev_priv))
+			bdw_get_bios_reserved(dev_priv, &bios_rsvd_base,
+					      &bios_rsvd_size);
+		else
+			gen8_get_bios_reserved(dev_priv, &bios_rsvd_base,
+					       &bios_rsvd_size);
+		break;
+	}
+
+	/* It is possible for the BIOS reserved base to be zero, but the
+	 * register field for size doesn't have a zero option. */
+	if (bios_rsvd_base == 0) {
+		bios_rsvd_size = 0;
+		bios_rsvd_base = stolen_top;
 	}
 
-	if (WARN_ON(bios_reserved > dev_priv->gtt.stolen_size))
+	if (bios_rsvd_base < dev_priv->mm.stolen_base ||
+	    bios_rsvd_base + bios_rsvd_size > stolen_top) {
+		DRM_ERROR("BIOS reserved area outside stolen memory\n");
 		return 0;
+	}
+
+	/* It is possible for the BIOS reserved area to end before the end of
+	 * stolen memory, so just consider the start. */
+	bios_rsvd_total = stolen_top - bios_rsvd_base;
+
+	DRM_DEBUG_KMS("%lu bytes of stolen memory reserved\n", bios_rsvd_total);
 
 	/* Basic memrange allocator for stolen space */
 	drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size -
-		    bios_reserved);
+		    bios_rsvd_total);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8cf7756..539127e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -178,13 +178,22 @@
 #define GAB_CTL				0x24000
 #define   GAB_CTL_CONT_AFTER_PAGEFAULT	(1<<8)
 
-#define GEN7_BIOS_RESERVED		0x1082C0
+#define GEN6_BIOS_RESERVED		0x1082C0
+#define GEN6_BIOS_RESERVED_ADDR_MASK	(0xFFF << 20)
+#define GEN7_BIOS_RESERVED_ADDR_MASK	(0x3FFF << 18)
+#define GEN6_BIOS_RESERVED_SIZE_MASK	(3 << 4)
+#define GEN6_BIOS_RESERVED_1M		(0 << 4)
+#define GEN6_BIOS_RESERVED_512K		(1 << 4)
+#define GEN6_BIOS_RESERVED_256K		(2 << 4)
+#define GEN6_BIOS_RESERVED_128K		(3 << 4)
+#define GEN7_BIOS_RESERVED_SIZE_MASK	(1 << 5)
 #define GEN7_BIOS_RESERVED_1M		(0 << 5)
 #define GEN7_BIOS_RESERVED_256K		(1 << 5)
-#define GEN8_BIOS_RESERVED_SHIFT       7
-#define GEN7_BIOS_RESERVED_MASK        0x1
-#define GEN8_BIOS_RESERVED_MASK        0x3
-
+#define GEN8_BIOS_RESERVED_SIZE_MASK	(3 << 7)
+#define GEN8_BIOS_RESERVED_1M		(0 << 7)
+#define GEN8_BIOS_RESERVED_2M		(1 << 7)
+#define GEN8_BIOS_RESERVED_4M		(2 << 7)
+#define GEN8_BIOS_RESERVED_8M		(3 << 7)
 
 /* VGA stuff */
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: fix stolen bios_reserved checks

[Chris Wilson]

On Tue, Aug 04, 2015 at 06:30:08PM -0300, Paulo Zanoni wrote:
> I started digging this when I noticed that the BDW code was just
> reserving 1mb by coincidence since it was reading reserved fields.
> Then I noticed we didn't have any values set for SNB and earlier, and
> that the HSW sizes were wrong. After that, I noticed that the reserved
> area has a specific start, and may not exactly end where the stolen
> memory ends. I also noticed the base pointer can be zero. So I decided
> to just write a single patch fixing everything instead of 20 patches
> that would be much harder to review.
> 
> This patch may solve random stolen memory corruption/problems on
> almost all platforms. Notice that since this is always dealing with
> the top of the stolen memory, the problems are not so easy to
> reproduce - especially since FBC is still disabled by default.
> 
> One of the major differences of this patch is that we now look at both
> the size and base address. By only looking at the size we were
> assuming that the bios reserved area was always at the very top of
> stolen, which is not always true: I have a HSW machine that falls into
> this category.
> 
> After we merge the patch series that allows user space to allocate
> stolen memory we'll be able to write IGT tests that maybe catch the
> bugs fixed by this patch.

Looks fun. Whilst we are here, can we drop the notion of "BIOS reserved"
and just label it as resered. It is reserved for functional aspects of
the display engine, not for the bios itself. (Calling it BIOS reserved
either makes me think the BIOS is operating behind our backs or the
reservation is only required across suspend etc)

GEN6_STOLEN_RESERVED makes much more sense both here and out-of-context.

>  int i915_gem_init_stolen(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 tmp;
> -	int bios_reserved = 0;
> +	unsigned long bios_rsvd_total, bios_rsvd_base, bios_rsvd_size;
> +	unsigned long stolen_top;
>  
>  	mutex_init(&dev_priv->mm.stolen_lock);
>  
> @@ -211,23 +303,61 @@ 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);
>  
> -	if (INTEL_INFO(dev)->gen >= 8) {
> -		tmp = I915_READ(GEN7_BIOS_RESERVED);
> -		tmp >>= GEN8_BIOS_RESERVED_SHIFT;
> -		tmp &= GEN8_BIOS_RESERVED_MASK;
> -		bios_reserved = (1024*1024) << tmp;
> -	} else if (IS_GEN7(dev)) {
> -		tmp = I915_READ(GEN7_BIOS_RESERVED);
> -		bios_reserved = tmp & GEN7_BIOS_RESERVED_256K ?
> -			256*1024 : 1024*1024;
> +	stolen_top = dev_priv->mm.stolen_base + dev_priv->gtt.stolen_size;
> +
> +	switch (INTEL_INFO(dev_priv)->gen) {
> +	case 2:
> +	case 3:
> +	case 4:
> +	case 5:
> +		/* Assume the gen6 maximum for the older platforms. */
> +		bios_rsvd_size = 1024 * 1024;
> +		bios_rsvd_base = stolen_top - bios_rsvd_size;
> +		break;
> +	case 6:
> +		gen6_get_bios_reserved(dev_priv, &bios_rsvd_base,
> +				       &bios_rsvd_size);
> +		break;
> +	case 7:
> +		if (IS_HASWELL(dev_priv))
> +			gen6_get_bios_reserved(dev_priv, &bios_rsvd_base,
> +					       &bios_rsvd_size);
> +		else
> +			gen7_get_bios_reserved(dev_priv, &bios_rsvd_base,
> +					       &bios_rsvd_size);
> +		break;
> +	default:
> +		if (IS_BROADWELL(dev_priv) || IS_SKYLAKE(dev_priv))
> +			bdw_get_bios_reserved(dev_priv, &bios_rsvd_base,
> +					      &bios_rsvd_size);
> +		else
> +			gen8_get_bios_reserved(dev_priv, &bios_rsvd_base,
> +					       &bios_rsvd_size);
> +		break;
> +	}
> +
> +	/* It is possible for the BIOS reserved base to be zero, but the
> +	 * register field for size doesn't have a zero option. */
> +	if (bios_rsvd_base == 0) {
> +		bios_rsvd_size = 0;
> +		bios_rsvd_base = stolen_top;
>  	}
>  
> -	if (WARN_ON(bios_reserved > dev_priv->gtt.stolen_size))
> +	if (bios_rsvd_base < dev_priv->mm.stolen_base ||
> +	    bios_rsvd_base + bios_rsvd_size > stolen_top) {
> +		DRM_ERROR("BIOS reserved area outside stolen memory\n");

This is not a driver error, nor something the user can fix, and the
message doesn't contain enough clues for the developer.

DRM_DEBUG_DRIVER("Reserved area [%x+%x] outside of stolen memory [%x+%x]\n", ...)

If you add a DRM_INFO() to report the amount of memory reserved by the
BIOS for the GPU and the amount of memory usable, that is both
interesting for the user (they see something that vaguely resembles a
BIOS option) and for us.

e.g.
DRM_INFO("Memory reserved for graphics device = %luM, usable %luM",
          dev_priv->gtt.stolen_size >> 20,
          (dev_priv->gtt.stolen_size - bios_rsvd_total) >> 20);

and adjust the code flow to match.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: fix stolen bios_reserved checks

[Paulo Zanoni]

I started digging this when I noticed that the BDW code was just
reserving 1mb by coincidence since it was reading reserved fields.
Then I noticed we didn't have any values set for SNB and earlier, and
that the HSW sizes were wrong. After that, I noticed that the reserved
area has a specific start, and may not exactly end where the stolen
memory ends. I also noticed the base pointer can be zero. So I decided
to just write a single patch fixing everything instead of 20 patches
that would be much harder to review.

This patch may solve random stolen memory corruption/problems on
almost all platforms. Notice that since this is always dealing with
the top of the stolen memory, the problems are not so easy to
reproduce - especially since FBC is still disabled by default.

One of the major differences of this patch is that we now look at both
the size and base address. By only looking at the size we were
assuming that the reserved area was always at the very top of
stolen, which is not always true.

After we merge the patch series that allows user space to allocate
stolen memory we'll be able to write IGT tests that maybe catch the
bugs fixed by this patch.

v2:
  - s/BIOS reserved/stolen reserved/g (Chris)
  - Don't DRM_ERROR if we can't do anything about it (Chris)
  - Improve debug messages (Chris).
  - Use the gen7 version instead of gen6 on HSW. Tom found some
    documentation problems, so I think with gen7 we're on the safer
    side (Tom).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 159 +++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_reg.h        |  23 +++--
 2 files changed, 159 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index ed682a9..a36cb95 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -186,11 +186,103 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
 	drm_mm_takedown(&dev_priv->mm.stolen);
 }
 
+static void gen6_get_stolen_reserved(struct drm_i915_private *dev_priv,
+				     unsigned long *base, unsigned long *size)
+{
+	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
+
+	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
+
+	switch (reg_val & GEN6_STOLEN_RESERVED_SIZE_MASK) {
+	case GEN6_STOLEN_RESERVED_1M:
+		*size = 1024 * 1024;
+		break;
+	case GEN6_STOLEN_RESERVED_512K:
+		*size = 512 * 1024;
+		break;
+	case GEN6_STOLEN_RESERVED_256K:
+		*size = 256 * 1024;
+		break;
+	case GEN6_STOLEN_RESERVED_128K:
+		*size = 128 * 1024;
+		break;
+	default:
+		*size = 1024 * 1024;
+		MISSING_CASE(reg_val & GEN6_STOLEN_RESERVED_SIZE_MASK);
+	}
+}
+
+static void gen7_get_stolen_reserved(struct drm_i915_private *dev_priv,
+				     unsigned long *base, unsigned long *size)
+{
+	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
+
+	*base = reg_val & GEN7_STOLEN_RESERVED_ADDR_MASK;
+
+	switch (reg_val & GEN7_STOLEN_RESERVED_SIZE_MASK) {
+	case GEN7_STOLEN_RESERVED_1M:
+		*size = 1024 * 1024;
+		break;
+	case GEN7_STOLEN_RESERVED_256K:
+		*size = 256 * 1024;
+		break;
+	default:
+		*size = 1024 * 1024;
+		MISSING_CASE(reg_val & GEN7_STOLEN_RESERVED_SIZE_MASK);
+	}
+}
+
+static void gen8_get_stolen_reserved(struct drm_i915_private *dev_priv,
+				     unsigned long *base, unsigned long *size)
+{
+	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
+
+	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
+
+	switch (reg_val & GEN8_STOLEN_RESERVED_SIZE_MASK) {
+	case GEN8_STOLEN_RESERVED_1M:
+		*size = 1024 * 1024;
+		break;
+	case GEN8_STOLEN_RESERVED_2M:
+		*size = 2 * 1024 * 1024;
+		break;
+	case GEN8_STOLEN_RESERVED_4M:
+		*size = 4 * 1024 * 1024;
+		break;
+	case GEN8_STOLEN_RESERVED_8M:
+		*size = 8 * 1024 * 1024;
+		break;
+	default:
+		*size = 8 * 1024 * 1024;
+		MISSING_CASE(reg_val & GEN8_STOLEN_RESERVED_SIZE_MASK);
+	}
+}
+
+static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv,
+				    unsigned long *base, unsigned long *size)
+{
+	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
+	unsigned long stolen_top;
+
+	stolen_top = dev_priv->mm.stolen_base + dev_priv->gtt.stolen_size;
+
+	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
+
+	/* On these platforms, the register doesn't have a size field, so the
+	 * size is the distance between the base and the top of the stolen
+	 * memory. We also have the genuine case where base is zero and there's
+	 * nothing reserved. */
+	if (*base == 0)
+		*size = 0;
+	else
+		*size = stolen_top - *base;
+}
+
 int i915_gem_init_stolen(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 tmp;
-	int bios_reserved = 0;
+	unsigned long reserved_total, reserved_base, reserved_size;
+	unsigned long stolen_top;
 
 	mutex_init(&dev_priv->mm.stolen_lock);
 
@@ -208,26 +300,61 @@ int i915_gem_init_stolen(struct drm_device *dev)
 	if (dev_priv->mm.stolen_base == 0)
 		return 0;
 
-	DRM_DEBUG_KMS("found %zd bytes of stolen memory at %08lx\n",
-		      dev_priv->gtt.stolen_size, dev_priv->mm.stolen_base);
-
-	if (INTEL_INFO(dev)->gen >= 8) {
-		tmp = I915_READ(GEN7_BIOS_RESERVED);
-		tmp >>= GEN8_BIOS_RESERVED_SHIFT;
-		tmp &= GEN8_BIOS_RESERVED_MASK;
-		bios_reserved = (1024*1024) << tmp;
-	} else if (IS_GEN7(dev)) {
-		tmp = I915_READ(GEN7_BIOS_RESERVED);
-		bios_reserved = tmp & GEN7_BIOS_RESERVED_256K ?
-			256*1024 : 1024*1024;
+	stolen_top = dev_priv->mm.stolen_base + dev_priv->gtt.stolen_size;
+
+	switch (INTEL_INFO(dev_priv)->gen) {
+	case 2:
+	case 3:
+	case 4:
+	case 5:
+		/* Assume the gen6 maximum for the older platforms. */
+		reserved_size = 1024 * 1024;
+		reserved_base = stolen_top - reserved_size;
+		break;
+	case 6:
+		gen6_get_stolen_reserved(dev_priv, &reserved_base,
+					 &reserved_size);
+		break;
+	case 7:
+		gen7_get_stolen_reserved(dev_priv, &reserved_base,
+					 &reserved_size);
+		break;
+	default:
+		if (IS_BROADWELL(dev_priv) || IS_SKYLAKE(dev_priv))
+			bdw_get_stolen_reserved(dev_priv, &reserved_base,
+						&reserved_size);
+		else
+			gen8_get_stolen_reserved(dev_priv, &reserved_base,
+						 &reserved_size);
+		break;
+	}
+
+	/* It is possible for the reserved base to be zero, but the register
+	 * field for size doesn't have a zero option. */
+	if (reserved_base == 0) {
+		reserved_size = 0;
+		reserved_base = stolen_top;
 	}
 
-	if (WARN_ON(bios_reserved > dev_priv->gtt.stolen_size))
+	if (reserved_base < dev_priv->mm.stolen_base ||
+	    reserved_base + reserved_size > stolen_top) {
+		DRM_DEBUG_KMS("Stolen reserved area [0x%08lx - 0x%08lx] outside stolen memory [0x%08lx - 0x%08lx]\n",
+			      reserved_base, reserved_base + reserved_size,
+			      dev_priv->mm.stolen_base, stolen_top);
 		return 0;
+	}
+
+	/* It is possible for the reserved area to end before the end of stolen
+	 * memory, so just consider the start. */
+	reserved_total = stolen_top - reserved_base;
+
+	DRM_DEBUG_KMS("Memory reserved for graphics device: %luK, usable: %luK\n",
+		      dev_priv->gtt.stolen_size >> 10,
+		      (dev_priv->gtt.stolen_size - reserved_total) >> 10);
 
 	/* Basic memrange allocator for stolen space */
 	drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size -
-		    bios_reserved);
+		    reserved_total);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ea46d68..1cb5562 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -178,13 +178,22 @@
 #define GAB_CTL				0x24000
 #define   GAB_CTL_CONT_AFTER_PAGEFAULT	(1<<8)
 
-#define GEN7_BIOS_RESERVED		0x1082C0
-#define GEN7_BIOS_RESERVED_1M		(0 << 5)
-#define GEN7_BIOS_RESERVED_256K		(1 << 5)
-#define GEN8_BIOS_RESERVED_SHIFT       7
-#define GEN7_BIOS_RESERVED_MASK        0x1
-#define GEN8_BIOS_RESERVED_MASK        0x3
-
+#define GEN6_STOLEN_RESERVED		0x1082C0
+#define GEN6_STOLEN_RESERVED_ADDR_MASK	(0xFFF << 20)
+#define GEN7_STOLEN_RESERVED_ADDR_MASK	(0x3FFF << 18)
+#define GEN6_STOLEN_RESERVED_SIZE_MASK	(3 << 4)
+#define GEN6_STOLEN_RESERVED_1M		(0 << 4)
+#define GEN6_STOLEN_RESERVED_512K	(1 << 4)
+#define GEN6_STOLEN_RESERVED_256K	(2 << 4)
+#define GEN6_STOLEN_RESERVED_128K	(3 << 4)
+#define GEN7_STOLEN_RESERVED_SIZE_MASK	(1 << 5)
+#define GEN7_STOLEN_RESERVED_1M		(0 << 5)
+#define GEN7_STOLEN_RESERVED_256K	(1 << 5)
+#define GEN8_STOLEN_RESERVED_SIZE_MASK	(3 << 7)
+#define GEN8_STOLEN_RESERVED_1M		(0 << 7)
+#define GEN8_STOLEN_RESERVED_2M		(1 << 7)
+#define GEN8_STOLEN_RESERVED_4M		(2 << 7)
+#define GEN8_STOLEN_RESERVED_8M		(3 << 7)
 
 /* VGA stuff */
 
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: fix stolen bios_reserved checks

[Chris Wilson]

On Mon, Aug 10, 2015 at 02:57:32PM -0300, Paulo Zanoni wrote:
> I started digging this when I noticed that the BDW code was just
> reserving 1mb by coincidence since it was reading reserved fields.
> Then I noticed we didn't have any values set for SNB and earlier, and
> that the HSW sizes were wrong. After that, I noticed that the reserved
> area has a specific start, and may not exactly end where the stolen
> memory ends. I also noticed the base pointer can be zero. So I decided
> to just write a single patch fixing everything instead of 20 patches
> that would be much harder to review.
> 
> This patch may solve random stolen memory corruption/problems on
> almost all platforms. Notice that since this is always dealing with
> the top of the stolen memory, the problems are not so easy to
> reproduce - especially since FBC is still disabled by default.
> 
> One of the major differences of this patch is that we now look at both
> the size and base address. By only looking at the size we were
> assuming that the reserved area was always at the very top of
> stolen, which is not always true.
> 
> After we merge the patch series that allows user space to allocate
> stolen memory we'll be able to write IGT tests that maybe catch the
> bugs fixed by this patch.
> 
> v2:
>   - s/BIOS reserved/stolen reserved/g (Chris)
>   - Don't DRM_ERROR if we can't do anything about it (Chris)
>   - Improve debug messages (Chris).
>   - Use the gen7 version instead of gen6 on HSW. Tom found some
>     documentation problems, so I think with gen7 we're on the safer
>     side (Tom).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Looks ok to me, I'd push for DRM_INFO() for the amount of memory
available (since I think that is interesting as a user).

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: fix stolen bios_reserved checks

[Daniel Vetter]

On Mon, Aug 10, 2015 at 07:06:44PM +0100, Chris Wilson wrote:
> On Mon, Aug 10, 2015 at 02:57:32PM -0300, Paulo Zanoni wrote:
> > I started digging this when I noticed that the BDW code was just
> > reserving 1mb by coincidence since it was reading reserved fields.
> > Then I noticed we didn't have any values set for SNB and earlier, and
> > that the HSW sizes were wrong. After that, I noticed that the reserved
> > area has a specific start, and may not exactly end where the stolen
> > memory ends. I also noticed the base pointer can be zero. So I decided
> > to just write a single patch fixing everything instead of 20 patches
> > that would be much harder to review.
> > 
> > This patch may solve random stolen memory corruption/problems on
> > almost all platforms. Notice that since this is always dealing with
> > the top of the stolen memory, the problems are not so easy to
> > reproduce - especially since FBC is still disabled by default.
> > 
> > One of the major differences of this patch is that we now look at both
> > the size and base address. By only looking at the size we were
> > assuming that the reserved area was always at the very top of
> > stolen, which is not always true.
> > 
> > After we merge the patch series that allows user space to allocate
> > stolen memory we'll be able to write IGT tests that maybe catch the
> > bugs fixed by this patch.
> > 
> > v2:
> >   - s/BIOS reserved/stolen reserved/g (Chris)
> >   - Don't DRM_ERROR if we can't do anything about it (Chris)
> >   - Improve debug messages (Chris).
> >   - Use the gen7 version instead of gen6 on HSW. Tom found some
> >     documentation problems, so I think with gen7 we're on the safer
> >     side (Tom).
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Looks ok to me, I'd push for DRM_INFO() for the amount of memory
> available (since I think that is interesting as a user).
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: fix stolen bios_reserved checks

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7018
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                 -1              283/283              282/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: fix stolen bios_reserved checks

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7138
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -1              302/302              301/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                                  283/283              283/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@wf_vblank-vs-modeset-interruptible      PASS(1)      DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx