[PATCH 0/1] drm/i915: Another version of the vga stop_machine monstrosity

[PATCH 0/1] drm/i915: Another version of the vga stop_machine monstrosity

[ville.syrjala]

Here's a slightly improved version of the VGA stop_machine() patch.

The main new thing is switching ctg/elk over to port IO. That appears to have
cured the hangs from Chris's lid_notify riddled ctg machine.

What I'd really like is some kind of tested-by note from Alex Williamson to
make sure I didn't accidentally break something important. Well, assuming people
are willing to overlook the uglyness to slip this in at all.

[PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We have several problems with out VGA handling:
- We try to use the GMCH control VGA disable bit even though it may
  be locked
- If we manage to disable VGA throuh GMCH control, we're no longer
  able to correctly disable the VGA plane
- Taking part in the VGA arbitration is too expensive for X [1]

So let's treat the GMCH control VGA disable bit as read-only and leave
it for the BIOS to set, as it was intended. To disable VGA we will use
the VGA misc register, and to disable VGA IO we will disable IO space
completely via the PCI command register.

But we still need VGA register access during resume (and possibly during
lid event on insane BIOSen) to disable the VGA plane. Also we need to
re-disable VGA memory decode via the VGA misc register on resume.

Luckily up to gen4, VGA registers can be accessed through MMIO.
Unfortunately from gen5 onwards only the legacy VGA IO port range
works. So on gen5+ we still need IO space to be enabled during those
few special moments when we need to access VGA registers.

We still want to opt out of VGA arbitration on gen5+, so we have keep
IO space disabled most of the time. And when we do need to poke at VGA
registers, we enable IO space briefly while no one is looking. To
guarantee that no one is looking we will use stop_machine().

[1] http://lists.x.org/archives/xorg-devel/2013-September/037763.html

v2: Use SNB_GMCH_TRL on SNB+
    Use port IO instead of MMIO on CTG/ELK
    Add WaEnableVGAAccessThroughIOPort comment
    Fix the max number of devices on the bus limit

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  39 +-----
 drivers/gpu/drm/i915/i915_suspend.c  |   4 +-
 drivers/gpu/drm/i915/intel_display.c | 248 +++++++++++++++++++++++++++++------
 3 files changed, 217 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index d35de1b..59a2048 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1225,19 +1225,6 @@ intel_teardown_mchbar(struct drm_device *dev)
 		release_resource(&dev_priv->mch_res);
 }
 
-/* true = enable decode, false = disable decoder */
-static unsigned int i915_vga_set_decode(void *cookie, bool state)
-{
-	struct drm_device *dev = cookie;
-
-	intel_modeset_vga_set_state(dev, state);
-	if (state)
-		return VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
-		       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
-	else
-		return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
-}
-
 static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_state state)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
@@ -1283,25 +1270,11 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		DRM_INFO("failed to find VBIOS tables\n");
 
-	/* If we have > 1 VGA cards, then we need to arbitrate access
-	 * to the common VGA resources.
-	 *
-	 * If we are a secondary display controller (!PCI_DISPLAY_CLASS_VGA),
-	 * then we do not take part in VGA arbitration and the
-	 * vga_client_register() fails with -ENODEV.
-	 */
-	if (!HAS_PCH_SPLIT(dev)) {
-		ret = vga_client_register(dev->pdev, dev, NULL,
-					  i915_vga_set_decode);
-		if (ret && ret != -ENODEV)
-			goto out;
-	}
-
 	intel_register_dsm_handler();
 
 	ret = vga_switcheroo_register_client(dev->pdev, &i915_switcheroo_ops, false);
 	if (ret)
-		goto cleanup_vga_client;
+		goto out;
 
 	/* Initialise stolen first so that we may reserve preallocated
 	 * objects for the BIOS to KMS transition.
@@ -1316,7 +1289,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_init_power_well(dev);
 
-	/* Keep VGA alive until i915_disable_vga_mem() */
+	/* Keep VGA alive until intel_modeset_vga_set_state() */
 	intel_display_power_get(dev, POWER_DOMAIN_VGA);
 
 	/* Important: The output setup functions called by modeset_init need
@@ -1359,10 +1332,13 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	intel_fbdev_initial_config(dev);
 
 	/*
+	 * Disable VGA IO and memory, and
+	 * tell the arbiter to ignore us.
+	 *
 	 * Must do this after fbcon init so that
 	 * vgacon_save_screen() works during the handover.
 	 */
-	i915_disable_vga_mem(dev);
+	intel_modeset_vga_set_state(dev, false);
 	intel_display_power_put(dev, POWER_DOMAIN_VGA);
 
 	/* Only enable hotplug handling once the fbdev is fully set up. */
@@ -1386,8 +1362,6 @@ cleanup_gem_stolen:
 	i915_gem_cleanup_stolen(dev);
 cleanup_vga_switcheroo:
 	vga_switcheroo_unregister_client(dev->pdev);
-cleanup_vga_client:
-	vga_client_register(dev->pdev, NULL, NULL, NULL);
 out:
 	return ret;
 }
@@ -1758,7 +1732,6 @@ int i915_driver_unload(struct drm_device *dev)
 		}
 
 		vga_switcheroo_unregister_client(dev->pdev);
-		vga_client_register(dev->pdev, NULL, NULL, NULL);
 	}
 
 	/* Free error state after interrupts are fully disabled. */
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 3538370..cc0d459 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -331,8 +331,10 @@ static void i915_restore_display(struct drm_device *dev)
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		i915_restore_vga(dev);
-	else
+	else {
 		i915_redisable_vga(dev);
+		intel_modeset_vga_set_state(dev, false);
+	}
 }
 
 int i915_save_state(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 76870f0..25955be 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -40,6 +40,7 @@
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <linux/dma_remapping.h>
+#include <linux/stop_machine.h>
 
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
@@ -10232,51 +10233,195 @@ static void intel_init_quirks(struct drm_device *dev)
 	}
 }
 
-/* Disable the VGA plane that we never use */
-static void i915_disable_vga(struct drm_device *dev)
+enum vga_op {
+	VGA_OP_MEM_DISABLE,
+	VGA_OP_MEM_ENABLE,
+	VGA_OP_SCREEN_OFF,
+};
+
+struct i915_vga_op {
+	enum vga_op op;
+	struct drm_device *dev;
+};
+
+/*
+ * 21 devices with 8 functions per device max on the same bus.
+ * We don't need locking for these due to stop_machine().
+ */
+static u16 vga_cmd[21*8];
+static u16 vga_ctl[21*8];
+
+/*
+ * Disable IO decode and VGA bridge forwarding
+ * for everyone else on the same bus.
+ */
+static void i915_vga_bus_disable(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u8 sr1;
-	u32 vga_reg = i915_vgacntrl_reg(dev);
+	struct pci_dev *pdev, *self = dev->pdev;
+	struct pci_bus *bus;
+	int i;
 
-	vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
-	outb(SR01, VGA_SR_INDEX);
-	sr1 = inb(VGA_SR_DATA);
-	outb(sr1 | 1<<5, VGA_SR_DATA);
-	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
-	udelay(300);
+	i = 0;
+	list_for_each_entry(bus, &self->bus->children, node) {
+		pci_read_config_word(bus->self, PCI_BRIDGE_CONTROL, &vga_ctl[i]);
+		pci_write_config_word(bus->self, PCI_BRIDGE_CONTROL, vga_ctl[i] & ~PCI_BRIDGE_CTL_VGA);
 
-	I915_WRITE(vga_reg, VGA_DISP_DISABLE);
-	POSTING_READ(vga_reg);
+		if (WARN_ON(++i >= ARRAY_SIZE(vga_ctl)))
+			break;
+	}
+
+	i = 0;
+	list_for_each_entry(pdev, &self->bus->devices, bus_list) {
+		if (pdev == self)
+			continue;
+
+		pci_read_config_word(pdev, PCI_COMMAND, &vga_cmd[i]);
+		pci_write_config_word(pdev, PCI_COMMAND, vga_cmd[i] & ~PCI_COMMAND_IO);
+
+		if (WARN_ON(++i >= ARRAY_SIZE(vga_cmd)))
+			break;
+	}
 }
 
-static void i915_enable_vga_mem(struct drm_device *dev)
+/*
+ * Restore IO decode and VGA bridge forwarding
+ * for everyone else on the same bus.
+ */
+static void i915_vga_bus_restore(struct drm_device *dev)
 {
-	/* Enable VGA memory on Intel HD */
-	if (HAS_PCH_SPLIT(dev)) {
-		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
-		outb(inb(VGA_MSR_READ) | VGA_MSR_MEM_EN, VGA_MSR_WRITE);
-		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
-						   VGA_RSRC_LEGACY_MEM |
-						   VGA_RSRC_NORMAL_IO |
-						   VGA_RSRC_NORMAL_MEM);
-		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
+	struct pci_dev *pdev, *self = dev->pdev;
+	struct pci_bus *bus;
+	int i;
+
+	i = 0;
+	list_for_each_entry(pdev, &self->bus->devices, bus_list) {
+		if (pdev == self)
+			continue;
+
+		pci_write_config_word(pdev, PCI_COMMAND, vga_cmd[i]);
+
+		if (WARN_ON(++i >= ARRAY_SIZE(vga_cmd)))
+			break;
+	}
+
+	i = 0;
+	list_for_each_entry(bus, &self->bus->children, node) {
+		pci_write_config_word(bus->self, PCI_BRIDGE_CONTROL, vga_ctl[i]);
+
+		if (WARN_ON(++i >= ARRAY_SIZE(vga_ctl)))
+			break;
 	}
 }
 
