2.4.20 AGP for I845 wrong ?

2.4.20 AGP for I845 wrong ?

[Margit Schubert-While]

 From drivers/char/agp/agpgart_be.c
4554,4559
     { PCI_DEVICE_ID_INTEL_845_G_0,
                  PCI_VENDOR_ID_INTEL,
                  INTEL_I845_G,
                  "Intel",
                  "i845G",
                  intel_830mp_setup },

Surely this is wrong or ?
Should be "intel_845_setup", I think.

Which might explain funny messages in th X/DRI/DRM log.

For info, the Intel M/B D845PESV(L) reports as a "G" -
00:00.0 Host bridge: Intel Corp. 82845G/GL [Brookdale-G] Chipset Host 
Bridge (rev 02)
00:01.0 PCI bridge: Intel Corp. 82845G/GL [Brookdale-G] Chipset AGP Bridge 
(rev 02)

Also in drivers/char/drm/drm_agpsupport.h, the switch statement at 262 is 
missing the
cases for INTEL_I830_M, INTEL_I845_G.

Margit

Re: 2.4.20 AGP for I845 wrong ?

[Nicolas ASPERT]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

Margit Schubert-While wrote:
>  From drivers/char/agp/agpgart_be.c
> 4554,4559
>     { PCI_DEVICE_ID_INTEL_845_G_0,
>                  PCI_VENDOR_ID_INTEL,
>                  INTEL_I845_G,
>                  "Intel",
>                  "i845G",
>                  intel_830mp_setup },
> 
> Surely this is wrong or ?
> Should be "intel_845_setup", I think.
> 

IIRC, the 845G is a "new" version of the 830MP chipset (it had been
added by Abraham vd Merwe & Graeme Fisher some months ago), but acts
basically just as the 830MP. Therefore the entry is correct.... Or maybe
if it gets confusing adding a comment would not hurt...

> 
> Also in drivers/char/drm/drm_agpsupport.h, the switch statement at 262 
> is missing the
> cases for INTEL_I830_M, INTEL_I845_G.


That's true. It is also missing in 2.5.51.
I attach two patches, one for 2.4.21-pre1 and one for 2.5.51 that should 
fix this.


Regards

Nicolas.



