[Qemu-devel] [PATCH v2 0/4] sdl: Let it be optional (in particular, on OpenBSD)

[Qemu-devel] [PATCH v2 0/4] sdl: Let it be optional (in particular, on OpenBSD)

[Philippe Mathieu-Daudé]

v1 was too simple to work, so here we go again.
I hit a problem with the Milkymist TMU device when disabling SDL,
so I fixed it and added another patch in this series which clean
a bit the device, but is not required for this series.
The important patches are 1-3, and eventually 1-2 could be squashed
altogether, although the changes are slighly different.

Regards,

Phil.

v1 https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05509.html

Philippe Mathieu-Daudé (4):
  configure: LM32 Milkymist Texture Mapping Unit (tmu2) also depends of
    X11
  hw/display/milkymist-tmu2: Explicit the dependency to both X11 /
    OpenGL
  configure: Let SDL support be optional on OpenBSD
  hw/display/milkymist-tmu2: Move inlined code from header to source

 MAINTAINERS                         |  1 +
 configure                           | 16 +++++++-
 default-configs/lm32-softmmu.mak    |  2 +-
 hw/display/Makefile.objs            |  4 +-
 hw/display/milkymist-tmu2.c         | 49 ++++++++++++++++++++++
 hw/lm32/milkymist-hw.h              | 63 -----------------------------
 hw/lm32/milkymist.c                 |  1 +
 include/hw/display/milkymist_tmu2.h | 41 +++++++++++++++++++
 8 files changed, 110 insertions(+), 67 deletions(-)
 create mode 100644 include/hw/display/milkymist_tmu2.h

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/4] configure: LM32 Milkymist Texture Mapping Unit (tmu2) also depends of X11

[Philippe Mathieu-Daudé]

Commit 5f9b1e35060b8 remove the dependency between OpenGL and X11.
However the milkymist-tmu2 device do require X11.
When using SDL, the configure script sets need_x11=yes, so the X11
flags are populated to the makefiles.
When building without SDL, X11 is not pulled and populated, leading
to a link failure:

    LINK    lm32-softmmu/qemu-system-lm32
  hw/lm32/milkymist.o: In function `milkymist_tmu2_create':
  hw/lm32/milkymist-hw.h:114: undefined reference to `XOpenDisplay'
  hw/lm32/milkymist-hw.h:140: undefined reference to `XFree'
  hw/lm32/milkymist-hw.h:141: undefined reference to `XCloseDisplay'
  hw/lm32/milkymist-hw.h:130: undefined reference to `XCloseDisplay'
  ../hw/display/milkymist-tmu2.o: In function `tmu2_glx_init':
  hw/display/milkymist-tmu2.c:112: undefined reference to `XOpenDisplay'
  hw/display/milkymist-tmu2.c:123: undefined reference to `XFree'
  collect2: error: ld returned 1 exit status
  gmake[1]: *** [Makefile:199: qemu-system-lm32] Error 1

Enforce the X11 dependency when the LM32 target is built.
This will allow us to build QEMU without SDL.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 configure | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/configure b/configure
index 8f312ac3e2..d50defceb7 100755
--- a/configure
+++ b/configure
@@ -4038,6 +4038,16 @@ EOF
   fi
 fi
 
+if test "$opengl" = "yes" -a "$have_x11" = "yes"; then
+  for target in $target_list; do
+    case $target in
+      lm32-softmmu) # milkymist-tmu2 requires X11 and OpenGL
+        need_x11=yes
+      ;;
+    esac
+  done
+fi
+
 ##########################################
 # libxml2 probe
 if test "$libxml2" != "no" ; then
-- 
2.20.1

[Qemu-devel] [PATCH v2 2/4] hw/display/milkymist-tmu2: Explicit the dependency to both X11 / OpenGL

[Philippe Mathieu-Daudé]

The TMU device requires both X11 and OpenGL.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 default-configs/lm32-softmmu.mak | 2 +-
 hw/display/Makefile.objs         | 4 ++--
 hw/lm32/milkymist-hw.h           | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/default-configs/lm32-softmmu.mak b/default-configs/lm32-softmmu.mak
index 4889348a10..4049b23562 100644
--- a/default-configs/lm32-softmmu.mak
+++ b/default-configs/lm32-softmmu.mak
@@ -2,7 +2,7 @@
 
 CONFIG_LM32=y
 CONFIG_MILKYMIST=y
-CONFIG_MILKYMIST_TMU2=$(CONFIG_OPENGL)
+CONFIG_MILKYMIST_TMU2=$(call land,$(CONFIG_X11),$(CONFIG_OPENGL))
 CONFIG_FRAMEBUFFER=y
 CONFIG_PTIMER=y
 CONFIG_PFLASH_CFI01=y
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index 97acd5b6cb..079e702f25 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -29,8 +29,8 @@ common-obj-$(CONFIG_MILKYMIST) += milkymist-vgafb.o
 common-obj-$(CONFIG_ZAURUS) += tc6393xb.o
 
 common-obj-$(CONFIG_MILKYMIST_TMU2) += milkymist-tmu2.o
-milkymist-tmu2.o-cflags := $(X11_CFLAGS)
-milkymist-tmu2.o-libs := $(X11_LIBS)
+milkymist-tmu2.o-cflags := $(X11_CFLAGS) $(OPENGL_CFLAGS)
+milkymist-tmu2.o-libs := $(X11_LIBS) $(OPENGL_LIBS)
 
 obj-$(CONFIG_OMAP) += omap_dss.o
 obj-$(CONFIG_OMAP) += omap_lcdc.o
diff --git a/hw/lm32/milkymist-hw.h b/hw/lm32/milkymist-hw.h
index d3be0cfb3a..32c344ef9f 100644
--- a/hw/lm32/milkymist-hw.h
+++ b/hw/lm32/milkymist-hw.h
@@ -88,7 +88,7 @@ static inline DeviceState *milkymist_pfpu_create(hwaddr base,
     return dev;
 }
 
-#ifdef CONFIG_OPENGL
+#if defined(CONFIG_X11) && defined(CONFIG_OPENGL)
 #include <X11/Xlib.h>
 #include <epoxy/gl.h>
 #include <epoxy/glx.h>
@@ -103,7 +103,7 @@ static const int glx_fbconfig_attr[] = {
 static inline DeviceState *milkymist_tmu2_create(hwaddr base,
         qemu_irq irq)
 {
-#ifdef CONFIG_OPENGL
+#if defined(CONFIG_X11) && defined(CONFIG_OPENGL)
     DeviceState *dev;
     Display *d;
     GLXFBConfig *configs;
-- 
2.20.1

[Qemu-devel] [PATCH v2 3/4] configure: Let SDL support be optional on OpenBSD

[Philippe Mathieu-Daudé]

Currently if we try to build QEMU on OpenBSD with SDL disabled, we get:

  $ ./configure --cc=x86_64-unknown-openbsd6.1-gcc-4.9.4 --python=python2.7 --disable-sdl

  ERROR: sdl not found or disabled, can not use sdl audio driver

Since SDL is not a requirement for OpenBSD, let it be optional.
If it is not found, or the user explicitly disable it, remove it
from the audio_drv_list.
If no audio backends are available, QEMU will fall back to the
null driver.
Instead of displaying nothing when audio_drv_list ends up empty,
display "none".

This does not change the default behavior:

  $ ./configure --cc=x86_64-unknown-openbsd6.1-gcc-4.9.4 --python=python2.7
  SDL support       yes (1.2.15)
  Audio drivers     sdl

  WARNING: Use of SDL 1.2 is deprecated and will be removed in
  WARNING: future releases. Please switch to using SDL 2.0

  GEN     config-host.h
  ...

but allows to build without SDL:

  $ ./configure --cc=x86_64-unknown-openbsd6.1-gcc-4.9.4 --python=python2.7 --disable-sdl
  WARN: sdl not found or disabled, can not use sdl audio driver
  SDL support       no
  Audio drivers     none
  GEN     config-host.h
  ...

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 configure | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index d50defceb7..fa444e9921 100755
--- a/configure
+++ b/configure
@@ -3379,7 +3379,8 @@ for drv in $audio_drv_list; do
 
     sdl)
     if test "$sdl" = "no"; then
-        error_exit "sdl not found or disabled, can not use sdl audio driver"
+        echo "WARN: sdl not found or disabled, can not use sdl audio driver"
+        audio_drv_list=$(echo "$audio_drv_list" | sed -e 's/sdl *//g')
     fi
     ;;
 
@@ -3408,6 +3409,9 @@ for drv in $audio_drv_list; do
     ;;
     esac
 done
+if test -z "$audio_drv_list"; then
+    audio_drv_list="none"
+fi
 
 ##########################################
 # BrlAPI probe
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 3/4] configure: Let SDL support be optional on OpenBSD

[Gerd Hoffmann]

>      sdl)
>      if test "$sdl" = "no"; then
> -        error_exit "sdl not found or disabled, can not use sdl audio driver"
> +        echo "WARN: sdl not found or disabled, can not use sdl audio driver"
> +        audio_drv_list=$(echo "$audio_drv_list" | sed -e 's/sdl *//g')

Busy tackling that in a separate patch series (audio: rework driver
probing)

>      fi
>      ;;
>  
> @@ -3408,6 +3409,9 @@ for drv in $audio_drv_list; do
>      ;;
>      esac
>  done
> +if test -z "$audio_drv_list"; then
> +    audio_drv_list="none"
> +fi

Not needed, "none" is used as fallback even without this.

The other tree patches look fine to me, I'll go queue them up.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 3/4] configure: Let SDL support be optional on OpenBSD

[Philippe Mathieu-Daudé]

On 1/24/19 12:23 PM, Gerd Hoffmann wrote:
>>      sdl)
>>      if test "$sdl" = "no"; then
>> -        error_exit "sdl not found or disabled, can not use sdl audio driver"
>> +        echo "WARN: sdl not found or disabled, can not use sdl audio driver"
>> +        audio_drv_list=$(echo "$audio_drv_list" | sed -e 's/sdl *//g')
> 
> Busy tackling that in a separate patch series (audio: rework driver
> probing)

But we need the Milkymist TMU fixes to build without SDL,
I still get this error when building the series you mentioned using
--disable-sdl on OpenBSD:

    LINK    lm32-softmmu/qemu-system-lm32
  hw/lm32/milkymist.o: In function `milkymist_tmu2_create':
  hw/lm32/milkymist-hw.h:114: undefined reference to `XOpenDisplay'
  hw/lm32/milkymist-hw.h:140: undefined reference to `XFree'
  hw/lm32/milkymist-hw.h:141: undefined reference to `XCloseDisplay'
  hw/lm32/milkymist-hw.h:130: undefined reference to `XCloseDisplay'
  ../hw/display/milkymist-tmu2.o: In function `tmu2_glx_init':
  hw/display/milkymist-tmu2.c:112: undefined reference to `XOpenDisplay'
  hw/display/milkymist-tmu2.c:123: undefined reference to `XFree'
  collect2: error: ld returned 1 exit status
  gmake[1]: *** [Makefile:199: qemu-system-lm32] Error 1

> 
>>      fi
>>      ;;
>>  
>> @@ -3408,6 +3409,9 @@ for drv in $audio_drv_list; do
>>      ;;
>>      esac
>>  done
>> +if test -z "$audio_drv_list"; then
>> +    audio_drv_list="none"
>> +fi
> 
> Not needed, "none" is used as fallback even without this.

It is not needed indeed, but I find it clearer when looking at the
./configure output, rather than having an empty list:

  SDL support       no
  Audio drivers
  GEN     config-host.h

vs:

  SDL support       no
  Audio drivers     none
  GEN     config-host.h

Maybe my error was to not clarify that this change is purely cosmetic.

Your call anyway.

> 
> The other tree patches look fine to me, I'll go queue them up.

Thanks! I'll wait for Michael Walle feedbacks before respining rebased
on your work, also fixing "WARN".

> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [PATCH v2 3/4] configure: Let SDL support be optional on OpenBSD

[Gerd Hoffmann]

On Thu, Jan 24, 2019 at 02:51:08PM +0100, Philippe Mathieu-Daudé wrote:
> On 1/24/19 12:23 PM, Gerd Hoffmann wrote:
> >>      sdl)
> >>      if test "$sdl" = "no"; then
> >> -        error_exit "sdl not found or disabled, can not use sdl audio driver"
> >> +        echo "WARN: sdl not found or disabled, can not use sdl audio driver"
> >> +        audio_drv_list=$(echo "$audio_drv_list" | sed -e 's/sdl *//g')
> > 
> > Busy tackling that in a separate patch series (audio: rework driver
> > probing)
> 
> But we need the Milkymist TMU fixes to build without SDL,
> I still get this error when building the series you mentioned using
> --disable-sdl on OpenBSD:

Yep.  Try https://git.kraxel.org/cgit/qemu/log/?h=sirius/drop-sdl1

That is the audio series and the first two patches of this series plus
Daniel's sdl1 removal patch rebased.  Looks good so far, need to run
some more build tests though.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 3/4] configure: Let SDL support be optional on OpenBSD

[Peter Maydell]

On Thu, 24 Jan 2019 at 01:16, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> --- a/configure
> +++ b/configure
> @@ -3379,7 +3379,8 @@ for drv in $audio_drv_list; do
>
>      sdl)
>      if test "$sdl" = "no"; then
> -        error_exit "sdl not found or disabled, can not use sdl audio driver"
> +        echo "WARN: sdl not found or disabled, can not use sdl audio driver"
> +        audio_drv_list=$(echo "$audio_drv_list" | sed -e 's/sdl *//g')

"WARNING:" is what configure mostly uses (though sometimes
also "warning:" and in one case "WARN:")...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/4] configure: Let SDL support be optional on OpenBSD

[Philippe Mathieu-Daudé]

On 1/24/19 12:44 PM, Peter Maydell wrote:
> On Thu, 24 Jan 2019 at 01:16, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> --- a/configure
>> +++ b/configure
>> @@ -3379,7 +3379,8 @@ for drv in $audio_drv_list; do
>>
>>      sdl)
>>      if test "$sdl" = "no"; then
>> -        error_exit "sdl not found or disabled, can not use sdl audio driver"
>> +        echo "WARN: sdl not found or disabled, can not use sdl audio driver"
>> +        audio_drv_list=$(echo "$audio_drv_list" | sed -e 's/sdl *//g')
> 
> "WARNING:" is what configure mostly uses (though sometimes
> also "warning:" and in one case "WARN:")...

Indeed, which makes me wonder why I used "WARN".

I'll fix, thanks.

Phil.

