Bug 1201877 - xorg-x11-drv-qxl is FTBFS on aarch64
Summary: xorg-x11-drv-qxl is FTBFS on aarch64
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: xorg-x11-server
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: X/OpenGL Maintenance List
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard: RejectedBlocker
Keywords:
Depends On:
Blocks: ARM64, F-ExcludeArch-aarch64 1221328
TreeView+ depends on / blocked
 
Reported: 2015-03-13 17:33 UTC by Peter Robinson
Modified: 2015-05-15 15:36 UTC (History)
12 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2015-05-15 15:36:03 UTC


Attachments (Terms of Use)
spec+patch (1.85 KB, patch)
2015-03-18 09:59 UTC, Marcin Juszkiewicz
no flags Details | Diff
test case (177 bytes, text/x-csrc)
2015-03-18 10:18 UTC, Christophe Fergeau
no flags Details
hack for xserver package to add in/out stubs for AArch64 (1.06 KB, patch)
2015-03-19 11:48 UTC, Marcin Juszkiewicz
no flags Details | Diff
patch + spec changeset (3.28 KB, patch)
2015-04-28 15:03 UTC, Marcin Juszkiewicz
no flags Details | Diff

Description Peter Robinson 2015-03-13 17:33:21 UTC
It's previously built, but we're not getting the following errors:

http://arm.koji.fedoraproject.org/koji/taskinfo?taskID=2919445

libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I.. -I/usr/include/spice-1 -fvisibility=hidden -I/usr/include/xorg -I/usr/include/pixman-1 -I/usr/include/libdrm -I/usr/include/X11/dri -Wall -Wpointer-arith -Wmissing-declarations -Wformat=2 -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Wbad-function-cast -Wold-style-definition -Wdeclaration-after-statement -Wunused -Wuninitialized -Wshadow -Wmissing-noreturn -Wmissing-format-attribute -Wredundant-decls -Wlogical-op -Werror=implicit -Werror=nonnull -Werror=init-self -Werror=main -Werror=missing-braces -Werror=sequence-point -Werror=return-type -Werror=trigraphs -Werror=array-bounds -Werror=write-strings -Werror=address -Werror=int-to-pointer-cast -Werror=pointer-to-int-cast -fno-strict-aliasing -I/usr/include/libdrm -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -c qxl_driver.c  -fPIC -DPIC -o .libs/qxl_driver.o
In file included from qxl_driver.c:45:0:
qxl.h: In function 'ioport_write':
qxl.h:637:5: error: implicit declaration of function 'outb' [-Werror=implicit-function-declaration]
     outb(qxl->io_base + port, val);
     ^
qxl.h:637:5: warning: nested extern declaration of 'outb' [-Wnested-externs]
qxl_driver.c: In function 'qxl_map_memory':
qxl_driver.c:386:36: warning: format '%d' expects argument of type 'int', but argument 5 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "framebuffer at %p (%d KB)\n",
                                    ^
qxl_driver.c:389:36: warning: format '%d' expects argument of type 'int', but argument 5 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "command ram at %p (%d KB)\n",
                                    ^
qxl_driver.c: In function 'qxl_screen_init':
qxl_driver.c:715:13: warning: format '%d' expects argument of type 'int', but argument 2 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     printf ("ram_header at %d\n", qxl->rom->ram_header_offset);
             ^
qxl_driver.c:716:13: warning: format '%d' expects argument of type 'int', but argument 2 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     printf ("surf0 size: %d\n", qxl->rom->surface0_area_size);
             ^
qxl_driver.c: In function 'print_modes':
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
              "%d: %dx%d, %d bits, stride %d, %dmm x %dmm, orientation %d\n",
              ^
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 5 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 6 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 7 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 8 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 9 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 10 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:931:14: warning: format '%d' expects argument of type 'int', but argument 11 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c: In function 'qxl_check_device':
qxl_driver.c:953:36: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "Device version %d.%d\n",
                                    ^
qxl_driver.c:953:36: warning: format '%d' expects argument of type 'int', but argument 5 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:956:36: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "Compression level %d, log level %d\n",
                                    ^
qxl_driver.c:956:36: warning: format '%d' expects argument of type 'int', but argument 5 has type 'uint32_t {aka unsigned int}' [-Wformat=]
qxl_driver.c:960:36: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "%d io pages at 0x%lx\n",
                                    ^
qxl_driver.c: In function 'qxl_pre_init_common':
qxl_driver.c:1007:39: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
         xf86DrvMsg(scrnIndex, X_INFO, "Deferred FPS: %d\n", qxl->deferred_fps);
                                       ^
qxl_driver.c: In function 'qxl_pre_init':
qxl_driver.c:1114:36: warning: format '%d' expects argument of type 'int', but argument 4 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     xf86DrvMsg (scrnIndex, X_INFO, "%d surfaces\n", qxl->rom->n_surfaces);
                                    ^
