radeon driver, mode selection, RADEONPreInitModes()

[Benjamin Herrenschmidt]

Hi !

I'm chasing a problem where the radeon driver basically doesn't get the
proper mode from DDC on the second head (both primary and secondary
heads are DFP flat panels).

I'm having a hard time trying to understand what RADEONPreInitModes() is
supposed to do I must admit :) Whatever it does is not right in practice
though. What I've figured out so far is:

 - We never call RADEONUpdatePanelSize() on the second head for some
reason I cannot explain. That causes us to never have panel infos for
the second head

 - Later on, that absence of panel infos cause us to call
xf86ValidateModes() which seems to replace all our valid DDC modes with
crap that doesn't work (especially on this 1920x1200 panel).

 - The problematic code looks like that:

	/* Next try to add DDC modes */
	modesFound = RADEONValidateDDCModes(pScrn, pScrn->display->modes,
					    info->DisplayType, 0);

The above works... It does return a valid mode. modesFound is 1.

	/* If that fails and we're connect to a flat panel, then try to
         * add the flat panel modes
	 */
	if (info->DisplayType != MT_CRT) {

Ok, we do get here... but the above didn't fail ! The comment is misleading...
Should we add a "&& modesFound < 1" or something like that in the above if ()
statement ?
	    /* some panels have DDC, but don't have internal scaler.
	     * in this case, we need to validate additional modes
	     * by using on-chip RMX.
	     */
	    int user_modes_asked = 0, user_modes_found = 0, i;
	    DisplayModePtr  tmp_mode = pScrn->modes;
	    while (pScrn->display->modes[user_modes_asked]) user_modes_asked++;

Ok, so here, we could the modes asked in the Subsection "Display" of Section "Screen"
and we get 5 of them (that's what I have in my xorg.conf).

	    if (tmp_mode) {
		for (i = 0; i < modesFound; i++) {
		    if (tmp_mode->type & M_T_USERDEF) user_modes_found++;
		    tmp_mode = tmp_mode->next;
		}
	    }

For some strange reason, we do have M_T_USERDEF on our DDC mode (despite not
having any mode actually be user defined), so we get 1 in user_modes_found.

Ahhh .. I see .. we do actually add ourselves M_T_USERDEF to any mode in the
DDC list that is also in the Display/Screen ... in fact, we only keep those
in the list it seems (if there is a display/screen subsection, if not, we mark
all DDC modes ... hrm... interesting).

	    if ((modesFound <= 1) || (user_modes_found < user_modes_asked)) {

Ok, so here the problem starts. So I do get user_modes_found < user_modes_asked,
because I did put all those bogus modes in my display/screen subsection (but we
would get there anyway even if I didn't, since modesFound == 1). I don't understand
why we get there when modesFound <= 1. Shouldn't this be < 1 instead ?

		/* when panel size is not valid, try to validate
		 * mode using xf86ValidateModes routine
		 * This can happen when DDC is disabled.
		 */
		if (info->PanelXRes < 320 || info->PanelYRes < 200)

Ahh ... Here we are on the second head, and we didn't have any panel infos for it,
so we do get in here. So that's intersting. If we had any panel infos, we wouldn't
get there, and we wouldn't get into RADEONValidateFPModes neither since we don't
allow calling it for the secondary head.... or do we do that just _because_ we have
no panel infos ? (It relies on them). 

Anyway, the point is, depending if DDC returns one mode or more here, we get
different behaviour. If DDC returns only one mode, like it is the case for me,
(or if the user specified more modes in display/screen than DDC modes found),
we get into xf86ValidateModes(), which doesn't quite work for me. It basically
doesn't find any valid mode which makes sense since it doesn't get passed the
list of mode we obtained from DDC at all. It gets passed pScrn->monitor->Modes
which seem to contain all possible nasty mode X can think about (but not the
one I want) and pScren->display->modes which is just the requested array.

The clockRanges might be ok, but since my 1920x1200 of my flat panel just doesn't
exist in pScrn->monitor->modes, that function will just do no good.

Also, I'm a bit worried about the pitch... Might xf96ValidateModes() try to do
something about it ? Because we have called RADEONSetPitch() once in RADEONValidate
DDCMode for out biggest DDC mode already, and we may sort-of get out of sync here,
no ?

		    modesFound =
			xf86ValidateModes(pScrn,
					  pScrn->monitor->Modes,
					  pScrn->display->modes,
					  clockRanges,
					  NULL,                  /* linePitches */
					  8 * 64,                /* minPitch */
					  8 * 1024,              /* maxPitch */
					  64 * pScrn->bitsPerPixel, /* pitchInc */
					  128,                   /* minHeight */
					  2048,                  /* maxHeight */
					  pScrn->display->virtualX,
					  pScrn->display->virtualY,
					  info->FbMapSize,
					  LOOKUP_BEST_REFRESH);
		else if (!info->IsSecondary)
		    modesFound = RADEONValidateFPModes(pScrn, pScrn->display->modes);
	    }
        }

Ok, at this point, we used to have a nice mode list that sort-of worked (of 1 mode)
from DDC, and instead of just discarding the bogus user modes, we decided to call
xf86ValidateModes() which discarded ... the only correct mode we had. So we end up
with no valid mode at all.

It would have worked if 

  1) I didn't pass anything in display/screen, or only one mode: the valid one
(remember, no RMX on second head)

  2) _and_ I had DDC giving me more than one mode so we don't hit the modesFound <=1
test. I really don't understand why this is not < 1 ....

It's all very fragile though. I think the problem is that the DDC modes are not
in the list that we pass to xf86ValidateModes which makes it do crap, and that we should
have modesFound test be < 1, but then, I can't be 100% certain.

I'll need some help here, I'm quite new to this X mode management stuff, but there
is definitelu something wrong here.

Regards,
Ben.