[Qemu-devel] [PATCH v2 4/4] hw/display/milkymist-tmu2: Move inlined code from header to source

[Philippe Mathieu-Daudé]

Move the complexity of milkymist_tmu2_create() into the
source file. Doing so we avoid to include the X11/OpenGL
headers in all LM32 devices, and we also avoid the duplicate
declaration of glx_fbconfig_attr[] (it is already declared
in hw/display/milkymist-tmu2.c).
Since TYPE_MILKYMIST_TMU2 is now accessible, use it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS                         |  1 +
 hw/display/milkymist-tmu2.c         | 49 ++++++++++++++++++++++
 hw/lm32/milkymist-hw.h              | 63 -----------------------------
 hw/lm32/milkymist.c                 |  1 +
 include/hw/display/milkymist_tmu2.h | 41 +++++++++++++++++++
 5 files changed, 92 insertions(+), 63 deletions(-)
 create mode 100644 include/hw/display/milkymist_tmu2.h

diff --git a/MAINTAINERS b/MAINTAINERS
index af339b86db..3c040f21e7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -185,6 +185,7 @@ F: disas/lm32.c
 F: hw/lm32/
 F: hw/*/lm32_*
 F: hw/*/milkymist-*
+F: include/hw/display/milkymist_tmu2.h
 F: include/hw/char/lm32_juart.h
 F: include/hw/lm32/
 F: tests/tcg/lm32/
diff --git a/hw/display/milkymist-tmu2.c b/hw/display/milkymist-tmu2.c
index 3ce44fdfce..b33fc234e9 100644
--- a/hw/display/milkymist-tmu2.c
+++ b/hw/display/milkymist-tmu2.c
@@ -31,6 +31,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
+#include "hw/display/milkymist_tmu2.h"
 
 #include <X11/Xlib.h>
 #include <epoxy/gl.h>
@@ -499,3 +500,51 @@ static void milkymist_tmu2_register_types(void)
 }
 
 type_init(milkymist_tmu2_register_types)
+
+DeviceState *milkymist_tmu2_create(hwaddr base, qemu_irq irq)
+{
+    DeviceState *dev;
+    Display *d;
+    GLXFBConfig *configs;
+    int nelements;
+    int ver_major, ver_minor;
+
+    /* check that GLX will work */
+    d = XOpenDisplay(NULL);
+    if (d == NULL) {
+        return NULL;
+    }
+
+    if (!glXQueryVersion(d, &ver_major, &ver_minor)) {
+        /*
+         * Yeah, sometimes getting the GLX version can fail.
+         * Isn't X beautiful?
+         */
+        XCloseDisplay(d);
+        return NULL;
+    }
+
+    if ((ver_major < 1) || ((ver_major == 1) && (ver_minor < 3))) {
+        printf("Your GLX version is %d.%d,"
+          "but TMU emulation needs at least 1.3. TMU disabled.\n",
+          ver_major, ver_minor);
+        XCloseDisplay(d);
+        return NULL;
+    }
+
+    configs = glXChooseFBConfig(d, 0, glx_fbconfig_attr, &nelements);
+    if (configs == NULL) {
+        XCloseDisplay(d);
+        return NULL;
+    }
+
+    XFree(configs);
+    XCloseDisplay(d);
+
+    dev = qdev_create(NULL, TYPE_MILKYMIST_TMU2);
+    qdev_init_nofail(dev);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
+
+    return dev;
+}
diff --git a/hw/lm32/milkymist-hw.h b/hw/lm32/milkymist-hw.h
index 32c344ef9f..976cf9254d 100644
--- a/hw/lm32/milkymist-hw.h
+++ b/hw/lm32/milkymist-hw.h
@@ -88,69 +88,6 @@ static inline DeviceState *milkymist_pfpu_create(hwaddr base,
     return dev;
 }
 
-#if defined(CONFIG_X11) && defined(CONFIG_OPENGL)
-#include <X11/Xlib.h>
-#include <epoxy/gl.h>
-#include <epoxy/glx.h>
-static const int glx_fbconfig_attr[] = {
-    GLX_GREEN_SIZE, 5,
-    GLX_GREEN_SIZE, 6,
-    GLX_BLUE_SIZE, 5,
-    None
-};
-#endif
-
-static inline DeviceState *milkymist_tmu2_create(hwaddr base,
-        qemu_irq irq)
-{
-#if defined(CONFIG_X11) && defined(CONFIG_OPENGL)
-    DeviceState *dev;
-    Display *d;
-    GLXFBConfig *configs;
-    int nelements;
-    int ver_major, ver_minor;
-
-    /* check that GLX will work */
-    d = XOpenDisplay(NULL);
-    if (d == NULL) {
-        return NULL;
-    }
-
-    if (!glXQueryVersion(d, &ver_major, &ver_minor)) {
-        /* Yeah, sometimes getting the GLX version can fail.
-         * Isn't X beautiful? */
-        XCloseDisplay(d);
-        return NULL;
-    }
-
-    if ((ver_major < 1) || ((ver_major == 1) && (ver_minor < 3))) {
-        printf("Your GLX version is %d.%d,"
-          "but TMU emulation needs at least 1.3. TMU disabled.\n",
-          ver_major, ver_minor);
-        XCloseDisplay(d);
-        return NULL;
-    }
-
-    configs = glXChooseFBConfig(d, 0, glx_fbconfig_attr, &nelements);
-    if (configs == NULL) {
-        XCloseDisplay(d);
-        return NULL;
-    }
-
-    XFree(configs);
-    XCloseDisplay(d);
-
-    dev = qdev_create(NULL, "milkymist-tmu2");
-    qdev_init_nofail(dev);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
-    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
-
-    return dev;
-#else
-    return NULL;
-#endif
-}
-
 static inline DeviceState *milkymist_ac97_create(hwaddr base,
         qemu_irq crrequest_irq, qemu_irq crreply_irq, qemu_irq dmar_irq,
         qemu_irq dmaw_irq)
diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
index 63c6894c95..26a2398354 100644
--- a/hw/lm32/milkymist.c
+++ b/hw/lm32/milkymist.c
@@ -32,6 +32,7 @@
 #include "hw/loader.h"
 #include "elf.h"
 #include "milkymist-hw.h"
+#include "hw/display/milkymist_tmu2.h"
 #include "lm32.h"
 #include "exec/address-spaces.h"
 
diff --git a/include/hw/display/milkymist_tmu2.h b/include/hw/display/milkymist_tmu2.h
new file mode 100644
index 0000000000..148a119a1d
--- /dev/null
+++ b/include/hw/display/milkymist_tmu2.h
@@ -0,0 +1,41 @@
+/*
+ *  QEMU model of the Milkymist texture mapping unit.
+ *
+ *  Copyright (c) 2010 Michael Walle <michael@walle.cc>
+ *  Copyright (c) 2010 Sebastien Bourdeauducq
+ *                       <sebastien.bourdeauducq@lekernel.net>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ *
+ * Specification available at:
+ *   http://milkymist.walle.cc/socdoc/tmu2.pdf
+ *
+ */
+
+#ifndef HW_DISPLAY_MILKYMIST_TMU2_H
+#define HW_DISPLAY_MILKYMIST_TMU2_H
+
+#include "hw/qdev.h"
+
+#if defined(CONFIG_X11) && defined(CONFIG_OPENGL)
+DeviceState *milkymist_tmu2_create(hwaddr base, qemu_irq irq);
+#else
+static inline DeviceState *milkymist_tmu2_create(hwaddr base, qemu_irq irq)
+{
+    return NULL;
+}
+#endif
+
+#endif /* HW_DISPLAY_MILKYMIST_TMU2_H */
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 4/4] hw/display/milkymist-tmu2: Move inlined code from header to source

[Gerd Hoffmann]

On Thu, Jan 24, 2019 at 02:15:54AM +0100, Philippe Mathieu-Daudé wrote:
> Move the complexity of milkymist_tmu2_create() into the
> source file. Doing so we avoid to include the X11/OpenGL
> headers in all LM32 devices, and we also avoid the duplicate
> declaration of glx_fbconfig_attr[] (it is already declared
> in hw/display/milkymist-tmu2.c).
> Since TYPE_MILKYMIST_TMU2 is now accessible, use it.

Oops, fails the build:

  LINK    lm32-softmmu/qemu-system-lm32
hw/lm32/milkymist.o: In function `milkymist_init':
milkymist.c:(.text+0xb0f): undefined reference to `milkymist_tmu2_create'

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 4/4] hw/display/milkymist-tmu2: Move inlined code from header to source

[Philippe Mathieu-Daudé]

On 1/24/19 12:43 PM, Gerd Hoffmann wrote:
> On Thu, Jan 24, 2019 at 02:15:54AM +0100, Philippe Mathieu-Daudé wrote:
>> Move the complexity of milkymist_tmu2_create() into the
>> source file. Doing so we avoid to include the X11/OpenGL
>> headers in all LM32 devices, and we also avoid the duplicate
>> declaration of glx_fbconfig_attr[] (it is already declared
>> in hw/display/milkymist-tmu2.c).
>> Since TYPE_MILKYMIST_TMU2 is now accessible, use it.
> 
> Oops, fails the build:
> 
>   LINK    lm32-softmmu/qemu-system-lm32
> hw/lm32/milkymist.o: In function `milkymist_init':
> milkymist.c:(.text+0xb0f): undefined reference to `milkymist_tmu2_create'

Funny, I was busy testing your other audio series while you were testing
mine :)

I'll have a look, thanks.

Phil.

Re: [Qemu-devel] [PATCH v2 4/4] hw/display/milkymist-tmu2: Move inlined code from header to source

[Philippe Mathieu-Daudé]

Cc'ing Thomas/Paolo for Makefile rules...

On 1/24/19 12:43 PM, Gerd Hoffmann wrote:
> On Thu, Jan 24, 2019 at 02:15:54AM +0100, Philippe Mathieu-Daudé wrote:
>> Move the complexity of milkymist_tmu2_create() into the
>> source file. Doing so we avoid to include the X11/OpenGL
>> headers in all LM32 devices, and we also avoid the duplicate
>> declaration of glx_fbconfig_attr[] (it is already declared
>> in hw/display/milkymist-tmu2.c).
>> Since TYPE_MILKYMIST_TMU2 is now accessible, use it.
> 
> Oops, fails the build:
> 
>   LINK    lm32-softmmu/qemu-system-lm32
> hw/lm32/milkymist.o: In function `milkymist_init':
> milkymist.c:(.text+0xb0f): undefined reference to `milkymist_tmu2_create'

The problem comes from patch #2:

> diff --git a/default-configs/lm32-softmmu.mak
b/default-configs/lm32-softmmu.mak
> index 4889348a10..4049b23562 100644
> --- a/default-configs/lm32-softmmu.mak
> +++ b/default-configs/lm32-softmmu.mak
> @@ -2,7 +2,7 @@
>
>  CONFIG_LM32=y
>  CONFIG_MILKYMIST=y
> -CONFIG_MILKYMIST_TMU2=$(CONFIG_OPENGL)
> +CONFIG_MILKYMIST_TMU2=$(call land,$(CONFIG_X11),$(CONFIG_OPENGL))
>  CONFIG_FRAMEBUFFER=y
>  CONFIG_PTIMER=y
>  CONFIG_PFLASH_CFI01=y
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index 97acd5b6cb..079e702f25 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -29,8 +29,8 @@ common-obj-$(CONFIG_MILKYMIST) += milkymist-vgafb.o
>  common-obj-$(CONFIG_ZAURUS) += tc6393xb.o
>
>  common-obj-$(CONFIG_MILKYMIST_TMU2) += milkymist-tmu2.o
> -milkymist-tmu2.o-cflags := $(X11_CFLAGS)
> -milkymist-tmu2.o-libs := $(X11_LIBS)
> +milkymist-tmu2.o-cflags := $(X11_CFLAGS) $(OPENGL_CFLAGS)
> +milkymist-tmu2.o-libs := $(X11_LIBS) $(OPENGL_LIBS)
>
>  obj-$(CONFIG_OMAP) += omap_dss.o
>  obj-$(CONFIG_OMAP) += omap_lcdc.o

Using $(call land) seems to break CONFIG_MILKYMIST_TMU2 availability in
$(common-obj), while it works correctly in the per-target $(obj).
I'm not sure what is the cause, but moving milkymist-tmu2.o to $(obj)
makes more sense and fix this, so I'll go this way.

Regards,

Phil.

Re: [Qemu-devel] [PATCH v2 4/4] hw/display/milkymist-tmu2: Move inlined code from header to source

[Peter Maydell]

On Mon, 28 Jan 2019 at 17:47, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Cc'ing Thomas/Paolo for Makefile rules...
>
> On 1/24/19 12:43 PM, Gerd Hoffmann wrote:
> > Oops, fails the build:
> >
> >   LINK    lm32-softmmu/qemu-system-lm32
> > hw/lm32/milkymist.o: In function `milkymist_init':
> > milkymist.c:(.text+0xb0f): undefined reference to `milkymist_tmu2_create'
>
> The problem comes from patch #2:
>
> > diff --git a/default-configs/lm32-softmmu.mak
> b/default-configs/lm32-softmmu.mak
> > index 4889348a10..4049b23562 100644
> > --- a/default-configs/lm32-softmmu.mak
> > +++ b/default-configs/lm32-softmmu.mak
> > @@ -2,7 +2,7 @@
> >
> >  CONFIG_LM32=y
> >  CONFIG_MILKYMIST=y
> > -CONFIG_MILKYMIST_TMU2=$(CONFIG_OPENGL)
> > +CONFIG_MILKYMIST_TMU2=$(call land,$(CONFIG_X11),$(CONFIG_OPENGL))
> >  CONFIG_FRAMEBUFFER=y
> >  CONFIG_PTIMER=y
> >  CONFIG_PFLASH_CFI01=y
> > diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> > index 97acd5b6cb..079e702f25 100644
> > --- a/hw/display/Makefile.objs
> > +++ b/hw/display/Makefile.objs
> > @@ -29,8 +29,8 @@ common-obj-$(CONFIG_MILKYMIST) += milkymist-vgafb.o
> >  common-obj-$(CONFIG_ZAURUS) += tc6393xb.o
> >
> >  common-obj-$(CONFIG_MILKYMIST_TMU2) += milkymist-tmu2.o
> > -milkymist-tmu2.o-cflags := $(X11_CFLAGS)
> > -milkymist-tmu2.o-libs := $(X11_LIBS)
> > +milkymist-tmu2.o-cflags := $(X11_CFLAGS) $(OPENGL_CFLAGS)
> > +milkymist-tmu2.o-libs := $(X11_LIBS) $(OPENGL_LIBS)
> >
> >  obj-$(CONFIG_OMAP) += omap_dss.o
> >  obj-$(CONFIG_OMAP) += omap_lcdc.o
>
> Using $(call land) seems to break CONFIG_MILKYMIST_TMU2 availability in
> $(common-obj), while it works correctly in the per-target $(obj).
> I'm not sure what is the cause, but moving milkymist-tmu2.o to $(obj)
> makes more sense and fix this, so I'll go this way.

Something weird definitely seems to be going on here -- 'call land'
ought not to be broken in this situation.

I don't understand why the build/config-all-devices.mak versions of
the defines of CONFIG_FOO variables are so weird:

CONFIG_ACPI:=$(findstring y,$(CONFIG_ACPI)y)

...are they really intended to be self-referential like that?

The per-target ones in build/foo-softmmu/config-devices.mak are
more like what I expected:

CONFIG_VGA=y

Paolo, git blame says you wrote the makefile rune back in 2013:
I don't suppose you can remember the intent ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/4] hw/display/milkymist-tmu2: Move inlined code from header to source

[Philippe Mathieu-Daudé]

On 1/28/19 7:02 PM, Peter Maydell wrote:
> On Mon, 28 Jan 2019 at 17:47, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Cc'ing Thomas/Paolo for Makefile rules...
>>
>> On 1/24/19 12:43 PM, Gerd Hoffmann wrote:
>>> Oops, fails the build:
>>>
>>>   LINK    lm32-softmmu/qemu-system-lm32
>>> hw/lm32/milkymist.o: In function `milkymist_init':
>>> milkymist.c:(.text+0xb0f): undefined reference to `milkymist_tmu2_create'
>>
>> The problem comes from patch #2:
>>
>>> diff --git a/default-configs/lm32-softmmu.mak
>> b/default-configs/lm32-softmmu.mak
>>> index 4889348a10..4049b23562 100644
>>> --- a/default-configs/lm32-softmmu.mak
>>> +++ b/default-configs/lm32-softmmu.mak
>>> @@ -2,7 +2,7 @@
>>>
>>>  CONFIG_LM32=y
>>>  CONFIG_MILKYMIST=y
>>> -CONFIG_MILKYMIST_TMU2=$(CONFIG_OPENGL)
>>> +CONFIG_MILKYMIST_TMU2=$(call land,$(CONFIG_X11),$(CONFIG_OPENGL))
>>>  CONFIG_FRAMEBUFFER=y
>>>  CONFIG_PTIMER=y
>>>  CONFIG_PFLASH_CFI01=y
>>> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
>>> index 97acd5b6cb..079e702f25 100644
>>> --- a/hw/display/Makefile.objs
>>> +++ b/hw/display/Makefile.objs
>>> @@ -29,8 +29,8 @@ common-obj-$(CONFIG_MILKYMIST) += milkymist-vgafb.o
>>>  common-obj-$(CONFIG_ZAURUS) += tc6393xb.o
>>>
>>>  common-obj-$(CONFIG_MILKYMIST_TMU2) += milkymist-tmu2.o
>>> -milkymist-tmu2.o-cflags := $(X11_CFLAGS)
>>> -milkymist-tmu2.o-libs := $(X11_LIBS)
>>> +milkymist-tmu2.o-cflags := $(X11_CFLAGS) $(OPENGL_CFLAGS)
>>> +milkymist-tmu2.o-libs := $(X11_LIBS) $(OPENGL_LIBS)
>>>
>>>  obj-$(CONFIG_OMAP) += omap_dss.o
>>>  obj-$(CONFIG_OMAP) += omap_lcdc.o
>>
>> Using $(call land) seems to break CONFIG_MILKYMIST_TMU2 availability in
>> $(common-obj), while it works correctly in the per-target $(obj).
>> I'm not sure what is the cause, but moving milkymist-tmu2.o to $(obj)
>> makes more sense and fix this, so I'll go this way.
> 
> Something weird definitely seems to be going on here -- 'call land'
> ought not to be broken in this situation.
> 
> I don't understand why the build/config-all-devices.mak versions of
> the defines of CONFIG_FOO variables are so weird:
> 
> CONFIG_ACPI:=$(findstring y,$(CONFIG_ACPI)y)
> 
> ...are they really intended to be self-referential like that?
> 
> The per-target ones in build/foo-softmmu/config-devices.mak are
> more like what I expected:
> 
> CONFIG_VGA=y
> 
> Paolo, git blame says you wrote the makefile rune back in 2013:
> I don't suppose you can remember the intent ?

Now than Kconfig is in good usable shape, I'm pretty sure Paolo doesn't
want to remember ;) Our time is better worth spent on Kconfig than
fixing those issues.

For this series goal (remove SDL1!) I'll go with moving LM32/Milkymist
devices to $(obj). Hopefully the next patch will be in Kconfig :)

Re: [Qemu-devel] [PATCH v2 4/4] hw/display/milkymist-tmu2: Move inlined code from header to source

