Re: Intel 915GM MCHBAR bug

From: Woody Suwalski
Date: Fri Jun 05 2009 - 17:17:36 EST


Andrew Morton wrote:
On Fri, 05 Jun 2009 11:02:21 +0300
Pekka Enberg <penberg@xxxxxxxxxxxxxx> wrote:

Hi Jesse,

(I am cc'ing linux-kernel.)

On Mon, 01 Jun 2009 22:31:13 +0300
Pekka Enberg <penberg@xxxxxxxxxxxxxx> wrote:

Hi Jesse,

I am seeing this on my Eee PC 701 which is running a Ubuntu 2.6.28-11.42-generic stock kernel:

[ 76.826966] [drm:i915_gem_detect_bit_6_swizzle] *ERROR* Couldn't
read from MCHBAR. Disabling tiling.
[ 76.827215] [drm] Initialized i915 1.6.0 20080730 on minor 0
[ 76.834037] [drm:i915_setparam] *ERROR* unknown parameter 4
[ 76.834104] [drm:i915_getparam] *ERROR* Unknown parameter 6

Google turned up this patch

http://lists.freedesktop.org/archives/intel-gfx/2009-January/001186.html

but I don't seem to find it mainline kernel.

Was the bug fixed in some other way? It seem that distributions have
not yet picked up your patch and I am unsure if it's in any of the
-stable kernels.
On Thu, 2009-06-04 at 11:27 +0100, Jesse Barnes wrote:
I think Eric acked it but may not have pushed it to drm-intel-next
yet. Should happen in the next week or two though as we prepare the
merge window series.
OK, thanks for the info! FYI, I tested 2.6.30-rc8 with your patch
applied and everything works smoothly on my EeePC 701. I did not test
plain 2.6.30-rc8 as I expect it to show same kind of behavior as Ubuntu
distro kernel. Is this correct or do you want me to test 2.6.30-rc8 too?

I wonder why the patch hasn't received more attention as it's a pretty
critical bug. The _default_ Ubuntu 9.04 netbook remix installation is
completely broken for EeePC 701 (and probably others as well) without
the patch. And it's not as if I'm the only one that suffers from it. A
quick Google search reveals that a lot of people are hitting it.

So I really do think we need to merge this patch to the upcoming 2.6.31
ASAP and backport it to -stable after it has gotten some more testing.


Wanna show us the patch?

Because the world could certainly do with more i915 bugfixes :(
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Here is Jesse's patch from http://lists.freedesktop.org/archives/intel-gfx/2009-January/001186.html adopted for 2.6.30-rc8... (needed to redo hunk #3 for i915_gem_tile.c)

Woody

--
Woody Suwalski, Xandros, Ottawa, Canada, 1-613-842-3498 x414

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9851589..5919537 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -143,6 +143,8 @@ typedef struct drm_i915_private {
drm_local_map_t hws_map;
struct drm_gem_object *hws_obj;

+ struct resource mch_res;
+
unsigned int cpp;
int back_offset;
int front_offset;
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c
b/drivers/gpu/drm/i915/i915_gem_tiling.c
index fe7877a..1441a9e 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -24,6 +24,7 @@
* Eric Anholt <eric@xxxxxxxxxx>
*
*/
+#include <linux/acpi.h>

#include "linux/string.h"
#include "linux/bitops.h"
@@ -81,6 +82,176 @@
* to match what the GPU expects.
*/

+#define MCHBAR_I915 0x44
+#define MCHBAR_I965 0x48
+#define MCHBAR_SIZE (4*4096)
+
+#define DEVEN_REG 0x54
+#define DEVEN_MCHBAR_EN (1 << 28)
+
+/*
+ * ACPI resource checking fun. So the MCHBAR has *probably* been set
+ * up by the BIOS since drivers need to poke at it, but out of paranoia
+ * or whatever, many BIOSes disable the MCHBAR at boot. So we check
+ * to make sure any existing address is reserved before using it. If
+ * we can't find a match or there is no address, allocate some new PCI
+ * space for it, and then enable it. And of course 915 has to be different
+ * and put its enable bit somewhere else...
+ */
+static acpi_status __init check_mch_resource(struct acpi_resource *res,
+ void *data)
+{
+ struct resource *mch_res = data;
+ struct acpi_resource_address64 address;
+ acpi_status status;
+
+ if (res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
+ struct acpi_resource_fixed_memory32 *fixmem32 =
+ &res->data.fixed_memory32;
+ if (!fixmem32)
+ return AE_OK;
+
+ if ((mch_res->start >= fixmem32->address) &&
+ (mch_res->end < (fixmem32->address +
+ fixmem32->address_length))) {
+ mch_res->flags = 1;
+ return AE_CTRL_TERMINATE;
+ }
+ }
+ if ((res->type != ACPI_RESOURCE_TYPE_ADDRESS32) &&
+ (res->type != ACPI_RESOURCE_TYPE_ADDRESS64))
+ return AE_OK;
+
+ status = acpi_resource_to_address64(res, &address);
+ if (ACPI_FAILURE(status) ||
+ (address.address_length <= 0) ||
+ (address.resource_type != ACPI_MEMORY_RANGE))
+ return AE_OK;
+
+ if ((mch_res->start >= address.minimum) &&
+ (mch_res->end < (address.minimum + address.address_length))) {
+ mch_res->flags = 1;
+ return AE_CTRL_TERMINATE;
+ }
+ return AE_OK;
+}
+
+static acpi_status __init find_mboard_resource(acpi_handle handle, u32 lvl,
+ void *context, void **rv)
+{
+ struct resource *mch_res = context;
+
+ acpi_walk_resources(handle, METHOD_NAME__CRS,
+ check_mch_resource, context);
+
+ if (mch_res->flags)
+ return AE_CTRL_TERMINATE;
+
+ return AE_OK;
+}
+
+static int __init is_acpi_reserved(u64 start, u64 end, unsigned not_used)
+{
+ struct resource mch_res;
+
+ mch_res.start = start;
+ mch_res.end = end;
+ mch_res.flags = 0;
+
+ acpi_get_devices("PNP0C01", find_mboard_resource, &mch_res, NULL);
+
+ if (!mch_res.flags)
+ acpi_get_devices("PNP0C02", find_mboard_resource, &mch_res,
+ NULL);
+
+ return mch_res.flags;
+}
+
+/* Allocate space for the MCH regs if needed, return nonzero on error */
+static int
+intel_alloc_mchbar_resource(struct drm_device *dev)
+{
+ struct pci_dev *bridge_dev;
+ drm_i915_private_t *dev_priv = dev->dev_private;
+ int reg = IS_I965G(dev) ? MCHBAR_I965 : MCHBAR_I915;
+ u32 temp_lo, temp_hi = 0;
+ u64 mchbar_addr;
+ int ret;
+
+ bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0,0));
+ if (!bridge_dev) {
+ DRM_DEBUG("no bridge dev?!\n");
+ return -ENODEV;
+ }
+
+ if (IS_I965G(dev))
+ pci_read_config_dword(bridge_dev, reg + 4, &temp_hi);
+ pci_read_config_dword(bridge_dev, reg, &temp_lo);
+ mchbar_addr = ((u64)temp_hi << 32) | temp_lo;
+
+ /* If ACPI doesn't have it, assume we need to allocate it ourselves */
+ if (mchbar_addr &&
+ is_acpi_reserved(mchbar_addr, mchbar_addr + MCHBAR_SIZE, 0))
+ return 0;
+
+ /* Get some space for it */
+ ret = pci_bus_alloc_resource(bridge_dev->bus, &dev_priv->mch_res,
+ MCHBAR_SIZE, MCHBAR_SIZE,
+ PCIBIOS_MIN_MEM,
+ 0, pcibios_align_resource,
+ bridge_dev);
+ if (ret) {
+ DRM_DEBUG("failed bus alloc: %d\n", ret);
+ dev_priv->mch_res.start = 0;
+ return ret;
+ }
+
+ if (IS_I965G(dev))
+ pci_write_config_dword(bridge_dev, reg + 4,
+ upper_32_bits(dev_priv->mch_res.start));
+
+ pci_write_config_dword(bridge_dev, reg,
+ lower_32_bits(dev_priv->mch_res.start));
+
+ return 0;
+}
+
+static void
+intel_setup_mchbar(struct drm_device *dev)
+{
+ struct pci_dev *bridge_dev;
+ int mchbar_reg = IS_I965G(dev) ? MCHBAR_I965 : MCHBAR_I915;
+ u32 temp;
+
+ bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0,0));
+ if (!bridge_dev) {
+ DRM_DEBUG("no bridge dev?!\n");
+ return;
+ }
+
+ if (intel_alloc_mchbar_resource(dev))
+ return;
+
+ /* Now enable it */
+ if (IS_I915G(dev) || IS_I915GM(dev)) {
+ pci_read_config_dword(bridge_dev, DEVEN_REG, &temp);
+ pci_write_config_dword(bridge_dev, DEVEN_REG,
+ temp | DEVEN_MCHBAR_EN);
+ } else {
+ pci_read_config_dword(bridge_dev, mchbar_reg, &temp);
+ pci_write_config_dword(bridge_dev, mchbar_reg, temp | 1);
+ }
+}
+
+static void
+intel_teardown_mchbar(struct drm_device *dev)
+{
+ drm_i915_private_t *dev_priv = dev->dev_private;
+
+ if (dev_priv->mch_res.start)
+ release_resource(&dev_priv->mch_res);
+}
+
/**
* Detects bit 6 swizzling of address lookup between IGD access and CPU
* access through main memory.
@@ -101,6 +272,9 @@ i915_gem_detect_bit_6_swizzle(struct drm
} else if (IS_MOBILE(dev)) {
uint32_t dcc;

+ /* Try to make sure MCHBAR is enabled before poking at it */
+ intel_setup_mchbar(dev);
+
/* On mobile 9xx chipsets, channel interleave by the CPU is
* determined by DCC. For single-channel, neither the CPU
* nor the GPU do swizzling. For dual channel interleaved,
@@ -140,6 +314,8 @@ i915_gem_detect_bit_6_swizzle(struct drm
swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN;
swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN;
}
+
+ intel_teardown_mchbar(dev);
} else {
/* The 965, G33, and newer, have a very flexible memory
* configuration. It will enable dual-channel mode