| Message ID | 20170901072744.2409-1-dja@axtens.net |
|---|---|
| Headers | show |
| Series | Split default display handling out from VGA arbiter | expand |
On 1 September 2017 at 08:27, Daniel Axtens <dja@axtens.net> wrote: > This patch set: > > - splits the default display handling out from VGA arbiter, into its > own file and behind its own Kconfig option (and gives the functions > better names). > > - adds extra detection of default devices. To be nominated, the vga > arbiter and platform hooks must not have nominated a default. A > card will then only be nominated if it has a driver attached and > has IO or memory decoding enabled. > > - adds relevant documentation. > > The practical impact of this is improved X autoconfiguration on some > arm64 systems. > > Changes in v3: > > - Add documentation - thanks Daniel Vetter for pointing it out. > > - Clarify explanations. Thanks to everyone for continuing to bear > with my incomplete understanding of PCI and provide some clarity. > > - Split refactoring and adding functionality. > > Changes in v2: https://www.spinics.net/lists/linux-pci/msg64007.html > > Drop all the powerpc patches. [explanation snipped] > > v1: https://www.spinics.net/lists/linux-pci/msg63581.html > If we are all in agreement that fixing X is not an option, I think this is a reasonable approach Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Hi Ard, > If we are all in agreement that fixing X is not an option, I think > this is a reasonable approach This did come up in discussion at some earlier point in one of the many spins we've done of this - I don't remember if you brought it up or someone else did - but my concern was this: If it were just X we would be fine, but if we go down that path I'm worried about also having to fix Mir/Wayland/whatever-the-new-exciting-display-server-is-this-week, ad nauseum. > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Thanks! Regards, Daniel
Hi all, The merge window is well over by now - is there anything else I can do to get this series to a mergeable state? I'm not particularly across the rules for drm-misc, so please let me know if there are things I need to be doing. Regards, Daniel Daniel Axtens <dja@axtens.net> writes: > This patch set: > > - splits the default display handling out from VGA arbiter, into its > own file and behind its own Kconfig option (and gives the functions > better names). > > - adds extra detection of default devices. To be nominated, the vga > arbiter and platform hooks must not have nominated a default. A > card will then only be nominated if it has a driver attached and > has IO or memory decoding enabled. > > - adds relevant documentation. > > The practical impact of this is improved X autoconfiguration on some > arm64 systems. > > Changes in v3: > > - Add documentation - thanks Daniel Vetter for pointing it out. > > - Clarify explanations. Thanks to everyone for continuing to bear > with my incomplete understanding of PCI and provide some clarity. > > - Split refactoring and adding functionality. > > Changes in v2: https://www.spinics.net/lists/linux-pci/msg64007.html > > Drop all the powerpc patches. [explanation snipped] > > v1: https://www.spinics.net/lists/linux-pci/msg63581.html > > Regards, > Daniel > > Daniel Axtens (3): > drm: split default display handler out of VGA arbiter > drm: add fallback default device detection > drm: documentation for default display device > > Documentation/gpu/default_display.rst | 93 +++++++++++++++++++ > Documentation/gpu/index.rst | 1 + > arch/ia64/pci/fixup.c | 6 +- > arch/powerpc/kernel/pci-common.c | 6 +- > arch/x86/pci/fixup.c | 6 +- > arch/x86/video/fbdev.c | 4 +- > drivers/gpu/vga/Kconfig | 12 +++ > drivers/gpu/vga/Makefile | 1 + > drivers/gpu/vga/default_display.c | 163 ++++++++++++++++++++++++++++++++++ > drivers/gpu/vga/vga_switcheroo.c | 8 +- > drivers/gpu/vga/vgaarb.c | 61 +++---------- > drivers/pci/pci-sysfs.c | 4 +- > include/linux/default_display.h | 44 +++++++++ > include/linux/vgaarb.h | 15 ---- > 14 files changed, 344 insertions(+), 80 deletions(-) > create mode 100644 Documentation/gpu/default_display.rst > create mode 100644 drivers/gpu/vga/default_display.c > create mode 100644 include/linux/default_display.h > > -- > 2.11.0
On Fri, Sep 01, 2017 at 05:27:41PM +1000, Daniel Axtens wrote: > This patch set: > > - splits the default display handling out from VGA arbiter, into its > own file and behind its own Kconfig option (and gives the functions > better names). > > - adds extra detection of default devices. To be nominated, the vga > arbiter and platform hooks must not have nominated a default. A > card will then only be nominated if it has a driver attached and > has IO or memory decoding enabled. > > - adds relevant documentation. > > The practical impact of this is improved X autoconfiguration on some > arm64 systems. I think I gave you bad advice about trying to separate the "default device" idea from the VGA arbiter. It is true that the "VGA arbiter" per se is related to routing the legacy VGA resources, and the arbiter currently only selects a default device if it finds a device to which those resources are routed. We have some cases where we want to select a default device that may not support the legacy VGA resources, or where they might not be routed to the device: - systems where we match the EFI framebuffer address with a BAR, and select that device as default, - powerpc systems where there may be no host bridge window that maps to the legacy VGA resources, - your ARM64 systems where the default device may be behind a bridge that doesn't support legacy VGA routing (PCI_BRIDGE_CTL_VGA) But I think trying to split the "default device" part out from the VGA arbiter ends up being overkill and making things more complicated instead of simpler. Would something like the following work for you as well as the powerpc case? On powerpc, we already use vga_set_default_device() to select a device that doesn't use legacy VGA resources, so maybe we can just do the same on ARM64? I suppose there might be wrinkles in how the arbiter deals with multiple graphics devices on those systems, since I don't think it identifies these devices that don't use the legacy resources, but it seems like we live with whatever those on are powerpc and probably can on ARM64 as well. diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index 02831a396419..0ac7aa346c69 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -1740,15 +1740,3 @@ static void fixup_hide_host_resource_fsl(struct pci_dev *dev) } DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID, fixup_hide_host_resource_fsl); DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl); - -static void fixup_vga(struct pci_dev *pdev) -{ - u16 cmd; - - pci_read_config_word(pdev, PCI_COMMAND, &cmd); - if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device()) - vga_set_default_device(pdev); - -} -DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID, - PCI_CLASS_DISPLAY_VGA, 8, fixup_vga); diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 76875f6299b8..9df4802c5f04 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/vgaarb.c @@ -1468,6 +1468,21 @@ static int __init vga_arb_device_init(void) vgaarb_info(dev, "no bridge control possible\n"); } + if (!vga_default_device()) { + list_for_each_entry(vgadev, &vga_list, list) { + struct device *dev = &vgadev->pdev->dev; + u16 cmd; + + pdev = vgadev->pdev; + pci_read_config_word(pdev, PCI_COMMAND, &cmd); + if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) { + vgaarb_info(dev, "setting as boot device\n"); + vga_set_default_device(pdev); + break; + } + } + } + pr_info("loaded\n"); return rc; }
Hi Bjorn, > But I think trying to split the "default device" part out from the VGA > arbiter ends up being overkill and making things more complicated > instead of simpler. Fair enough. > Would something like the following work for you as well as the powerpc > case? On powerpc, we already use vga_set_default_device() to select a > device that doesn't use legacy VGA resources, so maybe we can just do > the same on ARM64? It looks good. I'll try to get some time on the test system to test it and I'll pester my friends at IBM to give it a go as well. > I suppose there might be wrinkles in how the arbiter deals with > multiple graphics devices on those systems, since I don't think it > identifies these devices that don't use the legacy resources, but it > seems like we live with whatever those on are powerpc and probably can > on ARM64 as well. I would say so, yes. Thanks for sticking with this! Regards, Daniel > > > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > index 02831a396419..0ac7aa346c69 100644 > --- a/arch/powerpc/kernel/pci-common.c > +++ b/arch/powerpc/kernel/pci-common.c > @@ -1740,15 +1740,3 @@ static void fixup_hide_host_resource_fsl(struct pci_dev *dev) > } > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID, fixup_hide_host_resource_fsl); > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl); > - > -static void fixup_vga(struct pci_dev *pdev) > -{ > - u16 cmd; > - > - pci_read_config_word(pdev, PCI_COMMAND, &cmd); > - if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device()) > - vga_set_default_device(pdev); > - > -} > -DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID, > - PCI_CLASS_DISPLAY_VGA, 8, fixup_vga); > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c > index 76875f6299b8..9df4802c5f04 100644 > --- a/drivers/gpu/vga/vgaarb.c > +++ b/drivers/gpu/vga/vgaarb.c > @@ -1468,6 +1468,21 @@ static int __init vga_arb_device_init(void) > vgaarb_info(dev, "no bridge control possible\n"); > } > > + if (!vga_default_device()) { > + list_for_each_entry(vgadev, &vga_list, list) { > + struct device *dev = &vgadev->pdev->dev; > + u16 cmd; > + > + pdev = vgadev->pdev; > + pci_read_config_word(pdev, PCI_COMMAND, &cmd); > + if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) { > + vgaarb_info(dev, "setting as boot device\n"); > + vga_set_default_device(pdev); > + break; > + } > + } > + } > + > pr_info("loaded\n"); > return rc; > }
On Mon, Sep 18, 2017 at 7:49 AM, Daniel Axtens <dja@axtens.net> wrote: > Hi all, > > The merge window is well over by now - is there anything else I can do > to get this series to a mergeable state? I'm not particularly across the > rules for drm-misc, so please let me know if there are things I need to > be doing. drm-misc is always open for feature merging, but the patch series lacks full r-b by someone with clue on this stuff (i.e. probably not me). Pls find someone who can do the in-depth review. Just an Ack feels a bit thin. -Daniel > > Regards, > Daniel > > Daniel Axtens <dja@axtens.net> writes: > >> This patch set: >> >> - splits the default display handling out from VGA arbiter, into its >> own file and behind its own Kconfig option (and gives the functions >> better names). >> >> - adds extra detection of default devices. To be nominated, the vga >> arbiter and platform hooks must not have nominated a default. A >> card will then only be nominated if it has a driver attached and >> has IO or memory decoding enabled. >> >> - adds relevant documentation. >> >> The practical impact of this is improved X autoconfiguration on some >> arm64 systems. >> >> Changes in v3: >> >> - Add documentation - thanks Daniel Vetter for pointing it out. >> >> - Clarify explanations. Thanks to everyone for continuing to bear >> with my incomplete understanding of PCI and provide some clarity. >> >> - Split refactoring and adding functionality. >> >> Changes in v2: https://www.spinics.net/lists/linux-pci/msg64007.html >> >> Drop all the powerpc patches. [explanation snipped] >> >> v1: https://www.spinics.net/lists/linux-pci/msg63581.html >> >> Regards, >> Daniel >> >> Daniel Axtens (3): >> drm: split default display handler out of VGA arbiter >> drm: add fallback default device detection >> drm: documentation for default display device >> >> Documentation/gpu/default_display.rst | 93 +++++++++++++++++++ >> Documentation/gpu/index.rst | 1 + >> arch/ia64/pci/fixup.c | 6 +- >> arch/powerpc/kernel/pci-common.c | 6 +- >> arch/x86/pci/fixup.c | 6 +- >> arch/x86/video/fbdev.c | 4 +- >> drivers/gpu/vga/Kconfig | 12 +++ >> drivers/gpu/vga/Makefile | 1 + >> drivers/gpu/vga/default_display.c | 163 ++++++++++++++++++++++++++++++++++ >> drivers/gpu/vga/vga_switcheroo.c | 8 +- >> drivers/gpu/vga/vgaarb.c | 61 +++---------- >> drivers/pci/pci-sysfs.c | 4 +- >> include/linux/default_display.h | 44 +++++++++ >> include/linux/vgaarb.h | 15 ---- >> 14 files changed, 344 insertions(+), 80 deletions(-) >> create mode 100644 Documentation/gpu/default_display.rst >> create mode 100644 drivers/gpu/vga/default_display.c >> create mode 100644 include/linux/default_display.h >> >> -- >> 2.11.0 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Bjorn, Yes, this works: Tested-by: Daniel Axtens <dja@axtens.net> # arm64, ppc64-qemu-tcg Regards, Daniel > On Fri, Sep 01, 2017 at 05:27:41PM +1000, Daniel Axtens wrote: >> This patch set: >> >> - splits the default display handling out from VGA arbiter, into its >> own file and behind its own Kconfig option (and gives the functions >> better names). >> >> - adds extra detection of default devices. To be nominated, the vga >> arbiter and platform hooks must not have nominated a default. A >> card will then only be nominated if it has a driver attached and >> has IO or memory decoding enabled. >> >> - adds relevant documentation. >> >> The practical impact of this is improved X autoconfiguration on some >> arm64 systems. > > I think I gave you bad advice about trying to separate the "default > device" idea from the VGA arbiter. > > It is true that the "VGA arbiter" per se is related to routing the > legacy VGA resources, and the arbiter currently only selects a default > device if it finds a device to which those resources are routed. > > We have some cases where we want to select a default device that may > not support the legacy VGA resources, or where they might not be > routed to the device: > > - systems where we match the EFI framebuffer address with a BAR, and > select that device as default, > > - powerpc systems where there may be no host bridge window that maps > to the legacy VGA resources, > > - your ARM64 systems where the default device may be behind a bridge > that doesn't support legacy VGA routing (PCI_BRIDGE_CTL_VGA) > > But I think trying to split the "default device" part out from the VGA > arbiter ends up being overkill and making things more complicated > instead of simpler. > > Would something like the following work for you as well as the powerpc > case? On powerpc, we already use vga_set_default_device() to select a > device that doesn't use legacy VGA resources, so maybe we can just do > the same on ARM64? > > I suppose there might be wrinkles in how the arbiter deals with > multiple graphics devices on those systems, since I don't think it > identifies these devices that don't use the legacy resources, but it > seems like we live with whatever those on are powerpc and probably can > on ARM64 as well. > > > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > index 02831a396419..0ac7aa346c69 100644 > --- a/arch/powerpc/kernel/pci-common.c > +++ b/arch/powerpc/kernel/pci-common.c > @@ -1740,15 +1740,3 @@ static void fixup_hide_host_resource_fsl(struct pci_dev *dev) > } > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID, fixup_hide_host_resource_fsl); > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl); > - > -static void fixup_vga(struct pci_dev *pdev) > -{ > - u16 cmd; > - > - pci_read_config_word(pdev, PCI_COMMAND, &cmd); > - if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device()) > - vga_set_default_device(pdev); > - > -} > -DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID, > - PCI_CLASS_DISPLAY_VGA, 8, fixup_vga); > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c > index 76875f6299b8..9df4802c5f04 100644 > --- a/drivers/gpu/vga/vgaarb.c > +++ b/drivers/gpu/vga/vgaarb.c > @@ -1468,6 +1468,21 @@ static int __init vga_arb_device_init(void) > vgaarb_info(dev, "no bridge control possible\n"); > } > > + if (!vga_default_device()) { > + list_for_each_entry(vgadev, &vga_list, list) { > + struct device *dev = &vgadev->pdev->dev; > + u16 cmd; > + > + pdev = vgadev->pdev; > + pci_read_config_word(pdev, PCI_COMMAND, &cmd); > + if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) { > + vgaarb_info(dev, "setting as boot device\n"); > + vga_set_default_device(pdev); > + break; > + } > + } > + } > + > pr_info("loaded\n"); > return rc; > }
On Wed, Sep 27, 2017 at 01:52:55PM +1000, Daniel Axtens wrote: > Hi Bjorn, > > Yes, this works: > > Tested-by: Daniel Axtens <dja@axtens.net> # arm64, ppc64-qemu-tcg I guess I was assuming you'd pick this up, but that doesn't really make sense because I didn't give you a signed-off-by or anything. I'll post this with a changelog and signed-off-by so it doesn't get lost. I also noticed that I didn't correctly handle the powerpc quirk case where it doesn't require the device to be enabled at all. I'll try to fix that up, too. Bjorn > > On Fri, Sep 01, 2017 at 05:27:41PM +1000, Daniel Axtens wrote: > >> This patch set: > >> > >> - splits the default display handling out from VGA arbiter, into its > >> own file and behind its own Kconfig option (and gives the functions > >> better names). > >> > >> - adds extra detection of default devices. To be nominated, the vga > >> arbiter and platform hooks must not have nominated a default. A > >> card will then only be nominated if it has a driver attached and > >> has IO or memory decoding enabled. > >> > >> - adds relevant documentation. > >> > >> The practical impact of this is improved X autoconfiguration on some > >> arm64 systems. > > > > I think I gave you bad advice about trying to separate the "default > > device" idea from the VGA arbiter. > > > > It is true that the "VGA arbiter" per se is related to routing the > > legacy VGA resources, and the arbiter currently only selects a default > > device if it finds a device to which those resources are routed. > > > > We have some cases where we want to select a default device that may > > not support the legacy VGA resources, or where they might not be > > routed to the device: > > > > - systems where we match the EFI framebuffer address with a BAR, and > > select that device as default, > > > > - powerpc systems where there may be no host bridge window that maps > > to the legacy VGA resources, > > > > - your ARM64 systems where the default device may be behind a bridge > > that doesn't support legacy VGA routing (PCI_BRIDGE_CTL_VGA) > > > > But I think trying to split the "default device" part out from the VGA > > arbiter ends up being overkill and making things more complicated > > instead of simpler. > > > > Would something like the following work for you as well as the powerpc > > case? On powerpc, we already use vga_set_default_device() to select a > > device that doesn't use legacy VGA resources, so maybe we can just do > > the same on ARM64? > > > > I suppose there might be wrinkles in how the arbiter deals with > > multiple graphics devices on those systems, since I don't think it > > identifies these devices that don't use the legacy resources, but it > > seems like we live with whatever those on are powerpc and probably can > > on ARM64 as well. > > > > > > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > > index 02831a396419..0ac7aa346c69 100644 > > --- a/arch/powerpc/kernel/pci-common.c > > +++ b/arch/powerpc/kernel/pci-common.c > > @@ -1740,15 +1740,3 @@ static void fixup_hide_host_resource_fsl(struct pci_dev *dev) > > } > > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID, fixup_hide_host_resource_fsl); > > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl); > > - > > -static void fixup_vga(struct pci_dev *pdev) > > -{ > > - u16 cmd; > > - > > - pci_read_config_word(pdev, PCI_COMMAND, &cmd); > > - if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device()) > > - vga_set_default_device(pdev); > > - > > -} > > -DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID, > > - PCI_CLASS_DISPLAY_VGA, 8, fixup_vga); > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c > > index 76875f6299b8..9df4802c5f04 100644 > > --- a/drivers/gpu/vga/vgaarb.c > > +++ b/drivers/gpu/vga/vgaarb.c > > @@ -1468,6 +1468,21 @@ static int __init vga_arb_device_init(void) > > vgaarb_info(dev, "no bridge control possible\n"); > > } > > > > + if (!vga_default_device()) { > > + list_for_each_entry(vgadev, &vga_list, list) { > > + struct device *dev = &vgadev->pdev->dev; > > + u16 cmd; > > + > > + pdev = vgadev->pdev; > > + pci_read_config_word(pdev, PCI_COMMAND, &cmd); > > + if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) { > > + vgaarb_info(dev, "setting as boot device\n"); > > + vga_set_default_device(pdev); > > + break; > > + } > > + } > > + } > > + > > pr_info("loaded\n"); > > return rc; > > } > >