[-- Attachment #2: intelchipset-id-2.4.21-pre1.diff --]
[-- Type: text/plain, Size: 848 bytes --]

diff -ru linux-2.4.21-pre1.clean/drivers/char/drm/drm_agpsupport.h linux-2.4.21-pre1/drivers/char/drm/drm_agpsupport.h
--- linux-2.4.21-pre1.clean/drivers/char/drm/drm_agpsupport.h	Wed Dec 11 12:36:58 2002
+++ linux-2.4.21-pre1/drivers/char/drm/drm_agpsupport.h	Wed Dec 11 12:42:34 2002
@@ -267,8 +267,10 @@
 		case INTEL_I810:	head->chipset = "Intel i810";    break;
 		case INTEL_I815:	head->chipset = "Intel i815";	 break;
 	 	case INTEL_I820:	head->chipset = "Intel i820";	 break;
+		case INTEL_I830_M:	head->chipset = "Intel i830M";	 break;
 		case INTEL_I840:	head->chipset = "Intel i840";    break;
 		case INTEL_I845:	head->chipset = "Intel i845";    break;
+		case INTEL_I845_G:	head->chipset = "Intel i845G";	 break;
 		case INTEL_I850:	head->chipset = "Intel i850";	 break;
 
 		case VIA_GENERIC:	head->chipset = "VIA";           break;

[-- Attachment #3: intelchipset-id-2.5.51.diff --]
[-- Type: text/plain, Size: 854 bytes --]

diff -ru linux-2.5.51.clean/drivers/char/drm/drm_agpsupport.h linux-2.5.51/drivers/char/drm/drm_agpsupport.h
--- linux-2.5.51.clean/drivers/char/drm/drm_agpsupport.h	Tue Dec 10 03:45:39 2002
+++ linux-2.5.51/drivers/char/drm/drm_agpsupport.h	Wed Dec 11 12:55:08 2002
@@ -271,10 +271,12 @@
 #if LINUX_VERSION_CODE >= 0x02040f /* KERNEL_VERSION(2,4,15) */
 	 	case INTEL_I820:	head->chipset = "Intel i820";	 break;
 #endif
+		case INTEL_I830_M:	head->chipset = "Intel i830M";	 break;
 		case INTEL_I840:	head->chipset = "Intel i840";    break;
 #if LINUX_VERSION_CODE >= 0x02040f /* KERNEL_VERSION(2,4,15) */
 		case INTEL_I845:	head->chipset = "Intel i845";    break;
 #endif
+		case INTEL_I845:	head->chipset = "Intel i845G";	 break;
 		case INTEL_I850:	head->chipset = "Intel i850";	 break;
 		case INTEL_460GX:	head->chipset = "Intel 460GX";	 break;
 

Re: 2.4.20 AGP for I845 wrong ?

[Nicolas ASPERT]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

Ooops... the patch I sent for 2.5.51 is wrong, since there I added a 
INTEL_I845 instead of a INTEL_I845_G (I know vim *does* weird things in 
my back 8-)

Here is the correct one...

Regards

Nicolas.
-- 
Nicolas Aspert      Signal Processing Institute (ITS)
Swiss Federal Institute of Technology (EPFL)

[-- Attachment #2: intelchipset-id-2.5.51.diff --]
[-- Type: text/plain, Size: 856 bytes --]

diff -ru linux-2.5.51.clean/drivers/char/drm/drm_agpsupport.h linux-2.5.51/drivers/char/drm/drm_agpsupport.h
--- linux-2.5.51.clean/drivers/char/drm/drm_agpsupport.h	Tue Dec 10 03:45:39 2002
+++ linux-2.5.51/drivers/char/drm/drm_agpsupport.h	Wed Dec 11 12:55:08 2002
@@ -271,10 +271,12 @@
 #if LINUX_VERSION_CODE >= 0x02040f /* KERNEL_VERSION(2,4,15) */
 	 	case INTEL_I820:	head->chipset = "Intel i820";	 break;
 #endif
+		case INTEL_I830_M:	head->chipset = "Intel i830M";	 break;
 		case INTEL_I840:	head->chipset = "Intel i840";    break;
 #if LINUX_VERSION_CODE >= 0x02040f /* KERNEL_VERSION(2,4,15) */
 		case INTEL_I845:	head->chipset = "Intel i845";    break;
 #endif
+		case INTEL_I845_G:	head->chipset = "Intel i845G";	 break;
 		case INTEL_I850:	head->chipset = "Intel i850";	 break;
 		case INTEL_460GX:	head->chipset = "Intel 460GX";	 break;
 

Re: 2.4.20 AGP for I845 wrong ?

[Dave Jones]

On Wed, Dec 11, 2002 at 01:07:45PM +0100, Nicolas ASPERT wrote:
 > IIRC, the 845G is a "new" version of the 830MP chipset (it had been
 > added by Abraham vd Merwe & Graeme Fisher some months ago), but acts
 > basically just as the 830MP. Therefore the entry is correct.... Or maybe
 > if it gets confusing adding a comment would not hurt...

I'll check the chipset docs when I get time, and add a comment if
necessary. No-one seems to be complaining that it isn't working,
so I'm inclined to believe your diagnosis is correct.

 > > Also in drivers/char/drm/drm_agpsupport.h, the switch statement at 262 
 > > is missing the
 > > cases for INTEL_I830_M, INTEL_I845_G.
 > That's true. It is also missing in 2.5.51.
 > I attach two patches, one for 2.4.21-pre1 and one for 2.5.51 that should 
 > fix this.
 > diff -ru linux-2.5.51.clean/drivers/char/drm/drm_agpsupport.h linux-2.5.51/drivers/char/drm/drm_agpsupport.h
 > --- linux-2.5.51.clean/drivers/char/drm/drm_agpsupport.h	Tue Dec 10 03:45:39 2002
 > +++ linux-2.5.51/drivers/char/drm/drm_agpsupport.h	Wed Dec 11 12:55:08 2002
 > @@ -271,10 +271,12 @@
 >  #if LINUX_VERSION_CODE >= 0x02040f /* KERNEL_VERSION(2,4,15) */
 >  	 	case INTEL_I820:	head->chipset = "Intel i820";	 break;
 >  #endif
 > +		case INTEL_I830_M:	head->chipset = "Intel i830M";	 break;
 >  		case INTEL_I840:	head->chipset = "Intel i840";    break;
 >  #if LINUX_VERSION_CODE >= 0x02040f /* KERNEL_VERSION(2,4,15) */
 >  		case INTEL_I845:	head->chipset = "Intel i845";    break;
 >  #endif
 > +		case INTEL_I845:	head->chipset = "Intel i845G";	 break;
 >  		case INTEL_I850:	head->chipset = "Intel i850";	 break;
 >  		case INTEL_460GX:	head->chipset = "Intel 460GX";	 break;

DRI folks, this seems like duplication given that this data is available
in agpgart. How about changing this to read whatever agpgart has set in
.chipset_name ?

Keeping these two lists in sync seems somewhat pointless.

        Dave

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: 2.4.20 AGP for I845 wrong ?

[Nicolas ASPERT]

Dave Jones wrote:

> I'll check the chipset docs when I get time, and add a comment if
> necessary. No-one seems to be complaining that it isn't working,
> so I'm inclined to believe your diagnosis is correct.
> 

I found the thread of lkml containing the discussion about that ... here 
is the link to the original mail :

http://marc.theaimsgroup.com/?l=linux-kernel&m=102122146829865&w=2

> DRI folks, this seems like duplication given that this data is available
> in agpgart. How about changing this to read whatever agpgart has set in
> .chipset_name ?
> 

Sounds like a good idea to me ;-)

Best regards
Nicolas.
-- 
Nicolas Aspert      Signal Processing Institute (ITS)
Swiss Federal Institute of Technology (EPFL)

Re: [Dri-devel] Re: 2.4.20 AGP for I845 wrong ?

[Keith Whitwell]

Dave Jones wrote:
> On Wed, Dec 11, 2002 at 01:07:45PM +0100, Nicolas ASPERT wrote:
>  > IIRC, the 845G is a "new" version of the 830MP chipset (it had been
>  > added by Abraham vd Merwe & Graeme Fisher some months ago), but acts
>  > basically just as the 830MP. Therefore the entry is correct.... Or maybe
>  > if it gets confusing adding a comment would not hurt...
> 
> I'll check the chipset docs when I get time, and add a comment if
> necessary. No-one seems to be complaining that it isn't working,
> so I'm inclined to believe your diagnosis is correct.
> 
>  > > Also in drivers/char/drm/drm_agpsupport.h, the switch statement at 262 
>  > > is missing the
>  > > cases for INTEL_I830_M, INTEL_I845_G.
>  > That's true. It is also missing in 2.5.51.
>  > I attach two patches, one for 2.4.21-pre1 and one for 2.5.51 that should 
>  > fix this.
>  > diff -ru linux-2.5.51.clean/drivers/char/drm/drm_agpsupport.h linux-2.5.51/drivers/char/drm/drm_agpsupport.h
>  > --- linux-2.5.51.clean/drivers/char/drm/drm_agpsupport.h	Tue Dec 10 03:45:39 2002
>  > +++ linux-2.5.51/drivers/char/drm/drm_agpsupport.h	Wed Dec 11 12:55:08 2002
>  > @@ -271,10 +271,12 @@
>  >  #if LINUX_VERSION_CODE >= 0x02040f /* KERNEL_VERSION(2,4,15) */
>  >  	 	case INTEL_I820:	head->chipset = "Intel i820";	 break;
>  >  #endif
>  > +		case INTEL_I830_M:	head->chipset = "Intel i830M";	 break;
>  >  		case INTEL_I840:	head->chipset = "Intel i840";    break;
>  >  #if LINUX_VERSION_CODE >= 0x02040f /* KERNEL_VERSION(2,4,15) */
>  >  		case INTEL_I845:	head->chipset = "Intel i845";    break;
>  >  #endif
>  > +		case INTEL_I845:	head->chipset = "Intel i845G";	 break;
>  >  		case INTEL_I850:	head->chipset = "Intel i850";	 break;
>  >  		case INTEL_460GX:	head->chipset = "Intel 460GX";	 break;
> 
> DRI folks, this seems like duplication given that this data is available
> in agpgart. How about changing this to read whatever agpgart has set in
> .chipset_name ?
> 
> Keeping these two lists in sync seems somewhat pointless.

Yes, it's not even clear what particular use the string is.  It looks like 
it's just for the print statement at the bottom of the switch.  It would be 
safe to remove the whole thing -- agpgart has already printed out what 
hardware *it's* dealing with.

Keith

Re: [Dri-devel] Re: 2.4.20 AGP for I845 wrong ?

[Keith Whitwell]

> DRI folks, this seems like duplication given that this data is available
> in agpgart. How about changing this to read whatever agpgart has set in
> .chipset_name ?
> 

And it looks like the mechanism that drm uses for quering agp doesn't return 
the string in question.  (I don't really understand the mechanism these two 
modules use to talk to each other).

In any case I don't think the string in the informational is very useful -- 
it's a potentially inaccurate translation of state from *another* module, so 
I'm just removing the lot.

Keith

Re: [Dri-devel] Re: 2.4.20 AGP for I845 wrong ?

[Dave Jones]

On Wed, Dec 11, 2002 at 12:45:49PM +0000, Keith Whitwell wrote:
 > In any case I don't think the string in the informational is very useful -- 
 > it's a potentially inaccurate translation of state from *another* module, so 
 > I'm just removing the lot.

Cool, that gets my vote too 8-)

        Dave

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: [Dri-devel] Re: 2.4.20 AGP for I845 wrong ?

[Keith Whitwell]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

Dave Jones wrote:
> On Wed, Dec 11, 2002 at 12:45:49PM +0000, Keith Whitwell wrote:
>  > In any case I don't think the string in the informational is very useful -- 
>  > it's a potentially inaccurate translation of state from *another* module, so 
>  > I'm just removing the lot.
> 
> Cool, that gets my vote too 8-)
> 
>         Dave
> 

