| Message ID | 20230523155644.678524-1-marcin.juszkiewicz@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] docs: sbsa: correct graphics card name | expand |
On 23/05/2023 17.56, Marcin Juszkiewicz wrote: > We moved from VGA to Bochs to have PCIe card. > > Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org> > --- > docs/system/arm/sbsa.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst > index b499d7e927..fea4992df2 100644 > --- a/docs/system/arm/sbsa.rst > +++ b/docs/system/arm/sbsa.rst > @@ -27,6 +27,6 @@ The sbsa-ref board supports: > - System bus EHCI controller > - CDROM and hard disc on AHCI bus > - E1000E ethernet card on PCIe bus > - - VGA display adaptor on PCIe bus > + - Bochs display adaptor on PCIe bus > - A generic SBSA watchdog device > While you're at it, I'd suggest to replace "adaptor" with "adapter" which seems to be way more common in the QEMU sources: $ grep -r adaptor * | wc -l 5 $ grep -r adapter * | wc -l 385 With that changed: Reviewed-by: Thomas Huth <thuth@redhat.com> PS: An idea for another patch: I think the "config SBSA_REF" in hw/arm/Kconfig should select BOCHS_DISPLAY now, since this board seems to always require this card now (is there a reason why it can't be disabled with "-vga none" or "-nodefaults"?)
W dniu 23.05.2023 o 19:11, Thomas Huth pisze: > On 23/05/2023 17.56, Marcin Juszkiewicz wrote: >> We moved from VGA to Bochs to have PCIe card. >> >> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org> >> --- >> docs/system/arm/sbsa.rst | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst >> index b499d7e927..fea4992df2 100644 >> --- a/docs/system/arm/sbsa.rst >> +++ b/docs/system/arm/sbsa.rst >> @@ -27,6 +27,6 @@ The sbsa-ref board supports: >> - System bus EHCI controller >> - CDROM and hard disc on AHCI bus >> - E1000E ethernet card on PCIe bus >> - - VGA display adaptor on PCIe bus >> + - Bochs display adaptor on PCIe bus >> - A generic SBSA watchdog device > > While you're at it, I'd suggest to replace "adaptor" with "adapter" > which seems to be way more common in the QEMU sources: > > $ grep -r adaptor * | wc -l > 5 > $ grep -r adapter * | wc -l > 385 > > With that changed: > Reviewed-by: Thomas Huth <thuth@redhat.com> Thanks. changed. > PS: An idea for another patch: I think the "config SBSA_REF" in > hw/arm/Kconfig should select BOCHS_DISPLAY now, since this board seems > to always require this card now Thanks, patch sent. > (is there a reason why it can't be disabled with "-vga none" or "-nodefaults"?) That's something I need to check how it should be done. Should it also drop default_nic?
On 23/05/2023 19.30, Marcin Juszkiewicz wrote: ... >> (is there a reason why it can't be disabled with "-vga none" or >> "-nodefaults"?) > > That's something I need to check how it should be done. Other boards set mc->default_display in their ...class_init function and then use pci_vga_init() (or vga_interface_type) to instantiate their default display adapter ... however, that does not seem to support the bochs adapter yet (see vga_interfaces[] in softmmu/vl.c). Not sure whether it's worth the effort to extend vga_interfaces[] in vl.c, but you could at least check whether vga_interface_type is VGA_NONE and skip the creation of the bochs adapter in that case? > Should it also drop default_nic? Seems like sbsa-ref already uses nd_table[], so "-net none" should already work. For "configure --without-default-devices" builds, we still need a patch like this on top, though: diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c --- a/hw/arm/sbsa-ref.c +++ b/hw/arm/sbsa-ref.c @@ -596,6 +596,7 @@ static void create_pcie(SBSAMachineState *sms) hwaddr size_mmio_high = sbsa_ref_memmap[SBSA_PCIE_MMIO_HIGH].size; hwaddr base_pio = sbsa_ref_memmap[SBSA_PCIE_PIO].base; int irq = sbsa_ref_irqmap[SBSA_PCIE]; + MachineClass *mc = MACHINE_GET_CLASS(sms); MemoryRegion *mmio_alias, *mmio_alias_high, *mmio_reg; MemoryRegion *ecam_alias, *ecam_reg; DeviceState *dev; @@ -641,7 +642,7 @@ static void create_pcie(SBSAMachineState *sms) NICInfo *nd = &nd_table[i]; if (!nd->model) { - nd->model = g_strdup("e1000e"); + nd->model = g_strdup(mc->default_nic); } pci_nic_init_nofail(nd, pci->bus, nd->model, NULL); @@ -858,6 +859,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data) mc->minimum_page_bits = 12; mc->block_default_type = IF_IDE; mc->no_cdrom = 1; + mc->default_nic = "e1000e"; mc->default_ram_size = 1 * GiB; mc->default_ram_id = "sbsa-ref.ram"; mc->default_cpus = 4; (I'm doing that default_nic change for a lot of other boards currently, so I can send a proper patch for this later) Thomas
On Tue, 23 May 2023 at 19:41, Thomas Huth <thuth@redhat.com> wrote: > > On 23/05/2023 19.30, Marcin Juszkiewicz wrote: > ... > >> (is there a reason why it can't be disabled with "-vga none" or > >> "-nodefaults"?) > > > > That's something I need to check how it should be done. > > Other boards set mc->default_display in their ...class_init > function and then use pci_vga_init() (or vga_interface_type) > to instantiate their default display adapter ... however, that > does not seem to support the bochs adapter yet (see > vga_interfaces[] in softmmu/vl.c). > > Not sure whether it's worth the effort to extend vga_interfaces[] > in vl.c Isn't that a legacy-command-line-option thing? I feel like the recommended way to select a different graphics card these days would be to use -device my-pci-vga-card ... thanks -- PMM
On 25/05/2023 12.05, Peter Maydell wrote: > On Tue, 23 May 2023 at 19:41, Thomas Huth <thuth@redhat.com> wrote: >> >> On 23/05/2023 19.30, Marcin Juszkiewicz wrote: >> ... >>>> (is there a reason why it can't be disabled with "-vga none" or >>>> "-nodefaults"?) >>> >>> That's something I need to check how it should be done. >> >> Other boards set mc->default_display in their ...class_init >> function and then use pci_vga_init() (or vga_interface_type) >> to instantiate their default display adapter ... however, that >> does not seem to support the bochs adapter yet (see >> vga_interfaces[] in softmmu/vl.c). >> >> Not sure whether it's worth the effort to extend vga_interfaces[] >> in vl.c > > Isn't that a legacy-command-line-option thing? I feel like > the recommended way to select a different graphics card > these days would be to use -device my-pci-vga-card ... "-vga" is kind of legacy, indeed, but currently the sbsa-ref hard-codes the graphics card to be always available, so if you add a "-device something-vga-card" on the command line, you'd get two graphic cards on your machine, even if you use -nodefaults. So there needs to be at least some logic dealing with vga_interface_type if we want to be able to select a different graphics card for this machine. Then why not go the full way and use pci_vga_init() here, too? ... that's certainly the least confusing way for the users. Thomas
On Thu, 25 May 2023 at 11:32, Thomas Huth <thuth@redhat.com> wrote: > > On 25/05/2023 12.05, Peter Maydell wrote: > > On Tue, 23 May 2023 at 19:41, Thomas Huth <thuth@redhat.com> wrote: > >> > >> On 23/05/2023 19.30, Marcin Juszkiewicz wrote: > >> ... > >>>> (is there a reason why it can't be disabled with "-vga none" or > >>>> "-nodefaults"?) > >>> > >>> That's something I need to check how it should be done. > >> > >> Other boards set mc->default_display in their ...class_init > >> function and then use pci_vga_init() (or vga_interface_type) > >> to instantiate their default display adapter ... however, that > >> does not seem to support the bochs adapter yet (see > >> vga_interfaces[] in softmmu/vl.c). > >> > >> Not sure whether it's worth the effort to extend vga_interfaces[] > >> in vl.c > > > > Isn't that a legacy-command-line-option thing? I feel like > > the recommended way to select a different graphics card > > these days would be to use -device my-pci-vga-card ... > > "-vga" is kind of legacy, indeed, but currently the sbsa-ref hard-codes the > graphics card to be always available, so if you add a "-device > something-vga-card" on the command line, you'd get two graphic cards on your > machine, even if you use -nodefaults. At least some boards do "only create the default graphics type if vga_interface_type != VGA_NONE". Mostly what I would like here is consistency. But also, we haven't added a new item to the '-vga' option list since 2014, which makes me strongly suspect it's legacy and we should only be keeping it for backwards compatibility. > So there needs to be at least some logic dealing with vga_interface_type if > we want to be able to select a different graphics card for this machine. > Then why not go the full way and use pci_vga_init() here, too? ... that's > certainly the least confusing way for the users. Is it? From an Arm perspective, having "-vga" do anything at all is pretty confusing: it's a rather PC-centric option name. (Also pretty noticeable for the Sparc TCX/CG3 framebuffers, which are not VGA in any way.) thanks -- PMM
On 25/05/2023 12.44, Peter Maydell wrote: > On Thu, 25 May 2023 at 11:32, Thomas Huth <thuth@redhat.com> wrote: >> >> On 25/05/2023 12.05, Peter Maydell wrote: >>> On Tue, 23 May 2023 at 19:41, Thomas Huth <thuth@redhat.com> wrote: >>>> >>>> On 23/05/2023 19.30, Marcin Juszkiewicz wrote: >>>> ... >>>>>> (is there a reason why it can't be disabled with "-vga none" or >>>>>> "-nodefaults"?) >>>>> >>>>> That's something I need to check how it should be done. >>>> >>>> Other boards set mc->default_display in their ...class_init >>>> function and then use pci_vga_init() (or vga_interface_type) >>>> to instantiate their default display adapter ... however, that >>>> does not seem to support the bochs adapter yet (see >>>> vga_interfaces[] in softmmu/vl.c). >>>> >>>> Not sure whether it's worth the effort to extend vga_interfaces[] >>>> in vl.c >>> >>> Isn't that a legacy-command-line-option thing? I feel like >>> the recommended way to select a different graphics card >>> these days would be to use -device my-pci-vga-card ... >> >> "-vga" is kind of legacy, indeed, but currently the sbsa-ref hard-codes the >> graphics card to be always available, so if you add a "-device >> something-vga-card" on the command line, you'd get two graphic cards on your >> machine, even if you use -nodefaults. > > At least some boards do "only create the default graphics > type if vga_interface_type != VGA_NONE". > > Mostly what I would like here is consistency. But also, we > haven't added a new item to the '-vga' option list since > 2014, which makes me strongly suspect it's legacy and we > should only be keeping it for backwards compatibility. > >> So there needs to be at least some logic dealing with vga_interface_type if >> we want to be able to select a different graphics card for this machine. >> Then why not go the full way and use pci_vga_init() here, too? ... that's >> certainly the least confusing way for the users. > > Is it? From an Arm perspective, having "-vga" do anything > at all is pretty confusing: it's a rather PC-centric option name. > (Also pretty noticeable for the Sparc TCX/CG3 framebuffers, > which are not VGA in any way.) Ok, if this is rather an oddity on arm, then it's maybe better to do the "vga_interface_type != VGA_NONE" check instead. Thomas
On 25/05/2023 11:44, Peter Maydell wrote: > On Thu, 25 May 2023 at 11:32, Thomas Huth <thuth@redhat.com> wrote: >> >> On 25/05/2023 12.05, Peter Maydell wrote: >>> On Tue, 23 May 2023 at 19:41, Thomas Huth <thuth@redhat.com> wrote: >>>> >>>> On 23/05/2023 19.30, Marcin Juszkiewicz wrote: >>>> ... >>>>>> (is there a reason why it can't be disabled with "-vga none" or >>>>>> "-nodefaults"?) >>>>> >>>>> That's something I need to check how it should be done. >>>> >>>> Other boards set mc->default_display in their ...class_init >>>> function and then use pci_vga_init() (or vga_interface_type) >>>> to instantiate their default display adapter ... however, that >>>> does not seem to support the bochs adapter yet (see >>>> vga_interfaces[] in softmmu/vl.c). >>>> >>>> Not sure whether it's worth the effort to extend vga_interfaces[] >>>> in vl.c >>> >>> Isn't that a legacy-command-line-option thing? I feel like >>> the recommended way to select a different graphics card >>> these days would be to use -device my-pci-vga-card ... >> >> "-vga" is kind of legacy, indeed, but currently the sbsa-ref hard-codes the >> graphics card to be always available, so if you add a "-device >> something-vga-card" on the command line, you'd get two graphic cards on your >> machine, even if you use -nodefaults. > > At least some boards do "only create the default graphics > type if vga_interface_type != VGA_NONE". > > Mostly what I would like here is consistency. But also, we > haven't added a new item to the '-vga' option list since > 2014, which makes me strongly suspect it's legacy and we > should only be keeping it for backwards compatibility. > >> So there needs to be at least some logic dealing with vga_interface_type if >> we want to be able to select a different graphics card for this machine. >> Then why not go the full way and use pci_vga_init() here, too? ... that's >> certainly the least confusing way for the users. > > Is it? From an Arm perspective, having "-vga" do anything > at all is pretty confusing: it's a rather PC-centric option name. > (Also pretty noticeable for the Sparc TCX/CG3 framebuffers, > which are not VGA in any way.) Right. From the SPARC perspective it was added to allow the user to select either the TCX (default) or CG3 framebuffers from the command line. However I guess that shouldn't be needed anymore now that mc->default_display exists. Presumably there is now some kind of -global sun4m.default_display=cg3 command line option that could set the machine default_display property value instead? ATB, Mark.
On Thu, 25 May 2023 at 12:06, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 25/05/2023 11:44, Peter Maydell wrote: > > > On Thu, 25 May 2023 at 11:32, Thomas Huth <thuth@redhat.com> wrote: > > > >> So there needs to be at least some logic dealing with vga_interface_type if > >> we want to be able to select a different graphics card for this machine. > >> Then why not go the full way and use pci_vga_init() here, too? ... that's > >> certainly the least confusing way for the users. > > > > Is it? From an Arm perspective, having "-vga" do anything > > at all is pretty confusing: it's a rather PC-centric option name. > > (Also pretty noticeable for the Sparc TCX/CG3 framebuffers, > > which are not VGA in any way.) > > Right. From the SPARC perspective it was added to allow the user to select either the > TCX (default) or CG3 framebuffers from the command line. > > However I guess that shouldn't be needed anymore now that mc->default_display exists. > Presumably there is now some kind of -global sun4m.default_display=cg3 command line > option that could set the machine default_display property value instead? Maybe. Handling builtin default devices remains kind of awkward. But for this Arm board they're all just PCI cards, so the only thing we really need is a way to say "don't create that default device"... -- PMM
On 25/05/2023 13.39, Peter Maydell wrote: > On Thu, 25 May 2023 at 12:06, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> >> On 25/05/2023 11:44, Peter Maydell wrote: >> >>> On Thu, 25 May 2023 at 11:32, Thomas Huth <thuth@redhat.com> wrote: >>> >>>> So there needs to be at least some logic dealing with vga_interface_type if >>>> we want to be able to select a different graphics card for this machine. >>>> Then why not go the full way and use pci_vga_init() here, too? ... that's >>>> certainly the least confusing way for the users. >>> >>> Is it? From an Arm perspective, having "-vga" do anything >>> at all is pretty confusing: it's a rather PC-centric option name. >>> (Also pretty noticeable for the Sparc TCX/CG3 framebuffers, >>> which are not VGA in any way.) >> >> Right. From the SPARC perspective it was added to allow the user to select either the >> TCX (default) or CG3 framebuffers from the command line. >> >> However I guess that shouldn't be needed anymore now that mc->default_display exists. >> Presumably there is now some kind of -global sun4m.default_display=cg3 command line >> option that could set the machine default_display property value instead? > > Maybe. Handling builtin default devices remains kind of awkward. > But for this Arm board they're all just PCI cards, so the > only thing we really need is a way to say "don't create that > default device"... I wonder whether we could deprecate and finally remove "-vga" ... there is also the "graphics" machine property that is used by some boards instead, so maybe we could use that as a replacement for "-vga none" everywhere (and use "-device xxx" as replacement for "vga xxx" of course). Thoughts? Thomas
diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst index b499d7e927..fea4992df2 100644 --- a/docs/system/arm/sbsa.rst +++ b/docs/system/arm/sbsa.rst @@ -27,6 +27,6 @@ The sbsa-ref board supports: - System bus EHCI controller - CDROM and hard disc on AHCI bus - E1000E ethernet card on PCIe bus - - VGA display adaptor on PCIe bus + - Bochs display adaptor on PCIe bus - A generic SBSA watchdog device
We moved from VGA to Bochs to have PCIe card. Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org> --- docs/system/arm/sbsa.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)