xf86-video-geode: Changes to 'master'

[Huang, FrankR]

Sorry for late reply. I am out of office because of the national festival holiday.
First of all, I can not accept the manner you used in your mail on this issue. Every change has its reason in the world. I think Your "wrong wrong wrong" conclusion is too early. Science(absolutely include the software codes) has no absolute wrong and right. We can discuss and give the reason. If we are wrong, we can accept. Furthermore, have you gotten this issue very clear before you give this conclustion?
Let me put it in detail.
I have said it very exactly in previous mail, what I changed on code is based on the patch committed by Otavio on 5/29/2010. This patch has the limitation that modelines in xorg.conf can not be supported by the X server. The modes in xong.conf added by users will be pruned by X server because of the final return value MODE_BAD. Users can not get 1024x600 with the current code in git repository(For example, Ubuntu bugzilla #433142). About the patch in 5/29/2010, I have a further discussion with Otavio in geode IRC channel. He admitted that there is a limitation here and agreed with my point to change the last return value to MODE_OK that can let the modelines in xorg.conf be acceptted.
Next, I modified the code after I referred the mode_valid function in ATI&&INTEL drivers. In your mail, you said "we check all success condition", that is right. Because we have not known how to add code to prune which mode should be deleted. And Otavio's patch gives some MODE_OK conditions on 5/29 patch. So currectly in git repository, you see all conditions return MODE_OK. You said "other drivers check for failure conditions before return MODE_OK", that is not correct. You can see the code in ATI driver, 
http://cgit.freedesktop.org/xorg/driver/xf86-video-ati/tree/src/radeon_output.c
function radeon_mode_valid(), there are several MODE_OK return before the final MODE_OK return. So put some MODE_OK in geode driver, I don't think there is something wrong. We can add MODE_XXX in the future for the modes we can not supported. And I think that is why Otavio said "It is not equivalent". Otavio, am I right? For Intel driver,
http://cgit.freedesktop.org/xorg/driver/xf86-video-intel/tree/src/intel_display.c
function intel_output_mode_valid(), it is more simple with only one MODE_ PANEL return before the final MODE_OK return. Why we should put so much restrictions in this function?
For the mode you give for 1920x1440 120hz", I don't validate if that can be accepted in the geode HW. If that can not be supported by geode and X server does not prune that mode, we can add code in mode_valid to return MODE_XXX that leads to be pruned. I think it is not conflict with currect code. There is a balance for the risk and users' demand.

Thanks,
Frank
 

> -----Original Message-----
> From: xorg-devel-bounces+frankr.huang=amd.com at lists.x.org [mailto:xorg-
> devel-bounces+frankr.huang=amd.com at lists.x.org] On Behalf Of Adam Jackson
> Sent: 2010年9月30日 22:14
> To: Huang, FrankR
> Cc: Geode Mailing List; q-funk at iki.fi; xorg-devel at lists.x.org; Otavio
> Salvador; Julien Cristau
> Subject: RE: xf86-video-geode: Changes to 'master'
> 
> On Thu, 2010-09-30 at 15:49 +0800, Huang, FrankR wrote:
> > So we can keep the current git code with no change.
> 
> You can, but the code is still _wrong_.  Wrong wrong wrong.  Your
> hardware has real constraints; you have finite memory bandwidth, and
> finite DAC speed, and finite CRTC addressibility.  If I try this
> modeline on your hardware:
> 
> atropine:~% gtf 1920 1440 120
> 
>   # 1920x1440 @ 120.00 Hz (GTF) hsync: 185.16 kHz; pclk: 497.71 MHz
>   Modeline "1920x1440_120.00"  497.71  1920 2088 2304 2688  1440 1441 1444
> 1543  -HSync +Vsync
> 
> it will not work.  The code as you've now changed it will happily accept
> this mode.
> 
> And your commit message is wrong too.  The other drivers you mention
> check for failure conditions before returning MODE_OK.  You're checking
> for _success_ conditions and then returning MODE_OK all the time.
> Textually these look very similar; semantically they're completely
> different.
> 
> Think.  Please.
> 
> - ajax