Bug 14597

Created attachment 14470 [details]
vbios image nv34 on powerbook

bios image. If i read it correctly data[lvdsofs]=0x65, and to get the offset we should do recordlength * 1.

d.

Created attachment 14471 [details]
xorg log with randr enabled.

Xorg log with randr12 enabled.

Several observations:
-for some reason, X seems to thing 800x600 is a nice resolution. I have no clue why. I think this has been fixed in later versions as I saw a 1.4.0 log without this sillyness.
-all modes are rejected because VRefresh is out of range. Since DDC says they are all 0. Why don't we calculate a sensible VRefresh instead of asking people to remove the ranges from the xorg.conf (I may be ignorant here)
-the bitdepth of the console mode is incorrectly detected. However, from multiple test i think it only does it wrong the first time on boot (not sure). We do not seem to use this info currently.
-screen stays black. Even after fixing above problems and having it set the default DDC mode.

Created attachment 14472 [details]
reg dump randr12 inactive

register dump without randr12 running, as intialized by OF.

Created attachment 14473 [details]
reg dump randr12 active

register dump with randr12 active and when experiencing a "black" screen. The backlight is on in such case but the panel seems completely off (no slight flickering, which i can sometimes observe when a mode is set but the screen is black).

Created attachment 14474 [details] [review]
patch to provisionally work around the randr12 problems

I tried to have the registers match the values provided by OF better. Although for some reason, the calculated ppls are very different from OF (and also different from the kernel rivafb framebuffer driver) they do seem to work.

It would be nice if there be some was as to at least set some sensible virtualX value (ie matching the native size?). But perhaps it is just my old X server being broken.

The other thing is I wonder if once could not calculate a Vrefresh from DDC as to prevent the need to remove the ranges from xorg.conf.

The rest of the patch:
1) sets the manufacturerindex to 1 to get to the correct offset
2) prevents the setting of a high bit of fp_control[1] which makes my screen black if set
3) prevents running of the LVDS_RESET script. This changes the content of many registers leading again to a black screen. Perhaps we do not run the script correctly (some code there use again values got from the bios, which may be incorrect and leading to problems in both 2 and 3)

With the patch i can resize and rotate modes. Switch to console works most of the time, with the exception that the screen is slightly shifted to the left (ie, in text mode, login: appears somewhere (ca 6 chars from) the right of the display.

I have not tested the analog output yet with this patch (will do soon).

d.

Created attachment 14475 [details]
reg dump with randr12 active

register dump with patch and randr12 working.

Created attachment 14476 [details]
xorg log with randr12 enabled and working

finally the xorg.log of the working configuration.

update: with latest git, only the fp_control part of my patch is still needed.

d.

Fixed?

nope. Setting a transition frequency fixes fp_control, but causes the wrong LVDS_RESET scripts to run again.

d.

Please attach xorg logs for both before and after the default transition clock addition. Also, using the "before" case + the fp.dual_link patch (i.e. a working state), please get nv_output_save to print out all the members of regp->TMDS after they have been read in the loop, and attach that too.

Created attachment 14678 [details]
xorg.log working state

Xorg.log of a working configuration, with transition clock change commented and with regp->fp_control[nv_crtc->head] |= (8 << 28); commented.

TMDS registers printed out below "Printing TMDS registers:" string.

d.

Created attachment 14680 [details]
Xorg log transition clock change applied

This is pristine git head, with the transition clock change applied.

Note that several LVDS scripts are run which cause the screen to be black (even when switching to text mode). The TMDS regs seem to be the same as in previous log.

d.

Created attachment 14681 [details]
xorg.log without transition clock change

Xorg.log with default transition clock setting commented, but it now sets
regp->fp_control[nv_crtc->head] |= (8 << 28); which will cause a black screen.
Text mode restore still works fine in this configuration. And it is fixable "on the fly" by using radeontool to manually correct fp_control for crtc 1.

Again the outputted TMDS regs seem to be the same to me.

I hope I understood your requests correctly and this helps.



d.

Try this:

diff --git a/src/nv_bios.c b/src/nv_bios.c
index 08e4ad8..c62cfac 100644
--- a/src/nv_bios.c
+++ b/src/nv_bios.c
@@ -2828,8 +2828,15 @@ static void call_lvds_manufacturer_script(ScrnInfoPtr pScrn, int head, int dcb_e

        if (script == LVDS_PANEL_OFF)
                usleep(off_on_delay * 1000);
-       if (script == LVDS_RESET)
+       if (script == LVDS_RESET) {
+               nv32_wr(pScrn, 0x006828b4, 0x72);
+               nv32_wr(pScrn, 0x006828b0, 0x02);
+               nv32_wr(pScrn, 0x006828b4, 0x01);
+               nv32_wr(pScrn, 0x006828b0, 0x2f);
+               nv32_wr(pScrn, 0x00001588, 0);
+
                link_head_and_output(pScrn, head, dcb_entry, false);
+       }
 }

 static uint16_t clkcmptable(bios_t *bios, uint16_t clktable, int pxclk)

Ok, so afaics the problem here is something the output scripts are doing (the intention of the below patch was to put some regs back to the state they were in before running the script).

The scripts that are called are LVDS_PANEL_OFF, LVDS_RESET, and LVDS_PANEL_ON.

To confirm that the scripts are the problem, *first test that "return;"ing at the very start of call_lvds_manufacturer_script works*. If not, attach the xorg log here.

After that, do:
if (script == LVDS_PANEL_OFF || script == LVDS_PANEL_RESET)
        return;
to check if the LVDS_PANEL_ON script is broken.
If that works, do:
if (script == LVDS_PANEL_RESET)
        return;
to ensure LVDS_PANEL_OFF is ok.
If we get this far, we'll start doing the LVDS_RESET script by hand, to see what is broken in that...

So, to add some notes from IRC conversation:
LVDS_RESET is the problem causer.
It seems that even with the script disabled, link_head_and_output causes problems.

So:
Can you find what in link_head_and_output causes the problem? I'd advise something like the following (on a clean git copy):

1) double check that it works with just link_head... and the script disabled:

@@ -2838,12 +2838,13 @@ static void call_lvds_manufacturer_script(ScrnInfoPtr pScrn, int head, int dcb_e
        xf86DrvMsg(pScrn->scrnIndex, X_INFO, "Calling LVDS script %d:\n", script);
        nv_idx_port_wr(pScrn, CRTC_INDEX_COLOR, NV_VGA_CRTCX_OWNER,
                   head ? NV_VGA_CRTCX_OWNER_HEADB : NV_VGA_CRTCX_OWNER_HEADA);
-       parse_init_table(pScrn, bios, scriptofs, &iexec);
+       if (script != LVDS_RESET)
+               parse_init_table(pScrn, bios, scriptofs, &iexec);
 
        if (script == LVDS_PANEL_OFF)
                usleep(off_on_delay * 1000);
-       if (script == LVDS_RESET)
-               link_head_and_output(pScrn, head, dcb_entry);
+//     if (script == LVDS_RESET)
+//             link_head_and_output(pScrn, head, dcb_entry);
 }

2) re-enable call to link_head_and_output(), comment all five calls to nv32_wr in link_head_and_output(), and progressively re-enable, until it breaks. I'm hoping the first two are ok, and the last 3 are the problem here, but the hardware likes to defy me.

Note that changing preferred_output (p-o) is not a valid solution here, as it dictates the register set written: p-o of 0 causes the writes to go to 0x6808bX (which only affects the unused encoder on the unused output), and p-o of 1 causes them to go to 0x6828bX (the correct encoder) -- see the addition of NV_PRAMDAC0_SIZE to tmds_ctrlN.

Created attachment 14733 [details] [review]
Unrolled Apple nv34 lvds reset script

The recent commit affecting link_head_and_output() should have fixed that function (hopefully), leaving the remaining problems in something the LVDS_RESET script does.

This patch has a hand coded version of the script (taken from moondrake's xlog, but I think it's the same as sbriglie's), which will get executed instead of the script. Someone with the relevant hardware (ppc nv34 laptop) and problem needs to progressively enable the commented-out blocks and see what causes it to break.

The comment blocks are numbered in a suggested order to re-enable. Bonus points for subdividing the blocks to find the relevant line(s) causing failure.

Following some work by sbriglie, it now looks like the change in comment 15 might be sufficient (with or without the change to reg 0x1588). Testing of this idea welcome :)

(In reply to comment #19)
> Following some work by sbriglie, it now looks like the change in comment 15
> might be sufficient (with or without the change to reg 0x1588). Testing of this
> idea welcome :)
> 

After some testing I can say that the working code is this

@@ -2824,11 +2824,17 @@ static void call_lvds_manufacturer_script(ScrnInfoPtr pS
        xf86DrvMsg(pScrn->scrnIndex, X_INFO, "Calling LVDS script %d:\n", script
        nv_idx_port_wr(pScrn, CRTC_INDEX_COLOR, NV_VGA_CRTCX_OWNER,
                   head ? NV_VGA_CRTCX_OWNER_HEADB : NV_VGA_CRTCX_OWNER_HEADA);
-       parse_init_table(pScrn, bios, scriptofs, &iexec);
+       //parse_init_table(pScrn, bios, scriptofs, &iexec);
 
        if (script == LVDS_PANEL_OFF)
                usleep(off_on_delay * 1000);
        if (script == LVDS_RESET)
+               nv32_wr(pScrn, 0x006828b4, 0x72);
+               //nv32_wr(pScrn, 0x006828b0, 0x02);
+               nv32_wr(pScrn, 0x006828b4, 0x01);
+               nv32_wr(pScrn, 0x006828b0, 0x2f);
+               nv32_wr(pScrn, 0x00001588, 0);  
+               
                link_head_and_output(pScrn, head, dcb_entry);
 }

Enabling the second nv32_wr and/or the call to parse_init_table causes the usual badness

(In reply to comment #20)
> (In reply to comment #19)
> > Following some work by sbriglie, it now looks like the change in comment 15
> > might be sufficient (with or without the change to reg 0x1588). Testing of this
> > idea welcome :)
> > 
> 
> After some testing I can say that the working code is this
> 
> @@ -2824,11 +2824,17 @@ static void call_lvds_manufacturer_script(ScrnInfoPtr
> pS
>         xf86DrvMsg(pScrn->scrnIndex, X_INFO, "Calling LVDS script %d:\n",
> script
>         nv_idx_port_wr(pScrn, CRTC_INDEX_COLOR, NV_VGA_CRTCX_OWNER,
>                    head ? NV_VGA_CRTCX_OWNER_HEADB : NV_VGA_CRTCX_OWNER_HEADA);
> -       parse_init_table(pScrn, bios, scriptofs, &iexec);
> +       //parse_init_table(pScrn, bios, scriptofs, &iexec);
> 
>         if (script == LVDS_PANEL_OFF)
>                 usleep(off_on_delay * 1000);
>         if (script == LVDS_RESET)
> +               nv32_wr(pScrn, 0x006828b4, 0x72);
> +               //nv32_wr(pScrn, 0x006828b0, 0x02);
> +               nv32_wr(pScrn, 0x006828b4, 0x01);
> +               nv32_wr(pScrn, 0x006828b0, 0x2f);
> +               nv32_wr(pScrn, 0x00001588, 0);  
> +               
>                 link_head_and_output(pScrn, head, dcb_entry);
>  }
> 
> Enabling the second nv32_wr and/or the call to parse_init_table causes the
> usual badness
> 

Of course I forgot the brackets. The correct code is
if (script == LVDS_RESET){
               nv32_wr(pScrn, 0x006828b4, 0x72);
               //nv32_wr(pScrn, 0x006828b0, 0x02);
               nv32_wr(pScrn, 0x006828b4, 0x01);
               nv32_wr(pScrn, 0x006828b0, 0x2f);
               nv32_wr(pScrn, 0x00001588, 0);  
               
                 link_head_and_output(pScrn, head, dcb_entry);
 }

Yeah I realized that changing the preferred output wasn't making sense much.

I'll update to head and try the patch from comment 21 to see if it works like that for me to. I do not have the pbook with me now so it will take me a day or so to check.

d.

I'm afraid comments 20 & 21 miss the point; that I want parse_init_table to run, and any bad reg writes to be fixed up afterwards. If running parse_init_table is causing problems, then we need to go back to the code of comment 18, and try and figure out what breaks.

From sbriglie's comments on IRC, the code in comment 18 worked when the data writen at index 0x02 was changed from 0x32 to 0x72, and the suggestion of using comment 15's code was that it would be a patch run *after* the script, to fix up the contents of index 0x02 (much like link_head_and_output fixes index 0x04 currently).

I guess the step-by-step way forward here is to verify that the unrolled version in comment 18 works, when all the code is allowed to execute (no comments) and 0x32 is changed to 0x72. THEN, to change that 0x72 back to 0x32, and add 

nv32_wr(pScrn, 0x006828b4, 0x72);
nv32_wr(pScrn, 0x006828b0, 0x02);

on the end of the unrolled script as a fix-up, and check that works.

Assuming that's ok, calling parse_init_table, then calling that same fix-up code ought be have exactly the same result, and these two writes can be added as a ppc specific quirk.

Hopefully this all makes sense to you ppc people, but if I'm being incoherent please say and I'll code up a set of numbered patches of what I want testing :)

Actually you make very much sense. I didn't realize yesterday but Jacopo's comments seem inconsistent with mine and sbriglies reports on irc.

Your commit indeed fixed link_head_and_output. So it should work anyway when parse_init_table is commented, and there would be nothing to "fixup". So to me, comment 20 does not make sense. Yesterday evening i tried running with parse_init_table and indeed it does not work, but adding the lines from comment 15 fixes it. Actually only fixing reg 2 fixes it, ie:
               nv32_wr(pScrn, 0x006828b4, 0x72);
               nv32_wr(pScrn, 0x006828b0, 0x02);
               nv32_wr(pScrn, 0x00001588, 0); 
is enough for me.

Since this wonderfully matches your last comment I think we've solved it. Apart from how do you want to check _when_ to apply this fix and when not. It so far was only reported on powerbooks, but the hw components are usually not only specific to apple and could be used elsewhere. In addition, some types of powerbooks have different hw.

d.

Powerbook specific quirk applied to git. Let me know how it works for you

(In reply to comment #25)
> Powerbook specific quirk applied to git. Let me know how it works for you
> 

As I wrote in IRC too it works by changing
-if (pNv->Chipset & 0xffff == 0x0329) {
+if ((pNv->Chipset & 0xffff) == 0x0329) {

otherwise the if condition is always false and the fix never gets executed.

Btw moondrake, I am sbriglie :) On sunday (comments #20 and #21) I was probably doing something very dumb with the brackets, that's almost certainly why things didn't work for me back then. 

So hopefully this is fixed :) 

Also works fine for me. I close it then.

btw, 0x72 is just one bit different from 0x32, so I think it probably "means" something. What that is exactly, I do not know, but to me it seems unlikely to be powerpc or chip specific. However, for now, i am happy with this as a powerbook quirk.

danny