Bug 11652

Created attachment 10787 [details] [review]
patch #1

Created attachment 10788 [details] [review]
patch #2

Created attachment 10789 [details] [review]
patch #3

Created attachment 10790 [details]
patch #4

JFYI, patches #3/#4 are exactly the same.

It looks like a lot of this is already included in mga driver.  Can you check the latest git version and verify that it includes all the necessary fixes and if not, submit a patch against git master?

http://gitweb.freedesktop.org/?p=xorg/driver/xf86-video-mga.git;a=summary

Ben, could you verify this?

Most of patch 1 is already merged.  Some bits aren't.

The change to MGAdoDDC appears to turn off DDC on G200SE.  This seems intensely foolish, unless G200SE really doesn't have an I2C bus.  Pretty sure it does though.  I have no intention of merging this without explanation.

The change to MGAPreInit to disallow modes larger than 1600x1200 for G200SE-A will simply fail preinit completely, rather than just filtering away modes that big.  Also, it's broken:

+    if ((pScrn->modes->VDisplay > 1600) && 
+        (pScrn->modes->HDisplay > 1200) &&
+        (pMga->is_G200SE_A)) {
+        return FALSE;
+    }

That's rejecting taller than 1600 and wider than 1200; pretty sure you meant to do taller than 1200 _or_ wider than 1600.  I've added a check for this in MGAValidMode().

The changes to mga.h add a #define MAX_BW that's never used.  Why?

The MAX_BW thing in the first patch is apparently a bandwidth limitation that's used in the second patch to reject large video modes.  But, again, doing this outside of MGAValidMode, which means X will just fail to start instead of merely rejecting the bad modes.

Then there's this gem:

+    MGA_NOT_HAL(
+        if (pMga->is_G200SE) {
+            CARD8 value = 0x14;
+            if (pMga->is_G200SE_A) {
+                if (INREG(0x1E24) == 0x1) {
+                    value = 0x03;
+                }
+            }
+            OUTREG8(0x1FDE, 0x06);
+            OUTREG8(0x1FDF, value);
+        }
+    );

Based on the usage of 0x1E24 elsewhere, I'm going to assume this is setting up memory type or timing parameters, but comments would be helpful.

Also:

     if (pMga->is_G200SE) {
 	VRTemp = pScrn->videoRam;
 	FBTemp = pMga->FbMapSize;
-	pScrn->videoRam = 4096;
+	pScrn->videoRam = 8192;
 	pMga->FbMapSize = pScrn->videoRam * 1024;
     }

Pretty sure the card I've got isn't 8M.  Shame we can't tell this some more accurate way...

The modes greater than 1600x1200 are indeed blocked, but in the local current version i have i have also added maxHeight field to have xf86ValdiateMode to remove unwanted resolution for this card so it should never need to exit at the 1600x1200 check. 

MAX_BW is indeed used as a bandwidth limitation check to make sure the card doesn't try to set up for a resolution it can't handle.

The 0x1E24 check is to verify which pilot card is being used. And is used mainly to setup the hipri value of the card for use in a non hal enabled configuration as well as for when we check the max bandwidth for the latest board version of pilot.

The memory value hack is done for the HAL to setup it up in the screeninit function.

As to DDC when the patch was submitted it wasn't functional. Support for it will be added through the HAL at some point.

(In reply to comment #10)
> The modes greater than 1600x1200 are indeed blocked, but in the local current
> version i have i have also added maxHeight field to have xf86ValdiateMode to
> remove unwanted resolution for this card so it should never need to exit at the
> 1600x1200 check.

This "local current version", is it in CVS or something?  I would really like to see a change history for Matrox' version of mga(4) so I can be sure we've merged everything over that we should have.

> MAX_BW is indeed used as a bandwidth limitation check to make sure the card
> doesn't try to set up for a resolution it can't handle.

I've committed a version of this that uses the same mode bandwidth computation as the other drivers.

> The 0x1E24 check is to verify which pilot card is being used. And is used
> mainly to setup the hipri value of the card for use in a non hal enabled
> configuration as well as for when we check the max bandwidth for the latest
> board version of pilot.

Applied.

> The memory value hack is done for the HAL to setup it up in the screeninit
> function.

Applied.

> As to DDC when the patch was submitted it wasn't functional. Support for it
> will be added through the HAL at some point.

Applied, although nngh.  I2C is not magic, I don't know why you'd bother to hide it in the HAL.

This is probably as fixed as it's going to get in git.

(In reply to comment #11)
> (In reply to comment #10)
> > The modes greater than 1600x1200 are indeed blocked, but in the local current
> > version i have i have also added maxHeight field to have xf86ValdiateMode to
> > remove unwanted resolution for this card so it should never need to exit at the
> > 1600x1200 check.
> 
> This "local current version", is it in CVS or something?  I would really like
> to see a change history for Matrox' version of mga(4) so I can be sure we've
> merged everything over that we should have.

This would still be really nice to have.  But I suspect I can just hack something up with successive git imports of the matrox release tarballs.
 
> > As to DDC when the patch was submitted it wasn't functional. Support for it
> > will be added through the HAL at some point.
> 
> Applied, although nngh.  I2C is not magic, I don't know why you'd bother to
> hide it in the HAL.

DDC is still broken for this chip, and hiding it in HAL is utterly incomprehensible.  The HAL library has completely outlived its usefulness now.