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 at 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
>
--
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48
