On Fri, 08 Jun 2012, Daniel Vetter daniel.vetter@ffwll.ch 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);
Hi Daniel, I think unsigned ret will result in a positive ERR_PTR, which won't be caught by IS_ERR_VALUE(). (Either that, or I need more coffee to get the promotion rules right. Better use signed int no matter what.)
BR, Jani.
}
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
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx