diff mbox series

[1/2] docs: sbsa: correct graphics card name

Message ID 20230523155644.678524-1-marcin.juszkiewicz@linaro.org
State New
Headers show
Series [1/2] docs: sbsa: correct graphics card name | expand

Commit Message

Marcin Juszkiewicz May 23, 2023, 3:56 p.m. UTC
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(-)

Comments

Thomas Huth May 23, 2023, 5:11 p.m. UTC | #1
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"?)
Marcin Juszkiewicz May 23, 2023, 5:30 p.m. UTC | #2
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?
Thomas Huth May 23, 2023, 6:41 p.m. UTC | #3
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
Peter Maydell May 25, 2023, 10:05 a.m. UTC | #4
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
Thomas Huth May 25, 2023, 10:32 a.m. UTC | #5
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
Peter Maydell May 25, 2023, 10:44 a.m. UTC | #6
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
Thomas Huth May 25, 2023, 10:53 a.m. UTC | #7
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
Mark Cave-Ayland May 25, 2023, 11:06 a.m. UTC | #8
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.
Peter Maydell May 25, 2023, 11:39 a.m. UTC | #9
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
Thomas Huth May 25, 2023, 11:49 a.m. UTC | #10
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 mbox series

Patch

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