-void i915_disable_vga_mem(struct drm_device *dev)
+/*
+ * Hide our VGA IO access from the rest of the system
+ * using stop_machine().
+ *
+ * Note that we assume that the IGD is on the root bus.
+ */
+static int i915_vga_stop_machine_cb(void *data)
 {
-	/* Disable VGA memory on Intel HD */
-	if (HAS_PCH_SPLIT(dev)) {
-		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
-		outb(inb(VGA_MSR_READ) & ~VGA_MSR_MEM_EN, VGA_MSR_WRITE);
-		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
-						   VGA_RSRC_NORMAL_IO |
-						   VGA_RSRC_NORMAL_MEM);
-		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
+	struct i915_vga_op *op = data;
+	struct drm_device *dev = op->dev;
+	u16 cmd;
+	u8 tmp;
+
+	i915_vga_bus_disable(dev);
+
+	pci_read_config_word(dev->pdev, PCI_COMMAND, &cmd);
+	pci_write_config_word(dev->pdev, PCI_COMMAND, cmd | PCI_COMMAND_IO);
+
+	switch (op->op) {
+	case VGA_OP_MEM_DISABLE:
+		tmp = inb(VGA_MSR_READ);
+		tmp &= ~VGA_MSR_MEM_EN;
+		outb(tmp, VGA_MSR_WRITE);
+		break;
+	case VGA_OP_MEM_ENABLE:
+		tmp = inb(VGA_MSR_READ);
+		tmp |= VGA_MSR_MEM_EN;
+		outb(tmp, VGA_MSR_WRITE);
+		break;
+	case VGA_OP_SCREEN_OFF:
+		outb(SR01, VGA_SR_INDEX);
+		tmp = inb(VGA_SR_DATA);
+		tmp |= 1 << 5;
+		outb(tmp, VGA_SR_DATA);
+		break;
+	}
+
+	pci_write_config_word(dev->pdev, PCI_COMMAND, cmd);
+
+	i915_vga_bus_restore(dev);
+
+	return 0;
+}
+
+/* MMIO based VGA register access, pre-Gen5 only */
+static void i915_vga_execute_mmio(struct drm_device *dev, enum vga_op op)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u8 tmp;
+
+	switch (op) {
+	case VGA_OP_MEM_DISABLE:
+		tmp = I915_READ8(VGA_MSR_READ);
+		tmp &= ~VGA_MSR_MEM_EN;
+		I915_WRITE8(VGA_MSR_WRITE, tmp);
+		break;
+	case VGA_OP_MEM_ENABLE:
+		tmp = I915_READ8(VGA_MSR_READ);
+		tmp |= VGA_MSR_MEM_EN;
+		I915_WRITE8(VGA_MSR_WRITE, tmp);
+		break;
+	case VGA_OP_SCREEN_OFF:
+		I915_WRITE8(VGA_SR_INDEX, SR01);
+		tmp = I915_READ8(VGA_SR_DATA);
+		tmp |= 1 << 5;
+		I915_WRITE8(VGA_SR_DATA, tmp);
+		break;
 	}
 }
 
+static void i915_vga_execute(struct drm_device *dev, enum vga_op op)
+{
+	/* WaEnableVGAAccessThroughIOPort:ctg,elg,ilk,snb,ivb,vlv,hsw */
+	if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
+		struct i915_vga_op vga_op = {
+			.op = op,
+			.dev = dev,
+		};
+
+		stop_machine(i915_vga_stop_machine_cb, &vga_op, NULL);
+	} else {
+		i915_vga_execute_mmio(dev, op);
+	}
+}
+
+/* Disable the VGA plane that we never use */
+static void i915_disable_vga(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 vga_reg = i915_vgacntrl_reg(dev);
+	u16 gmch_ctrl;
+
+	/* already disabled? */
+	if (I915_READ(vga_reg) & VGA_DISP_DISABLE)
+		return;
+
+	pci_read_config_word(dev_priv->bridge_dev, INTEL_INFO(dev)->gen >= 6 ?
+			     SNB_GMCH_CTRL : INTEL_GMCH_CTRL, &gmch_ctrl);
+	if (gmch_ctrl & INTEL_GMCH_VGA_DISABLE)
+		DRM_ERROR("VGA plane enabled while VGA disabled via GMCH control\n");
+
+	i915_vga_execute(dev, VGA_OP_SCREEN_OFF);
+
+	udelay(300);
+
+	I915_WRITE(vga_reg, VGA_DISP_DISABLE);
+	POSTING_READ(vga_reg);
+}
+
 void intel_modeset_init_hw(struct drm_device *dev)
 {
 	intel_prepare_ddi(dev);
@@ -10553,7 +10698,6 @@ void i915_redisable_vga(struct drm_device *dev)
 	if (I915_READ(vga_reg) != VGA_DISP_DISABLE) {
 		DRM_DEBUG_KMS("Something enabled VGA plane, disabling it\n");
 		i915_disable_vga(dev);
-		i915_disable_vga_mem(dev);
 	}
 }
 
@@ -10755,7 +10899,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_disable_fbc(dev);
 
-	i915_enable_vga_mem(dev);
+	intel_modeset_vga_set_state(dev, true);
 
 	intel_disable_gt_powersave(dev);
 
@@ -10798,12 +10942,36 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u16 gmch_ctrl;
 
-	pci_read_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, &gmch_ctrl);
-	if (state)
-		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
-	else
-		gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
-	pci_write_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, gmch_ctrl);
+	/* Is VGA totally disabled for the IGD? */
+	pci_read_config_word(dev_priv->bridge_dev, INTEL_INFO(dev)->gen >= 6 ?
+			     SNB_GMCH_CTRL : INTEL_GMCH_CTRL, &gmch_ctrl);
+	if (gmch_ctrl & INTEL_GMCH_VGA_DISABLE)
+		return 0;
+
+	if (state) {
+		i915_vga_execute(dev, VGA_OP_MEM_ENABLE);
+
+		/*
+		 * Leave PCI_COMMAND_IO alone for now. vgaarb
+		 * should re-enable it if and when needed.
+		 */
+
+		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
+						   VGA_RSRC_LEGACY_MEM |
+						   VGA_RSRC_NORMAL_IO |
+						   VGA_RSRC_NORMAL_MEM);
+	} else {
+		u16 cmd;
+
+		i915_vga_execute(dev, VGA_OP_MEM_DISABLE);
+
+		pci_read_config_word(dev->pdev, PCI_COMMAND, &cmd);
+		cmd &= ~PCI_COMMAND_IO;
+		pci_write_config_word(dev->pdev, PCI_COMMAND, cmd);
+
+		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_NORMAL_MEM);
+	}
+
 	return 0;
 }
 
-- 
1.8.1.5

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

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Chris Wilson]

On Mon, Sep 30, 2013 at 05:08:31PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We have several problems with out VGA handling:
> - We try to use the GMCH control VGA disable bit even though it may
>   be locked
> - If we manage to disable VGA throuh GMCH control, we're no longer
>   able to correctly disable the VGA plane
> - Taking part in the VGA arbitration is too expensive for X [1]

I'd like to emphasize that X disables DRI if it detects 2 vga cards,
effectively breaking all machines with a discrete GPU. Even if one of
those is not being used.
 
> +/*
> + * 21 devices with 8 functions per device max on the same bus.
> + * We don't need locking for these due to stop_machine().
> + */
> +static u16 vga_cmd[21*8];
> +static u16 vga_ctl[21*8];

Should we just allocate storage for when we need it? We are now adding
several hundred bytes to our module, which is bound to cause us to use
an extra page, and they can be passed around through the stop_machine
closure rather than static.

But anyway, it does what is says on the tin and makes my dGPU testing
box usable again, without breaking any other machine that I've tested on
so far,
Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Ville Syrjälä]

On Mon, Sep 30, 2013 at 03:24:37PM +0100, Chris Wilson wrote:
> On Mon, Sep 30, 2013 at 05:08:31PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We have several problems with out VGA handling:
> > - We try to use the GMCH control VGA disable bit even though it may
> >   be locked
> > - If we manage to disable VGA throuh GMCH control, we're no longer
> >   able to correctly disable the VGA plane
> > - Taking part in the VGA arbitration is too expensive for X [1]
> 
> I'd like to emphasize that X disables DRI if it detects 2 vga cards,
> effectively breaking all machines with a discrete GPU. Even if one of
> those is not being used.
>  
> > +/*
> > + * 21 devices with 8 functions per device max on the same bus.
> > + * We don't need locking for these due to stop_machine().
> > + */
> > +static u16 vga_cmd[21*8];
> > +static u16 vga_ctl[21*8];
> 
> Should we just allocate storage for when we need it? We are now adding
> several hundred bytes to our module, which is bound to cause us to use
> an extra page, and they can be passed around through the stop_machine
> closure rather than static.

I guess we could do that. Although I do wonder a bit if we'd race with
hotplug. Not sure there's a way to hotplug stuff onto the root bus...

> 
> But anyway, it does what is says on the tin and makes my dGPU testing
> box usable again, without breaking any other machine that I've tested on
> so far,
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Alex Williamson]

On Mon, 2013-09-30 at 17:08 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We have several problems with out VGA handling:
> - We try to use the GMCH control VGA disable bit even though it may
>   be locked
> - If we manage to disable VGA throuh GMCH control, we're no longer
>   able to correctly disable the VGA plane
> - Taking part in the VGA arbitration is too expensive for X [1]
> 
> So let's treat the GMCH control VGA disable bit as read-only and leave
> it for the BIOS to set, as it was intended. To disable VGA we will use
> the VGA misc register, and to disable VGA IO we will disable IO space
> completely via the PCI command register.
> 
> But we still need VGA register access during resume (and possibly during
> lid event on insane BIOSen) to disable the VGA plane. Also we need to
> re-disable VGA memory decode via the VGA misc register on resume.
> 
> Luckily up to gen4, VGA registers can be accessed through MMIO.
> Unfortunately from gen5 onwards only the legacy VGA IO port range
> works. So on gen5+ we still need IO space to be enabled during those
> few special moments when we need to access VGA registers.
> 
> We still want to opt out of VGA arbitration on gen5+, so we have keep
> IO space disabled most of the time. And when we do need to poke at VGA
> registers, we enable IO space briefly while no one is looking. To
> guarantee that no one is looking we will use stop_machine().

