Hi Bjorn, Thanks for reading the patch and providing some feedback. >> This will work as intended if there is only one device, but if >> there are multiple devices, we may override the device the VGA >> arbiter picked. > > This quirk only runs on VGA class devices. If there's more than one > VGA device in the system, and we assume that firmware only enables > PCI_COMMAND_IO or PCI_COMMAND_MEMORY on "the one initialized by > firmware", which seems reasonable to me, I think the existing code > does match the commit message. > > We set the first VGA device we find to be the default. Then, if we > find another VGA device that's enabled, we make *it* the default > instead. > >> Instead, set a device as default if there is no default device AND >> this device decodes. >> >> This will not change behaviour on single-headed systems. > > If there is no enabled VGA device on the system, your new code means > there will be no default VGA device. Ah. Right you are. > > It's not clear from this changelog what problem this patch solves. > Maybe it's the "some displays not being picked up by the VGA arbiter" > you mentioned, but there's not enough detail to connect it with the > patch, especially since the patch means we'll set the default device > in fewer cases than we did before. > > With the patch, we only set the default if we find an enabled VGA > device. Previously we also set the default if we found a VGA device > that had not been enabled. My overall problem is not having default devices on some machines. Initially, to solve this, I wanted to make this code generic. To do that I wanted to make sure it played nice with the vga arbiter, so not overriding default devices willy-nilly was a requirement. Evidently, I was overly keen and restricted its behaviour too much. Regardless, in this current approach I don't make this powerpc code generic after all, so this patch is unnecessary. I will remove it for v2, which I will post after further testing. I document the effects of the new approach for powerpc in more detail in patch 3 of this series. If powerpc wants to keep the existing approach we can arrange for that too; it's a simple matter of ifdefs. Regards, Daniel > >> Cc: Brian King <brking@xxxxxxxxxxxxxxxxxx> >> Signed-off-by: Daniel Axtens <dja@xxxxxxxxxx> >> >> --- >> >> Tested in TCG (the card provided by qemu doesn't automatically >> register with vgaarb, so the relevant code path has been tested) >> but I would appreciate any tests on real hardware. >> >> Informal benh ack: https://patchwork.kernel.org/patch/9850235/ >> --- >> arch/powerpc/kernel/pci-common.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c >> index 341a7469cab8..c95fdda3a2dc 100644 >> --- a/arch/powerpc/kernel/pci-common.c >> +++ b/arch/powerpc/kernel/pci-common.c >> @@ -1746,8 +1746,11 @@ static void fixup_vga(struct pci_dev *pdev) >> { >> u16 cmd; >> >> + if (vga_default_device()) >> + return; >> + >> pci_read_config_word(pdev, PCI_COMMAND, &cmd); >> - if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device()) >> + if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) >> vga_set_default_device(pdev); >> >> } >> -- >> 2.11.0 >>
