Re: [PATCH 00/67] Make pixman an optional dependency

[Daniel P . Berrangé]

On Wed, Aug 30, 2023 at 03:01:27PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Aug 30, 2023 at 2:53 PM Thomas Huth <thuth@redhat.com> wrote:
> >
> > On 30/08/2023 11.37, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Hi,
> > >
> > > QEMU system emulators can be made to compile and work without pixman.
> > >
> > > Given how pervasively pixman types and API is used in all the code base, 
> > > it was
> > > a bit difficult to figure out how to cut the dependency.
> > >
> > > I decided that it was important to keep VGA and graphics device working 
> > > for
> > > compatibility reasons, although some devices, such as xlnx Display Port, 
> > > have
> > > stronger dependency and have to be disabled. The ui/console code also has 
> > > a lot
> > > of pixman usage and a bit of a mess to deal with. I made large 
> > > refactoring to
> > > allow to compile out the VC code.
> > >
> > > The series can be roughly divded as:
> > > - a few related preliminary cleanups
> > > - ui/console refactoring to allow ui/vc split
> > > - add a 'pixman' option, and a minimal pixman-compat.h
> > > - make some parts depend on 'pixman'
> > >
> > > Graphic -display still work, although with some caveats. For ex, -display 
> > > sdl or
> > > cocoa don't have VCs, so starting QEMU will print the following warnings 
> > > when
> > > pixman is disabled:
> >
> > I just had a quick look at the series, but for me it looks like this is
> > adding a lot of additional complexity to the code (adds lots of #ifdefs, and
> > adds a subset of the pixman library to the code base), which seems somewhat
> 
> The #ifdef aren't so bad (at least I can bare it). Just a quick stat:
> 
> $ git diff origin/master | grep "+#ifdef CONFIG_PIXMAN" | wc -l
> 11
> 
> > unfortunate for such a marginal feature request. What's really so bad about
> > requiring pixman for building QEMU?
> 
> Not that a good part of the series is cleaning up ui/console.c code
> that really deserved it. It makes it use QOM, and split VC code. It's
> still worth it regardless of the outcome for pixman.

I've done a review of the start and like the cleanup patches, and the
adaption to make sane use of inheritance in QOM rather than the poor
man's inheritance via the console type field. I agree that's worthwhile
regardless of what we think about pixman optionality.

> > IMHO, if we really want to go down this path, I think we should rather
> > disable all graphic related stuff in QEMU instead, i.e. disable VGA cards,
> > Spice, SDL, etc. completely. I think this is also what has been requested in
> >
> 
> The various features and devices can be disabled by other means. I
> think we should aim at making the different configure options
> orthogonal, so QEMU without pixman can still provide most gfx/vga/UI
> experience too, by default.

To me where this series becomes dubious is roughly around the patch
that adds pixman-compat.h providing a bunch of pixman API as stubs.

If we can use Kconfig and/or meson options to simply drop the build
of files in QEMU that require pixman that's reasonable. If we're
adding compat APIs the provide local impls of pixman APIs it feels
wrong to me.

> > the original gitlab issue ticket where the reporter wanted to compile a
> > text-mode only QEMU binary...?
> 
> So that is not an incompatible goal, further tuning of configure
> options can help.



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|