What?!  Why would we not simply wait for the arbiter lock?

> [1] http://lists.x.org/archives/xorg-devel/2013-September/037763.html

Eek, does this X patch effectively kill VGA Arbiter interaction unless
an instance of X has multiple adapters needing VGA?
        
        "Secondly, we then only wrap the operations with vgaarb get/put
        for the drivers that require VGA access. If we only have a
        single driver requring VGA access, we just wrap Enter/LeaveVT
        and lock the VGA arbiter for the entire duration that the
        Xserver is active."

Maybe I'm reading the code wrong, but it looks like if
xf86VGAarbiterInit() is not called multiple times then X just locks VGA
and never unlocks it.  If so, this is broken!  How is this supposed to
work with multiple X sessions?  How can it ever work if we want to
assign a VGA device to a virtual machine?  Chris, please tell me I'm
misreading the intent of this patch.  Thanks,

Alex

> 
> v2: Use SNB_GMCH_TRL on SNB+
>     Use port IO instead of MMIO on CTG/ELK
>     Add WaEnableVGAAccessThroughIOPort comment
>     Fix the max number of devices on the bus limit
> 
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  39 +-----
>  drivers/gpu/drm/i915/i915_suspend.c  |   4 +-
>  drivers/gpu/drm/i915/intel_display.c | 248 +++++++++++++++++++++++++++++------
>  3 files changed, 217 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index d35de1b..59a2048 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1225,19 +1225,6 @@ intel_teardown_mchbar(struct drm_device *dev)
>  		release_resource(&dev_priv->mch_res);
>  }
>  
> -/* true = enable decode, false = disable decoder */
> -static unsigned int i915_vga_set_decode(void *cookie, bool state)
> -{
> -	struct drm_device *dev = cookie;
> -
> -	intel_modeset_vga_set_state(dev, state);
> -	if (state)
> -		return VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
> -		       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
> -	else
> -		return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
> -}
> -
>  static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_state state)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
> @@ -1283,25 +1270,11 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	if (ret)
>  		DRM_INFO("failed to find VBIOS tables\n");
>  
> -	/* If we have > 1 VGA cards, then we need to arbitrate access
> -	 * to the common VGA resources.
> -	 *
> -	 * If we are a secondary display controller (!PCI_DISPLAY_CLASS_VGA),
> -	 * then we do not take part in VGA arbitration and the
> -	 * vga_client_register() fails with -ENODEV.
> -	 */
> -	if (!HAS_PCH_SPLIT(dev)) {
> -		ret = vga_client_register(dev->pdev, dev, NULL,
> -					  i915_vga_set_decode);
> -		if (ret && ret != -ENODEV)
> -			goto out;
> -	}
> -
>  	intel_register_dsm_handler();
>  
>  	ret = vga_switcheroo_register_client(dev->pdev, &i915_switcheroo_ops, false);
>  	if (ret)
> -		goto cleanup_vga_client;
> +		goto out;
>  
>  	/* Initialise stolen first so that we may reserve preallocated
>  	 * objects for the BIOS to KMS transition.
> @@ -1316,7 +1289,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  	intel_init_power_well(dev);
>  
> -	/* Keep VGA alive until i915_disable_vga_mem() */
> +	/* Keep VGA alive until intel_modeset_vga_set_state() */
>  	intel_display_power_get(dev, POWER_DOMAIN_VGA);
>  
>  	/* Important: The output setup functions called by modeset_init need
> @@ -1359,10 +1332,13 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	intel_fbdev_initial_config(dev);
>  
>  	/*
> +	 * Disable VGA IO and memory, and
> +	 * tell the arbiter to ignore us.
> +	 *
>  	 * Must do this after fbcon init so that
>  	 * vgacon_save_screen() works during the handover.
>  	 */
> -	i915_disable_vga_mem(dev);
> +	intel_modeset_vga_set_state(dev, false);
>  	intel_display_power_put(dev, POWER_DOMAIN_VGA);
>  
>  	/* Only enable hotplug handling once the fbdev is fully set up. */
> @@ -1386,8 +1362,6 @@ cleanup_gem_stolen:
>  	i915_gem_cleanup_stolen(dev);
>  cleanup_vga_switcheroo:
>  	vga_switcheroo_unregister_client(dev->pdev);
> -cleanup_vga_client:
> -	vga_client_register(dev->pdev, NULL, NULL, NULL);
>  out:
>  	return ret;
>  }
> @@ -1758,7 +1732,6 @@ int i915_driver_unload(struct drm_device *dev)
>  		}
>  
>  		vga_switcheroo_unregister_client(dev->pdev);
> -		vga_client_register(dev->pdev, NULL, NULL, NULL);
>  	}
>  
>  	/* Free error state after interrupts are fully disabled. */
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 3538370..cc0d459 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -331,8 +331,10 @@ static void i915_restore_display(struct drm_device *dev)
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		i915_restore_vga(dev);
> -	else
> +	else {
>  		i915_redisable_vga(dev);
> +		intel_modeset_vga_set_state(dev, false);
> +	}
>  }
>  
>  int i915_save_state(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 76870f0..25955be 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -40,6 +40,7 @@
>  #include <drm/drm_dp_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <linux/dma_remapping.h>
> +#include <linux/stop_machine.h>
>  
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
> @@ -10232,51 +10233,195 @@ static void intel_init_quirks(struct drm_device *dev)
>  	}
>  }
>  
> -/* Disable the VGA plane that we never use */
> -static void i915_disable_vga(struct drm_device *dev)
> +enum vga_op {
> +	VGA_OP_MEM_DISABLE,
> +	VGA_OP_MEM_ENABLE,
> +	VGA_OP_SCREEN_OFF,
> +};
> +
> +struct i915_vga_op {
> +	enum vga_op op;
> +	struct drm_device *dev;
> +};
> +
> +/*
> + * 21 devices with 8 functions per device max on the same bus.
> + * We don't need locking for these due to stop_machine().
> + */
> +static u16 vga_cmd[21*8];
> +static u16 vga_ctl[21*8];
> +
> +/*
> + * Disable IO decode and VGA bridge forwarding
> + * for everyone else on the same bus.
> + */
> +static void i915_vga_bus_disable(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u8 sr1;
> -	u32 vga_reg = i915_vgacntrl_reg(dev);
> +	struct pci_dev *pdev, *self = dev->pdev;
> +	struct pci_bus *bus;
> +	int i;
>  
> -	vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
> -	outb(SR01, VGA_SR_INDEX);
> -	sr1 = inb(VGA_SR_DATA);
> -	outb(sr1 | 1<<5, VGA_SR_DATA);
> -	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
> -	udelay(300);
> +	i = 0;
> +	list_for_each_entry(bus, &self->bus->children, node) {
> +		pci_read_config_word(bus->self, PCI_BRIDGE_CONTROL, &vga_ctl[i]);
> +		pci_write_config_word(bus->self, PCI_BRIDGE_CONTROL, vga_ctl[i] & ~PCI_BRIDGE_CTL_VGA);
>  
> -	I915_WRITE(vga_reg, VGA_DISP_DISABLE);
> -	POSTING_READ(vga_reg);
> +		if (WARN_ON(++i >= ARRAY_SIZE(vga_ctl)))
> +			break;
> +	}
> +
> +	i = 0;
> +	list_for_each_entry(pdev, &self->bus->devices, bus_list) {
> +		if (pdev == self)
> +			continue;
> +
> +		pci_read_config_word(pdev, PCI_COMMAND, &vga_cmd[i]);
> +		pci_write_config_word(pdev, PCI_COMMAND, vga_cmd[i] & ~PCI_COMMAND_IO);
> +
> +		if (WARN_ON(++i >= ARRAY_SIZE(vga_cmd)))
> +			break;
> +	}
>  }
>  
> -static void i915_enable_vga_mem(struct drm_device *dev)
> +/*
> + * Restore IO decode and VGA bridge forwarding
> + * for everyone else on the same bus.
> + */
> +static void i915_vga_bus_restore(struct drm_device *dev)
>  {
> -	/* Enable VGA memory on Intel HD */
> -	if (HAS_PCH_SPLIT(dev)) {
> -		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
> -		outb(inb(VGA_MSR_READ) | VGA_MSR_MEM_EN, VGA_MSR_WRITE);
> -		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
> -						   VGA_RSRC_LEGACY_MEM |
> -						   VGA_RSRC_NORMAL_IO |
> -						   VGA_RSRC_NORMAL_MEM);
> -		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
> +	struct pci_dev *pdev, *self = dev->pdev;
> +	struct pci_bus *bus;
> +	int i;
> +
> +	i = 0;
> +	list_for_each_entry(pdev, &self->bus->devices, bus_list) {
> +		if (pdev == self)
> +			continue;
> +
> +		pci_write_config_word(pdev, PCI_COMMAND, vga_cmd[i]);
> +
> +		if (WARN_ON(++i >= ARRAY_SIZE(vga_cmd)))
> +			break;
> +	}
> +
> +	i = 0;
> +	list_for_each_entry(bus, &self->bus->children, node) {
> +		pci_write_config_word(bus->self, PCI_BRIDGE_CONTROL, vga_ctl[i]);
> +
> +		if (WARN_ON(++i >= ARRAY_SIZE(vga_ctl)))
> +			break;
>  	}
>  }
>  
> -void i915_disable_vga_mem(struct drm_device *dev)
> +/*
> + * Hide our VGA IO access from the rest of the system
> + * using stop_machine().
> + *
> + * Note that we assume that the IGD is on the root bus.
> + */
> +static int i915_vga_stop_machine_cb(void *data)
>  {
> -	/* Disable VGA memory on Intel HD */
> -	if (HAS_PCH_SPLIT(dev)) {
> -		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
> -		outb(inb(VGA_MSR_READ) & ~VGA_MSR_MEM_EN, VGA_MSR_WRITE);
> -		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
> -						   VGA_RSRC_NORMAL_IO |
> -						   VGA_RSRC_NORMAL_MEM);
> -		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
> +	struct i915_vga_op *op = data;
> +	struct drm_device *dev = op->dev;
> +	u16 cmd;
> +	u8 tmp;
> +
> +	i915_vga_bus_disable(dev);
> +
> +	pci_read_config_word(dev->pdev, PCI_COMMAND, &cmd);
> +	pci_write_config_word(dev->pdev, PCI_COMMAND, cmd | PCI_COMMAND_IO);
> +
> +	switch (op->op) {
> +	case VGA_OP_MEM_DISABLE:
> +		tmp = inb(VGA_MSR_READ);
> +		tmp &= ~VGA_MSR_MEM_EN;
> +		outb(tmp, VGA_MSR_WRITE);
> +		break;
> +	case VGA_OP_MEM_ENABLE:
> +		tmp = inb(VGA_MSR_READ);
> +		tmp |= VGA_MSR_MEM_EN;
> +		outb(tmp, VGA_MSR_WRITE);
> +		break;
> +	case VGA_OP_SCREEN_OFF:
> +		outb(SR01, VGA_SR_INDEX);
> +		tmp = inb(VGA_SR_DATA);
> +		tmp |= 1 << 5;
> +		outb(tmp, VGA_SR_DATA);
> +		break;
> +	}
> +
> +	pci_write_config_word(dev->pdev, PCI_COMMAND, cmd);
> +
> +	i915_vga_bus_restore(dev);
> +
> +	return 0;
> +}
> +
> +/* MMIO based VGA register access, pre-Gen5 only */
> +static void i915_vga_execute_mmio(struct drm_device *dev, enum vga_op op)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u8 tmp;
> +
> +	switch (op) {
> +	case VGA_OP_MEM_DISABLE:
> +		tmp = I915_READ8(VGA_MSR_READ);
> +		tmp &= ~VGA_MSR_MEM_EN;
> +		I915_WRITE8(VGA_MSR_WRITE, tmp);
> +		break;
> +	case VGA_OP_MEM_ENABLE:
> +		tmp = I915_READ8(VGA_MSR_READ);
> +		tmp |= VGA_MSR_MEM_EN;
> +		I915_WRITE8(VGA_MSR_WRITE, tmp);
> +		break;
> +	case VGA_OP_SCREEN_OFF:
> +		I915_WRITE8(VGA_SR_INDEX, SR01);
> +		tmp = I915_READ8(VGA_SR_DATA);
> +		tmp |= 1 << 5;
> +		I915_WRITE8(VGA_SR_DATA, tmp);
> +		break;
>  	}
>  }
>  
> +static void i915_vga_execute(struct drm_device *dev, enum vga_op op)
> +{
> +	/* WaEnableVGAAccessThroughIOPort:ctg,elg,ilk,snb,ivb,vlv,hsw */
> +	if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
> +		struct i915_vga_op vga_op = {
> +			.op = op,
> +			.dev = dev,
> +		};
> +
> +		stop_machine(i915_vga_stop_machine_cb, &vga_op, NULL);
> +	} else {
> +		i915_vga_execute_mmio(dev, op);
> +	}
> +}
> +
> +/* Disable the VGA plane that we never use */
> +static void i915_disable_vga(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 vga_reg = i915_vgacntrl_reg(dev);
> +	u16 gmch_ctrl;
> +
> +	/* already disabled? */
> +	if (I915_READ(vga_reg) & VGA_DISP_DISABLE)
> +		return;
> +
> +	pci_read_config_word(dev_priv->bridge_dev, INTEL_INFO(dev)->gen >= 6 ?
> +			     SNB_GMCH_CTRL : INTEL_GMCH_CTRL, &gmch_ctrl);
> +	if (gmch_ctrl & INTEL_GMCH_VGA_DISABLE)
> +		DRM_ERROR("VGA plane enabled while VGA disabled via GMCH control\n");
> +
> +	i915_vga_execute(dev, VGA_OP_SCREEN_OFF);
> +
> +	udelay(300);
> +
> +	I915_WRITE(vga_reg, VGA_DISP_DISABLE);
> +	POSTING_READ(vga_reg);
> +}
> +
>  void intel_modeset_init_hw(struct drm_device *dev)
>  {
>  	intel_prepare_ddi(dev);
> @@ -10553,7 +10698,6 @@ void i915_redisable_vga(struct drm_device *dev)
>  	if (I915_READ(vga_reg) != VGA_DISP_DISABLE) {
>  		DRM_DEBUG_KMS("Something enabled VGA plane, disabling it\n");
>  		i915_disable_vga(dev);
> -		i915_disable_vga_mem(dev);
>  	}
>  }
>  
> @@ -10755,7 +10899,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  
>  	intel_disable_fbc(dev);
>  
> -	i915_enable_vga_mem(dev);
> +	intel_modeset_vga_set_state(dev, true);
>  
>  	intel_disable_gt_powersave(dev);
>  
> @@ -10798,12 +10942,36 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u16 gmch_ctrl;
>  
> -	pci_read_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, &gmch_ctrl);
> -	if (state)
> -		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
> -	else
> -		gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
> -	pci_write_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, gmch_ctrl);
> +	/* Is VGA totally disabled for the IGD? */
> +	pci_read_config_word(dev_priv->bridge_dev, INTEL_INFO(dev)->gen >= 6 ?
> +			     SNB_GMCH_CTRL : INTEL_GMCH_CTRL, &gmch_ctrl);
> +	if (gmch_ctrl & INTEL_GMCH_VGA_DISABLE)
> +		return 0;
> +
> +	if (state) {
> +		i915_vga_execute(dev, VGA_OP_MEM_ENABLE);
> +
> +		/*
> +		 * Leave PCI_COMMAND_IO alone for now. vgaarb
> +		 * should re-enable it if and when needed.
> +		 */
> +
> +		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
> +						   VGA_RSRC_LEGACY_MEM |
> +						   VGA_RSRC_NORMAL_IO |
> +						   VGA_RSRC_NORMAL_MEM);
> +	} else {
> +		u16 cmd;
> +
> +		i915_vga_execute(dev, VGA_OP_MEM_DISABLE);
> +
> +		pci_read_config_word(dev->pdev, PCI_COMMAND, &cmd);
> +		cmd &= ~PCI_COMMAND_IO;
> +		pci_write_config_word(dev->pdev, PCI_COMMAND, cmd);
> +
> +		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_NORMAL_MEM);
> +	}
> +
>  	return 0;
>  }
>  



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

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Chris Wilson]

