Re: AMD 64 AGP Patch (Was Re: [Dri-devel] r200 in cvs broken?)
The ( i > MAX_HAMMER_GARTS) fix was just an example. The test really needs to be == and be moved before the hammers[i++] = loop_dev; assignment, or hammers will be overflowed, as I mentioned in my previous email. Also, it really seems like this test should be done before you go through all the trouble of detecting another gart. If we already have the max number of hammer garts, why try to detect another one, we can just return -1 and be done with it right? Thanks for the fix though. -James Dave Jones wrote: On Wed, Nov 19, 2003 at 12:13:37PM -0800, James Jones wrote: > > > Ronny V. Vindenes wrote > > >It looks like you'll add GARTS up to MAX_HAMMER_GARTS-1 then bomb if > >there is an MAX_HAMMER_GARTS'th GART. > > > > > Yes, thanks for putting it more clearly Ronny. > > Dave, try walking through the code with MAX_HAMMER_GARTS=2 and SMP > enabled. You should quickly see what we mean. Even with > MAX_HAMMER_GARTS=1 and SMP enabled (running SMP support on one > processor) which was the case I was trying to explain, it should fail. Now changed to your proposed change (i > MAX_HAMMER_GARTS) Thanks again. Dave --- This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/ ___ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: AMD 64 AGP Patch (Was Re: [Dri-devel] r200 in cvs broken?)
Ronny V. Vindenes wrote It looks like you'll add GARTS up to MAX_HAMMER_GARTS-1 then bomb if there is an MAX_HAMMER_GARTS'th GART. Yes, thanks for putting it more clearly Ronny. Dave, try walking through the code with MAX_HAMMER_GARTS=2 and SMP enabled. You should quickly see what we mean. Even with MAX_HAMMER_GARTS=1 and SMP enabled (running SMP support on one processor) which was the case I was trying to explain, it should fail. -James --- This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/ ___ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: AMD 64 AGP Patch (Was Re: [Dri-devel] r200 in cvs broken?)
On Wed, Nov 19, 2003 at 12:13:37PM -0800, James Jones wrote: > > > Ronny V. Vindenes wrote > > >It looks like you'll add GARTS up to MAX_HAMMER_GARTS-1 then bomb if > >there is an MAX_HAMMER_GARTS'th GART. > > > > > Yes, thanks for putting it more clearly Ronny. > > Dave, try walking through the code with MAX_HAMMER_GARTS=2 and SMP > enabled. You should quickly see what we mean. Even with > MAX_HAMMER_GARTS=1 and SMP enabled (running SMP support on one > processor) which was the case I was trying to explain, it should fail. Now changed to your proposed change (i > MAX_HAMMER_GARTS) Thanks again. Dave --- This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/ ___ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: AMD 64 AGP Patch (Was Re: [Dri-devel] r200 in cvs broken?)
On Wed, Nov 19, 2003 at 08:56:32PM +0100, Ronny V. Vindenes wrote: > > If we have an SMP system with an SMP kernel, we add however many > > GARTs to the table, up to a limit of MAX_HAMMER_GARTS. > > > > It looks like you'll add GARTS up to MAX_HAMMER_GARTS-1 then bomb if > there is an MAX_HAMMER_GARTS'th GART. Ah, yes I think you're right. I'll fix that up. Thanks for double checking. Dave --- This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/ ___ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: AMD 64 AGP Patch (Was Re: [Dri-devel] r200 in cvs broken?)
On Wed, Nov 19, 2003 at 11:44:42AM -0800, James Jones wrote: > > > > > > hammers[i++] = loop_dev; > > nr_garts = i; > >#ifdef CONFIG_SMP > > if (i == MAX_HAMMER_GARTS) { > > printk(KERN_INFO PFX "Too many northbridges for AGP\n"); > > return -1; > > } > > > > Seems wrong to me... wouldn't this return -1 if say, MAX_HAMMER_GARTS == > 1 and 1 gart was found ( nr_garts == i == 1 when the comparison is made > ). It would need to be: MAX_HAMMER_GARTS can only be 1 if CONFIG_SMP=n, so the above code is skipped, and we fall through, and return 0. > This would also be wrong, as the test would be too late, and hammer[] > would be overflowed by the time the test is performed. This is why the > test was moved before the assignment in our patches. The way we did it > would handle the SMP and non-smp cases I believe, the code you quoted > would only work right in the uniproc case. That's how it should be. If we have a UP system, with UP kernel, we just return 0 after finding the first GART If we have a UP system with an SMP kernel, we find the first GART, and eventually end up returning 0 after finding no further garts. If we have an SMP system with UP kernel, we exit early after finding the first GART. The 2nd (And above) GARTs are irrelevant when not running in SMP. If we have an SMP system with an SMP kernel, we add however many GARTs to the table, up to a limit of MAX_HAMMER_GARTS. I don't see any problems. Dave --- This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/ ___ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: AMD 64 AGP Patch (Was Re: [Dri-devel] r200 in cvs broken?)
hammers[i++] = loop_dev; nr_garts = i; #ifdef CONFIG_SMP if (i == MAX_HAMMER_GARTS) { printk(KERN_INFO PFX "Too many northbridges for AGP\n"); return -1; } Seems wrong to me... wouldn't this return -1 if say, MAX_HAMMER_GARTS == 1 and 1 gart was found ( nr_garts == i == 1 when the comparison is made ). It would need to be: if (i > MAX_HAMMER_GARTS) { ... This would also be wrong, as the test would be too late, and hammer[] would be overflowed by the time the test is performed. This is why the test was moved before the assignment in our patches. The way we did it would handle the SMP and non-smp cases I believe, the code you quoted would only work right in the uniproc case. I can test this when I get home today though I suppose. -James #else /* Uniprocessor case, return after finding first bridge. (There may be more, but in UP, we don't care). */ return 0; #endif } return i == 0 ? -1 : 0; } --- This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/ ___ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: AMD 64 AGP Patch (Was Re: [Dri-devel] r200 in cvs broken?)
On Wed, Nov 19, 2003 at 11:17:17AM -0800, James Jones wrote: > diff -ruN linux-2.6.0-test7/arch/x86_64/kernel/pci-gart.c > linux-2.6.0-test7-fixed/arch/x86_64/kernel/pci-gart.c > --- linux-2.6.0-test7/arch/x86_64/kernel/pci-gart.c 2003-10-08 12:24:04.0 > -0700 > +++ linux-2.6.0-test7-fixed/arch/x86_64/kernel/pci-gart.c2003-10-09 > 04:40:44.179994208 -0700 > @@ -681,7 +681,7 @@ > unsigned long iommu_start; > struct pci_dev *dev; > > -#ifndef CONFIG_AGP_AMD_8151 > +#ifndef CONFIG_AGP_AMD64 > no_agp = 1; Already in agpgart bitkeeper tree. Waiting for Linus to pull. Andi also sent this to Linus.. > /* Makefile puts PCI initialization via subsys_initcall first. */ > diff -ruN linux-2.6.0-test7/drivers/char/agp/amd64-agp.c > linux-2.6.0-test7-fixed/drivers/char/agp/amd64-agp.c > --- linux-2.6.0-test7/drivers/char/agp/amd64-agp.c 2003-10-08 12:24:17.0 > -0700 > +++ linux-2.6.0-test7-fixed/drivers/char/agp/amd64-agp.c 2003-10-09 > 04:41:44.563814472 -0700 > @@ -355,12 +355,14 @@ > #endif > return -1; > } > -hammers[i++] = loop_dev; > -nr_garts = i; > + > if (i == MAX_HAMMER_GARTS) { > printk(KERN_INFO PFX "Too many northbridges for AGP\n"); > return -1; > } > + > +hammers[i++] = loop_dev; > +nr_garts = i; > } > return i == 0 ? -1 : 0; > } The current code in -bk looks like this, which should do the right thing on SMP and UP.. Can you confirm ? Dave ... hammers[i++] = loop_dev; nr_garts = i; #ifdef CONFIG_SMP if (i == MAX_HAMMER_GARTS) { printk(KERN_INFO PFX "Too many northbridges for AGP\n"); return -1; } #else /* Uniprocessor case, return after finding first bridge. (There may be more, but in UP, we don't care). */ return 0; #endif } return i == 0 ? -1 : 0; } --- This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/ ___ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: AMD 64 AGP Patch (Was Re: [Dri-devel] r200 in cvs broken?)
On Wed, 2003-11-19 at 20:46, Dave Jones wrote: > On Wed, Nov 19, 2003 at 11:44:42AM -0800, James Jones wrote: > > > > > > > > > hammers[i++] = loop_dev; > > > nr_garts = i; > > >#ifdef CONFIG_SMP > > > if (i == MAX_HAMMER_GARTS) { > > > printk(KERN_INFO PFX "Too many northbridges for AGP\n"); > > > return -1; > > > } > > > > > > > Seems wrong to me... wouldn't this return -1 if say, MAX_HAMMER_GARTS == > > 1 and 1 gart was found ( nr_garts == i == 1 when the comparison is made > > ). It would need to be: > > MAX_HAMMER_GARTS can only be 1 if CONFIG_SMP=n, so the above code > is skipped, and we fall through, and return 0. > > > This would also be wrong, as the test would be too late, and hammer[] > > would be overflowed by the time the test is performed. This is why the > > test was moved before the assignment in our patches. The way we did it > > would handle the SMP and non-smp cases I believe, the code you quoted > > would only work right in the uniproc case. > > That's how it should be. > > If we have a UP system, with UP kernel, we just return 0 after > finding the first GART > > If we have a UP system with an SMP kernel, we find the first GART, > and eventually end up returning 0 after finding no further garts. > > If we have an SMP system with UP kernel, we exit early after > finding the first GART. The 2nd (And above) GARTs are irrelevant > when not running in SMP. Agreed. > > If we have an SMP system with an SMP kernel, we add however many > GARTs to the table, up to a limit of MAX_HAMMER_GARTS. > It looks like you'll add GARTS up to MAX_HAMMER_GARTS-1 then bomb if there is an MAX_HAMMER_GARTS'th GART. -- Ronny V. Vindenes <[EMAIL PROTECTED]> --- This SF.net email is sponsored by: SF.net Giveback Program. Does SourceForge.net help you be more productive? Does it help you create better code? SHARE THE LOVE, and help us help YOU! Click Here: http://sourceforge.net/donate/ ___ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel