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
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 (?)
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.
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.
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?
> 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
(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.
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.
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> Signed-off-by: Adam Jackson <ajax> Signed-off-by: Keith Packard <keithp> But looks like AArch64 used those. I am now running build with stubs added for aarch64.
xorg-x11-drv-qxl built fine with patched xserver devel package.
test code also builds
Can this be reviewed please? It's causing us issues composing aarch64
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
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/
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.
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?
(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.
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).
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...
I tested a build with this patch on an x86 guest/host and this seems to be working fine there.
Patch from comment #14 is upstream http://cgit.freedesktop.org/xorg/driver/xf86-video-qxl/commit/?id=c1e88427d1763bf9bbf9f2dd738980cee644443f
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.