pci rework not quite ready yet?

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Keith Packard wrote:
> After converting the intel driver to the pciaccess API, I've discovered
> a couple of shortcomings that may need to be addressed before this code
> is really ready for use.
> 
> The first problem was that we continue to have a legacy code in the
> server which passes bus addresses around, like xf86MapVidMem.

A big part of the reason I started on this project is that on some
platforms, using bus addresses in this way is nonsensical.  The only
reason that xf86MapVidMem still lives is for ISA, sbus, and other
non-PCI buses.

> The second problem is that only xf86MapVidMem has the mtrr manipulation
> code required for X servers to run on x86 machines. libpciaccess must
> perform mtrr (or PAT, at some point) manipulation as a part of the
> memory mapping/unmapping process.

I haven't been able to find the relevant discussions in my IRC logs, but
I'd *swear* that when this came up in the past, the rough consensus was
that libpciaccess wasn't the right place.  That's why I didn't do
anything with it.

Clearly, the discussions over the last few days show that to have been a
mistake.

One reason for the current design of the mapping interface was to keep
tracking the mappings easy.  It doesn't get much easier than a 1-to-1
correlation between mappings and BARs.  Would something like this be
suitable?

int pci_device_map_range(struct pci_device *dev, int bar,
    pciaddr_t offset, size_t size, unsigned flags, int *map);

#define PCI_DEV_MAP_FLAG_WRITABLE       (1U<<0)
#define PCI_DEV_MAP_FLAG_WRITE_COMBINE  (1U<<1)
#define PCI_DEV_MAP_FLAG_CACHABLE       (1U<<2)

int pci_device_unmap_range(struct pci_device *dev, int map);

The pci_device structure then gets the following field added:

    struct pci_device_mapping mappings[PCI_DEV_MAX_MAPPINGS];

struct pci_device_mapping {
    pciaddr_t offset;
    size_t size;
    unsigned flags;
    void *memory;
};

After doing the mapping, the caller uses *map to look up in mappings.
The unmap is done by passing *map back.  I prefer this route because
dealing with the case where there are multiple mappings of the same
range is trivial.  I came across a couple similar cases while designing
the original API, but they all turned into subsets of this case.

> Finally, I've been unable to figure out why DRI-based Mesa locks up my
> machines; switching back to the legacy code without changing the kernel
> module, DRM library or Mesa driver makes things work again, so it's
> clearly something in the X server or my driver causing the problem.
> Examining log files shows no obvious cause.

Is it possible that has something to do with MTRRs?

> I've pushed a 'no-pci-rework' branch to the X server repository so that
> people can continue to use master for their work in areas not related to
> fixing the pciaccess code. I'm wondering if it makes sense to make that
> 'master' until pciaccess resolves these issues.

I'd *really* rather not.  This code sat unreviewed and untested by
anyone but me for over a year.  I don't think that returning it to the
back burner is going to help resolve the issues.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)

iD8DBQFG1OR2X1gOwKyEAw8RAiEZAJ4/NKCXAW6vE+4gwFdb9UDaQyLo+wCfasUY
9zI+ENvyRVSseg25BvKtBww=
=lP/G
-----END PGP SIGNATURE-----