Commit 4e4e7dc5 authored by Alex Williamson's avatar Alex Williamson Committed by Dave Airlie

vgaarb: We can own non-decoded resources

The VGA arbiter does not allow devices to "own" resources that it
doesn't "decode".  However, it does allow devices to "lock" resources
that it doesn't decode.  This gets us into trouble because locking
the resource goes through the same bridge routing updates regardless
of whether we decode the resource.  This means that when a non-decoded
resource is released, the bridge is left with VGA routing enabled and
locking a different device won't clear it.

This happens in the following scenario:

VGA device 01:00.0 (VGA1) is owned by the radeon driver, which
registers a set_vga_decode function which releases legacy VGA decodes.

VGA device 02:00.0 (VGA2) is any VGA device.

VGA1 user locks VGA resources triggering first_use callback of
set_vga_decoded, clearing "decode" and "owns" of legacy resources
on VGA1.

VGA1 user unlocks VGA resources.

VGA2 user locks VGA resources, which skips VGA1 as conflicting as it
does not "own" legacy resources, although VGA routing is still enabled
for the VGA1 bridge.  VGA routing is enabled on VGA2 bridge.

VGA2 may or may not receive VGA transactions depending on the bus
priority of VGA1 vs VGA2 bridge.

To resolve this, we need to allow devices to "own" resources that they
do not "decode".  This way we can track bus ownership of VGA.  When a
device decodes VGA, it only means that we must update the command bits
in cases where the conflicting device is on the same bus.
Signed-off-by: default avatarAlex Williamson <alex.williamson@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: default avatarDave Airlie <airlied@redhat.com>
parent f71c5d9d
...@@ -237,12 +237,10 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, ...@@ -237,12 +237,10 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
if (conflict->locks & lwants) if (conflict->locks & lwants)
return conflict; return conflict;
/* Ok, now check if he owns the resource we want. We don't need /* Ok, now check if it owns the resource we want. We can
* to check "decodes" since it should be impossible to own * lock resources that are not decoded, therefore a device
* own legacy resources you don't decode unless I have a bug * can own resources it doesn't decode.
* in this code...
*/ */
WARN_ON(conflict->owns & ~conflict->decodes);
match = lwants & conflict->owns; match = lwants & conflict->owns;
if (!match) if (!match)
continue; continue;
...@@ -254,13 +252,19 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, ...@@ -254,13 +252,19 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
flags = 0; flags = 0;
pci_bits = 0; pci_bits = 0;
/* If we can't control legacy resources via the bridge, we
* also need to disable normal decoding.
*/
if (!conflict->bridge_has_one_vga) { if (!conflict->bridge_has_one_vga) {
vga_irq_set_state(conflict, false); if ((match & conflict->decodes) & VGA_RSRC_LEGACY_MEM)
flags |= PCI_VGA_STATE_CHANGE_DECODES;
if (match & (VGA_RSRC_LEGACY_MEM|VGA_RSRC_NORMAL_MEM))
pci_bits |= PCI_COMMAND_MEMORY; pci_bits |= PCI_COMMAND_MEMORY;
if (match & (VGA_RSRC_LEGACY_IO|VGA_RSRC_NORMAL_IO)) if ((match & conflict->decodes) & VGA_RSRC_LEGACY_IO)
pci_bits |= PCI_COMMAND_IO; pci_bits |= PCI_COMMAND_IO;
if (pci_bits) {
vga_irq_set_state(conflict, false);
flags |= PCI_VGA_STATE_CHANGE_DECODES;
}
} }
if (change_bridge) if (change_bridge)
...@@ -268,18 +272,19 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, ...@@ -268,18 +272,19 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
pci_set_vga_state(conflict->pdev, false, pci_bits, flags); pci_set_vga_state(conflict->pdev, false, pci_bits, flags);
conflict->owns &= ~match; conflict->owns &= ~match;
/* If he also owned non-legacy, that is no longer the case */
if (match & VGA_RSRC_LEGACY_MEM) /* If we disabled normal decoding, reflect it in owns */
if (pci_bits & PCI_COMMAND_MEMORY)
conflict->owns &= ~VGA_RSRC_NORMAL_MEM; conflict->owns &= ~VGA_RSRC_NORMAL_MEM;
if (match & VGA_RSRC_LEGACY_IO) if (pci_bits & PCI_COMMAND_IO)
conflict->owns &= ~VGA_RSRC_NORMAL_IO; conflict->owns &= ~VGA_RSRC_NORMAL_IO;
} }
enable_them: enable_them:
/* ok dude, we got it, everybody conflicting has been disabled, let's /* ok dude, we got it, everybody conflicting has been disabled, let's
* enable us. Make sure we don't mark a bit in "owns" that we don't * enable us. Mark any bits in "owns" regardless of whether we
* also have in "decodes". We can lock resources we don't decode but * decoded them. We can lock resources we don't decode, therefore
* not own them. * we must track them via "owns".
*/ */
flags = 0; flags = 0;
pci_bits = 0; pci_bits = 0;
...@@ -291,7 +296,7 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, ...@@ -291,7 +296,7 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
if (wants & (VGA_RSRC_LEGACY_IO|VGA_RSRC_NORMAL_IO)) if (wants & (VGA_RSRC_LEGACY_IO|VGA_RSRC_NORMAL_IO))
pci_bits |= PCI_COMMAND_IO; pci_bits |= PCI_COMMAND_IO;
} }
if (!!(wants & VGA_RSRC_LEGACY_MASK)) if (wants & VGA_RSRC_LEGACY_MASK)
flags |= PCI_VGA_STATE_CHANGE_BRIDGE; flags |= PCI_VGA_STATE_CHANGE_BRIDGE;
pci_set_vga_state(vgadev->pdev, true, pci_bits, flags); pci_set_vga_state(vgadev->pdev, true, pci_bits, flags);
...@@ -299,7 +304,7 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev, ...@@ -299,7 +304,7 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
if (!vgadev->bridge_has_one_vga) { if (!vgadev->bridge_has_one_vga) {
vga_irq_set_state(vgadev, true); vga_irq_set_state(vgadev, true);
} }
vgadev->owns |= (wants & vgadev->decodes); vgadev->owns |= wants;
lock_them: lock_them:
vgadev->locks |= (rsrc & VGA_RSRC_LEGACY_MASK); vgadev->locks |= (rsrc & VGA_RSRC_LEGACY_MASK);
if (rsrc & VGA_RSRC_LEGACY_IO) if (rsrc & VGA_RSRC_LEGACY_IO)
...@@ -649,7 +654,6 @@ static inline void vga_update_device_decodes(struct vga_device *vgadev, ...@@ -649,7 +654,6 @@ static inline void vga_update_device_decodes(struct vga_device *vgadev,
old_decodes = vgadev->decodes; old_decodes = vgadev->decodes;
decodes_removed = ~new_decodes & old_decodes; decodes_removed = ~new_decodes & old_decodes;
decodes_unlocked = vgadev->locks & decodes_removed; decodes_unlocked = vgadev->locks & decodes_removed;
vgadev->owns &= ~decodes_removed;
vgadev->decodes = new_decodes; vgadev->decodes = new_decodes;
pr_info("vgaarb: device changed decodes: PCI:%s,olddecodes=%s,decodes=%s:owns=%s\n", pr_info("vgaarb: device changed decodes: PCI:%s,olddecodes=%s,decodes=%s:owns=%s\n",
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment