Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/2] vga: don't pick cirrus by default

[Eduardo Habkost <ehabkost@redhat.com>]

On Thu, Jul 12, 2018 at 06:57:46AM +0200, Thomas Huth wrote:
> On 11.07.2018 20:43, Eduardo Habkost wrote:
> > On Wed, Jul 11, 2018 at 07:00:54PM +0200, Sebastian Bauer wrote:
> >> Am 2018-07-11 17:48, schrieb Eduardo Habkost:
> >>> "none" looked like a false positive when I first looked, but now
> >>> I think it's not.  Shouldn't it set default_display="none"?
> >>
> >> I think that there is some other logic burried that these machine doesn't
> >> get a graphics display. But overall it is indeed not clearly defined.
> >>
> >> But see below.
> >>
> >>>> If the patch is applied to 3.1 then I think there is enough time to
> >>>> fix
> >>>> issues caused by the patch. Additionally, a warning could be put in
> >>>> the
> >>>> ChangeLog for 3.0 that in 3.1 that the default mode will be std unless
> >>>> machines define an own default. This is should be enough time for
> >>>> people to
> >>>> complain or to fix things.
> >>> I don't think we will really make user-visible changes: we can
> >>> simply work to keep existing behavior, but the difference is that
> >>> this will be implemented by setting default_display explicitly on
> >>> all machines.
> >>
> >> Even if all machines were using explicit default settings the patch will
> >> affect machines that are not inside the QEMU tree. If the patch is to be
> >> applied as it is these are affected. To warn users (or devs in this case)
> >> about this, an entry in the ChangeLog would be appropriate.
> > 
> > What do you mean by "machines that are not inside the QEMU tree"?
> > MachineClass registration is not an API for external use.
> > 
> >>
> >>>> If the patch is to be applied to 3.0 then all non-ppc ones need to be
> >>>> reconsidered.
> >>>> The "important" ppc machines have been fixed already. I can do the
> >>>> remaining
> >>>> if this is wanted.
> >>> This part worries me: do we have other machines that are broken
> >>> right now?
> >>
> >> I don't know which of them are broken or how this can be elaborated, but
> >> they are potentially affected. For instance, the sam460ex platform doesn't
> >> care about this setting, I can say that it is not broken. Other platforms
> >> like the mac apparently were broken (and fixed in the meantime). It is hard
> >> to tell which of them are really broken without someone that knows the
> >> platform trying it and telling it. 'Make check' did catch only one single
> >> case. It could also be that nobody cares about other affected machines.
> >>
> >> Overall I think the patch is an improvement over the previous state as
> >> preferring the Cirrus doesn't make much sense if most machines don't prefer
> >> it. The more I think over it, the more I think that the concept needs
> >> further fine-tuning though (not necessarily in this patch).
> > 
> > I'm not sure yet if most machines with default_display==NULL
> > don't want Cirrus.  I'm also unsure if any of the machines from
> > that list will break if we start using VGA by default.
> 
> I think most machines with default_display == NULL simply do not want
> any graphic cards at all. So we likely document that default_display ==
> NULL means no graphic card, and fix the machines that have other
> assumptions if necessary.

We already have a value meaning "no graphics card": "none".

But like with the "cirrus" and "vga" options, I also don't know
if any of the existing machines with default_display==NULL will
break if we start using "none" by default.

> 
> >>                     [...]  I still think that the most common value can be a
> >> default (strictly bailing out when it is not available unlike it is done
> >> now), but this is a matter of taste I guess.
> > 
> > This is doable too, if we have a clear consensus on what would be
> > a reasonable default on TYPE_MACHINE.  Personally I prefer the
> > default on TYPE_MACHINE to be "none".
> 
> Yes, please, no magic default hardware for all machines. That will only
> cause confusion and problems in the future. People who add new boards
> will keep forgetting to set a value in their new boards and then you
> later wonder whether that was on purpose or by accident ==> If there is
> no default display set, this should simply mean "no display".

Agreed.

-- 
Eduardo