[Intel-gfx] [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