Here's the changes I've committed to dri cvs.

Keith

[-- Attachment #2: drm-agp-info.diff --]
[-- Type: text/plain, Size: 3831 bytes --]

? diff
Index: drmP.h
===================================================================
RCS file: /cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drmP.h,v
retrieving revision 1.54
diff -u -r1.54 drmP.h
--- drmP.h	3 Dec 2002 00:43:47 -0000	1.54
+++ drmP.h	11 Dec 2002 13:29:18 -0000
@@ -488,7 +488,6 @@
 
 typedef struct drm_agp_head {
 	agp_kern_info      agp_info;
-	const char         *chipset;
 	drm_agp_mem_t      *memory;
 	unsigned long      mode;
 	int                enabled;
Index: drm_agpsupport.h
===================================================================
RCS file: /cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_agpsupport.h,v
retrieving revision 1.9
diff -u -r1.9 drm_agpsupport.h
--- drm_agpsupport.h	22 Aug 2002 19:35:31 -0000	1.9
+++ drm_agpsupport.h	11 Dec 2002 13:29:18 -0000
@@ -260,60 +260,6 @@
 			return NULL;
 		}
 		head->memory = NULL;
-		switch (head->agp_info.chipset) {
-		case INTEL_GENERIC:	head->chipset = "Intel";         break;
-		case INTEL_LX:		head->chipset = "Intel 440LX";   break;
-		case INTEL_BX:		head->chipset = "Intel 440BX";   break;
-		case INTEL_GX:		head->chipset = "Intel 440GX";   break;
-		case INTEL_I810:	head->chipset = "Intel i810";    break;
-
-		case INTEL_I815:	head->chipset = "Intel i815";	 break;
-#if LINUX_VERSION_CODE >= 0x02040f /* KERNEL_VERSION(2,4,15) */
-	 	case INTEL_I820:	head->chipset = "Intel i820";	 break;
-#endif
-		case INTEL_I840:	head->chipset = "Intel i840";    break;
-#if LINUX_VERSION_CODE >= 0x02040f /* KERNEL_VERSION(2,4,15) */
-		case INTEL_I845:	head->chipset = "Intel i845";    break;
-#endif
-		case INTEL_I850:	head->chipset = "Intel i850";	 break;
-
-		case VIA_GENERIC:	head->chipset = "VIA";           break;
-		case VIA_VP3:		head->chipset = "VIA VP3";       break;
-		case VIA_MVP3:		head->chipset = "VIA MVP3";      break;
-		case VIA_MVP4:		head->chipset = "VIA MVP4";      break;
-		case VIA_APOLLO_KX133:	head->chipset = "VIA Apollo KX133";
-			break;
-		case VIA_APOLLO_KT133:	head->chipset = "VIA Apollo KT133";
-			break;
-
-		case VIA_APOLLO_PRO: 	head->chipset = "VIA Apollo Pro";
-			break;
-		case SIS_GENERIC:	head->chipset = "SiS";           break;
-		case AMD_GENERIC:	head->chipset = "AMD";           break;
-		case AMD_IRONGATE:	head->chipset = "AMD Irongate";  break;
-		case ALI_GENERIC:	head->chipset = "ALi";           break;
-		case ALI_M1541: 	head->chipset = "ALi M1541";     break;
-
-#if LINUX_VERSION_CODE >= 0x020402
-		case ALI_M1621: 	head->chipset = "ALi M1621";	 break;
-		case ALI_M1631: 	head->chipset = "ALi M1631";	 break;
-		case ALI_M1632: 	head->chipset = "ALi M1632";	 break;
-		case ALI_M1641: 	head->chipset = "ALi M1641";	 break;
-		case ALI_M1647: 	head->chipset = "ALi M1647";	 break;
-		case ALI_M1651: 	head->chipset = "ALi M1651";	 break;
-#endif
-
-#if LINUX_VERSION_CODE >= 0x020406
-		case SVWRKS_HE: 	head->chipset = "Serverworks HE";
-			break;
-		case SVWRKS_LE: 	head->chipset = "Serverworks LE";
-			break;
-		case SVWRKS_GENERIC: 	head->chipset = "Serverworks Generic";
-			break;
-#endif
-
-		default:		head->chipset = "Unknown";       break;
-		}
 #if LINUX_VERSION_CODE <= 0x020408
 		head->cant_use_aperture = 0;
 		head->page_mask = ~(0xfff);
@@ -321,13 +267,12 @@
 		head->cant_use_aperture = head->agp_info.cant_use_aperture;
 		head->page_mask = head->agp_info.page_mask;
 #endif
-
-		DRM_INFO("AGP %d.%d on %s @ 0x%08lx %ZuMB\n",
-			 head->agp_info.version.major,
-			 head->agp_info.version.minor,
-			 head->chipset,
-			 head->agp_info.aper_base,
-			 head->agp_info.aper_size);
+		
+		DRM_DEBUG("AGP %d.%d, aperture @ 0x%08lx %ZuMB\n",
+			  head->agp_info.version.major,
+			  head->agp_info.version.minor,
+			  head->agp_info.aper_base,
+			  head->agp_info.aper_size);
 	}
 	return head;
 }

