Bug 1201877

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.