pci rework not quite ready yet?

[Ian Romanick]

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

Jesse Barnes wrote:
> On Tuesday, August 28, 2007 8:39:02 pm Keith Packard wrote:
>> On Tue, 2007-08-28 at 20:13 -0700, Ian Romanick wrote:
>>>
>>> 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.
>> Mapping memory by address as xf86MapVidMem does makes sense if you know
>> what device (or PCI domain) the address is related to. Adding a new API
>> that includes a reference to the device would enable the address to be
>> appropriately based.
> 
> Then the address in the BAR is relegated to "cookie" status.  This would be 
> fine, but the code is (or at least used to be) somewhat confused in this 
> regard.  At times the address was used as a 'real' address, resulting in bugs 
> and BAR corruption on various platforms.  Also, the code didn't properly tie 
> a given BAR value to a specific bus or PCI domain, resulting in duplicates 
> and other bugs.  I'd be much happier if we just got away from using bus 
> addresses entirely.  There's really no way we can use them reliably short of 
> growing sophisticated bus drivers in the X server...

And this is a fair portion of the reason I started on pci-rework in the
first place.  Or did I mention that before?  In some cases just getting
the bus address of the device is a huge PITA.

> Also, adding this new interface seems like a lot of churn for just dealing 
> with cacheability attributes.  Maybe it would be best to have a separate, 
> madvise-like call to set the attributes instead?  The user would be required 
> to pass in a range or subrange already mapped by a previous mapping call.  
> E.g.
> 
>   error_t pci_device_set_range_attributes(struct pci_device *dev,
> 					  pciaddr_t offset, size_t size,
> 					  unsigned flags)
> 
> This has the added bonus of making error checking easy, e.g. if you try to set 
> WC on a range and it fails, you'll still have the mapping but you can warn 
> the user that it won't be very performant.

I think we want to set the access bits when we do the mapping.  It looks
like some platforms (IA64) set these bits from flags passed to mmap.
This is somewhat unfamiliar territory for me, so I may be mistaken.
Using flags passed to mmap seems like the right way to interface with
PAT, but that's just my $0.02.

>> Also, let's work to transition the existing xf86 PCI apis to
>> libpciaccess as well; the needn't be efficient, they just need to work.
>> Just pulling apart the PCITAG given to these APIs should make it
>> possible for them to operate as they used to. Most of them are not used
>> in performance sensitive code anyway.
> 
> Ick, yeah, getting rid of PCITAG entirely would be nice.  But any cleanups (I 
> think there are many more we could pursue) should be put off until things are 
> working well.

There are interfaces in libpciaccess that allow us to use PCITAGs with
relatively little pain (see pci_device_find_by_slot).  The big problem
with PCITAGs is that they're misused in a few places.  Lots of drivers
and one or two places in the pre-pci-rework X server don't set / use the
domain.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)

iD8DBQFG1Z1VX1gOwKyEAw8RAmfCAJ9L5lTe5ZFKXewUmw6C/MARt5vVBQCfSKDM
YQv9Z5aNdkpsiGDF6SnRSzU=
=Q2aY
-----END PGP SIGNATURE-----