Hi Dave,

This one plus the pci_disable_device and related error path cleanups are part of the series to move the agp setup into drivers.

I've implemented your suggestion to work around the midlayer, so drm/i915 doesn't block for this at all. But I still think this is the right way to solve this. Shall I resend all these patches so you have them in context?

Yours, Daniel

On Fri, Jun 08, 2012 at 06:20:39PM +0200, Daniel Vetter wrote:

There's the neat little problem that some systems die if vga decoding isn't decoded anywhere, because linux disabled that pci device.

Atm we solve that problem by simple not calling pci_disable_device at driver unregister time for drm pci devices. Which isn't to great because it leaks a pci_enable_device reference on module reload and also is rather inconsistent, since the driver load codcode in drm/drm_pci.c _does_ call pci_disable_device on the load error path.

To fix this issue, let the vga arbiter hold a pci_enable_device reference on its own when the vga device decodes anything. This leaves the issue still open when the vga arbiter isn't loaded/compiled in, but I guess since drm module unload is riddled with dragons it's ok to say "just don't do this".

Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch

drivers/gpu/vga/vgaarb.c | 27 +++++++++++++++++++++++++-- 1 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 3df8fc0..82f19a1 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/vgaarb.c @@ -49,7 +49,11 @@ static void vga_arbiter_notify_clients(void); /*

• We keep a list of all vga devices in the system to speed

• • up the various operations of the arbiter

• • up the various operations of the arbiter. Note that vgaarb keeps a
• • pci_enable_device reference for all vga devices with owns != 0. This is to
• • avoid drm devices from killing the system when they call pci_disable_device
• • on module unload (as they should), because some systems don't like it if no
• • one decodes vga.
  */

struct vga_device { struct list_head list; @@ -171,7 +175,7 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, { unsigned int wants, legacy_wants, match; struct vga_device *conflict;

• unsigned int pci_bits;

• unsigned int pci_bits, ret; u32 flags = 0;

  /* Account for "normal" resources to lock. If we decode the legacy,

@@ -264,6 +268,10 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,

pci_set_vga_state(conflict->pdev, false, pci_bits, flags);
conflict->owns &= ~lwants;

• if (conflict->owns == 0)
  

• 	pci_disable_device(conflict->pdev);
  

• /* If he also owned non-legacy, that is no longer the case */ if (lwants & VGA_RSRC_LEGACY_MEM) conflict->owns &= ~VGA_RSRC_NORMAL_MEM;

@@ -290,6 +298,12 @@ enable_them: if (!!(wants & VGA_RSRC_LEGACY_MASK)) flags |= PCI_VGA_STATE_CHANGE_BRIDGE;

• if (vgadev->owns == 0) {

• ret = pci_enable_device(vgadev->pdev);
  

• if (ret)
  

• 	return ERR_PTR(ret);
  

• }

• pci_set_vga_state(vgadev->pdev, true, pci_bits, flags);

  if (!vgadev->bridge_has_one_vga) {

@@ -376,6 +390,8 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible) if (conflict == NULL) break;

• if (IS_ERR(conflict))
  

• 	return PTR_ERR(conflict);
  
  

  /* We have a conflict, we wait until somebody kicks the

  • work queue. Currently we have one work queue that we

@@ -513,6 +529,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) unsigned long flags; struct pci_bus *bus; struct pci_dev *bridge;

• int ret; u16 cmd;

  /* Only deal with VGA class devices */

@@ -582,6 +599,12 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)

vga_arbiter_check_bridge_sharing(vgadev);

• if (vgadev->owns != 0) {

• ret = pci_enable_device(vgadev->pdev);
  

• if (ret)
  

• 	goto fail;
  

• }
• /* Add to the list */ list_add(&vgadev->list, &vga_list); vga_count++;

-- 1.7.7.6