Re: AGP bogosities

[Dave Jones <davej@redhat.com>]

On Fri, Mar 11, 2005 at 12:24:54PM +1100, Paul Mackerras wrote:

 > In fact there are other bogosities in drivers/char/agp/generic.c.  I
 > can't believe Dave ever tested that code with an AGP 3.0 device.

Hrmm, I'm fairly sure I did. It's also been sat in -mm without complaint
for a few weeks, which is odd.

 > you pass in a mode that has the AGP 3.0 bit set, agp_v3_parse_one()
 > will first clear that bit (and print a message), and then complain
 > because you haven't got that bit set in the mode, with a message that
 > the caller is broken.  Furthermore, if the mode passed in has both the
 > 4x and 8x bits set, the new code will give you 4x where the old code
 > would give you 8x (which is what the caller wanted).
 > 
 > The patch below fixes these problems.  It will work in the 99.99% of
 > cases where we have one AGP bridge and one AGP video card.  We should
 > eventually cope with multiple AGP bridges, but doing the matching of
 > bridges to video cards is a hard problem because the video card is not
 > necessarily a child or sibling of the PCI device that we use for
 > controlling the AGP bridge.  I think we need to see an actual example
 > of a system with multiple AGP bridges first.
 > 
 > Oh, and by the way, I have 3D working relatively well on my G5 with a
 > 64-bit kernel (and 32-bit X server and clients), which is why I care
 > about AGP 3.0 support. :)
 > 
 > Paul.
 > 
 > diff -urN linux-2.5/drivers/char/agp/agp.h g5-bad/drivers/char/agp/agp.h
 > --- linux-2.5/drivers/char/agp/agp.h	2005-03-07 14:01:44.000000000 +1100
 > +++ g5/drivers/char/agp/agp.h	2005-03-11 11:54:54.000000000 +1100
 > @@ -322,7 +322,7 @@
 >  #define AGPCTRL_GTLBEN		(1<<7)
 >  
 >  #define AGP2_RESERVED_MASK 0x00fffcc8
 > -#define AGP3_RESERVED_MASK 0x00ff00cc
 > +#define AGP3_RESERVED_MASK 0x00ff00c4
 >  
 >  #define AGP_ERRATA_FASTWRITES 1<<0
 >  #define AGP_ERRATA_SBA	 1<<1
 > diff -urN linux-2.5/drivers/char/agp/generic.c g5-bad/drivers/char/agp/generic.c
 > --- linux-2.5/drivers/char/agp/generic.c	2005-03-11 11:47:37.000000000 +1100
 > +++ g5/drivers/char/agp/generic.c	2005-03-11 12:08:29.000000000 +1100
 > @@ -515,13 +515,9 @@
 >  		printk (KERN_INFO PFX "%s tried to set rate=x0. Setting to AGP3 x4 mode.\n", current->comm);
 >  		*requested_mode |= AGPSTAT3_4X;
 >  	}
 > -	if (tmp == 3) {
 > -		printk (KERN_INFO PFX "%s tried to set rate=x3. Setting to AGP3 x4 mode.\n", current->comm);
 > -		*requested_mode |= AGPSTAT3_4X;
 > -	}
 > -	if (tmp >3) {
 > -		printk (KERN_INFO PFX "%s tried to set rate=x%d. Setting to AGP3 x8 mode.\n", current->comm, tmp);
 > -		*requested_mode |= AGPSTAT3_8X;
 > +	if (tmp >= 3) {
 > +		printk (KERN_INFO PFX "%s tried to set rate=x%d. Setting to AGP3 x8 mode.\n", current->comm, tmp * 4);
 > +		*requested_mode = (*requested_mode & ~7) | AGPSTAT3_8X;
 >  	}

This seems to make sense.

 >  	/* ARQSZ - Set the value to the maximum one.
 > @@ -642,11 +638,6 @@
 >  			return 0;
 >  		}
 >  		cap_ptr = pci_find_capability(device, PCI_CAP_ID_AGP);
 > -		if (!cap_ptr) {
 > -			pci_dev_put(device);
 > -			continue;
 > -		}
 > -			cap_ptr = 0;
 >  	}

This part I'm not so sure about.
The pci_get_class() call a few lines above will get a refcount that
we will now never release.

Thanks for taking a look at this. The absense of hardware to test
on means I pretty much rely on feedback from inclusion in -mm
to hear about problems like this before it hits mainline.
Unfortunatly, no-one with ppc64 tested it there it seems :-(

		Dave