On Mon, Sep 30, 2013 at 05:41:44PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 30, 2013 at 03:24:37PM +0100, Chris Wilson wrote:
> > On Mon, Sep 30, 2013 at 05:08:31PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We have several problems with out VGA handling:
> > > - We try to use the GMCH control VGA disable bit even though it may
> > >   be locked
> > > - If we manage to disable VGA throuh GMCH control, we're no longer
> > >   able to correctly disable the VGA plane
> > > - Taking part in the VGA arbitration is too expensive for X [1]
> > 
> > I'd like to emphasize that X disables DRI if it detects 2 vga cards,
> > effectively breaking all machines with a discrete GPU. Even if one of
> > those is not being used.
> >  
> > > +/*
> > > + * 21 devices with 8 functions per device max on the same bus.
> > > + * We don't need locking for these due to stop_machine().
> > > + */
> > > +static u16 vga_cmd[21*8];
> > > +static u16 vga_ctl[21*8];
> > 
> > Should we just allocate storage for when we need it? We are now adding
> > several hundred bytes to our module, which is bound to cause us to use
> > an extra page, and they can be passed around through the stop_machine
> > closure rather than static.
> 
> I guess we could do that. Although I do wonder a bit if we'd race with
> hotplug. Not sure there's a way to hotplug stuff onto the root bus...

Within stop-machine? I surely hope not!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Ville Syrjälä]

On Mon, Sep 30, 2013 at 08:43:51AM -0600, Alex Williamson wrote:
> On Mon, 2013-09-30 at 17:08 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We have several problems with out VGA handling:
> > - We try to use the GMCH control VGA disable bit even though it may
> >   be locked
> > - If we manage to disable VGA throuh GMCH control, we're no longer
> >   able to correctly disable the VGA plane
> > - Taking part in the VGA arbitration is too expensive for X [1]
> > 
> > So let's treat the GMCH control VGA disable bit as read-only and leave
> > it for the BIOS to set, as it was intended. To disable VGA we will use
> > the VGA misc register, and to disable VGA IO we will disable IO space
> > completely via the PCI command register.
> > 
> > But we still need VGA register access during resume (and possibly during
> > lid event on insane BIOSen) to disable the VGA plane. Also we need to
> > re-disable VGA memory decode via the VGA misc register on resume.
> > 
> > Luckily up to gen4, VGA registers can be accessed through MMIO.
> > Unfortunately from gen5 onwards only the legacy VGA IO port range
> > works. So on gen5+ we still need IO space to be enabled during those
> > few special moments when we need to access VGA registers.
> > 
> > We still want to opt out of VGA arbitration on gen5+, so we have keep
> > IO space disabled most of the time. And when we do need to poke at VGA
> > registers, we enable IO space briefly while no one is looking. To
> > guarantee that no one is looking we will use stop_machine().
> 
> What?!  Why would we not simply wait for the arbiter lock?

