pci rework not quite ready yet?

[Keith Packard]

On Tue, 2007-08-28 at 20:13 -0700, Ian Romanick wrote:
> -----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.

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. 

We cannot simply map 'all' of a particular BAR anyway -- devices have
non-BAR objects, and sometimes have multiple objects mapped to the same
BAR that require different caching requirements. So, we need to use the
range mapping APIs anyway.

> 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.

libpciaccess must be the right place -- only it knows the relationship
between the bus address of the device and how that maps to a CPU address
which is programmed into the MTRR.

> 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);

No, as described above there are devices which have mappings outside of
any BAR (!). Or so anholt claims. I'm not sure what the BAR would be for
in this case anyway; the only information needed here is which PCI
domain the device is in anyway. Tracking a list of mappings by address
seems easy enough; that's what the existing xf86Map routines do.

Given that we can track by address, simply using the physical address as
the tag for unmap should suffice. It can be an error to map the same
address twice.

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

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


>     struct pci_device_mapping mappings[PCI_DEV_MAX_MAPPINGS];

ick. static limits considered harmful. Just keep this all private
please, and return a pointer to me.

> Is it possible that has something to do with MTRRs?

No. I switched back to xf86MapVidMem, verified that the MTRRs were set
correctly but the bug still occurred.

> 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.

Let's get it fixed quickly then; anyone living on master will be hurting
until it's done.

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.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg/attachments/20070828/3d3c3c8b/attachment.pgp>