[Xorg-driver-geode] xf86-video-geode: Changes to 'master'

[Huang, FrankR]

Thanks for your mail. I have read this part code and use DDD to debug in X server. I can answer you question as far as I can.
See my reply below.
 
> OK, let me see if I can explain:
> 
> The purpose of the *_mode_valid function in each device-dependent X
> ...[snip]
Agree.
> 
> xf86-video-ati's has many checks: for bandwidth, for clock range, and
> ...[snip]
Agree. Not all mode validation work is done here. This function just validates the hardware specific modes that X server functions(see below I list) does not validate. In xf86Crtc.c, quite a lot functions are used to validate the mode besides the driver's *_mode_valid. For example:
	Xf86ValidateModesReducedBlanking()
	Xf86ValidateModesSync()
	Xf86ValidateModesClocks()
	Xf86ValidateModesSize()
	Xf86ValidateModesFlags()
The last validation function is the *_mode_valid. From here, we can see apparently *_mode_valid function is just one of the validation functions. It is not the only one.
	
> 
> Otavio Salvador's May 29th commit adds additional requirements for a
> ...[snip]
Appreciate you can see the code carefully. You can see the code before this patch is as below:
	if(some states)
		return MODE_BAD
	...
	return MODE_OK
After the patch in 5/29 Otavio commited, the patch is as below:
	if(some states)
		return MODE_OK
	...
	return MODE_BAD
With this patch, the modeline(1024x600) in xorg.conf will be pruned by X server because of the final MODE_BAD return value. So modelines can not be used by driver. That is why I commit this simple patch(Just invert the last return value from MODE_BAD to MODE_OK). For geode, if the user uses the panel instead of VGA(If that is VGA, EDID info can be used to set 1024x600), output_modes is gotten from LXGetLegacyPanelMode(), if you look at the code, you will find the mode item(lx_panel_modes[ret]) is set by VBIOS and can not be changed. So if that item is not 1024x600, the user will not get the this resolution forever!
> 
> So, with your current code, regardless of input given to
> lx_output_mode_valid, it always tells the driver that "this mode is
> OK." This is wrong. For instance, I can ask lx_output_mode_valid
> whether the totally bogus mode of 0x0-64 at 900Hz is acceptable and it
> will gladly tell me MODE_OK (but still do a lot of work to determine
> this result). Under no circumstances does lx_output_mode_valid ever
> tell the driver that the proposed mode is bad. Clearly you can see
> that this is wrong.
Reject the "0x0-64 at 900Hz" is not only the work for *mode_valid. Intel's driver function intel_output_mode_valid() will gladly accept this too.

> 
> Because of this, Julien has (sarcastically) suggested that you change
> lx_output_mode_valid to only return MODE_OK, which is totally
> equivalent to the current code, since lx_output_mode_valid has no
> side-effects and modifies none of its arguments. This is not a real
> suggestion, as it's just as wrong as the current code.
I don't think Julien's suggestion is wrong. Just return MODE_OK is enough. I can accept it. The reason why I dis not delete other three MODE_OK return is because the patch is committed by Otavio. I think the best patch to fix this should be committed by him. I just change the last return value from MODE_BAD to MODE_OK. You should notice this.

> 
> So, what you need to do is explain what was wrong with the
> lx_output_mode_valid function before you made the change. That is,
> what proposed mode was incorrectly rejected? From here, I'm sure
> someone can help you to find the appropriate fix.
What I want to do is certainly to check which mode should be filtered out by this *_mode_valid function. I can add MODE_XXX for these conditions.
> 
> Please search for 'mode_valid' in the xserver's
> hw/xfree86/modes/xf86Crtc.c and hw/xfree86/modes/xf86RandR12.c to see
> how xf86-video-geode's lx_output_mode_valid function is called.
This part, I have debugged this several days ago. It is in xf86Crtc.c, 
		mode->status = (*output->funcs->mode_valid)(output, mode);

> 
> Do you understand and agree with this (and Matthew and Adam's)
> assessment? If not, please precisely explain why not.
Please answer my question above to persuade me firstly. Thanks!
> 
> Best of luck,
> Matt


Thanks
Frank