Well, there are the X problems which I really don't want to
attempt solving.

Also the arbiter looks a lot like deadlock heaven to me.

What if the other guy doesn't release the arbiter lock in a timely
fashion? It could be some userspace process that's stopped inside
gdb or something.

What if we're doing the restore thing in intel_lid_notify()
and we've already locked the modeset locks and are now waiting
for the arbiter lock, but the other guy who is holding the arbiter
lock is doing a modeset ioctl at the same time and gets stuck
waiting for a modeset lock?

I guess we might be able to solve those problems by killing
the userspace client after a while. But I'd rather just hide
and go code up something more productive ;)

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Ville Syrjälä]

On Mon, Sep 30, 2013 at 03:53:10PM +0100, Chris Wilson wrote:
> On Mon, Sep 30, 2013 at 05:41:44PM +0300, Ville Syrjälä wrote:
> > On Mon, Sep 30, 2013 at 03:24:37PM +0100, Chris Wilson wrote:
> > > On Mon, Sep 30, 2013 at 05:08:31PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > We have several problems with out VGA handling:
> > > > - We try to use the GMCH control VGA disable bit even though it may
> > > >   be locked
> > > > - If we manage to disable VGA throuh GMCH control, we're no longer
> > > >   able to correctly disable the VGA plane
> > > > - Taking part in the VGA arbitration is too expensive for X [1]
> > > 
> > > I'd like to emphasize that X disables DRI if it detects 2 vga cards,
> > > effectively breaking all machines with a discrete GPU. Even if one of
> > > those is not being used.
> > >  
> > > > +/*
> > > > + * 21 devices with 8 functions per device max on the same bus.
> > > > + * We don't need locking for these due to stop_machine().
> > > > + */
> > > > +static u16 vga_cmd[21*8];
> > > > +static u16 vga_ctl[21*8];
> > > 
> > > Should we just allocate storage for when we need it? We are now adding
> > > several hundred bytes to our module, which is bound to cause us to use
> > > an extra page, and they can be passed around through the stop_machine
> > > closure rather than static.
> > 
> > I guess we could do that. Although I do wonder a bit if we'd race with
> > hotplug. Not sure there's a way to hotplug stuff onto the root bus...
> 
> Within stop-machine? I surely hope not!

Can we allocate there? I was thinking of allocating before stop_machine().

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Alex Williamson]

On Mon, 2013-09-30 at 18:09 +0300, Ville Syrjälä wrote:
> On Mon, Sep 30, 2013 at 08:43:51AM -0600, Alex Williamson wrote:
> > On Mon, 2013-09-30 at 17:08 +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We have several problems with out VGA handling:
> > > - We try to use the GMCH control VGA disable bit even though it may
> > >   be locked
> > > - If we manage to disable VGA throuh GMCH control, we're no longer
> > >   able to correctly disable the VGA plane
> > > - Taking part in the VGA arbitration is too expensive for X [1]
> > > 
> > > So let's treat the GMCH control VGA disable bit as read-only and leave
> > > it for the BIOS to set, as it was intended. To disable VGA we will use
> > > the VGA misc register, and to disable VGA IO we will disable IO space
> > > completely via the PCI command register.
> > > 
> > > But we still need VGA register access during resume (and possibly during
> > > lid event on insane BIOSen) to disable the VGA plane. Also we need to
> > > re-disable VGA memory decode via the VGA misc register on resume.
> > > 
> > > Luckily up to gen4, VGA registers can be accessed through MMIO.
> > > Unfortunately from gen5 onwards only the legacy VGA IO port range
> > > works. So on gen5+ we still need IO space to be enabled during those
> > > few special moments when we need to access VGA registers.
> > > 
> > > We still want to opt out of VGA arbitration on gen5+, so we have keep
> > > IO space disabled most of the time. And when we do need to poke at VGA
> > > registers, we enable IO space briefly while no one is looking. To
> > > guarantee that no one is looking we will use stop_machine().
> > 
> > What?!  Why would we not simply wait for the arbiter lock?
> 
> Well, there are the X problems which I really don't want to
> attempt solving.
> 
> Also the arbiter looks a lot like deadlock heaven to me.
> 
> What if the other guy doesn't release the arbiter lock in a timely
> fashion? It could be some userspace process that's stopped inside
> gdb or something.
> 
> What if we're doing the restore thing in intel_lid_notify()
> and we've already locked the modeset locks and are now waiting
> for the arbiter lock, but the other guy who is holding the arbiter
> lock is doing a modeset ioctl at the same time and gets stuck
> waiting for a modeset lock?
> 
> I guess we might be able to solve those problems by killing
> the userspace client after a while. But I'd rather just hide
> and go code up something more productive ;)

So in summary, ignore the infrastructure intended to solve this problem
because it's hard and may have corner cases and instead stop the entire
machine so we can be sure our access is exclusive.  If this is the
solution then VGA arbiter has failed us.  Thanks,

Alex

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

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Chris Wilson]

On Mon, Sep 30, 2013 at 06:10:23PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 30, 2013 at 03:53:10PM +0100, Chris Wilson wrote:
> > On Mon, Sep 30, 2013 at 05:41:44PM +0300, Ville Syrjälä wrote:
> > > On Mon, Sep 30, 2013 at 03:24:37PM +0100, Chris Wilson wrote:
> > > > On Mon, Sep 30, 2013 at 05:08:31PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > We have several problems with out VGA handling:
> > > > > - We try to use the GMCH control VGA disable bit even though it may
> > > > >   be locked
> > > > > - If we manage to disable VGA throuh GMCH control, we're no longer
> > > > >   able to correctly disable the VGA plane
> > > > > - Taking part in the VGA arbitration is too expensive for X [1]
> > > > 
> > > > I'd like to emphasize that X disables DRI if it detects 2 vga cards,
> > > > effectively breaking all machines with a discrete GPU. Even if one of
> > > > those is not being used.
> > > >  
> > > > > +/*
> > > > > + * 21 devices with 8 functions per device max on the same bus.
> > > > > + * We don't need locking for these due to stop_machine().
> > > > > + */
> > > > > +static u16 vga_cmd[21*8];
> > > > > +static u16 vga_ctl[21*8];
> > > > 
> > > > Should we just allocate storage for when we need it? We are now adding
> > > > several hundred bytes to our module, which is bound to cause us to use
> > > > an extra page, and they can be passed around through the stop_machine
> > > > closure rather than static.
> > > 
> > > I guess we could do that. Although I do wonder a bit if we'd race with
> > > hotplug. Not sure there's a way to hotplug stuff onto the root bus...
> > 
> > Within stop-machine? I surely hope not!
> 
> Can we allocate there? I was thinking of allocating before stop_machine().

Using GFP_ATOMIC should be permissible, I think. But yeah, you can
allocate space upfront for struct vga_op.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Ville Syrjälä]

On Mon, Sep 30, 2013 at 10:45:03AM -0600, Alex Williamson wrote:
> On Mon, 2013-09-30 at 18:09 +0300, Ville Syrjälä wrote:
> > On Mon, Sep 30, 2013 at 08:43:51AM -0600, Alex Williamson wrote:
> > > On Mon, 2013-09-30 at 17:08 +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > We have several problems with out VGA handling:
> > > > - We try to use the GMCH control VGA disable bit even though it may
> > > >   be locked
> > > > - If we manage to disable VGA throuh GMCH control, we're no longer
> > > >   able to correctly disable the VGA plane
> > > > - Taking part in the VGA arbitration is too expensive for X [1]
> > > > 
> > > > So let's treat the GMCH control VGA disable bit as read-only and leave
> > > > it for the BIOS to set, as it was intended. To disable VGA we will use
> > > > the VGA misc register, and to disable VGA IO we will disable IO space
> > > > completely via the PCI command register.
> > > > 
> > > > But we still need VGA register access during resume (and possibly during
> > > > lid event on insane BIOSen) to disable the VGA plane. Also we need to
> > > > re-disable VGA memory decode via the VGA misc register on resume.
> > > > 
> > > > Luckily up to gen4, VGA registers can be accessed through MMIO.
> > > > Unfortunately from gen5 onwards only the legacy VGA IO port range
> > > > works. So on gen5+ we still need IO space to be enabled during those
> > > > few special moments when we need to access VGA registers.
> > > > 
> > > > We still want to opt out of VGA arbitration on gen5+, so we have keep
> > > > IO space disabled most of the time. And when we do need to poke at VGA
> > > > registers, we enable IO space briefly while no one is looking. To
> > > > guarantee that no one is looking we will use stop_machine().
> > > 
> > > What?!  Why would we not simply wait for the arbiter lock?
> > 
> > Well, there are the X problems which I really don't want to
> > attempt solving.
> > 
> > Also the arbiter looks a lot like deadlock heaven to me.
> > 
> > What if the other guy doesn't release the arbiter lock in a timely
> > fashion? It could be some userspace process that's stopped inside
> > gdb or something.
> > 
> > What if we're doing the restore thing in intel_lid_notify()
> > and we've already locked the modeset locks and are now waiting
> > for the arbiter lock, but the other guy who is holding the arbiter
> > lock is doing a modeset ioctl at the same time and gets stuck
> > waiting for a modeset lock?
> > 
> > I guess we might be able to solve those problems by killing
> > the userspace client after a while. But I'd rather just hide
> > and go code up something more productive ;)
> 
> So in summary, ignore the infrastructure intended to solve this problem
> because it's hard and may have corner cases and instead stop the entire
> machine so we can be sure our access is exclusive.  If this is the
> solution then VGA arbiter has failed us.  Thanks,

