Patch 2/2 has the description, but suffice it to say I'm not really
pleased with this, though it does solve a problem we have. On some
machines, we get MMIO space allocated on top of this hidden memory,
which can cause problems. I'm not sure if there are similar problems
for other hunks of the address space; if so it's possible this could be
made more general (though the bits for looking up the address of this
region are definitely Intel graphics specific).
Chris has some patches on top to add a new E820 type so we can look up
the region later, which removes some redundant code in the i915 driver
at least.
Any comments? I assume no one likes this, but maybe it's just another
early quirk we'll have to live with...
Thanks,
Jesse
For use by userspace (at some point in the future) and other kernel code.
v2: move PCI IDs to uabi (Chris)
move PCI IDs to drm/ (Dave)
v3: fixup Quanta detection - needs to come first (Daniel)
Signed-off-by: Jesse Barnes <[email protected]>
---
drivers/gpu/drm/i915/i915_drv.c | 164 +++++++-----------------------
include/drm/i915_drm.h | 2 +
include/drm/i915_pciids.h | 208 +++++++++++++++++++++++++++++++++++++++
3 files changed, 244 insertions(+), 130 deletions(-)
create mode 100644 include/drm/i915_pciids.h
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b07362f..e87bccf 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -140,25 +140,6 @@ MODULE_PARM_DESC(fastboot, "Try to skip unnecessary mode sets at boot time "
static struct drm_driver driver;
extern int intel_agp_enabled;
-#define INTEL_VGA_DEVICE(id, info) { \
- .class = PCI_BASE_CLASS_DISPLAY << 16, \
- .class_mask = 0xff0000, \
- .vendor = 0x8086, \
- .device = id, \
- .subvendor = PCI_ANY_ID, \
- .subdevice = PCI_ANY_ID, \
- .driver_data = (unsigned long) info }
-
-#define INTEL_QUANTA_VGA_DEVICE(info) { \
- .class = PCI_BASE_CLASS_DISPLAY << 16, \
- .class_mask = 0xff0000, \
- .vendor = 0x8086, \
- .device = 0x16a, \
- .subvendor = 0x152d, \
- .subdevice = 0x8990, \
- .driver_data = (unsigned long) info }
-
-
static const struct intel_device_info intel_i830_info = {
.gen = 2, .is_mobile = 1, .cursor_needs_physical = 1, .num_pipes = 2,
.has_overlay = 1, .overlay_needs_physical = 1,
@@ -333,118 +314,41 @@ static const struct intel_device_info intel_haswell_m_info = {
.has_vebox_ring = 1,
};
+/*
+ * Make sure any device matches here are from most specific to most
+ * general. For example, since the Quanta match is based on the subsystem
+ * and subvendor IDs, we need it to come before the more general IVB
+ * PCI ID matches, otherwise we'll use the wrong info struct above.
+ */
+#define INTEL_PCI_IDS \
+ INTEL_I830_IDS(&intel_i830_info), \
+ INTEL_I845G_IDS(&intel_845g_info), \
+ INTEL_I85X_IDS(&intel_i85x_info), \
+ INTEL_I865G_IDS(&intel_i865g_info), \
+ INTEL_I915G_IDS(&intel_i915g_info), \
+ INTEL_I915GM_IDS(&intel_i915gm_info), \
+ INTEL_I945G_IDS(&intel_i945g_info), \
+ INTEL_I945GM_IDS(&intel_i945gm_info), \
+ INTEL_I965G_IDS(&intel_i965g_info), \
+ INTEL_G33_IDS(&intel_g33_info), \
+ INTEL_I965GM_IDS(&intel_i965gm_info), \
+ INTEL_GM45_IDS(&intel_gm45_info), \
+ INTEL_G45_IDS(&intel_g45_info), \
+ INTEL_PINEVIEW_IDS(&intel_pineview_info), \
+ INTEL_IRONLAKE_D_IDS(&intel_ironlake_d_info), \
+ INTEL_IRONLAKE_M_IDS(&intel_ironlake_m_info), \
+ INTEL_SNB_D_IDS(&intel_sandybridge_d_info), \
+ INTEL_SNB_M_IDS(&intel_sandybridge_m_info), \
+ INTEL_IVB_Q_IDS(&intel_ivybridge_q_info), /* must be first IVB */ \
+ INTEL_IVB_M_IDS(&intel_ivybridge_m_info), \
+ INTEL_IVB_D_IDS(&intel_ivybridge_d_info), \
+ INTEL_HSW_D_IDS(&intel_haswell_d_info), \
+ INTEL_HSW_M_IDS(&intel_haswell_m_info), \
+ INTEL_VLV_M_IDS(&intel_valleyview_m_info), \
+ INTEL_VLV_D_IDS(&intel_valleyview_d_info)
+
static const struct pci_device_id pciidlist[] = { /* aka */
- INTEL_VGA_DEVICE(0x3577, &intel_i830_info), /* I830_M */
- INTEL_VGA_DEVICE(0x2562, &intel_845g_info), /* 845_G */
- INTEL_VGA_DEVICE(0x3582, &intel_i85x_info), /* I855_GM */
- INTEL_VGA_DEVICE(0x358e, &intel_i85x_info),
- INTEL_VGA_DEVICE(0x2572, &intel_i865g_info), /* I865_G */
- INTEL_VGA_DEVICE(0x2582, &intel_i915g_info), /* I915_G */
- INTEL_VGA_DEVICE(0x258a, &intel_i915g_info), /* E7221_G */
- INTEL_VGA_DEVICE(0x2592, &intel_i915gm_info), /* I915_GM */
- INTEL_VGA_DEVICE(0x2772, &intel_i945g_info), /* I945_G */
- INTEL_VGA_DEVICE(0x27a2, &intel_i945gm_info), /* I945_GM */
- INTEL_VGA_DEVICE(0x27ae, &intel_i945gm_info), /* I945_GME */
- INTEL_VGA_DEVICE(0x2972, &intel_i965g_info), /* I946_GZ */
- INTEL_VGA_DEVICE(0x2982, &intel_i965g_info), /* G35_G */
- INTEL_VGA_DEVICE(0x2992, &intel_i965g_info), /* I965_Q */
- INTEL_VGA_DEVICE(0x29a2, &intel_i965g_info), /* I965_G */
- INTEL_VGA_DEVICE(0x29b2, &intel_g33_info), /* Q35_G */
- INTEL_VGA_DEVICE(0x29c2, &intel_g33_info), /* G33_G */
- INTEL_VGA_DEVICE(0x29d2, &intel_g33_info), /* Q33_G */
- INTEL_VGA_DEVICE(0x2a02, &intel_i965gm_info), /* I965_GM */
- INTEL_VGA_DEVICE(0x2a12, &intel_i965gm_info), /* I965_GME */
- INTEL_VGA_DEVICE(0x2a42, &intel_gm45_info), /* GM45_G */
- INTEL_VGA_DEVICE(0x2e02, &intel_g45_info), /* IGD_E_G */
- INTEL_VGA_DEVICE(0x2e12, &intel_g45_info), /* Q45_G */
- INTEL_VGA_DEVICE(0x2e22, &intel_g45_info), /* G45_G */
- INTEL_VGA_DEVICE(0x2e32, &intel_g45_info), /* G41_G */
- INTEL_VGA_DEVICE(0x2e42, &intel_g45_info), /* B43_G */
- INTEL_VGA_DEVICE(0x2e92, &intel_g45_info), /* B43_G.1 */
- INTEL_VGA_DEVICE(0xa001, &intel_pineview_info),
- INTEL_VGA_DEVICE(0xa011, &intel_pineview_info),
- INTEL_VGA_DEVICE(0x0042, &intel_ironlake_d_info),
- INTEL_VGA_DEVICE(0x0046, &intel_ironlake_m_info),
- INTEL_VGA_DEVICE(0x0102, &intel_sandybridge_d_info),
- INTEL_VGA_DEVICE(0x0112, &intel_sandybridge_d_info),
- INTEL_VGA_DEVICE(0x0122, &intel_sandybridge_d_info),
- INTEL_VGA_DEVICE(0x0106, &intel_sandybridge_m_info),
- INTEL_VGA_DEVICE(0x0116, &intel_sandybridge_m_info),
- INTEL_VGA_DEVICE(0x0126, &intel_sandybridge_m_info),
- INTEL_VGA_DEVICE(0x010A, &intel_sandybridge_d_info),
- INTEL_VGA_DEVICE(0x0156, &intel_ivybridge_m_info), /* GT1 mobile */
- INTEL_VGA_DEVICE(0x0166, &intel_ivybridge_m_info), /* GT2 mobile */
- INTEL_VGA_DEVICE(0x0152, &intel_ivybridge_d_info), /* GT1 desktop */
- INTEL_VGA_DEVICE(0x0162, &intel_ivybridge_d_info), /* GT2 desktop */
- INTEL_VGA_DEVICE(0x015a, &intel_ivybridge_d_info), /* GT1 server */
- INTEL_QUANTA_VGA_DEVICE(&intel_ivybridge_q_info), /* Quanta transcode */
- INTEL_VGA_DEVICE(0x016a, &intel_ivybridge_d_info), /* GT2 server */
- INTEL_VGA_DEVICE(0x0402, &intel_haswell_d_info), /* GT1 desktop */
- INTEL_VGA_DEVICE(0x0412, &intel_haswell_d_info), /* GT2 desktop */
- INTEL_VGA_DEVICE(0x0422, &intel_haswell_d_info), /* GT3 desktop */
- INTEL_VGA_DEVICE(0x040a, &intel_haswell_d_info), /* GT1 server */
- INTEL_VGA_DEVICE(0x041a, &intel_haswell_d_info), /* GT2 server */
- INTEL_VGA_DEVICE(0x042a, &intel_haswell_d_info), /* GT3 server */
- INTEL_VGA_DEVICE(0x0406, &intel_haswell_m_info), /* GT1 mobile */
- INTEL_VGA_DEVICE(0x0416, &intel_haswell_m_info), /* GT2 mobile */
- INTEL_VGA_DEVICE(0x0426, &intel_haswell_m_info), /* GT2 mobile */
- INTEL_VGA_DEVICE(0x040B, &intel_haswell_d_info), /* GT1 reserved */
- INTEL_VGA_DEVICE(0x041B, &intel_haswell_d_info), /* GT2 reserved */
- INTEL_VGA_DEVICE(0x042B, &intel_haswell_d_info), /* GT3 reserved */
- INTEL_VGA_DEVICE(0x040E, &intel_haswell_d_info), /* GT1 reserved */
- INTEL_VGA_DEVICE(0x041E, &intel_haswell_d_info), /* GT2 reserved */
- INTEL_VGA_DEVICE(0x042E, &intel_haswell_d_info), /* GT3 reserved */
- INTEL_VGA_DEVICE(0x0C02, &intel_haswell_d_info), /* SDV GT1 desktop */
- INTEL_VGA_DEVICE(0x0C12, &intel_haswell_d_info), /* SDV GT2 desktop */
- INTEL_VGA_DEVICE(0x0C22, &intel_haswell_d_info), /* SDV GT3 desktop */
- INTEL_VGA_DEVICE(0x0C0A, &intel_haswell_d_info), /* SDV GT1 server */
- INTEL_VGA_DEVICE(0x0C1A, &intel_haswell_d_info), /* SDV GT2 server */
- INTEL_VGA_DEVICE(0x0C2A, &intel_haswell_d_info), /* SDV GT3 server */
- INTEL_VGA_DEVICE(0x0C06, &intel_haswell_m_info), /* SDV GT1 mobile */
- INTEL_VGA_DEVICE(0x0C16, &intel_haswell_m_info), /* SDV GT2 mobile */
- INTEL_VGA_DEVICE(0x0C26, &intel_haswell_m_info), /* SDV GT3 mobile */
- INTEL_VGA_DEVICE(0x0C0B, &intel_haswell_d_info), /* SDV GT1 reserved */
- INTEL_VGA_DEVICE(0x0C1B, &intel_haswell_d_info), /* SDV GT2 reserved */
- INTEL_VGA_DEVICE(0x0C2B, &intel_haswell_d_info), /* SDV GT3 reserved */
- INTEL_VGA_DEVICE(0x0C0E, &intel_haswell_d_info), /* SDV GT1 reserved */
- INTEL_VGA_DEVICE(0x0C1E, &intel_haswell_d_info), /* SDV GT2 reserved */
- INTEL_VGA_DEVICE(0x0C2E, &intel_haswell_d_info), /* SDV GT3 reserved */
- INTEL_VGA_DEVICE(0x0A02, &intel_haswell_d_info), /* ULT GT1 desktop */
- INTEL_VGA_DEVICE(0x0A12, &intel_haswell_d_info), /* ULT GT2 desktop */
- INTEL_VGA_DEVICE(0x0A22, &intel_haswell_d_info), /* ULT GT3 desktop */
- INTEL_VGA_DEVICE(0x0A0A, &intel_haswell_d_info), /* ULT GT1 server */
- INTEL_VGA_DEVICE(0x0A1A, &intel_haswell_d_info), /* ULT GT2 server */
- INTEL_VGA_DEVICE(0x0A2A, &intel_haswell_d_info), /* ULT GT3 server */
- INTEL_VGA_DEVICE(0x0A06, &intel_haswell_m_info), /* ULT GT1 mobile */
- INTEL_VGA_DEVICE(0x0A16, &intel_haswell_m_info), /* ULT GT2 mobile */
- INTEL_VGA_DEVICE(0x0A26, &intel_haswell_m_info), /* ULT GT3 mobile */
- INTEL_VGA_DEVICE(0x0A0B, &intel_haswell_d_info), /* ULT GT1 reserved */
- INTEL_VGA_DEVICE(0x0A1B, &intel_haswell_d_info), /* ULT GT2 reserved */
- INTEL_VGA_DEVICE(0x0A2B, &intel_haswell_d_info), /* ULT GT3 reserved */
- INTEL_VGA_DEVICE(0x0A0E, &intel_haswell_m_info), /* ULT GT1 reserved */
- INTEL_VGA_DEVICE(0x0A1E, &intel_haswell_m_info), /* ULT GT2 reserved */
- INTEL_VGA_DEVICE(0x0A2E, &intel_haswell_m_info), /* ULT GT3 reserved */
- INTEL_VGA_DEVICE(0x0D02, &intel_haswell_d_info), /* CRW GT1 desktop */
- INTEL_VGA_DEVICE(0x0D12, &intel_haswell_d_info), /* CRW GT2 desktop */
- INTEL_VGA_DEVICE(0x0D22, &intel_haswell_d_info), /* CRW GT3 desktop */
- INTEL_VGA_DEVICE(0x0D0A, &intel_haswell_d_info), /* CRW GT1 server */
- INTEL_VGA_DEVICE(0x0D1A, &intel_haswell_d_info), /* CRW GT2 server */
- INTEL_VGA_DEVICE(0x0D2A, &intel_haswell_d_info), /* CRW GT3 server */
- INTEL_VGA_DEVICE(0x0D06, &intel_haswell_m_info), /* CRW GT1 mobile */
- INTEL_VGA_DEVICE(0x0D16, &intel_haswell_m_info), /* CRW GT2 mobile */
- INTEL_VGA_DEVICE(0x0D26, &intel_haswell_m_info), /* CRW GT3 mobile */
- INTEL_VGA_DEVICE(0x0D0B, &intel_haswell_d_info), /* CRW GT1 reserved */
- INTEL_VGA_DEVICE(0x0D1B, &intel_haswell_d_info), /* CRW GT2 reserved */
- INTEL_VGA_DEVICE(0x0D2B, &intel_haswell_d_info), /* CRW GT3 reserved */
- INTEL_VGA_DEVICE(0x0D0E, &intel_haswell_d_info), /* CRW GT1 reserved */
- INTEL_VGA_DEVICE(0x0D1E, &intel_haswell_d_info), /* CRW GT2 reserved */
- INTEL_VGA_DEVICE(0x0D2E, &intel_haswell_d_info), /* CRW GT3 reserved */
- INTEL_VGA_DEVICE(0x0f30, &intel_valleyview_m_info),
- INTEL_VGA_DEVICE(0x0f31, &intel_valleyview_m_info),
- INTEL_VGA_DEVICE(0x0f32, &intel_valleyview_m_info),
- INTEL_VGA_DEVICE(0x0f33, &intel_valleyview_m_info),
- INTEL_VGA_DEVICE(0x0157, &intel_valleyview_m_info),
- INTEL_VGA_DEVICE(0x0155, &intel_valleyview_d_info),
+ INTEL_PCI_IDS,
{0, 0, 0}
};
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 63d609d..7276a72 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -26,6 +26,7 @@
#ifndef _I915_DRM_H_
#define _I915_DRM_H_
+#include <drm/i915_pciids.h>
#include <uapi/drm/i915_drm.h>
/* For use by IPS driver */
@@ -34,4 +35,5 @@ extern bool i915_gpu_raise(void);
extern bool i915_gpu_lower(void);
extern bool i915_gpu_busy(void);
extern bool i915_gpu_turbo_disable(void);
+
#endif /* _I915_DRM_H_ */
diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
new file mode 100644
index 0000000..3f03544
--- /dev/null
+++ b/include/drm/i915_pciids.h
@@ -0,0 +1,208 @@
+/*
+ * Copyright 2013 Intel Corporation
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
+ * IN NO EVENT SHALL TUNGSTEN GRAPHICS AND/OR ITS SUPPLIERS BE LIABLE FOR
+ * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
+ * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
+ * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+#ifndef _I915_PCIIDS_H
+#define _I915_PCIIDS_H
+
+#define INTEL_VGA_DEVICE(id, info) { \
+ .class = PCI_BASE_CLASS_DISPLAY << 16, \
+ .class_mask = 0xff0000, \
+ .vendor = 0x8086, \
+ .device = id, \
+ .subvendor = PCI_ANY_ID, \
+ .subdevice = PCI_ANY_ID, \
+ .driver_data = (unsigned long) info }
+
+#define INTEL_QUANTA_VGA_DEVICE(info) { \
+ .class = PCI_BASE_CLASS_DISPLAY << 16, \
+ .class_mask = 0xff0000, \
+ .vendor = 0x8086, \
+ .device = 0x16a, \
+ .subvendor = 0x152d, \
+ .subdevice = 0x8990, \
+ .driver_data = (unsigned long) info }
+
+#define INTEL_I830_IDS(info) \
+ INTEL_VGA_DEVICE(0x3577, info)
+
+#define INTEL_I845G_IDS(info) \
+ INTEL_VGA_DEVICE(0x2562, info)
+
+#define INTEL_I85X_IDS(info) \
+ INTEL_VGA_DEVICE(0x3582, info), /* I855_GM */ \
+ INTEL_VGA_DEVICE(0x358e, info)
+
+#define INTEL_I865G_IDS(info) \
+ INTEL_VGA_DEVICE(0x2572, info) /* I865_G */
+
+#define INTEL_I915G_IDS(info) \
+ INTEL_VGA_DEVICE(0x2582, info), /* I915_G */ \
+ INTEL_VGA_DEVICE(0x258a, info) /* E7221_G */
+
+#define INTEL_I915GM_IDS(info) \
+ INTEL_VGA_DEVICE(0x2592, info) /* I915_GM */
+
+#define INTEL_I945G_IDS(info) \
+ INTEL_VGA_DEVICE(0x2772, info) /* I945_G */
+
+#define INTEL_I945GM_IDS(info) \
+ INTEL_VGA_DEVICE(0x27a2, info), /* I945_GM */ \
+ INTEL_VGA_DEVICE(0x27ae, info) /* I945_GME */
+
+#define INTEL_I965G_IDS(info) \
+ INTEL_VGA_DEVICE(0x2972, info), /* I946_GZ */ \
+ INTEL_VGA_DEVICE(0x2982, info), /* G35_G */ \
+ INTEL_VGA_DEVICE(0x2992, info), /* I965_Q */ \
+ INTEL_VGA_DEVICE(0x29a2, info) /* I965_G */
+
+#define INTEL_G33_IDS(info) \
+ INTEL_VGA_DEVICE(0x29b2, info), /* Q35_G */ \
+ INTEL_VGA_DEVICE(0x29c2, info), /* G33_G */ \
+ INTEL_VGA_DEVICE(0x29d2, info) /* Q33_G */
+
+#define INTEL_I965GM_IDS(info) \
+ INTEL_VGA_DEVICE(0x2a02, info), /* I965_GM */ \
+ INTEL_VGA_DEVICE(0x2a12, info) /* I965_GME */
+
+#define INTEL_GM45_IDS(info) \
+ INTEL_VGA_DEVICE(0x2a42, info) /* GM45_G */
+
+#define INTEL_G45_IDS(info) \
+ INTEL_VGA_DEVICE(0x2e02, info), /* IGD_E_G */ \
+ INTEL_VGA_DEVICE(0x2e12, info), /* Q45_G */ \
+ INTEL_VGA_DEVICE(0x2e22, info), /* G45_G */ \
+ INTEL_VGA_DEVICE(0x2e32, info), /* G41_G */ \
+ INTEL_VGA_DEVICE(0x2e42, info), /* B43_G */ \
+ INTEL_VGA_DEVICE(0x2e92, info) /* B43_G.1 */
+
+#define INTEL_PINEVIEW_IDS(info) \
+ INTEL_VGA_DEVICE(0xa001, info), \
+ INTEL_VGA_DEVICE(0xa011, info)
+
+#define INTEL_IRONLAKE_D_IDS(info) \
+ INTEL_VGA_DEVICE(0x0042, info)
+
+#define INTEL_IRONLAKE_M_IDS(info) \
+ INTEL_VGA_DEVICE(0x0046, info)
+
+#define INTEL_SNB_D_IDS(info) \
+ INTEL_VGA_DEVICE(0x0102, info), \
+ INTEL_VGA_DEVICE(0x0112, info), \
+ INTEL_VGA_DEVICE(0x0122, info), \
+ INTEL_VGA_DEVICE(0x010A, info)
+
+#define INTEL_SNB_M_IDS(info) \
+ INTEL_VGA_DEVICE(0x0106, info), \
+ INTEL_VGA_DEVICE(0x0116, info), \
+ INTEL_VGA_DEVICE(0x0126, info)
+
+#define INTEL_IVB_M_IDS(info) \
+ INTEL_VGA_DEVICE(0x0156, info), /* GT1 mobile */ \
+ INTEL_VGA_DEVICE(0x0166, info) /* GT2 mobile */
+
+#define INTEL_IVB_D_IDS(info) \
+ INTEL_VGA_DEVICE(0x0152, info), /* GT1 desktop */ \
+ INTEL_VGA_DEVICE(0x0162, info), /* GT2 desktop */ \
+ INTEL_VGA_DEVICE(0x015a, info), /* GT1 server */ \
+ INTEL_VGA_DEVICE(0x016a, info) /* GT2 server */
+
+#define INTEL_IVB_Q_IDS(info) \
+ INTEL_QUANTA_VGA_DEVICE(info) /* Quanta transcode */
+
+#define INTEL_HSW_D_IDS(info) \
+ INTEL_VGA_DEVICE(0x0402, info), /* GT1 desktop */ \
+ INTEL_VGA_DEVICE(0x0412, info), /* GT2 desktop */ \
+ INTEL_VGA_DEVICE(0x0422, info), /* GT3 desktop */ \
+ INTEL_VGA_DEVICE(0x040a, info), /* GT1 server */ \
+ INTEL_VGA_DEVICE(0x041a, info), /* GT2 server */ \
+ INTEL_VGA_DEVICE(0x042a, info), /* GT3 server */ \
+ INTEL_VGA_DEVICE(0x040B, info), /* GT1 reserved */ \
+ INTEL_VGA_DEVICE(0x041B, info), /* GT2 reserved */ \
+ INTEL_VGA_DEVICE(0x042B, info), /* GT3 reserved */ \
+ INTEL_VGA_DEVICE(0x040E, info), /* GT1 reserved */ \
+ INTEL_VGA_DEVICE(0x041E, info), /* GT2 reserved */ \
+ INTEL_VGA_DEVICE(0x042E, info), /* GT3 reserved */ \
+ INTEL_VGA_DEVICE(0x0C02, info), /* SDV GT1 desktop */ \
+ INTEL_VGA_DEVICE(0x0C12, info), /* SDV GT2 desktop */ \
+ INTEL_VGA_DEVICE(0x0C22, info), /* SDV GT3 desktop */ \
+ INTEL_VGA_DEVICE(0x0C0A, info), /* SDV GT1 server */ \
+ INTEL_VGA_DEVICE(0x0C1A, info), /* SDV GT2 server */ \
+ INTEL_VGA_DEVICE(0x0C2A, info), /* SDV GT3 server */ \
+ INTEL_VGA_DEVICE(0x0C0B, info), /* SDV GT1 reserved */ \
+ INTEL_VGA_DEVICE(0x0C1B, info), /* SDV GT2 reserved */ \
+ INTEL_VGA_DEVICE(0x0C2B, info), /* SDV GT3 reserved */ \
+ INTEL_VGA_DEVICE(0x0C0E, info), /* SDV GT1 reserved */ \
+ INTEL_VGA_DEVICE(0x0C1E, info), /* SDV GT2 reserved */ \
+ INTEL_VGA_DEVICE(0x0C2E, info), /* SDV GT3 reserved */ \
+ INTEL_VGA_DEVICE(0x0A02, info), /* ULT GT1 desktop */ \
+ INTEL_VGA_DEVICE(0x0A12, info), /* ULT GT2 desktop */ \
+ INTEL_VGA_DEVICE(0x0A22, info), /* ULT GT3 desktop */ \
+ INTEL_VGA_DEVICE(0x0A0A, info), /* ULT GT1 server */ \
+ INTEL_VGA_DEVICE(0x0A1A, info), /* ULT GT2 server */ \
+ INTEL_VGA_DEVICE(0x0A2A, info), /* ULT GT3 server */ \
+ INTEL_VGA_DEVICE(0x0A0B, info), /* ULT GT1 reserved */ \
+ INTEL_VGA_DEVICE(0x0A1B, info), /* ULT GT2 reserved */ \
+ INTEL_VGA_DEVICE(0x0A2B, info), /* ULT GT3 reserved */ \
+ INTEL_VGA_DEVICE(0x0D02, info), /* CRW GT1 desktop */ \
+ INTEL_VGA_DEVICE(0x0D12, info), /* CRW GT2 desktop */ \
+ INTEL_VGA_DEVICE(0x0D22, info), /* CRW GT3 desktop */ \
+ INTEL_VGA_DEVICE(0x0D0A, info), /* CRW GT1 server */ \
+ INTEL_VGA_DEVICE(0x0D1A, info), /* CRW GT2 server */ \
+ INTEL_VGA_DEVICE(0x0D2A, info), /* CRW GT3 server */ \
+ INTEL_VGA_DEVICE(0x0D0B, info), /* CRW GT1 reserved */ \
+ INTEL_VGA_DEVICE(0x0D1B, info), /* CRW GT2 reserved */ \
+ INTEL_VGA_DEVICE(0x0D2B, info), /* CRW GT3 reserved */ \
+ INTEL_VGA_DEVICE(0x0D0E, info), /* CRW GT1 reserved */ \
+ INTEL_VGA_DEVICE(0x0D1E, info), /* CRW GT2 reserved */ \
+ INTEL_VGA_DEVICE(0x0D2E, info) /* CRW GT3 reserved */ \
+
+#define INTEL_HSW_M_IDS(info) \
+ INTEL_VGA_DEVICE(0x0406, info), /* GT1 mobile */ \
+ INTEL_VGA_DEVICE(0x0416, info), /* GT2 mobile */ \
+ INTEL_VGA_DEVICE(0x0426, info), /* GT2 mobile */ \
+ INTEL_VGA_DEVICE(0x0C06, info), /* SDV GT1 mobile */ \
+ INTEL_VGA_DEVICE(0x0C16, info), /* SDV GT2 mobile */ \
+ INTEL_VGA_DEVICE(0x0C26, info), /* SDV GT3 mobile */ \
+ INTEL_VGA_DEVICE(0x0A06, info), /* ULT GT1 mobile */ \
+ INTEL_VGA_DEVICE(0x0A16, info), /* ULT GT2 mobile */ \
+ INTEL_VGA_DEVICE(0x0A26, info), /* ULT GT3 mobile */ \
+ INTEL_VGA_DEVICE(0x0A0E, info), /* ULT GT1 reserved */ \
+ INTEL_VGA_DEVICE(0x0A1E, info), /* ULT GT2 reserved */ \
+ INTEL_VGA_DEVICE(0x0A2E, info), /* ULT GT3 reserved */ \
+ INTEL_VGA_DEVICE(0x0D06, info), /* CRW GT1 mobile */ \
+ INTEL_VGA_DEVICE(0x0D16, info), /* CRW GT2 mobile */ \
+ INTEL_VGA_DEVICE(0x0D26, info) /* CRW GT3 mobile */
+
+#define INTEL_VLV_M_IDS(info) \
+ INTEL_VGA_DEVICE(0x0f30, info), \
+ INTEL_VGA_DEVICE(0x0f31, info), \
+ INTEL_VGA_DEVICE(0x0f32, info), \
+ INTEL_VGA_DEVICE(0x0f33, info), \
+ INTEL_VGA_DEVICE(0x0157, info)
+
+#define INTEL_VLV_D_IDS(info) \
+ INTEL_VGA_DEVICE(0x0155, info)
+
+#endif /* _I915_PCIIDS_H */
--
1.7.9.5
Systems with Intel graphics controllers set aside memory exclusively for
gfx driver use. This memory is not marked in the E820 as reserved or as
RAM, and so is subject to overlap from E820 manipulation later in the
boot process. On some systems, MMIO space is allocated on top, despite
the efforts of the "RAM buffer" approach, which simply rounds memory
boundaries up to 64M to try to catch space that may decode as RAM and so
is not suitable for MMIO.
v2: use read_pci_config for 32 bit reads instead of adding a new one
(Chris)
add gen6 stolen size function (Chris)
use a function pointer (Chris)
drop gen2 bits (Daniel)
Signed-off-by: Jesse Barnes <[email protected]>
---
arch/x86/kernel/early-quirks.c | 158 +++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_reg.h | 15 ----
include/drm/i915_drm.h | 32 ++++++++
3 files changed, 190 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 94ab6b9..bff8a6f 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -12,6 +12,7 @@
#include <linux/pci.h>
#include <linux/acpi.h>
#include <linux/pci_ids.h>
+#include <drm/i915_drm.h>
#include <asm/pci-direct.h>
#include <asm/dma.h>
#include <asm/io_apic.h>
@@ -208,6 +209,161 @@ static void __init intel_remapping_check(int num, int slot, int func)
}
+/*
+ * Systems with Intel graphics controllers set aside memory exclusively
+ * for gfx driver use. This memory is not marked in the E820 as reserved
+ * or as RAM, and so is subject to overlap from E820 manipulation later
+ * in the boot process. On some systems, MMIO space is allocated on top,
+ * despite the efforts of the "RAM buffer" approach, which simply rounds
+ * memory boundaries up to 64M to try to catch space that may decode
+ * as RAM and so is not suitable for MMIO.
+ *
+ * And yes, so far on current devices the base addr is always under 4G.
+ */
+static u32 __init intel_stolen_base(int num, int slot, int func)
+{
+ u32 base;
+
+ /*
+ * Almost universally we can find the Graphics Base of Stolen Memory
+ * at offset 0x5c in the igfx configuration space. On a few (desktop)
+ * machines this is also mirrored in the bridge device at different
+ * locations, or in the MCHBAR.
+ */
+ base = read_pci_config(num, slot, func, 0x5c);
+ base &= ~((1<<20) - 1);
+
+ return base;
+}
+
+#define KB(x) ((x) * 1024)
+#define MB(x) (KB (KB (x)))
+#define GB(x) (MB (KB (x)))
+
+static size_t __init gen3_stolen_size(int num, int slot, int func)
+{
+ size_t stolen_size;
+ u16 gmch_ctrl;
+
+ gmch_ctrl = read_pci_config_16(0, 0, 0, I830_GMCH_CTRL);
+
+ switch (gmch_ctrl & I855_GMCH_GMS_MASK) {
+ case I855_GMCH_GMS_STOLEN_1M:
+ stolen_size = MB(1);
+ break;
+ case I855_GMCH_GMS_STOLEN_4M:
+ stolen_size = MB(4);
+ break;
+ case I855_GMCH_GMS_STOLEN_8M:
+ stolen_size = MB(8);
+ break;
+ case I855_GMCH_GMS_STOLEN_16M:
+ stolen_size = MB(16);
+ break;
+ case I855_GMCH_GMS_STOLEN_32M:
+ stolen_size = MB(32);
+ break;
+ case I915_GMCH_GMS_STOLEN_48M:
+ stolen_size = MB(48);
+ break;
+ case I915_GMCH_GMS_STOLEN_64M:
+ stolen_size = MB(64);
+ break;
+ case G33_GMCH_GMS_STOLEN_128M:
+ stolen_size = MB(128);
+ break;
+ case G33_GMCH_GMS_STOLEN_256M:
+ stolen_size = MB(256);
+ break;
+ case INTEL_GMCH_GMS_STOLEN_96M:
+ stolen_size = MB(96);
+ break;
+ case INTEL_GMCH_GMS_STOLEN_160M:
+ stolen_size = MB(160);
+ break;
+ case INTEL_GMCH_GMS_STOLEN_224M:
+ stolen_size = MB(224);
+ break;
+ case INTEL_GMCH_GMS_STOLEN_352M:
+ stolen_size = MB(352);
+ break;
+ default:
+ stolen_size = 0;
+ break;
+ }
+
+ return stolen_size;
+}
+
+static size_t __init gen6_stolen_size(int num, int slot, int func)
+{
+ u16 gmch_ctrl;
+
+ gmch_ctrl = read_pci_config_16(num, slot, func, SNB_GMCH_CTRL);
+ gmch_ctrl >>= SNB_GMCH_GMS_SHIFT;
+ gmch_ctrl &= SNB_GMCH_GMS_MASK;
+
+ return gmch_ctrl << 25; /* 32 MB units */
+}
+
+typedef size_t (*stolen_size_fn)(int num, int slot, int func);
+
+static struct pci_device_id intel_stolen_ids[] __initdata = {
+ INTEL_I915G_IDS(gen3_stolen_size),
+ INTEL_I915GM_IDS(gen3_stolen_size),
+ INTEL_I945G_IDS(gen3_stolen_size),
+ INTEL_I945GM_IDS(gen3_stolen_size),
+ INTEL_VLV_M_IDS(gen3_stolen_size),
+ INTEL_VLV_D_IDS(gen3_stolen_size),
+ INTEL_PINEVIEW_IDS(gen3_stolen_size),
+ INTEL_I965G_IDS(gen3_stolen_size),
+ INTEL_G33_IDS(gen3_stolen_size),
+ INTEL_I965GM_IDS(gen3_stolen_size),
+ INTEL_GM45_IDS(gen3_stolen_size),
+ INTEL_G45_IDS(gen3_stolen_size),
+ INTEL_IRONLAKE_D_IDS(gen3_stolen_size),
+ INTEL_IRONLAKE_M_IDS(gen3_stolen_size),
+ INTEL_SNB_D_IDS(gen6_stolen_size),
+ INTEL_SNB_M_IDS(gen6_stolen_size),
+ INTEL_IVB_M_IDS(gen6_stolen_size),
+ INTEL_IVB_D_IDS(gen6_stolen_size),
+ INTEL_HSW_D_IDS(gen6_stolen_size),
+ INTEL_HSW_M_IDS(gen6_stolen_size),
+};
+
+static void __init intel_graphics_stolen(int num, int slot, int func)
+{
+ size_t size;
+ int i;
+ u32 start;
+ u16 device, subvendor, subdevice;
+
+ device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
+ subvendor = read_pci_config_16(num, slot, func,
+ PCI_SUBSYSTEM_VENDOR_ID);
+ subdevice = read_pci_config_16(num, slot, func, PCI_SUBSYSTEM_ID);
+
+ for (i = 0; i < ARRAY_SIZE(intel_stolen_ids); i++) {
+ if (intel_stolen_ids[i].device == device) {
+ stolen_size_fn stolen_size =
+ (stolen_size_fn)intel_stolen_ids[i].driver_data;
+ size = stolen_size(num, slot, func);
+ start = intel_stolen_base(num, slot, func);
+ if (size && start)
+ goto found;
+ else
+ break;
+ }
+ }
+
+ /* No match or invalid data, don't bother reserving */
+ return;
+found:
+ /* Mark this space as reserved */
+ e820_add_region(start, size, E820_RESERVED);
+ return;
+}
+
#define QFLAG_APPLY_ONCE 0x1
#define QFLAG_APPLIED 0x2
#define QFLAG_DONE (QFLAG_APPLY_ONCE|QFLAG_APPLIED)
@@ -241,6 +397,8 @@ static struct chipset early_qrk[] __initdata = {
PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
{ PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
+ { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
+ QFLAG_APPLY_ONCE, intel_graphics_stolen },
{}
};
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5e58a44..d707ce6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -33,21 +33,6 @@
#define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
#define _MASKED_BIT_DISABLE(a) ((a) << 16)
-/*
- * The Bridge device's PCI config space has information about the
- * fb aperture size and the amount of pre-reserved memory.
- * This is all handled in the intel-gtt.ko module. i915.ko only
- * cares about the vga bit for the vga rbiter.
- */
-#define INTEL_GMCH_CTRL 0x52
-#define INTEL_GMCH_VGA_DISABLE (1 << 1)
-#define SNB_GMCH_CTRL 0x50
-#define SNB_GMCH_GGMS_SHIFT 8 /* GTT Graphics Memory Size */
-#define SNB_GMCH_GGMS_MASK 0x3
-#define SNB_GMCH_GMS_SHIFT 3 /* Graphics Mode Select */
-#define SNB_GMCH_GMS_MASK 0x1f
-
-
/* PCI config space */
#define HPLLCC 0xc0 /* 855 only */
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 7276a72..3abfa6e 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -36,4 +36,36 @@ extern bool i915_gpu_lower(void);
extern bool i915_gpu_busy(void);
extern bool i915_gpu_turbo_disable(void);
+/*
+ * The Bridge device's PCI config space has information about the
+ * fb aperture size and the amount of pre-reserved memory.
+ * This is all handled in the intel-gtt.ko module. i915.ko only
+ * cares about the vga bit for the vga rbiter.
+ */
+#define INTEL_GMCH_CTRL 0x52
+#define INTEL_GMCH_VGA_DISABLE (1 << 1)
+#define SNB_GMCH_CTRL 0x50
+#define SNB_GMCH_GGMS_SHIFT 8 /* GTT Graphics Memory Size */
+#define SNB_GMCH_GGMS_MASK 0x3
+#define SNB_GMCH_GMS_SHIFT 3 /* Graphics Mode Select */
+#define SNB_GMCH_GMS_MASK 0x1f
+
+#define I830_GMCH_CTRL 0x52
+
+#define I855_GMCH_GMS_MASK 0xF0
+#define I855_GMCH_GMS_STOLEN_0M 0x0
+#define I855_GMCH_GMS_STOLEN_1M (0x1 << 4)
+#define I855_GMCH_GMS_STOLEN_4M (0x2 << 4)
+#define I855_GMCH_GMS_STOLEN_8M (0x3 << 4)
+#define I855_GMCH_GMS_STOLEN_16M (0x4 << 4)
+#define I855_GMCH_GMS_STOLEN_32M (0x5 << 4)
+#define I915_GMCH_GMS_STOLEN_48M (0x6 << 4)
+#define I915_GMCH_GMS_STOLEN_64M (0x7 << 4)
+#define G33_GMCH_GMS_STOLEN_128M (0x8 << 4)
+#define G33_GMCH_GMS_STOLEN_256M (0x9 << 4)
+#define INTEL_GMCH_GMS_STOLEN_96M (0xa << 4)
+#define INTEL_GMCH_GMS_STOLEN_160M (0xb << 4)
+#define INTEL_GMCH_GMS_STOLEN_224M (0xc << 4)
+#define INTEL_GMCH_GMS_STOLEN_352M (0xd << 4)
+
#endif /* _I915_DRM_H_ */
--
1.7.9.5
* Jesse Barnes <[email protected]> wrote:
> Patch 2/2 has the description, but suffice it to say I'm
> not really pleased with this, though it does solve a
> problem we have. On some machines, we get MMIO space
> allocated on top of this hidden memory, which can cause
> problems. I'm not sure if there are similar problems for
> other hunks of the address space; if so it's possible
> this could be made more general (though the bits for
> looking up the address of this region are definitely
> Intel graphics specific).
It looks pretty hardware specific. Discovering it the hard
way and marking it e820 reserved in an early quirk is what
the firmware should have done to begin with - and I doubt
the kernel could do anything significantly cleaner.
How does Windows manage to not crash? By luckily never
allocating PCI resources on top of the RAM? Or does it have
a quirk?
> Chris has some patches on top to add a new E820 type so
> we can look up the region later, which removes some
> redundant code in the i915 driver at least.
>
> Any comments? I assume no one likes this, but maybe it's
> just another early quirk we'll have to live with...
No strong feelings against it - my only suggestion would be
to make this more visible - right now it's added as e820
reserved which hides amongst other areas already marked
reserved - would a low-key printk() of the range added make
it more apparent that a kernel quirk activated here?
Just so that people know that it came from the kernel, not
the firmware.
But in any case:
Acked-by: Ingo Molnar <[email protected]>
Thanks,
Ingo
On Thu, 25 Jul 2013 22:05:51 +0200
Ingo Molnar <[email protected]> wrote:
>
> * Jesse Barnes <[email protected]> wrote:
>
> > Patch 2/2 has the description, but suffice it to say I'm
> > not really pleased with this, though it does solve a
> > problem we have. On some machines, we get MMIO space
> > allocated on top of this hidden memory, which can cause
> > problems. I'm not sure if there are similar problems for
> > other hunks of the address space; if so it's possible
> > this could be made more general (though the bits for
> > looking up the address of this region are definitely
> > Intel graphics specific).
>
> It looks pretty hardware specific. Discovering it the hard
> way and marking it e820 reserved in an early quirk is what
> the firmware should have done to begin with - and I doubt
> the kernel could do anything significantly cleaner.
>
> How does Windows manage to not crash? By luckily never
> allocating PCI resources on top of the RAM? Or does it have
> a quirk?
Pretty sure Windows doesn't allocate MMIO space the same way we do, so
doesn't run into this on platforms where it's not E820_RESERVED. On
top of that, BIOS vendors probably just move things around until
Windows boots and the device manager doesn't have any dreaded "yellow
bang" icons that would prevent them from getting their "designed for
windows" sticker.
> > Chris has some patches on top to add a new E820 type so
> > we can look up the region later, which removes some
> > redundant code in the i915 driver at least.
> >
> > Any comments? I assume no one likes this, but maybe it's
> > just another early quirk we'll have to live with...
>
> No strong feelings against it - my only suggestion would be
> to make this more visible - right now it's added as e820
> reserved which hides amongst other areas already marked
> reserved - would a low-key printk() of the range added make
> it more apparent that a kernel quirk activated here?
Sounds good, I think Chris's patches should satisfy there. They make
it a new E820 type so it's clear in /proc/iomem too.
>
> Just so that people know that it came from the kernel, not
> the firmware.
>
> But in any case:
>
> Acked-by: Ingo Molnar <[email protected]>
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
On Thu, Jul 25, 2013 at 01:16:48PM -0700, Jesse Barnes wrote:
> On Thu, 25 Jul 2013 22:05:51 +0200
> Ingo Molnar <[email protected]> wrote:
> > > Chris has some patches on top to add a new E820 type so
> > > we can look up the region later, which removes some
> > > redundant code in the i915 driver at least.
> > >
> > > Any comments? I assume no one likes this, but maybe it's
> > > just another early quirk we'll have to live with...
> >
> > No strong feelings against it - my only suggestion would be
> > to make this more visible - right now it's added as e820
> > reserved which hides amongst other areas already marked
> > reserved - would a low-key printk() of the range added make
> > it more apparent that a kernel quirk activated here?
>
> Sounds good, I think Chris's patches should satisfy there. They make
> it a new E820 type so it's clear in /proc/iomem too.
I think it'd be good to get it all in in one go, since with Chris' patches
i915 will also use the detection logic from the quirk code, so more
testing for it. And imo the i915 cleanups shouldn't conflict really with
ongoing work in drm/i915, so could all go in through x86 trees.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
So the bootloader is just as likely to step on things... what happens when/if it does?
Ingo Molnar <[email protected]> wrote:
>
>* Jesse Barnes <[email protected]> wrote:
>
>> Patch 2/2 has the description, but suffice it to say I'm
>> not really pleased with this, though it does solve a
>> problem we have. On some machines, we get MMIO space
>> allocated on top of this hidden memory, which can cause
>> problems. I'm not sure if there are similar problems for
>> other hunks of the address space; if so it's possible
>> this could be made more general (though the bits for
>> looking up the address of this region are definitely
>> Intel graphics specific).
>
>It looks pretty hardware specific. Discovering it the hard
>way and marking it e820 reserved in an early quirk is what
>the firmware should have done to begin with - and I doubt
>the kernel could do anything significantly cleaner.
>
>How does Windows manage to not crash? By luckily never
>allocating PCI resources on top of the RAM? Or does it have
>a quirk?
>
>> Chris has some patches on top to add a new E820 type so
>> we can look up the region later, which removes some
>> redundant code in the i915 driver at least.
>>
>> Any comments? I assume no one likes this, but maybe it's
>> just another early quirk we'll have to live with...
>
>No strong feelings against it - my only suggestion would be
>to make this more visible - right now it's added as e820
>reserved which hides amongst other areas already marked
>reserved - would a low-key printk() of the range added make
>it more apparent that a kernel quirk activated here?
>
>Just so that people know that it came from the kernel, not
>the firmware.
>
>But in any case:
>
>Acked-by: Ingo Molnar <[email protected]>
>
>Thanks,
>
> Ingo
--
Sent from my mobile phone. Please excuse brevity and lack of formatting.
On Thu, Jul 25, 2013 at 09:37:49AM -0700, Jesse Barnes wrote:
> Systems with Intel graphics controllers set aside memory exclusively for
> gfx driver use. This memory is not marked in the E820 as reserved or as
> RAM, and so is subject to overlap from E820 manipulation later in the
> boot process. On some systems, MMIO space is allocated on top, despite
> the efforts of the "RAM buffer" approach, which simply rounds memory
> boundaries up to 64M to try to catch space that may decode as RAM and so
> is not suitable for MMIO.
>
> v2: use read_pci_config for 32 bit reads instead of adding a new one
> (Chris)
> add gen6 stolen size function (Chris)
> use a function pointer (Chris)
> drop gen2 bits (Daniel)
As a reminder, can you please reorder the entries in the macro to ease
compilation by userspace - c++ doesn't like having .class and so I have
to massage the file with
intel_pciids.h: i915_pciids.h
sed 's/_I915_PCIIDS/INTEL_PCIIDS/; s/\..*= //' < $^ > $@
to strip out the C99-style struct declarations (as my cpp skills were
obviously not strong enough) but then that requires the member ordering
is correct. Alternatively, if we discard the clean C99 code, both the
kernel struct pci_device_id and libpciaccess's struct pci_match_data are
the same - and I can just use i915_pciids.h directly. My perference is
to keep using C99 in the kernel as it makes the header much more
readable.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Hmm, interesting licence block in i915_pciids.h - our claim to
grant licence but TG disclaims liability.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
On Thu, Jul 25, 2013 at 09:37:48AM -0700, Jesse Barnes wrote:
> For use by userspace (at some point in the future) and other kernel code.
>
> v2: move PCI IDs to uabi (Chris)
> move PCI IDs to drm/ (Dave)
> v3: fixup Quanta detection - needs to come first (Daniel)
One last comment!
> +#define INTEL_VGA_DEVICE(id, info) { \
> + .class = PCI_BASE_CLASS_DISPLAY << 16, \
> + .class_mask = 0xff0000, \
> + .vendor = 0x8086, \
> + .device = id, \
> + .subvendor = PCI_ANY_ID, \
> + .subdevice = PCI_ANY_ID, \
> + .driver_data = (unsigned long) info }
libpciaccess doesn't define either PCI_BASE_CLASS_DISPLAY or PCI_ANY_ID.
Since we use the hex values for the rest of the elements, it would seem
to be tidier to also use 0x3 << 16 instead of PCI_BASE_CLASS_DISPLAY << 16.
I would leave PCI_ANY_ID as a symbolic value though.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Well, it's ok if the boot loader writes to this memory, the worst
that'll happen is you'll see garbage on the screen. If the boot loader
tries to do MMIO mapping on top it'll get into trouble... but why would
it do that?
Jesse
On Thu, 25 Jul 2013 15:42:25 -0700
"H. Peter Anvin" <[email protected]> wrote:
> So the bootloader is just as likely to step on things... what happens when/if it does?
>
> Ingo Molnar <[email protected]> wrote:
> >
> >* Jesse Barnes <[email protected]> wrote:
> >
> >> Patch 2/2 has the description, but suffice it to say I'm
> >> not really pleased with this, though it does solve a
> >> problem we have. On some machines, we get MMIO space
> >> allocated on top of this hidden memory, which can cause
> >> problems. I'm not sure if there are similar problems for
> >> other hunks of the address space; if so it's possible
> >> this could be made more general (though the bits for
> >> looking up the address of this region are definitely
> >> Intel graphics specific).
> >
> >It looks pretty hardware specific. Discovering it the hard
> >way and marking it e820 reserved in an early quirk is what
> >the firmware should have done to begin with - and I doubt
> >the kernel could do anything significantly cleaner.
> >
> >How does Windows manage to not crash? By luckily never
> >allocating PCI resources on top of the RAM? Or does it have
> >a quirk?
> >
> >> Chris has some patches on top to add a new E820 type so
> >> we can look up the region later, which removes some
> >> redundant code in the i915 driver at least.
> >>
> >> Any comments? I assume no one likes this, but maybe it's
> >> just another early quirk we'll have to live with...
> >
> >No strong feelings against it - my only suggestion would be
> >to make this more visible - right now it's added as e820
> >reserved which hides amongst other areas already marked
> >reserved - would a low-key printk() of the range added make
> >it more apparent that a kernel quirk activated here?
> >
> >Just so that people know that it came from the kernel, not
> >the firmware.
> >
> >But in any case:
> >
> >Acked-by: Ingo Molnar <[email protected]>
> >
> >Thanks,
> >
> > Ingo
>
--
Jesse Barnes, Intel Open Source Technology Center
On Thu, 25 Jul 2013 23:59:25 +0100
Chris Wilson <[email protected]> wrote:
> Hmm, interesting licence block in i915_pciids.h - our claim to
> grant licence but TG disclaims liability.
Oops my manual search & replace obviously failed. Will fix up.
--
Jesse Barnes, Intel Open Source Technology Center
On Thu, Jul 25, 2013 at 3:42 PM, H. Peter Anvin <[email protected]> wrote:
> So the bootloader is just as likely to step on things... what happens when/if it does?
This isn't a new problem. We've had this "firmware tables don't show
all devices" issue before.
The only odd thing about this one is how the quirk in question uses
"e820_add_region()" instead of just adding things to the MMIO list.
And I think that's actually likely a mistake.
So Jesse, why don't you do what the other quirks do, and claim an
actual MMIO resource? If you make it a real resource, you'll get to
use fancy things like REAL NAMES, and actually document it. With
human-readable strings.
See quirk_io_region() in drivers/pci/quirks.c for example. The same
code except for IORESOURCE_MEM should do a lovely job..
And even *if* it's already marked reserved in the e820 table, it just
looks nice in /proc/iomem.
Hmm?
Linus
On 07/25/2013 05:31 PM, Linus Torvalds wrote:
> On Thu, Jul 25, 2013 at 3:42 PM, H. Peter Anvin <[email protected]> wrote:
>> So the bootloader is just as likely to step on things... what happens when/if it does?
>
> This isn't a new problem. We've had this "firmware tables don't show
> all devices" issue before.
>
Yes, I just want to know what happens.
> The only odd thing about this one is how the quirk in question uses
> "e820_add_region()" instead of just adding things to the MMIO list.
> And I think that's actually likely a mistake.
>
> So Jesse, why don't you do what the other quirks do, and claim an
> actual MMIO resource? If you make it a real resource, you'll get to
> use fancy things like REAL NAMES, and actually document it. With
> human-readable strings.
>
> See quirk_io_region() in drivers/pci/quirks.c for example. The same
> code except for IORESOURCE_MEM should do a lovely job..
>
> And even *if* it's already marked reserved in the e820 table, it just
> looks nice in /proc/iomem.
We should do both -- mark it reserved in early boot, and add it as an
MMIO region later during boot.
The problem here, if I'm reading this right, is that this memory region
is marked as normal RAM in e820, which is much worse than just not
marking it as reserved; we need to intercept this memory before we
genuinely turn it into normal RAM.
At the same time, we have no protection against the bootloader using
this as memory or even placing the kernel there. The BIOS needs to be
fixed regardless of what workarounds we do in the kernel.
-hpa
On 07/25/2013 04:17 PM, Jesse Barnes wrote:
> Well, it's ok if the boot loader writes to this memory, the worst
> that'll happen is you'll see garbage on the screen. If the boot loader
> tries to do MMIO mapping on top it'll get into trouble... but why would
> it do that?
>
> Jesse
Much worse: it could be hunting for a place to put the kernel, and put
it there.
-hpa
On 07/25/2013 07:14 PM, Jesse Barnes wrote:
> To clarify: it'll either be marked reserved or not listed at all in e820, which is why I did this early, before any other e820 stuff like the "RAM buffer" are allocated, and before we could use the iomem resource (or maybe we could even early per Linus? I'll check).
>
> Jesse
If it is marked reserved or not listed at all it is much less of an
issue. Reserved is in fact the correct thing; not listed at all really
isn't very problematic in most cases.
-hpa
On Thu, Jul 25, 2013 at 09:37:49AM -0700, Jesse Barnes wrote:
> Systems with Intel graphics controllers set aside memory exclusively for
> gfx driver use. This memory is not marked in the E820 as reserved or as
> RAM, and so is subject to overlap from E820 manipulation later in the
> boot process. On some systems, MMIO space is allocated on top, despite
> the efforts of the "RAM buffer" approach, which simply rounds memory
> boundaries up to 64M to try to catch space that may decode as RAM and so
> is not suitable for MMIO.
>
> v2: use read_pci_config for 32 bit reads instead of adding a new one
> (Chris)
> add gen6 stolen size function (Chris)
> use a function pointer (Chris)
> drop gen2 bits (Daniel)
>
> Signed-off-by: Jesse Barnes <[email protected]>
Our QA seems to be happy with this.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=66844
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=66726
Tested-by: lu hua <[email protected]>
Cheers, Daniel
> ---
> arch/x86/kernel/early-quirks.c | 158 +++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_reg.h | 15 ----
> include/drm/i915_drm.h | 32 ++++++++
> 3 files changed, 190 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index 94ab6b9..bff8a6f 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -12,6 +12,7 @@
> #include <linux/pci.h>
> #include <linux/acpi.h>
> #include <linux/pci_ids.h>
> +#include <drm/i915_drm.h>
> #include <asm/pci-direct.h>
> #include <asm/dma.h>
> #include <asm/io_apic.h>
> @@ -208,6 +209,161 @@ static void __init intel_remapping_check(int num, int slot, int func)
>
> }
>
> +/*
> + * Systems with Intel graphics controllers set aside memory exclusively
> + * for gfx driver use. This memory is not marked in the E820 as reserved
> + * or as RAM, and so is subject to overlap from E820 manipulation later
> + * in the boot process. On some systems, MMIO space is allocated on top,
> + * despite the efforts of the "RAM buffer" approach, which simply rounds
> + * memory boundaries up to 64M to try to catch space that may decode
> + * as RAM and so is not suitable for MMIO.
> + *
> + * And yes, so far on current devices the base addr is always under 4G.
> + */
> +static u32 __init intel_stolen_base(int num, int slot, int func)
> +{
> + u32 base;
> +
> + /*
> + * Almost universally we can find the Graphics Base of Stolen Memory
> + * at offset 0x5c in the igfx configuration space. On a few (desktop)
> + * machines this is also mirrored in the bridge device at different
> + * locations, or in the MCHBAR.
> + */
> + base = read_pci_config(num, slot, func, 0x5c);
> + base &= ~((1<<20) - 1);
> +
> + return base;
> +}
> +
> +#define KB(x) ((x) * 1024)
> +#define MB(x) (KB (KB (x)))
> +#define GB(x) (MB (KB (x)))
> +
> +static size_t __init gen3_stolen_size(int num, int slot, int func)
> +{
> + size_t stolen_size;
> + u16 gmch_ctrl;
> +
> + gmch_ctrl = read_pci_config_16(0, 0, 0, I830_GMCH_CTRL);
> +
> + switch (gmch_ctrl & I855_GMCH_GMS_MASK) {
> + case I855_GMCH_GMS_STOLEN_1M:
> + stolen_size = MB(1);
> + break;
> + case I855_GMCH_GMS_STOLEN_4M:
> + stolen_size = MB(4);
> + break;
> + case I855_GMCH_GMS_STOLEN_8M:
> + stolen_size = MB(8);
> + break;
> + case I855_GMCH_GMS_STOLEN_16M:
> + stolen_size = MB(16);
> + break;
> + case I855_GMCH_GMS_STOLEN_32M:
> + stolen_size = MB(32);
> + break;
> + case I915_GMCH_GMS_STOLEN_48M:
> + stolen_size = MB(48);
> + break;
> + case I915_GMCH_GMS_STOLEN_64M:
> + stolen_size = MB(64);
> + break;
> + case G33_GMCH_GMS_STOLEN_128M:
> + stolen_size = MB(128);
> + break;
> + case G33_GMCH_GMS_STOLEN_256M:
> + stolen_size = MB(256);
> + break;
> + case INTEL_GMCH_GMS_STOLEN_96M:
> + stolen_size = MB(96);
> + break;
> + case INTEL_GMCH_GMS_STOLEN_160M:
> + stolen_size = MB(160);
> + break;
> + case INTEL_GMCH_GMS_STOLEN_224M:
> + stolen_size = MB(224);
> + break;
> + case INTEL_GMCH_GMS_STOLEN_352M:
> + stolen_size = MB(352);
> + break;
> + default:
> + stolen_size = 0;
> + break;
> + }
> +
> + return stolen_size;
> +}
> +
> +static size_t __init gen6_stolen_size(int num, int slot, int func)
> +{
> + u16 gmch_ctrl;
> +
> + gmch_ctrl = read_pci_config_16(num, slot, func, SNB_GMCH_CTRL);
> + gmch_ctrl >>= SNB_GMCH_GMS_SHIFT;
> + gmch_ctrl &= SNB_GMCH_GMS_MASK;
> +
> + return gmch_ctrl << 25; /* 32 MB units */
> +}
> +
> +typedef size_t (*stolen_size_fn)(int num, int slot, int func);
> +
> +static struct pci_device_id intel_stolen_ids[] __initdata = {
> + INTEL_I915G_IDS(gen3_stolen_size),
> + INTEL_I915GM_IDS(gen3_stolen_size),
> + INTEL_I945G_IDS(gen3_stolen_size),
> + INTEL_I945GM_IDS(gen3_stolen_size),
> + INTEL_VLV_M_IDS(gen3_stolen_size),
> + INTEL_VLV_D_IDS(gen3_stolen_size),
> + INTEL_PINEVIEW_IDS(gen3_stolen_size),
> + INTEL_I965G_IDS(gen3_stolen_size),
> + INTEL_G33_IDS(gen3_stolen_size),
> + INTEL_I965GM_IDS(gen3_stolen_size),
> + INTEL_GM45_IDS(gen3_stolen_size),
> + INTEL_G45_IDS(gen3_stolen_size),
> + INTEL_IRONLAKE_D_IDS(gen3_stolen_size),
> + INTEL_IRONLAKE_M_IDS(gen3_stolen_size),
> + INTEL_SNB_D_IDS(gen6_stolen_size),
> + INTEL_SNB_M_IDS(gen6_stolen_size),
> + INTEL_IVB_M_IDS(gen6_stolen_size),
> + INTEL_IVB_D_IDS(gen6_stolen_size),
> + INTEL_HSW_D_IDS(gen6_stolen_size),
> + INTEL_HSW_M_IDS(gen6_stolen_size),
> +};
> +
> +static void __init intel_graphics_stolen(int num, int slot, int func)
> +{
> + size_t size;
> + int i;
> + u32 start;
> + u16 device, subvendor, subdevice;
> +
> + device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
> + subvendor = read_pci_config_16(num, slot, func,
> + PCI_SUBSYSTEM_VENDOR_ID);
> + subdevice = read_pci_config_16(num, slot, func, PCI_SUBSYSTEM_ID);
> +
> + for (i = 0; i < ARRAY_SIZE(intel_stolen_ids); i++) {
> + if (intel_stolen_ids[i].device == device) {
> + stolen_size_fn stolen_size =
> + (stolen_size_fn)intel_stolen_ids[i].driver_data;
> + size = stolen_size(num, slot, func);
> + start = intel_stolen_base(num, slot, func);
> + if (size && start)
> + goto found;
> + else
> + break;
> + }
> + }
> +
> + /* No match or invalid data, don't bother reserving */
> + return;
> +found:
> + /* Mark this space as reserved */
> + e820_add_region(start, size, E820_RESERVED);
> + return;
> +}
> +
> #define QFLAG_APPLY_ONCE 0x1
> #define QFLAG_APPLIED 0x2
> #define QFLAG_DONE (QFLAG_APPLY_ONCE|QFLAG_APPLIED)
> @@ -241,6 +397,8 @@ static struct chipset early_qrk[] __initdata = {
> PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
> { PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
> PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
> + { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
> + QFLAG_APPLY_ONCE, intel_graphics_stolen },
> {}
> };
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5e58a44..d707ce6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -33,21 +33,6 @@
> #define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
> #define _MASKED_BIT_DISABLE(a) ((a) << 16)
>
> -/*
> - * The Bridge device's PCI config space has information about the
> - * fb aperture size and the amount of pre-reserved memory.
> - * This is all handled in the intel-gtt.ko module. i915.ko only
> - * cares about the vga bit for the vga rbiter.
> - */
> -#define INTEL_GMCH_CTRL 0x52
> -#define INTEL_GMCH_VGA_DISABLE (1 << 1)
> -#define SNB_GMCH_CTRL 0x50
> -#define SNB_GMCH_GGMS_SHIFT 8 /* GTT Graphics Memory Size */
> -#define SNB_GMCH_GGMS_MASK 0x3
> -#define SNB_GMCH_GMS_SHIFT 3 /* Graphics Mode Select */
> -#define SNB_GMCH_GMS_MASK 0x1f
> -
> -
> /* PCI config space */
>
> #define HPLLCC 0xc0 /* 855 only */
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index 7276a72..3abfa6e 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -36,4 +36,36 @@ extern bool i915_gpu_lower(void);
> extern bool i915_gpu_busy(void);
> extern bool i915_gpu_turbo_disable(void);
>
> +/*
> + * The Bridge device's PCI config space has information about the
> + * fb aperture size and the amount of pre-reserved memory.
> + * This is all handled in the intel-gtt.ko module. i915.ko only
> + * cares about the vga bit for the vga rbiter.
> + */
> +#define INTEL_GMCH_CTRL 0x52
> +#define INTEL_GMCH_VGA_DISABLE (1 << 1)
> +#define SNB_GMCH_CTRL 0x50
> +#define SNB_GMCH_GGMS_SHIFT 8 /* GTT Graphics Memory Size */
> +#define SNB_GMCH_GGMS_MASK 0x3
> +#define SNB_GMCH_GMS_SHIFT 3 /* Graphics Mode Select */
> +#define SNB_GMCH_GMS_MASK 0x1f
> +
> +#define I830_GMCH_CTRL 0x52
> +
> +#define I855_GMCH_GMS_MASK 0xF0
> +#define I855_GMCH_GMS_STOLEN_0M 0x0
> +#define I855_GMCH_GMS_STOLEN_1M (0x1 << 4)
> +#define I855_GMCH_GMS_STOLEN_4M (0x2 << 4)
> +#define I855_GMCH_GMS_STOLEN_8M (0x3 << 4)
> +#define I855_GMCH_GMS_STOLEN_16M (0x4 << 4)
> +#define I855_GMCH_GMS_STOLEN_32M (0x5 << 4)
> +#define I915_GMCH_GMS_STOLEN_48M (0x6 << 4)
> +#define I915_GMCH_GMS_STOLEN_64M (0x7 << 4)
> +#define G33_GMCH_GMS_STOLEN_128M (0x8 << 4)
> +#define G33_GMCH_GMS_STOLEN_256M (0x9 << 4)
> +#define INTEL_GMCH_GMS_STOLEN_96M (0xa << 4)
> +#define INTEL_GMCH_GMS_STOLEN_160M (0xb << 4)
> +#define INTEL_GMCH_GMS_STOLEN_224M (0xc << 4)
> +#define INTEL_GMCH_GMS_STOLEN_352M (0xd << 4)
> +
> #endif /* _I915_DRM_H_ */
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, 25 Jul 2013 20:31:27 -0700
"H. Peter Anvin" <[email protected]> wrote:
> On 07/25/2013 07:14 PM, Jesse Barnes wrote:
> > To clarify: it'll either be marked reserved or not listed at all in e820, which is why I did this early, before any other e820 stuff like the "RAM buffer" are allocated, and before we could use the iomem resource (or maybe we could even early per Linus? I'll check).
> >
> > Jesse
>
> If it is marked reserved or not listed at all it is much less of an
> issue. Reserved is in fact the correct thing; not listed at all really
> isn't very problematic in most cases.
Yeah the problems seem to fall into two categories:
1) mmio space is allocated in this range:
https://bugs.freedesktop.org/show_bug.cgi?id=66726
2) range gets partially allocated to the "RAM buffer"
https://bugs.freedesktop.org/show_bug.cgi?id=66844
Case (1) is the one that worries me. I'm guessing it'll mainly be a
problem on machines where MMIO space is limited or somehow structured
such that PCI resources end up there when we allocate them. Depending
on what gets put there and the decode priority, behavior may be poor.
Case (2) isn't harmful, but ends up causing our driver to skip stolen
memory initialization, because of the conflict.
Anyway I'll look at Linus's suggestion of reserving in the iomem
resource really early and roll in Chris's stuff if that doesn't work
out.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
On Thu, 25 Jul 2013 17:31:29 -0700
Linus Torvalds <[email protected]> wrote:
> On Thu, Jul 25, 2013 at 3:42 PM, H. Peter Anvin <[email protected]> wrote:
> > So the bootloader is just as likely to step on things... what happens when/if it does?
>
> This isn't a new problem. We've had this "firmware tables don't show
> all devices" issue before.
>
> The only odd thing about this one is how the quirk in question uses
> "e820_add_region()" instead of just adding things to the MMIO list.
> And I think that's actually likely a mistake.
>
> So Jesse, why don't you do what the other quirks do, and claim an
> actual MMIO resource? If you make it a real resource, you'll get to
> use fancy things like REAL NAMES, and actually document it. With
> human-readable strings.
>
> See quirk_io_region() in drivers/pci/quirks.c for example. The same
> code except for IORESOURCE_MEM should do a lovely job..
>
> And even *if* it's already marked reserved in the e820 table, it just
> looks nice in /proc/iomem.
>
> Hmm?
I should have mentioned yesterday that we *do* try to reserve the
resource in our driver init, with pretty name and everything.
The issue here is making sure we are actually *able* to reserve it
without conflicts at driver init time.
If the PCI layer has put something there (misc MMIO or the "RAM
buffer" intended to prevent stuff like this) because it's not listed in
the E820 map, we'll fail to get at this memory in our driver init.
Thus the early reservation in early-quirks, followed by a real
request_mem_region later on.
Doing the request_mem_region before the PCI layer gets its hands on it
isn't really possible, because __request_region depends on the memory
allocator being initialized. So rather than add a new hook elsewhere in
setup_arch or start_kernel I figured I'd use an early quirk.
Reasonable? Note iomem in both cases.
Before (RAM buffer prevents our allocation):
cafff000-caffffff : System RAM
cb000000-cbffffff : RAM buffer
cfa00000-feafffff : PCI Bus 0000:00
d0000000-dfffffff : 0000:00:02.0
After (yay):
cb000000-cb9fffff : RAM buffer
cba00000-cf9fffff : reserved
cba00000-cf9fffff : Graphics Stolen Memory
cfa00000-feafffff : PCI Bus 0000:00
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
On 07/25/2013 09:37 AM, Jesse Barnes wrote:
> Systems with Intel graphics controllers set aside memory exclusively for
> + /*
> + * Almost universally we can find the Graphics Base of Stolen Memory
> + * at offset 0x5c in the igfx configuration space. On a few (desktop)
> + * machines this is also mirrored in the bridge device at different
> + * locations, or in the MCHBAR.
> + */
This comment makes me nervous. It isn't clear to me if it is saying:
- All igfx devices has the graphics base at 0x5c, a few have it in
other places, too (which doesn't matter, we can use 0x5c anyway), or
- Most igfx devices have the graphics base at 0x5c, some don't, and we
hope and pray we're not on one of those systems because we're not
checking.
I assume it is the former, but it really needs to be phrased better.
-hpa
On Fri, 26 Jul 2013 09:58:45 -0700
"H. Peter Anvin" <[email protected]> wrote:
> On 07/25/2013 09:37 AM, Jesse Barnes wrote:
> > Systems with Intel graphics controllers set aside memory exclusively for
> > + /*
> > + * Almost universally we can find the Graphics Base of Stolen Memory
> > + * at offset 0x5c in the igfx configuration space. On a few (desktop)
> > + * machines this is also mirrored in the bridge device at different
> > + * locations, or in the MCHBAR.
> > + */
>
> This comment makes me nervous. It isn't clear to me if it is saying:
>
> - All igfx devices has the graphics base at 0x5c, a few have it in
> other places, too (which doesn't matter, we can use 0x5c anyway), or
> - Most igfx devices have the graphics base at 0x5c, some don't, and we
> hope and pray we're not on one of those systems because we're not
> checking.
>
> I assume it is the former, but it really needs to be phrased better.
I mostly copied it from the driver where we had some other ways of
finding it too. For the PCI IDs listed, we can always use 0x5c. I'll
fix the comment.
--
Jesse Barnes, Intel Open Source Technology Center
On Thu, Jul 25, 2013 at 05:48:42PM -0700, H. Peter Anvin wrote:
> On 07/25/2013 05:31 PM, Linus Torvalds wrote:
> > On Thu, Jul 25, 2013 at 3:42 PM, H. Peter Anvin <[email protected]> wrote:
> >> So the bootloader is just as likely to step on things... what happens when/if it does?
> >
> > This isn't a new problem. We've had this "firmware tables don't show
> > all devices" issue before.
> >
>
> Yes, I just want to know what happens.
>
> > The only odd thing about this one is how the quirk in question uses
> > "e820_add_region()" instead of just adding things to the MMIO list.
> > And I think that's actually likely a mistake.
> >
> > So Jesse, why don't you do what the other quirks do, and claim an
> > actual MMIO resource? If you make it a real resource, you'll get to
> > use fancy things like REAL NAMES, and actually document it. With
> > human-readable strings.
> >
> > See quirk_io_region() in drivers/pci/quirks.c for example. The same
> > code except for IORESOURCE_MEM should do a lovely job..
> >
> > And even *if* it's already marked reserved in the e820 table, it just
> > looks nice in /proc/iomem.
>
> We should do both -- mark it reserved in early boot, and add it as an
> MMIO region later during boot.
We started to reserve this range in drm/i915 (and still do), that's why
we've noticed that something is amiss.
>
> The problem here, if I'm reading this right, is that this memory region
> is marked as normal RAM in e820, which is much worse than just not
> marking it as reserved; we need to intercept this memory before we
> genuinely turn it into normal RAM.
I haven't seen a case where it's marked as ram, but many systems don't
mark it as reserved. Then the pci code can go around and put a bar into
the middle of that range. The other issue (benign but ugly) is that the
RAM buffer (used to protect stolen memory without knowing where exactly it
is because it's not properly reserved in the e820 map) sometimes ends up
in the graphics stolen range. With the new iomeme reserve code in drm/i915
we've stopped using stolen in such cases to avoid surprises, but that
kills a few neat features.
Thus far we haven't tracked down any bugs to such overlaps yet, but otoh
we don't use stolen all that much yet. But we have plans to put it to real
use (and use the full range) and I'd like to get the reservation conflicts
sorted out before we do so.
> At the same time, we have no protection against the bootloader using
> this as memory or even placing the kernel there. The BIOS needs to be
> fixed regardless of what workarounds we do in the kernel.
Like I've said we haven't seen it earmarked as ram anywhere yet, so I
don't think this is something we need to concern ousrselves with.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
* Jesse Barnes <[email protected]> wrote:
> On Thu, 25 Jul 2013 20:31:27 -0700
> "H. Peter Anvin" <[email protected]> wrote:
>
> > On 07/25/2013 07:14 PM, Jesse Barnes wrote:
> > > To clarify: it'll either be marked reserved or not listed at all in e820, which is why I did this early, before any other e820 stuff like the "RAM buffer" are allocated, and before we could use the iomem resource (or maybe we could even early per Linus? I'll check).
> > >
> > > Jesse
> >
> > If it is marked reserved or not listed at all it is much less of an
> > issue. Reserved is in fact the correct thing; not listed at all really
> > isn't very problematic in most cases.
>
> Yeah the problems seem to fall into two categories:
> 1) mmio space is allocated in this range:
> https://bugs.freedesktop.org/show_bug.cgi?id=66726
> 2) range gets partially allocated to the "RAM buffer"
> https://bugs.freedesktop.org/show_bug.cgi?id=66844
>
> Case (1) is the one that worries me. I'm guessing it'll mainly be a
> problem on machines where MMIO space is limited or somehow structured
> such that PCI resources end up there when we allocate them. Depending
> on what gets put there and the decode priority, behavior may be poor.
>
> Case (2) isn't harmful, but ends up causing our driver to skip stolen
> memory initialization, because of the conflict.
>
> Anyway I'll look at Linus's suggestion of reserving in the iomem
> resource really early and roll in Chris's stuff if that doesn't work
> out.
Am I being too pedantic in expecting that we still mark it
e820-reserved?
This area really isnt an ordinary PCI resource such as a
BAR or an MMIO region. It's truly system RAM (which cannot
be moved/reallocated), used by graphics hardware, and the
firmware should have marked it reserved.
(The end result should be the same in any case, so this is
a detail.)
Thanks,
Ingo
On 07/26/2013 01:24 PM, Ingo Molnar wrote:
>
> Am I being too pedantic in expecting that we still mark it
> e820-reserved?
>
> This area really isnt an ordinary PCI resource such as a
> BAR or an MMIO region. It's truly system RAM (which cannot
> be moved/reallocated), used by graphics hardware, and the
> firmware should have marked it reserved.
>
> (The end result should be the same in any case, so this is
> a detail.)
>
I, too, would prefer to see it marked as reserved.
-hpa
On Fri, Jul 26, 2013 at 10:28 PM, H. Peter Anvin <[email protected]> wrote:
> On 07/26/2013 01:24 PM, Ingo Molnar wrote:
>>
>> Am I being too pedantic in expecting that we still mark it
>> e820-reserved?
>>
>> This area really isnt an ordinary PCI resource such as a
>> BAR or an MMIO region. It's truly system RAM (which cannot
>> be moved/reallocated), used by graphics hardware, and the
>> firmware should have marked it reserved.
>>
>> (The end result should be the same in any case, so this is
>> a detail.)
>>
>
> I, too, would prefer to see it marked as reserved.
Yeah, current patches mark it as reserved. But since we don't want to
duplicate the detection code we want to switch to a more unique name
when mapping the e820 region into a iomem reservation so that when
i915.ko loads we can dig it out again.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch