Bug 3248

Created attachment 2649 [details] [review]
First attempt at detecting PCI G450 cards

This patch detects PCI G450 cards by looking at the PCI vendor / device of the
bus the card is on.  If the vendor / device matches that of the PCI-to-PCI
bridge that is known to be used on the PCI G450 cards, the card is rejected. 
Currently, this rejection happens when the card's device is opened.  Since the
open fails, the X DDX will not try to initialize DRI for the card.

Currently, this fix is only in linux-core/mga_drv.c.  I'm not sure how to do
this for BSD.

Since I haven't done anything in the kernel since the 2.2 / 2.3 time frame, I'm
going to let this be reviewed before I commit it.

I've been thinking about this some more.  I've decided that I don't like the
idea of failing at device open.  The problem is that if the MGA DRM is updated
to support PCI cards, the device open can't fail.  In that case, old versions of
the DDX will go back to being broken (i.e., exactly as they are today).

After digging a bit deeper, I think a better sollution is to add a
device-specific hook in drm_agp_init.  At the start of drm_agp_init,
dev->driver->agp_preinit().  agp_preinit could then determine if the card was
really an AGP card or not.  If it's not, it would return false and drm_agp_init
would fail.  In the case of drivers that require AGP (i.e., the current MGA
DRM), this would result in driver failing to initialize.

If nothing else, it looks like it should make the drmAgpAcquire call in the DDX
fail.  That /should/ be good enough for now.  In the future, when the DRM and
DDX support DRI on PCI cards, the DDX could take a different, non-failure path
when drmAgpAcquire fails.

I'll code up something in the morning and post a patch.  Does this seem like the
right approach?

Created attachment 2654 [details] [review]
Reject PCI G450 cards in drm_agp_init path

This patch implements the mechanism that I proposed in my previous post.  This
patch prevents DRI from begin initialized on PCI G450 with the current DDX even
if DRIVER_REQUIRE_AGP is removed from driver_features.	This means that old DDX
versions will still be "fixed" even when the DRM is updated to support PCI
cards.

Unless there are objections, I will commit this on 2005-May-11.

Created attachment 2676 [details] [review]
Reject PCI G450 cards via drm_device_is_agp callback

This is the thrird (and hopefully final!) iteration of this patch. 
drm_device_is_agp was modified to use a device-specific callback on both Linux
and BSD.  The Linux code was then modified to use drm_device_is_agp before
calling drm_agp_init (this is the way BSD already works).

This achieves essentially the same effect as the previous patch, but, since it
makes the Linux code more like the BSD code and will require less changes to
BSD to implement, I think I like this version better.

I'm not convinced the patches attached above are the correct approach for
this.  The only safe way to determine wether a card is actually AGP or not,
is to walk the PCI extended capabilities list.  I wrote a patch for the
Radeon driver a couple of years ago which does just that, which is present
in Xorg 6.7.0 and later releases.  My code was initially just a Radeon driver
specific temporary implementation, but the code has been copied aparently
into the SiS or some other driver (I don't recall which).

Ideally, the code should be factored into os-support somewhere, and all
drivers modified to use the genericized code instead of cut and pasting
it everywhere.

Initially I would have placed the code X server side, but perhaps it fits
better in kernel land?

Any thoughts?

mike, that's what drm_device_is_agp does:

static __inline__ int drm_device_is_agp(drm_device_t *dev)
{
        return pci_find_capability(dev->pdev, PCI_CAP_ID_AGP);
}

the problem is that the PCI G450 will appear to be an AGP device all the time. 
so merely walking the cap list will actually be wrong for this device, yay
matrox.  you have to both walk the cap list _and_ allow for a device-specific check.

My patches have been commited to Mesa and DRM CVS.  Eric Anholt committed
similar patches for *BSD in DRM CVS.  Closing.