Well, if you want to fix the vga arbiter (assuming it's possible) feel
free to send patches. I can even give a vague promise of reviewing them.
But I guess we need to fix this ASAP or face the wrath of the angry mob
who lost their dri capabilities.

The other option is to move the stop_machine infrastructure inside
vgaarb and then we can say we're using vgaarb as *deity* intended ;)

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Alex Williamson]

On Mon, 2013-09-30 at 15:24 +0100, Chris Wilson wrote:
> On Mon, Sep 30, 2013 at 05:08:31PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We have several problems with out VGA handling:
> > - We try to use the GMCH control VGA disable bit even though it may
> >   be locked
> > - If we manage to disable VGA throuh GMCH control, we're no longer
> >   able to correctly disable the VGA plane
> > - Taking part in the VGA arbitration is too expensive for X [1]
> 
> I'd like to emphasize that X disables DRI if it detects 2 vga cards,
> effectively breaking all machines with a discrete GPU. Even if one of
> those is not being used.

Why does it do this?  It seems like DRI would make little or no use of
VGA space.  Having more than one VGA card seems like a pretty common
condition when integrated graphics are available.  We also seem to have
quite some interest in assigning one or more of the cards to a virtual
machine, so I worry we're headed the wrong way if X starts deciding not
to release the VGA arbiter lock.  On a modern desktop what touches VGA
space with a high enough frequency that we care about it's performance?
Thanks,

Alex

> > +/*
> > + * 21 devices with 8 functions per device max on the same bus.
> > + * We don't need locking for these due to stop_machine().
> > + */
> > +static u16 vga_cmd[21*8];
> > +static u16 vga_ctl[21*8];
> 
> Should we just allocate storage for when we need it? We are now adding
> several hundred bytes to our module, which is bound to cause us to use
> an extra page, and they can be passed around through the stop_machine
> closure rather than static.
> 
> But anyway, it does what is says on the tin and makes my dGPU testing
> box usable again, without breaking any other machine that I've tested on
> so far,
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> 



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

[PATCH v3] drm/i915: Fix VGA handling using stop_machine() or mmio

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We have several problems with out VGA handling:
- We try to use the GMCH control VGA disable bit even though it may
  be locked
- If we manage to disable VGA throuh GMCH control, we're no longer
  able to correctly disable the VGA plane
- Taking part in the VGA arbitration is too expensive for X [1]

So let's treat the GMCH control VGA disable bit as read-only and leave
it for the BIOS to set, as it was intended. To disable VGA we will use
the VGA misc register, and to disable VGA IO we will disable IO space
completely via the PCI command register.

But we still need VGA register access during resume (and possibly during
lid event on insane BIOSen) to disable the VGA plane. Also we need to
re-disable VGA memory decode via the VGA misc register on resume.

Luckily up to gen4, VGA registers can be accessed through MMIO.
Unfortunately from gen5 onwards only the legacy VGA IO port range
works. So on gen5+ we still need IO space to be enabled during those
few special moments when we need to access VGA registers.

We still want to opt out of VGA arbitration on gen5+, so we have keep
IO space disabled most of the time. And when we do need to poke at VGA
registers, we enable IO space briefly while no one is looking. To
guarantee that no one is looking we will use stop_machine().

[1] http://lists.x.org/archives/xorg-devel/2013-September/037763.html

v2: Use SNB_GMCH_TRL on SNB+
    Use port IO instead of MMIO on CTG/ELK
    Add WaEnableVGAAccessThroughIOPort comment
    Fix the max number of devices on the bus limit
v3: Allocate the temp space dynamically
    Print some errors if we fail to execute the vga "op" due to alloc failure

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  39 +----
 drivers/gpu/drm/i915/i915_suspend.c  |   4 +-
 drivers/gpu/drm/i915/intel_display.c | 318 ++++++++++++++++++++++++++++++-----
 3 files changed, 287 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index d35de1b..59a2048 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1225,19 +1225,6 @@ intel_teardown_mchbar(struct drm_device *dev)
 		release_resource(&dev_priv->mch_res);
 }
 
-/* true = enable decode, false = disable decoder */
-static unsigned int i915_vga_set_decode(void *cookie, bool state)
-{
-	struct drm_device *dev = cookie;
-
-	intel_modeset_vga_set_state(dev, state);
-	if (state)
-		return VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
-		       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
-	else
-		return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
-}
-
 static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_state state)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
@@ -1283,25 +1270,11 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		DRM_INFO("failed to find VBIOS tables\n");
 
-	/* If we have > 1 VGA cards, then we need to arbitrate access
-	 * to the common VGA resources.
-	 *
-	 * If we are a secondary display controller (!PCI_DISPLAY_CLASS_VGA),
-	 * then we do not take part in VGA arbitration and the
-	 * vga_client_register() fails with -ENODEV.
-	 */
-	if (!HAS_PCH_SPLIT(dev)) {
-		ret = vga_client_register(dev->pdev, dev, NULL,
-					  i915_vga_set_decode);
-		if (ret && ret != -ENODEV)
-			goto out;
-	}
-
 	intel_register_dsm_handler();
 
 	ret = vga_switcheroo_register_client(dev->pdev, &i915_switcheroo_ops, false);
 	if (ret)
-		goto cleanup_vga_client;
+		goto out;
 
 	/* Initialise stolen first so that we may reserve preallocated
 	 * objects for the BIOS to KMS transition.
@@ -1316,7 +1289,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_init_power_well(dev);
 
-	/* Keep VGA alive until i915_disable_vga_mem() */
+	/* Keep VGA alive until intel_modeset_vga_set_state() */
 	intel_display_power_get(dev, POWER_DOMAIN_VGA);
 
 	/* Important: The output setup functions called by modeset_init need
@@ -1359,10 +1332,13 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	intel_fbdev_initial_config(dev);
 
 	/*
+	 * Disable VGA IO and memory, and
+	 * tell the arbiter to ignore us.
+	 *
 	 * Must do this after fbcon init so that
 	 * vgacon_save_screen() works during the handover.
 	 */
-	i915_disable_vga_mem(dev);
+	intel_modeset_vga_set_state(dev, false);
 	intel_display_power_put(dev, POWER_DOMAIN_VGA);
 
 	/* Only enable hotplug handling once the fbdev is fully set up. */
@@ -1386,8 +1362,6 @@ cleanup_gem_stolen:
 	i915_gem_cleanup_stolen(dev);
 cleanup_vga_switcheroo:
 	vga_switcheroo_unregister_client(dev->pdev);
-cleanup_vga_client:
-	vga_client_register(dev->pdev, NULL, NULL, NULL);
 out:
 	return ret;
 }
@@ -1758,7 +1732,6 @@ int i915_driver_unload(struct drm_device *dev)
 		}
 
 		vga_switcheroo_unregister_client(dev->pdev);
-		vga_client_register(dev->pdev, NULL, NULL, NULL);
 	}
 
 	/* Free error state after interrupts are fully disabled. */
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 3538370..cc0d459 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -331,8 +331,10 @@ static void i915_restore_display(struct drm_device *dev)
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		i915_restore_vga(dev);
-	else
+	else {
 		i915_redisable_vga(dev);
+		intel_modeset_vga_set_state(dev, false);
+	}
 }
 
 int i915_save_state(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 76870f0..84e5a5a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -40,6 +40,8 @@
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <linux/dma_remapping.h>
+#include <linux/stop_machine.h>
+#include "../../../pci/pci.h"
 
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
@@ -10232,51 +10234,253 @@ static void intel_init_quirks(struct drm_device *dev)
 	}
 }
 
-/* Disable the VGA plane that we never use */
-static void i915_disable_vga(struct drm_device *dev)
+enum vga_op {
+	VGA_OP_MEM_DISABLE,
+	VGA_OP_MEM_ENABLE,
+	VGA_OP_SCREEN_OFF,
+};
+
+struct i915_vga_op {
+	enum vga_op op;
+	struct drm_device *dev;
+	u16 *cmd, *ctl;
+	int cmd_num, ctl_num;
+};
+
+static int i915_vga_temp_alloc(struct i915_vga_op *op)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u8 sr1;
-	u32 vga_reg = i915_vgacntrl_reg(dev);
+	struct drm_device *dev = op->dev;
+	struct pci_dev *pdev, *self = dev->pdev;
+	struct pci_bus *bus;
 
-	vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
-	outb(SR01, VGA_SR_INDEX);
-	sr1 = inb(VGA_SR_DATA);
-	outb(sr1 | 1<<5, VGA_SR_DATA);
-	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
-	udelay(300);
+	list_for_each_entry(bus, &self->bus->children, node)
+		op->ctl_num++;
 
-	I915_WRITE(vga_reg, VGA_DISP_DISABLE);
-	POSTING_READ(vga_reg);
+	list_for_each_entry(pdev, &self->bus->devices, bus_list)
+		if (pdev != self)
+			op->cmd_num++;
+
+	op->cmd = kmalloc_array(op->cmd_num, sizeof(op->cmd[0]), GFP_KERNEL);
+	if (!op->cmd)
+		return -ENOMEM;
+
+	op->ctl = kmalloc_array(op->ctl_num, sizeof(op->ctl[0]), GFP_KERNEL);
+	if (!op->ctl) {
+		kfree(op->cmd);
+		return -ENOMEM;
+	}
+
+	return 0;
 }
 
