Re: AMD 64 AGP Patch (Was Re: [Dri-devel] r200 in cvs broken?)

2003-11-19 Thread Ronny V. Vindenes 
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

Re: AMD 64 AGP Patch (Was Re: [Dri-devel] r200 in cvs broken?)

2003-11-19 Thread Dave Jones 
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?)

2003-11-19 Thread James Jones 


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

2003-11-19 Thread Dave Jones 
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?)

2003-11-19 Thread Dave Jones 
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?)

2003-11-19 Thread Dave Jones 
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?)

2003-11-19 Thread James Jones 


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

2003-11-19 Thread James Jones 
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