[Paolo Bonzini]

On 28/01/19 19:02, Peter Maydell wrote:
> 
> I don't understand why the build/config-all-devices.mak versions of
> the defines of CONFIG_FOO variables are so weird:
> 
> CONFIG_ACPI:=$(findstring y,$(CONFIG_ACPI)y)
> 
> ...are they really intended to be self-referential like that?

Yes, config-all-devices.mak is basically on "OR" of all the
config-devices.mak files.

Are CONFIG_X11 and CONFIG_OPENGL defined at the time
config-all-devices.mak is included?

Paolo

Re: [Qemu-devel] [PATCH v2 4/4] hw/display/milkymist-tmu2: Move inlined code from header to source

[Peter Maydell]

On Tue, 29 Jan 2019 at 08:21, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 28/01/19 19:02, Peter Maydell wrote:
> >
> > I don't understand why the build/config-all-devices.mak versions of
> > the defines of CONFIG_FOO variables are so weird:
> >
> > CONFIG_ACPI:=$(findstring y,$(CONFIG_ACPI)y)
> >
> > ...are they really intended to be self-referential like that?
>
> Yes, config-all-devices.mak is basically on "OR" of all the
> config-devices.mak files.
>
> Are CONFIG_X11 and CONFIG_OPENGL defined at the time
> config-all-devices.mak is included?

They should be -- they're defined in config-host.mak, which
Makefile includes before config-all-devices.mak.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/4] hw/display/milkymist-tmu2: Move inlined code from header to source

[Thomas Huth]

On 2019-01-28 18:47, Philippe Mathieu-Daudé wrote:
> Cc'ing Thomas/Paolo for Makefile rules...
> 
> On 1/24/19 12:43 PM, Gerd Hoffmann wrote:
>> On Thu, Jan 24, 2019 at 02:15:54AM +0100, Philippe Mathieu-Daudé wrote:
>>> Move the complexity of milkymist_tmu2_create() into the
>>> source file. Doing so we avoid to include the X11/OpenGL
>>> headers in all LM32 devices, and we also avoid the duplicate
>>> declaration of glx_fbconfig_attr[] (it is already declared
>>> in hw/display/milkymist-tmu2.c).
>>> Since TYPE_MILKYMIST_TMU2 is now accessible, use it.
>>
>> Oops, fails the build:
>>
>>   LINK    lm32-softmmu/qemu-system-lm32
>> hw/lm32/milkymist.o: In function `milkymist_init':
>> milkymist.c:(.text+0xb0f): undefined reference to `milkymist_tmu2_create'
> 
> The problem comes from patch #2:
> 
>> diff --git a/default-configs/lm32-softmmu.mak
> b/default-configs/lm32-softmmu.mak
>> index 4889348a10..4049b23562 100644
>> --- a/default-configs/lm32-softmmu.mak
>> +++ b/default-configs/lm32-softmmu.mak
>> @@ -2,7 +2,7 @@
>>
>>  CONFIG_LM32=y
>>  CONFIG_MILKYMIST=y
>> -CONFIG_MILKYMIST_TMU2=$(CONFIG_OPENGL)
>> +CONFIG_MILKYMIST_TMU2=$(call land,$(CONFIG_X11),$(CONFIG_OPENGL))
>>  CONFIG_FRAMEBUFFER=y
>>  CONFIG_PTIMER=y
>>  CONFIG_PFLASH_CFI01=y
>> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
>> index 97acd5b6cb..079e702f25 100644
>> --- a/hw/display/Makefile.objs
>> +++ b/hw/display/Makefile.objs
>> @@ -29,8 +29,8 @@ common-obj-$(CONFIG_MILKYMIST) += milkymist-vgafb.o
>>  common-obj-$(CONFIG_ZAURUS) += tc6393xb.o
>>
>>  common-obj-$(CONFIG_MILKYMIST_TMU2) += milkymist-tmu2.o
>> -milkymist-tmu2.o-cflags := $(X11_CFLAGS)
>> -milkymist-tmu2.o-libs := $(X11_LIBS)
>> +milkymist-tmu2.o-cflags := $(X11_CFLAGS) $(OPENGL_CFLAGS)
>> +milkymist-tmu2.o-libs := $(X11_LIBS) $(OPENGL_LIBS)
>>
>>  obj-$(CONFIG_OMAP) += omap_dss.o
>>  obj-$(CONFIG_OMAP) += omap_lcdc.o
> 
> Using $(call land) seems to break CONFIG_MILKYMIST_TMU2 availability in
> $(common-obj), while it works correctly in the per-target $(obj).
> I'm not sure what is the cause, but moving milkymist-tmu2.o to $(obj)
> makes more sense and fix this, so I'll go this way.

You could try whether adding an additional

 common-obj-$(CONFIG_ALL) += milkymist-tmu2.o

fixes the issue for you, too.

OTOH, milkymist is only used by one target, so there is really no reason
that this file should be added to common-obj, thus using $(obj) is fine
here.

 Thomas