-static void i915_enable_vga_mem(struct drm_device *dev)
+static void i915_vga_temp_free(struct i915_vga_op *op)
 {
-	/* Enable VGA memory on Intel HD */
-	if (HAS_PCH_SPLIT(dev)) {
-		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
-		outb(inb(VGA_MSR_READ) | VGA_MSR_MEM_EN, VGA_MSR_WRITE);
-		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
-						   VGA_RSRC_LEGACY_MEM |
-						   VGA_RSRC_NORMAL_IO |
-						   VGA_RSRC_NORMAL_MEM);
-		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
+	kfree(op->cmd);
+	kfree(op->ctl);
+}
+
+/*
+ * Disable IO decode and VGA bridge forwarding
+ * for everyone else on the same bus.
+ */
+static void i915_vga_bus_disable(struct i915_vga_op *op)
+{
+	struct drm_device *dev = op->dev;
+	struct pci_dev *pdev, *self = dev->pdev;
+	struct pci_bus *bus;
+	int i;
+
+	i = 0;
+	list_for_each_entry(bus, &self->bus->children, node) {
+		if (WARN_ON(i >= op->ctl_num))
+			break;
+
+		pci_read_config_word(bus->self, PCI_BRIDGE_CONTROL,
+				     &op->ctl[i]);
+		pci_write_config_word(bus->self, PCI_BRIDGE_CONTROL,
+				      op->ctl[i] & ~PCI_BRIDGE_CTL_VGA);
+		i++;
+	}
+
+	i = 0;
+	list_for_each_entry(pdev, &self->bus->devices, bus_list) {
+		if (pdev == self)
+			continue;
+
+		if (WARN_ON(i >= op->cmd_num))
+			break;
+
+		pci_read_config_word(pdev, PCI_COMMAND,
+				     &op->cmd[i]);
+		pci_write_config_word(pdev, PCI_COMMAND,
+				      op->cmd[i] & ~PCI_COMMAND_IO);
+		i++;
 	}
 }
 
-void i915_disable_vga_mem(struct drm_device *dev)
+/*
+ * Restore IO decode and VGA bridge forwarding
+ * for everyone else on the same bus.
+ */
+static void i915_vga_bus_restore(struct i915_vga_op *op)
 {
-	/* Disable VGA memory on Intel HD */
-	if (HAS_PCH_SPLIT(dev)) {
-		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
-		outb(inb(VGA_MSR_READ) & ~VGA_MSR_MEM_EN, VGA_MSR_WRITE);
-		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
-						   VGA_RSRC_NORMAL_IO |
-						   VGA_RSRC_NORMAL_MEM);
-		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
+	struct drm_device *dev = op->dev;
+	struct pci_dev *pdev, *self = dev->pdev;
+	struct pci_bus *bus;
+	int i;
+
+	i = 0;
+	list_for_each_entry(pdev, &self->bus->devices, bus_list) {
+		if (pdev == self)
+			continue;
+
+		if (i >= op->cmd_num)
+			break;
+
+		pci_write_config_word(pdev, PCI_COMMAND,
+				      op->cmd[i]);
+		i++;
+	}
+
+	i = 0;
+	list_for_each_entry(bus, &self->bus->children, node) {
+		if (i >= op->ctl_num)
+			break;
+
+		pci_write_config_word(bus->self, PCI_BRIDGE_CONTROL,
+				      op->ctl[i]);
+		i++;
 	}
 }
 
+/*
+ * Hide our VGA IO access from the rest of the system
+ * using stop_machine().
+ *
+ * Note that we assume that the IGD is on the root bus.
+ */
+static int i915_vga_stop_machine_cb(void *data)
+{
+	struct i915_vga_op *op = data;
+	struct drm_device *dev = op->dev;
+	u16 cmd;
+	u8 tmp;
+
+	i915_vga_bus_disable(op);
+
+	pci_read_config_word(dev->pdev, PCI_COMMAND, &cmd);
+	pci_write_config_word(dev->pdev, PCI_COMMAND, cmd | PCI_COMMAND_IO);
+
+	switch (op->op) {
+	case VGA_OP_MEM_DISABLE:
+		tmp = inb(VGA_MSR_READ);
+		tmp &= ~VGA_MSR_MEM_EN;
+		outb(tmp, VGA_MSR_WRITE);
+		break;
+	case VGA_OP_MEM_ENABLE:
+		tmp = inb(VGA_MSR_READ);
+		tmp |= VGA_MSR_MEM_EN;
+		outb(tmp, VGA_MSR_WRITE);
+		break;
+	case VGA_OP_SCREEN_OFF:
+		outb(SR01, VGA_SR_INDEX);
+		tmp = inb(VGA_SR_DATA);
+		tmp |= 1 << 5;
+		outb(tmp, VGA_SR_DATA);
+		break;
+	}
+
+	pci_write_config_word(dev->pdev, PCI_COMMAND, cmd);
+
+	i915_vga_bus_restore(op);
+
+	return 0;
+}
+
+/* MMIO based VGA register access, pre-Gen5 only */
+static void i915_vga_execute_mmio(struct drm_device *dev, enum vga_op op)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u8 tmp;
+
+	switch (op) {
+	case VGA_OP_MEM_DISABLE:
+		tmp = I915_READ8(VGA_MSR_READ);
+		tmp &= ~VGA_MSR_MEM_EN;
+		I915_WRITE8(VGA_MSR_WRITE, tmp);
+		break;
+	case VGA_OP_MEM_ENABLE:
+		tmp = I915_READ8(VGA_MSR_READ);
+		tmp |= VGA_MSR_MEM_EN;
+		I915_WRITE8(VGA_MSR_WRITE, tmp);
+		break;
+	case VGA_OP_SCREEN_OFF:
+		I915_WRITE8(VGA_SR_INDEX, SR01);
+		tmp = I915_READ8(VGA_SR_DATA);
+		tmp |= 1 << 5;
+		I915_WRITE8(VGA_SR_DATA, tmp);
+		break;
+	}
+}
+
+static int i915_vga_execute(struct drm_device *dev, enum vga_op op)
+{
+	int ret = 0;
+
+	/* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */
+	if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
+		struct i915_vga_op vga_op = {
+			.op = op,
+			.dev = dev,
+		};
+
+		/*
+		 * Protect against hot(un)plug during/after
+		 * counting how much temp space we need.
+		 */
+		down_read(&pci_bus_sem);
+
+		ret = i915_vga_temp_alloc(&vga_op);
+
+		if (ret == 0) {
+			stop_machine(i915_vga_stop_machine_cb, &vga_op, NULL);
+
+			i915_vga_temp_free(&vga_op);
+		}
+
+		up_read(&pci_bus_sem);
+	} else {
+		i915_vga_execute_mmio(dev, op);
+	}
+
+	return ret;
+}
+
+/* Disable the VGA plane that we never use */
+static void i915_disable_vga(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 vga_reg = i915_vgacntrl_reg(dev);
+	u16 gmch_ctrl;
+
+	/* already disabled? */
+	if (I915_READ(vga_reg) & VGA_DISP_DISABLE)
+		return;
+
+	pci_read_config_word(dev_priv->bridge_dev, INTEL_INFO(dev)->gen >= 6 ?
+			     SNB_GMCH_CTRL : INTEL_GMCH_CTRL, &gmch_ctrl);
+	if (gmch_ctrl & INTEL_GMCH_VGA_DISABLE)
+		DRM_ERROR("VGA plane enabled while VGA disabled via GMCH control\n");
+
+	if (i915_vga_execute(dev, VGA_OP_SCREEN_OFF))
+		DRM_ERROR("Failed to disable VGA plane correctly\n");
+
+	udelay(300);
+
+	I915_WRITE(vga_reg, VGA_DISP_DISABLE);
+	POSTING_READ(vga_reg);
+}
+
 void intel_modeset_init_hw(struct drm_device *dev)
 {
 	intel_prepare_ddi(dev);
@@ -10553,7 +10757,6 @@ void i915_redisable_vga(struct drm_device *dev)
 	if (I915_READ(vga_reg) != VGA_DISP_DISABLE) {
 		DRM_DEBUG_KMS("Something enabled VGA plane, disabling it\n");
 		i915_disable_vga(dev);
-		i915_disable_vga_mem(dev);
 	}
 }
 
@@ -10755,7 +10958,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_disable_fbc(dev);
 
-	i915_enable_vga_mem(dev);
+	intel_modeset_vga_set_state(dev, true);
 
 	intel_disable_gt_powersave(dev);
 
@@ -10798,12 +11001,47 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u16 gmch_ctrl;
 
-	pci_read_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, &gmch_ctrl);
-	if (state)
-		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
-	else
-		gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
-	pci_write_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, gmch_ctrl);
+	/* Is VGA totally disabled for the IGD? */
+	pci_read_config_word(dev_priv->bridge_dev, INTEL_INFO(dev)->gen >= 6 ?
+			     SNB_GMCH_CTRL : INTEL_GMCH_CTRL, &gmch_ctrl);
+	if (gmch_ctrl & INTEL_GMCH_VGA_DISABLE)
+		return 0;
+
+	if (state) {
+		int ret;
+
+		ret = i915_vga_execute(dev, VGA_OP_MEM_ENABLE);
+		if (ret) {
+			DRM_ERROR("Failed to enable VGA memory decode\n");
+			return ret;
+		}
+
+		/*
+		 * Leave PCI_COMMAND_IO alone for now. vgaarb
+		 * should re-enable it if and when needed.
+		 */
+
+		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
+						   VGA_RSRC_LEGACY_MEM |
+						   VGA_RSRC_NORMAL_IO |
+						   VGA_RSRC_NORMAL_MEM);
+	} else {
+		int ret;
+		u16 cmd;
+
+		ret = i915_vga_execute(dev, VGA_OP_MEM_DISABLE);
+		if (ret) {
+			DRM_ERROR("Failed to disable VGA memory decode\n");
+			return ret;
+		}
+
+		pci_read_config_word(dev->pdev, PCI_COMMAND, &cmd);
+		cmd &= ~PCI_COMMAND_IO;
+		pci_write_config_word(dev->pdev, PCI_COMMAND, cmd);
+
+		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_NORMAL_MEM);
+	}
+
 	return 0;
 }
 