Re: [Dri-devel] Re: 2.4.20 AGP for I845 wrong ?

[David Dawes]

On Wed, Dec 11, 2002 at 01:07:45PM +0100, Nicolas ASPERT wrote:
>Margit Schubert-While wrote:
>>  From drivers/char/agp/agpgart_be.c
>> 4554,4559
>>     { PCI_DEVICE_ID_INTEL_845_G_0,
>>                  PCI_VENDOR_ID_INTEL,
>>                  INTEL_I845_G,
>>                  "Intel",
>>                  "i845G",
>>                  intel_830mp_setup },
>> 
>> Surely this is wrong or ?
>> Should be "intel_845_setup", I think.
>> 
>
>IIRC, the 845G is a "new" version of the 830MP chipset (it had been
>added by Abraham vd Merwe & Graeme Fisher some months ago), but acts
>basically just as the 830MP. Therefore the entry is correct.... Or maybe
>if it gets confusing adding a comment would not hurt...

No, I think it should be intel_845_setup too, since the 845G docs on
Intel's public web site show that the behaviour is like the 845 when
the on-board graphics isn't enabled.  I made that change in my
locally maintained version of the agpgart driver a little while ago,
but haven't had the opportunity to test it with an external AGP card
in an 845G box yet.

David
-- 
David Dawes
Release Engineer/Architect                      The XFree86 Project
www.XFree86.org/~dawes

Re: [Dri-devel] Re: 2.4.20 AGP for I845 wrong ?

[Nicolas ASPERT]

David Dawes wrote:

> 
> No, I think it should be intel_845_setup too, since the 845G docs on
> Intel's public web site show that the behaviour is like the 845 when
> the on-board graphics isn't enabled.  I made that change in my
> locally maintained version of the agpgart driver a little while ago,
> but haven't had the opportunity to test it with an external AGP card
> in an 845G box yet.

Damn, you're right. Now I got the docs from Intel (at the time were the 
patch to support 845g was submitted, they were just not available yet), 
and truly the specs are closer to the 845, so let's switch to 
'intel_845_setup' to initialize the 845g. Not that it should change 
things too much, but it will avoid further confusions....

Best regards.

Nicolas

PS: I hope the IBM annoyances for mails sent to lkml stopped...
-- 
Nicolas Aspert      Signal Processing Institute (ITS)
Swiss Federal Institute of Technology (EPFL)

Re: 2.4.20 AGP for I845 wrong ?

[Margit Schubert-While]

DJ et al. I thought this had been cleared up on Dec 10 in this thread -

http://marc.theaimsgroup.com/?l=linux-kernel&m=103954588016915&w=2

Margit