cc1: some warnings being treated as errors
Makefile:731: recipe for target 'qxl_driver.lo' failed

Comment 1 Christophe Fergeau 2015-03-16 09:36:23 UTC
0.1.3 did build in the past:
http://arm.koji.fedoraproject.org//packages/xorg-x11-drv-qxl/0.1.3/1.fc22/data/logs/aarch64/build.log
There were few enough patches to the package since that build that this looks like it is caused by some external changes (?)

Comment 2 Marcin Juszkiewicz 2015-03-18 09:59:24 UTC
Created attachment 1003118 [details]
spec+patch

Build failed due to lack of outb() so I checked X11 headers. Attached patch is a hack which allows to build that package but I am not quite sure is it the proper way of doing it.

Last built version was with xorg-xserver 1.16 so maybe some headers cleanup happened in meantime which changed situation for aarch64.

Comment 3 Christophe Fergeau 2015-03-18 10:18:58 UTC
Created attachment 1003124 [details]
test case

Is this testcase failing to build on ARM? (when xorg-x11-server-devel is installed). If yes, this is something which should be fixed on the xorg side in my opinion.

Comment 4 Christophe Fergeau 2015-03-18 10:24:26 UTC
compiler.h has:

#elif defined(__arm__) && defined(__linux__)

/* for Linux on ARM, we use the LIBC inx/outx routines */
/* note that the appropriate setup via "ioperm" needs to be done */
/*  *before* any inx/outx is done. */

I don't know if __arm__ is defined for aarch64 nor if the same code path should be used for aarch64?

Comment 5 Peter Robinson 2015-03-18 10:39:20 UTC
> I don't know if __arm__ is defined for aarch64 nor if the same code path
> should be used for aarch64?

It's not, and whether the same code path should be used is it depends, some things are the same, some are not