-- 
1.8.1.5

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

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Dave Airlie]

On Tue, Oct 1, 2013 at 4:37 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 2013-09-30 at 15:24 +0100, Chris Wilson wrote:
>> On Mon, Sep 30, 2013 at 05:08:31PM +0300, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > We have several problems with out VGA handling:
>> > - We try to use the GMCH control VGA disable bit even though it may
>> >   be locked
>> > - If we manage to disable VGA throuh GMCH control, we're no longer
>> >   able to correctly disable the VGA plane
>> > - Taking part in the VGA arbitration is too expensive for X [1]
>>
>> I'd like to emphasize that X disables DRI if it detects 2 vga cards,
>> effectively breaking all machines with a discrete GPU. Even if one of
>> those is not being used.
>
> Why does it do this?  It seems like DRI would make little or no use of
> VGA space.  Having more than one VGA card seems like a pretty common
> condition when integrated graphics are available.  We also seem to have
> quite some interest in assigning one or more of the cards to a virtual
> machine, so I worry we're headed the wrong way if X starts deciding not
> to release the VGA arbiter lock.  On a modern desktop what touches VGA
> space with a high enough frequency that we care about it's performance?
> Thanks,
>

Because we don't know what DRI clients can do, and a lot of this was
constructed when DRI1 still occured. The idea being that sane GPUs
would remove themselves from arbitration by just switching off their
VGA spaces, who was to know Intel hw guys would fuck this up, probably
should have guessed though.

So the problem we have is that VGA ARB's current interface causes
interactions with the current X server that probably aren't totally
required, so by trying to actually VGA arbitrate properly we are going
to regress userspace so we now need to design a solution that avoids
the regression but possibly allows us to move forward.

I'm think I'm going to make an executive decision to merge Ville's
latest patch to avoid the regression, the other option being just to
revert everything back to the status quo by reverting all the patches
from the past few months, if people are happier with that then maybe
we should just do that for now and try and design our way out of it
properly by reengineering userspace and plan to avoid the regression
next time.

Dave.

Re: [PATCH v2 1/1] drm/i915: Fix VGA handling using stop_machine() or mmio

[Daniel Vetter]

On Mon, Oct 7, 2013 at 2:23 AM, Dave Airlie <airlied@gmail.com> wrote:
> I'm think I'm going to make an executive decision to merge Ville's
> latest patch to avoid the regression, the other option being just to
> revert everything back to the status quo by reverting all the patches
> from the past few months, if people are happier with that then maybe
> we should just do that for now and try and design our way out of it
> properly by reengineering userspace and plan to avoid the regression
> next time.

Ack on merging Ville's hack. Longer-term we could beautify the pig a
bit by moving it into vgaarb.c and avoiding the stop_machine if no one
else has a vga lock. And I think in next we could try to limit the vga
redisable hack in the lid notifier a bit on more modern platforms to
avoid the dreaded stop_machine at runtime and relegate it to just
resume/driver load.

Really long term (once broken X servers have died) we can try to rip
this hack out again. Or maybe intel hw engineers get their act
together beforehand ;-)

So can you directly pick it up since I've just flushed out my -fixes queue?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v3] drm/i915: Fix VGA handling using stop_machine() or mmio

[Chris Wilson]

On Wed, Oct 02, 2013 at 04:42:55PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We have several problems with out VGA handling:
> - We try to use the GMCH control VGA disable bit even though it may
>   be locked
> - If we manage to disable VGA throuh GMCH control, we're no longer
>   able to correctly disable the VGA plane
> - Taking part in the VGA arbitration is too expensive for X [1]
> 
> So let's treat the GMCH control VGA disable bit as read-only and leave
> it for the BIOS to set, as it was intended. To disable VGA we will use
> the VGA misc register, and to disable VGA IO we will disable IO space
> completely via the PCI command register.
> 
> But we still need VGA register access during resume (and possibly during
> lid event on insane BIOSen) to disable the VGA plane. Also we need to
> re-disable VGA memory decode via the VGA misc register on resume.
> 
> Luckily up to gen4, VGA registers can be accessed through MMIO.
> Unfortunately from gen5 onwards only the legacy VGA IO port range
> works. So on gen5+ we still need IO space to be enabled during those
> few special moments when we need to access VGA registers.
> 
> We still want to opt out of VGA arbitration on gen5+, so we have keep
> IO space disabled most of the time. And when we do need to poke at VGA
> registers, we enable IO space briefly while no one is looking. To
> guarantee that no one is looking we will use stop_machine().
> 
> [1] http://lists.x.org/archives/xorg-devel/2013-September/037763.html
> 
> v2: Use SNB_GMCH_TRL on SNB+
>     Use port IO instead of MMIO on CTG/ELK
>     Add WaEnableVGAAccessThroughIOPort comment
>     Fix the max number of devices on the bus limit
> v3: Allocate the temp space dynamically
>     Print some errors if we fail to execute the vga "op" due to alloc failure

Passes the dGPU test, the SNB laptop, but is doa for CTG.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3] drm/i915: Fix VGA handling using stop_machine() or mmio

[Ville Syrjälä]

On Mon, Oct 07, 2013 at 10:15:16AM +0100, Chris Wilson wrote:
> On Wed, Oct 02, 2013 at 04:42:55PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We have several problems with out VGA handling:
> > - We try to use the GMCH control VGA disable bit even though it may
> >   be locked
> > - If we manage to disable VGA throuh GMCH control, we're no longer
> >   able to correctly disable the VGA plane
> > - Taking part in the VGA arbitration is too expensive for X [1]
> > 
> > So let's treat the GMCH control VGA disable bit as read-only and leave
> > it for the BIOS to set, as it was intended. To disable VGA we will use
> > the VGA misc register, and to disable VGA IO we will disable IO space
> > completely via the PCI command register.
> > 
> > But we still need VGA register access during resume (and possibly during
> > lid event on insane BIOSen) to disable the VGA plane. Also we need to
> > re-disable VGA memory decode via the VGA misc register on resume.
> > 
> > Luckily up to gen4, VGA registers can be accessed through MMIO.
> > Unfortunately from gen5 onwards only the legacy VGA IO port range
> > works. So on gen5+ we still need IO space to be enabled during those
> > few special moments when we need to access VGA registers.
> > 
> > We still want to opt out of VGA arbitration on gen5+, so we have keep
> > IO space disabled most of the time. And when we do need to poke at VGA
> > registers, we enable IO space briefly while no one is looking. To
> > guarantee that no one is looking we will use stop_machine().
> > 
> > [1] http://lists.x.org/archives/xorg-devel/2013-September/037763.html
> > 
> > v2: Use SNB_GMCH_TRL on SNB+
> >     Use port IO instead of MMIO on CTG/ELK
> >     Add WaEnableVGAAccessThroughIOPort comment
> >     Fix the max number of devices on the bus limit
> > v3: Allocate the temp space dynamically
> >     Print some errors if we fail to execute the vga "op" due to alloc failure
> 
> Passes the dGPU test, the SNB laptop, but is doa for CTG.

Crap. And v2 still works all right there?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3] drm/i915: Fix VGA handling using stop_machine() or mmio

[Chris Wilson]

On Mon, Oct 07, 2013 at 12:25:03PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 07, 2013 at 10:15:16AM +0100, Chris Wilson wrote:
> > On Wed, Oct 02, 2013 at 04:42:55PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We have several problems with out VGA handling:
> > > - We try to use the GMCH control VGA disable bit even though it may
> > >   be locked
> > > - If we manage to disable VGA throuh GMCH control, we're no longer
> > >   able to correctly disable the VGA plane
> > > - Taking part in the VGA arbitration is too expensive for X [1]
> > > 
> > > So let's treat the GMCH control VGA disable bit as read-only and leave
> > > it for the BIOS to set, as it was intended. To disable VGA we will use
> > > the VGA misc register, and to disable VGA IO we will disable IO space
> > > completely via the PCI command register.
> > > 
> > > But we still need VGA register access during resume (and possibly during
> > > lid event on insane BIOSen) to disable the VGA plane. Also we need to
> > > re-disable VGA memory decode via the VGA misc register on resume.
> > > 
> > > Luckily up to gen4, VGA registers can be accessed through MMIO.
> > > Unfortunately from gen5 onwards only the legacy VGA IO port range
> > > works. So on gen5+ we still need IO space to be enabled during those
> > > few special moments when we need to access VGA registers.
> > > 
> > > We still want to opt out of VGA arbitration on gen5+, so we have keep
> > > IO space disabled most of the time. And when we do need to poke at VGA
> > > registers, we enable IO space briefly while no one is looking. To
> > > guarantee that no one is looking we will use stop_machine().
> > > 
> > > [1] http://lists.x.org/archives/xorg-devel/2013-September/037763.html
> > > 
> > > v2: Use SNB_GMCH_TRL on SNB+
> > >     Use port IO instead of MMIO on CTG/ELK
> > >     Add WaEnableVGAAccessThroughIOPort comment
> > >     Fix the max number of devices on the bus limit
> > > v3: Allocate the temp space dynamically
> > >     Print some errors if we fail to execute the vga "op" due to alloc failure
> > 
> > Passes the dGPU test, the SNB laptop, but is doa for CTG.
> 
> Crap. And v2 still works all right there?

No, appears I mislead you. The last one to successfully work was the
composite v1 patch set. Sorry.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre