答复: [PATCH]siliconmotion new driver initial patch
Hi Alan, Thank you for your reply. We will change the configure.ac/Makefile.am. But one more thing I need your advice. This is a initial patch. There are many code has been changed. In order to make the patch smaller shall I separate the patch into small parts? What kind of small parts shall I made? Aaron -邮件原件- 发件人: Alan Coopersmith [mailto:alan.coopersm...@oracle.com] 发送时间: 2012年8月13日 22:35 收件人: Aaron.Chen 陈俊杰 抄送: xorg-devel@lists.x.org 主题: Re: [PATCH]siliconmotion new driver initial patch On 08/12/12 11:07 PM, Aaron.Chen 陈俊杰 wrote: I didn't found this mail show up on the xorg-devel mail list and there is no response. Maybe it didn't sent successfully, so I sent it again. 2.2 MB mails require moderator approval to be sent to the mailing list. Please don't send multiple copies of multi-megabyte mails. Though really, you can also assume sending a single 2.2MB patch is a waste of time that no one will ever review, especially since the first few files shown continue to repeat mistakes we already told you we would not accept, such as undoing past cleanups of configure.ac/Makefile.am to revert them to older, less functional states. -- -Alan Coopersmith- alan.coopersm...@oracle.com Oracle Solaris Engineering - http://blogs.oracle.com/alanc ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 1/3]siliconmotion new driver initial patch 1
Hi, We have updated the copyrights and repatched the whole new driver into 3 parts. Here is the first one. man/Makefile.am | 21 ++--- 1 files changed, 18 insertions(+), 3 deletions(-) diff --git a/man/Makefile.am b/man/Makefile.am index e1182ee..c6c85ab 100644 --- a/man/Makefile.am +++ b/man/Makefile.am @@ -31,10 +31,25 @@ EXTRA_DIST = @DRIVER_NAME@.man CLEANFILES = $(driverman_DATA) -# String replacements in MAN_SUBSTS now come from xorg-macros.m4 via configure - +SED = sed + +# Strings to replace in man pages +XORGRELSTRING = @PACKAGE_STRING@ + XORGMANNAME = X Version 11 + +MAN_SUBSTS = \ + -e 's|__vendorversion__|$(XORGRELSTRING) $(XORGMANNAME)|' \ + -e 's|__xorgversion__|$(XORGRELSTRING) $(XORGMANNAME)|' \ + -e 's|__xservername__|Xorg|g' \ + -e 's|__xconfigfile__|xorg.conf|g' \ + -e 's|__projectroot__|$(prefix)|g' \ + -e 's|__appmansuffix__|$(APP_MAN_SUFFIX)|g' \ + -e 's|__drivermansuffix__|$(DRIVER_MAN_SUFFIX)|g' \ + -e 's|__adminmansuffix__|$(ADMIN_MAN_SUFFIX)|g' \ + -e 's|__miscmansuffix__|$(MISC_MAN_SUFFIX)|g' \ + -e 's|__filemansuffix__|$(FILE_MAN_SUFFIX)|g' SUFFIXES = .$(DRIVER_MAN_SUFFIX) .man .man.$(DRIVER_MAN_SUFFIX): - $(AM_V_GEN)$(SED) $(MAN_SUBSTS) $ $@ + sed $(MAN_SUBSTS) $ $@ -- 1.7.5.4 Aaron -邮件原件- 发件人: Alan Coopersmith [mailto:alan.coopersm...@oracle.com] 发送时间: 2012年7月21日 1:35 收件人: Aaron.Chen 陈俊杰 抄送: Michal Suchanek; Matt Turner; caesar.qiu 裘赛海; mill.chen 陈军; xorg-devel@lists.x.org; Paul.Chen 陈波; Ilena.Zhou周菁华 主题: Re: 答复: 答复: [PATCH]new driver for siliconmotion On 07/20/12 02:56 AM, Aaron.Chen 陈俊杰 wrote: 2. man/Makefile.am | 64 +- You replaced the Oracle copyright notice with the old Sun copyright notice. Clearly wrong, and it makes it clear that this wasn't well reviewed or rebased. We've replaced old Sun copyright notice with the Oracle copyright notice. Is it OK? For this and several other points the correct answer is not to change things that aren't part of your changes, but to use the latest code base and only submit patches for things you are intentionally changing as part of your submission - your patches should not show any change to Sun or Oracle copyrights, as they're not part of what you should be changing. This should involve rebasing your git trees to the current head of the master branch - if you're not using git to manage these changes, you'll find it incredibly difficult to integrate upstream on a regular basis. 5. +* Copyright (c) 2007 by Silicon Motion, Inc. (SMI) +* +* All rights are reserved. Reproduction or in part is prohibited +* without the written consent of the copyright owner. That doesn't look good. We will use the same old format:Copyright (C) Silicon Motion, Inc. All Rights Reserved. Is this OK? No. It's not the copyright notice that matters but the license terms afterwards, which must be an open source license notice. Reproduction or in part is prohibited is completely unacceptable - why would we accept code your notice clearly says we cannot use? Your company's lawyers need to provide you with the license statement your company is willing to provide the code under - X.Org prefers the MIT/X11 license, as you can see in lots of our existing code - the current preferred form is: Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the Software), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions: The above copyright notice and this permission notice (including the next paragraph) shall be included in all copies or substantial portions of the Software. THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. but we will accept certain variations on that form as long as they allow others to reuse the code without further complications. -- -Alan Coopersmith- alan.coopersm...@oracle.com Oracle Solaris Engineering - http://blogs.oracle.com/alanc 0001-siliconmotion-new-driver-initial-patch-1.patch Description: 0001-siliconmotion-new-driver-initial-patch-1.patch ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info:
[PATCH 3/3]siliconmotion new driver initial patch 3
We've updated the configuration for compiling because the change of source code. Signed-off-by: Aaron Chen aaron.c...@siliconmotion.com --- COPYING | 20 + Makefile.am | 46 ++--- configure.ac | 128 +++--- 3 files changed, 110 insertions(+), 84 deletions(-) mode change 100755 = 100644 autogen.sh 0003-siliconmotion-new-driver-initial-patch-3.patch Description: 0003-siliconmotion-new-driver-initial-patch-3.patch ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 1/4]new driver initial patch
Hi, From 0766145c2e2134325b45072e8e52b9bef7ff463b Mon Sep 17 00:00:00 2001 From: Aaron Chen aaron.c...@siliconmotion.com Date: Wed, 1 Aug 2012 16:26:24 +0800 Subject: [PATCH 1/4] new driver initial patch 1 modified man folder, which is suitable for the new driver. Signed-off-by: Aaron.Chen aaron.c...@siliconmotion.com --- man/Makefile.am | 21 ++--- 1 files changed, 18 insertions(+), 3 deletions(-) diff --git a/man/Makefile.am b/man/Makefile.am index e1182ee..c6c85ab 100644 --- a/man/Makefile.am +++ b/man/Makefile.am @@ -31,10 +31,25 @@ EXTRA_DIST = @DRIVER_NAME@.man CLEANFILES = $(driverman_DATA) -# String replacements in MAN_SUBSTS now come from xorg-macros.m4 via configure - +SED = sed + +# Strings to replace in man pages +XORGRELSTRING = @PACKAGE_STRING@ + XORGMANNAME = X Version 11 + +MAN_SUBSTS = \ + -e 's|__vendorversion__|$(XORGRELSTRING) $(XORGMANNAME)|' \ + -e 's|__xorgversion__|$(XORGRELSTRING) $(XORGMANNAME)|' \ + -e 's|__xservername__|Xorg|g' \ + -e 's|__xconfigfile__|xorg.conf|g' \ + -e 's|__projectroot__|$(prefix)|g' \ + -e 's|__appmansuffix__|$(APP_MAN_SUFFIX)|g' \ + -e 's|__drivermansuffix__|$(DRIVER_MAN_SUFFIX)|g' \ + -e 's|__adminmansuffix__|$(ADMIN_MAN_SUFFIX)|g' \ + -e 's|__miscmansuffix__|$(MISC_MAN_SUFFIX)|g' \ + -e 's|__filemansuffix__|$(FILE_MAN_SUFFIX)|g' SUFFIXES = .$(DRIVER_MAN_SUFFIX) .man .man.$(DRIVER_MAN_SUFFIX): - $(AM_V_GEN)$(SED) $(MAN_SUBSTS) $ $@ + sed $(MAN_SUBSTS) $ $@ -- 1.7.5.4 -邮件原件- 发件人: Michal Suchanek [mailto:hramr...@gmail.com] 发送时间: 2012年7月20日 18:43 收件人: Aaron.Chen 陈俊杰 抄送: Matt Turner; xorg-devel@lists.x.org; caesar.qiu 裘赛海; Ilena.Zhou周菁华; Paul.Chen 陈波; mill.chen 陈军 主题: Re: 答复: 答复: [PATCH]new driver for siliconmotion On 20 July 2012 11:56, Aaron.Chen 陈俊杰 aaron.c...@siliconmotion.com wrote: Hi MattMichal, Thank you for your advice. We are updating our codes in order to submit the source code successfully. We are doing the following changes. Next patch will be in the end of this month. (by the way, shall I make a new patch based on older source code or based on last patch?) The patch(es) should be based on the current upstream smi driver git tree ( http://cgit.freedesktop.org/xorg/driver/xf86-video-siliconmotion/ in this case). This is somewhat implied but not explicitly written on http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches 1. configure.ac| 81 +- There are a lot of useless changes in configure.ac, like lowering the AC_PREREQ number and changing the bugzilla link. Also, the version number in configure.ac is wrong. We are updating the file. 2. man/Makefile.am | 64 +- You replaced the Oracle copyright notice with the old Sun copyright notice. Clearly wrong, and it makes it clear that this wasn't well reviewed or rebased. We've replaced old Sun copyright notice with the Oracle copyright notice. Is it OK? 3. src/CALLMAP | 22 + This is a useless file that was probably removed from git a long time ago. Don't add it back. The file has been deleted. 4. src/Imakefile | 116 + WTF? No. Just no. I see that the driver has RANDR support. No version of the X server ever had an Imake build system and also RANDR support. Remove this Imakefile. The file has been deleted. All the above issues discovered by the OP show that you have not merged your code properly with upstream git code. That is, you add back obsolete files that have been removed previously or go back to old revisions of files that have been updated in the git repository. You need to fix not only the issues explicitly mentioned above but review the patch for possible other similar problems. 5. +* Copyright (c) 2007 by Silicon Motion, Inc. (SMI) +* +* All rights are reserved. Reproduction or in part is prohibited +* without the written consent of the copyright owner. That doesn't look good. We will use the same old format:Copyright (C) Silicon Motion, Inc. All Rights Reserved. Is this OK? The part that is definitely not OK is Reproduction or in part is prohibited +* without the written consent of the copyright owner. Thanks Michal 0001-new-driver-initial-patch-1.patch Description: 0001-new-driver-initial-patch-1.patch ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
答复: [PATCH 3/4]new driver initial patch 3
Hi, From 7b54308ec7f2d7f2515298aa2989ea21a89161ed Mon Sep 17 00:00:00 2001 From: Aaron Chen aaron.c...@siliconmotion.com Date: Wed, 1 Aug 2012 16:58:35 +0800 Subject: [PATCH 3/4] new driver initial patch 3 new driver added to the source tree and deleted the old useless files.These files works for all of the smi graphics chips Signed-off-by: Aaron Chen aaron.c...@siliconmotion.com --- src/Makefile.am | 107 +- src/smi_accel.c | 1577 -- src/smi_accel.h | 195 ++ src/smi_common.c | 32 + src/smi_common.h | 714 ++ src/smi_crtc.c | 310 ++- src/smi_crtc.h | 75 +- src/smi_dbg.h| 51 + src/smi_driver.c | 4072 - src/smi_driver.h | 98 + src/smi_output.c | 245 ++- src/smi_output.h | 45 + src/smi_ver.h| 42 + src/smi_video.c | 6777 +- src/smi_video.h | 302 ++- src/version.h| 38 + 16 files changed, 9431 insertions(+), 5249 deletions(-) Aaron ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
答复: [PATCH 4/4]new driver initial patch 4
Hi, From dc32739681307e54455464c51611bafa6274 Mon Sep 17 00:00:00 2001 From: Aaron Chen aaron.c...@siliconmotion.com Date: Wed, 1 Aug 2012 17:15:19 +0800 Subject: [PATCH 4/4] new driver initial patch 4 Delete useless files. Signed-off-by: Aaron Chen aaron.c...@siliconmotion.com --- Makefile.am | 11 +++ configure.ac | 94 -- 2 files changed, 57 insertions(+), 48 deletions(-) Aaron 0004-new-driver-initial-patch-4.patch Description: 0004-new-driver-initial-patch-4.patch ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
答复: 答复: [PATCH]new driver for siliconmotion
Hi MattMichal, Thank you for your advice. We are updating our codes in order to submit the source code successfully. We are doing the following changes. Next patch will be in the end of this month. (by the way, shall I make a new patch based on older source code or based on last patch?) 1. configure.ac| 81 +- There are a lot of useless changes in configure.ac, like lowering the AC_PREREQ number and changing the bugzilla link. Also, the version number in configure.ac is wrong. We are updating the file. 2. man/Makefile.am | 64 +- You replaced the Oracle copyright notice with the old Sun copyright notice. Clearly wrong, and it makes it clear that this wasn't well reviewed or rebased. We've replaced old Sun copyright notice with the Oracle copyright notice. Is it OK? 3. src/CALLMAP | 22 + This is a useless file that was probably removed from git a long time ago. Don't add it back. The file has been deleted. 4. src/Imakefile | 116 + WTF? No. Just no. I see that the driver has RANDR support. No version of the X server ever had an Imake build system and also RANDR support. Remove this Imakefile. The file has been deleted. 5. +* Copyright (c) 2007 by Silicon Motion, Inc. (SMI) +* +* All rights are reserved. Reproduction or in part is prohibited +* without the written consent of the copyright owner. That doesn't look good. We will use the same old format:Copyright (C) Silicon Motion, Inc. All Rights Reserved. Is this OK? I guess this is not really a requirement but makes the code needlessly unattractive. Since this is an initial submission it is the right time to change this. If you look at the other drivers they have rather simple structure with all files in a single directory. Large part of the code is shared. When support for new chipset family is added a few files specific to that chipset are typically required which have the chipset name in their file name but much of the rest is just updated slightly. Now you add support for half dozen of chips and I have no idea how they are related to each other and to the chips previously supported by the smi driver. You should know the best since you wrote the code. However, the file naming suggests that you started with separate driver for every chip and just added hooks in the old driver to call one or the other with very little sharing. This has both advantages and drawbacks but with few people working on the code the less needs to be maintained the better. Then sharing as much code as reasonably possible makes sense, and making bazillion subdirectories does not help that. We've talked about this issue. We prefer not to change the driver's structure. There is a lots of reason why we decide to separate the ddk file for each chip, and we decide to separate the code in the beginning of the driver design. So we need to confirm one thing: Is this driver ok to be added into the X.Org source tree or we must change the codes structure otherwise we cannot be accepted. Is there anything else we need to change to be accepted by the X.Org. We really appreciate your advice. Thank you so much! Aaron -邮件原件- 发件人: Michal Suchanek [mailto:hramr...@gmail.com] 发送时间: 2012年7月17日 20:15 收件人: Aaron.Chen 陈俊杰 抄送: Matt Turner; xorg-devel@lists.x.org; caesar.qiu 裘赛海; Ilena.Zhou周菁华 主题: Re: 答复: [PATCH]new driver for siliconmotion On 17 July 2012 10:31, Aaron.Chen 陈俊杰 aaron.c...@siliconmotion.com wrote: Hi Matt, We really appreciate your advice! The project is very important to us! We have worked for the project for two years. It can support all the SMI graphics chips and works OK on FC, SUSE, Ubuntu Red Hat, etc. for both 32 and 64 bit OS. The code contains two different types of driver. One is support XRandr and the other is not which is for multi-adapter. So it may remain some old structure files. And one more question asked by our develop team: Do we really have to prefix all these files and directories with 'ddk'? So, We'd better change all the name of files named ddk*. Is that right? Aaron I guess this is not really a requirement but makes the code needlessly unattractive. Since this is an initial submission it is the right time to change this. If you look at the other drivers they have rather simple structure with all files in a single directory. Large part of the code is shared. When support for new chipset family is added a few files specific to that chipset are typically required which have the chipset name in their file name but much of the rest is just updated slightly. Now you add support for half dozen of chips and I have no idea how they are related to each other and to the chips previously supported by the smi driver. You should know the best since you wrote the code. However, the file naming suggests that you started with separate driver for every chip and just added hooks in the old
答复: [PATCH]new driver for siliconmotion
Hi Matt, We really appreciate your advice! The project is very important to us! We have worked for the project for two years. It can support all the SMI graphics chips and works OK on FC, SUSE, Ubuntu Red Hat, etc. for both 32 and 64 bit OS. The code contains two different types of driver. One is support XRandr and the other is not which is for multi-adapter. So it may remain some old structure files. And one more question asked by our develop team: Do we really have to prefix all these files and directories with 'ddk'? So, We'd better change all the name of files named ddk*. Is that right? Aaron -邮件原件- 发件人: Aaron.Chen 陈俊杰 发送时间: 2012年7月12日 10:17 收件人: 'Matt Turner' 抄送: xorg-devel@lists.x.org; caesar.qiu 裘赛海 主题: 答复: [PATCH]new driver for siliconmotion Hi Matt, Thank you for your review. We really appreciate that you've pick out so many issues we need to improve. It seems that we still have a lot of work to do to match the quality which can be accepted by X.Org. We will fix the issue you've reported before next submission. One more question: How many patches shall I make instead of one big patch? Aaron -邮件原件- 发件人: Matt Turner [mailto:matts...@gmail.com] 发送时间: 2012年7月12日 0:32 收件人: Aaron.Chen 陈俊杰 抄送: xorg-devel@lists.x.org; caesar.qiu 裘赛海 主题: Re: [PATCH]new driver for siliconmotion On Tue, Jul 10, 2012 at 8:24 PM, Matt Turner matts...@gmail.com wrote: 64 Megabytes in bytes is too large for a signed int. This code cannot work. It's clear that someone thought about this, since there's a more-correct commented-out function signature above it. Math fail. Ignore this hunk. The commented-out signature should be removed though. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
答复: [PATCH]new driver for siliconmotion
Hi Matt, Thank you for your review. We really appreciate that you've pick out so many issues we need to improve. It seems that we still have a lot of work to do to match the quality which can be accepted by X.Org. We will fix the issue you've reported before next submission. One more question: How many patches shall I make instead of one big patch? Aaron -邮件原件- 发件人: Matt Turner [mailto:matts...@gmail.com] 发送时间: 2012年7月12日 0:32 收件人: Aaron.Chen 陈俊杰 抄送: xorg-devel@lists.x.org; caesar.qiu 裘赛海 主题: Re: [PATCH]new driver for siliconmotion On Tue, Jul 10, 2012 at 8:24 PM, Matt Turner matts...@gmail.com wrote: 64 Megabytes in bytes is too large for a signed int. This code cannot work. It's clear that someone thought about this, since there's a more-correct commented-out function signature above it. Math fail. Ignore this hunk. The commented-out signature should be removed though. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