Comment 6 Marcin Juszkiewicz 2015-03-18 11:13:49 UTC
(In reply to Christophe Fergeau from comment #3)

> Is this testcase failing to build on ARM? (when xorg-x11-server-devel is
> installed). If yes, this is something which should be fixed on the xorg side
> in my opinion.

fails:

12:09 hrw@pinkiepie-rawhide:del$ gcc -Wall $(pkg-config --cflags --libs xorg-server) outb.c
outb.c: In function ‘main’:
outb.c:7:9: warning: implicit declaration of function ‘outb’ [-Wimplicit-function-declaration]
         outb(0, 0);
         ^
/tmp/cccvCtz8.o: In function `main':
outb.c:(.text+0x18): undefined reference to `outb'
collect2: error: ld returned 1 exit status


(In reply to Christophe Fergeau from comment #4)
> compiler.h has:
> 
> #elif defined(__arm__) && defined(__linux__)
> 
> /* for Linux on ARM, we use the LIBC inx/outx routines */

> I don't know if __arm__ is defined for aarch64 nor if the same code path
> should be used for aarch64?

AArch64 does not have sys/io.h header.

Comment 7 Christophe Fergeau 2015-03-18 13:46:21 UTC
Looking again at the initial report, appending -Wno-error to the build cflags in the .spec (make %{?_smp_mflags} CFLAGS="-Wno-error") would probably work around that issue too.

Fixing this bug for good would mean fixing the xserver headers, reassigning the bug there.

Comment 8 Marcin Juszkiewicz 2015-03-19 11:48:32 UTC
Created attachment 1003815 [details]
hack for xserver package to add in/out stubs for AArch64

Checked changes in hw/xfree86/common/compiler.h header.

There was set of cleanups done. One commit removed FAKEIT variable and set of stubs for in/out functions:

http://cgit.freedesktop.org/xorg/xserver/commit/hw/xfree86/common/compiler.h?id=80446086b9cfcc5e23a400d7fa38ec773fae68fc

xfree86: Undef FAKEIT
I guess this is meant to stub out all I/O port calls?  Whatever, it's
not been defined by the buildsystem at least as far back as monolith
6.8.2.

Reviewed-by: Julien Cristau <jcristau@debian.org>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Keith Packard <keithp@keithp.com>

But looks like AArch64 used those. I am now running build with stubs added for aarch64.

Comment 9 Marcin Juszkiewicz 2015-03-19 11:56:36 UTC
xorg-x11-drv-qxl built fine with patched xserver devel package.

Comment 10 Marcin Juszkiewicz 2015-03-19 12:07:50 UTC
test code also builds

Comment 11 Peter Robinson 2015-04-19 08:27:54 UTC
Can this be reviewed please? It's causing us issues composing aarch64

Comment 12 Fedora Blocker Bugs Application 2015-04-19 08:30:43 UTC
Proposed as a Blocker for 22-final by Fedora user pbrobinson using the blocker tracking app because:

 Regression in xorg, causing issues with composes which require kludgy workarounds

Comment 13 Petr Schindler 2015-04-20 19:00:53 UTC
Discussed at today's blocker review meeting [1].

This bug was rejected as Final Blocker - this appears to be an issue in a secondary architecture, and cannot possibly block the release.

[1] http://meetbot.fedoraproject.org/fedora-blocker-review/2015-04-20/

Comment 14 Adam Jackson 2015-04-23 14:55:58 UTC
When the FAKEIT code was there all the I/O port calls would compile to nothing, which means this driver probably couldn't have worked at all on arm64.  So this can't possibly be a blocker.

Making this actually work on arm64 probably requires a patch along the lines of:

====
diff --git a/src/qxl.h b/src/qxl.h
index f46bc58..73c5378 100644
--- a/src/qxl.h
+++ b/src/qxl.h
@@ -273,6 +273,7 @@ struct _qxl_screen_t
 #ifndef XSPICE
 #ifdef XSERVER_LIBPCIACCESS
     struct pci_device *                pci;
+    struct pci_io_handle *     io;
 #else
     pciVideoPtr                        pci;
     PCITAG                     pci_tag;
@@ -631,7 +632,7 @@ void ioport_write(qxl_screen_t *qxl, uint32_t io_port, uint32_t val);
 #else
 static inline void ioport_write(qxl_screen_t *qxl, int port, int val)
 {
-    outb(qxl->io_base + port, val);
+    pci_io_write8(qxl->io, port, val)
 }
 #endif
 
diff --git a/src/qxl_driver.c b/src/qxl_driver.c
index 942067f..d6c33f0 100644
--- a/src/qxl_driver.c
+++ b/src/qxl_driver.c
@@ -219,6 +219,8 @@ unmap_memory_helper (qxl_screen_t *qxl)
        pci_device_unmap_range (qxl->pci, qxl->vram, qxl->pci->regions[1].size);
     if (qxl->rom)
        pci_device_unmap_range (qxl->pci, qxl->rom, qxl->pci->regions[2].size);
+    if (qxl->io)
+       pci_device_close_io (qxl->pci, qxl->io);
 #else
     if (qxl->ram)
        xf86UnMapVidMem (scrnIndex, qxl->ram, (1 << qxl->pci->size[0]));
@@ -251,6 +253,9 @@ map_memory_helper (qxl_screen_t *qxl)
                           qxl->pci->regions[2].size, 0,
                           (void **)&qxl->rom);
     
+    qxl->io = pci_device_open_io(qxl->pci,
+                                qxl->pci->regions[3].base_addr,
+                                qxl->pci->regions[3].size);
     qxl->io_base = qxl->pci->regions[3].base_addr;
 #else
     qxl->ram = xf86MapPciMem (scrnIndex, VIDMEM_FRAMEBUFFER,
====

I can't test easily without hardware though.

Comment 15 Christophe Fergeau 2015-04-24 11:21:07 UTC
Ah, thanks for the patch suggestion!

(In reply to Adam Jackson from comment #14)
>  static inline void ioport_write(qxl_screen_t *qxl, int port, int val)
>  {
> -    outb(qxl->io_base + port, val);
> +    pci_io_write8(qxl->io, port, val)
>  }

Is this going to do the right thing on arches which use outb, or do we need one outb codepath, and one aarch64 pci_io_write() codepath?

Comment 16 Adam Jackson 2015-04-27 19:35:11 UTC
(In reply to Christophe Fergeau from comment #15)
> Ah, thanks for the patch suggestion!
> 
> (In reply to Adam Jackson from comment #14)
> >  static inline void ioport_write(qxl_screen_t *qxl, int port, int val)
> >  {
> > -    outb(qxl->io_base + port, val);
> > +    pci_io_write8(qxl->io, port, val)
> >  }
> 
> Is this going to do the right thing on arches which use outb, or do we need
> one outb codepath, and one aarch64 pci_io_write() codepath?

On linux pci_io_* try to open the sysfs map file corresponding to the I/O port range, which the kernel translates arch-appropriately.  If there is no such file then it'll try to use port instructions if that's a thing the architecture has.  So you only need one path, pciaccess exists to get the portability right for you.

Comment 17 Marcin Juszkiewicz 2015-04-28 15:03:07 UTC
Created attachment 1019726 [details]
patch + spec changeset

Integrated patch from comment 14 and now it builds. 

Do not know how to check does it work properly (have hw).

Comment 18 Cole Robinson 2015-04-28 15:40:50 UTC
Given that qxl is virt only, and aarch64 VMs don't support video or PCI yet, I don't know if there _is_ any way to test it...

Comment 19 Christophe Fergeau 2015-04-28 16:04:52 UTC
I tested a build with this patch on an x86 guest/host and this seems to be working fine there.

Comment 21 Christophe Fergeau 2015-05-15 15:36:03 UTC
I pushed your .spec patch Marcin with the patch which was committed upstream (the one from comment #20 ). So build should work in rawhide even though it's probably not possible to use this code.


Note You need to log in before you can comment on or make changes to this bug.