Re: [PATCHv2 00/21] marvell-ccic: drop and fix formats
On 03/13/2015 10:59 PM, Hans Verkuil wrote: Hi Jon, On 03/13/2015 10:28 PM, Jonathan Corbet wrote: On Wed, 11 Mar 2015 09:10:24 +0100 Hans Verkuil hverk...@xs4all.nl wrote: After some more testing I realized that the 422P format produced wrong colors and I couldn't get it to work. Since it never worked and nobody complained about it (and it is a fairly obscure format as well) I've dropped it. I'm not sure how that format came in anymore; I didn't add it. No objections to its removal. It came in with the patches from Marvell. I also tested RGB444 format for the first time, and that had wrong colors as well, but that was easy to fix. Finally there was a Bayer format reported, but it was never implemented. So that too was dropped. The RGB444 change worries me somewhat; that was the default format on the XO1 and worked for years. I vaguely remember some discussions about the ordering of the colors there, but that was a while ago. Did you test it with any of the Sugar apps? I've tested with the 'Record' app, and that picks a YUV format, not RGB444. Are there other apps that I can test with where you can select the capture format? Urgh. I did some more digging and this driver really supported a big endian version of RGB444. So the description in the documentation of the RGB444 format and what this driver returns is different. It looks like Michael Schimek's question regarding endianness went unanswered: http://www.spinics.net/lists/vfl/msg28921.html He probably assumed the same order as for RGB555/565 formats. I have three options: 1) fix the driver as I did in my patch so RGB444 follows the documentation. 2) add a new RGB444X big endian pixel format and switch the driver to that. So RGB444 is no longer supported, instead RGB444X is now supported. Apps will have to change PIX_FMT_RGB444 to PIX_FMT_RGB444X. 3) add support for both RGB444 and RGB444X to the driver. Note that it is not possible to change the RGB444 documentation since this format is used in other drivers as well, and there it is in proper little endian format. I am actually favoring option 2, since that prevents current applications using RGB444 from working with the new kernel, but it is easy to fix by changing RGB444 to RGB444X. OLPC specific apps can even just assume that RGB444 and RGB444X are the same, and so they will work with both the new and old driver. Let me know what you prefer. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 00/21] marvell-ccic: drop and fix formats
On 03/14/2015 10:15 AM, Hans Verkuil wrote: On 03/13/2015 10:59 PM, Hans Verkuil wrote: Hi Jon, On 03/13/2015 10:28 PM, Jonathan Corbet wrote: On Wed, 11 Mar 2015 09:10:24 +0100 Hans Verkuil hverk...@xs4all.nl wrote: After some more testing I realized that the 422P format produced wrong colors and I couldn't get it to work. Since it never worked and nobody complained about it (and it is a fairly obscure format as well) I've dropped it. I'm not sure how that format came in anymore; I didn't add it. No objections to its removal. It came in with the patches from Marvell. I also tested RGB444 format for the first time, and that had wrong colors as well, but that was easy to fix. Finally there was a Bayer format reported, but it was never implemented. So that too was dropped. The RGB444 change worries me somewhat; that was the default format on the XO1 and worked for years. I vaguely remember some discussions about the ordering of the colors there, but that was a while ago. Did you test it with any of the Sugar apps? I've tested with the 'Record' app, and that picks a YUV format, not RGB444. Are there other apps that I can test with where you can select the capture format? Urgh. I did some more digging and this driver really supported a big endian version of RGB444. So the description in the documentation of the RGB444 format and what this driver returns is different. OK, after some more research it turns out that it is not actually the big endian format at all. This is the actual format compared to RGB444 and big endian RGB444: Byte 0 in memoryByte 1 in memory RGB444 RGB444 BE Marvell RGB444 Basically RGB444 but with R and B reversed. It looks like Michael Schimek's question regarding endianness went unanswered: http://www.spinics.net/lists/vfl/msg28921.html He probably assumed the same order as for RGB555/565 formats. I have three options: 1) fix the driver as I did in my patch so RGB444 follows the documentation. 2) add a new RGB444X big endian pixel format and switch the driver to that. So RGB444 is no longer supported, instead RGB444X is now supported. Apps will have to change PIX_FMT_RGB444 to PIX_FMT_RGB444X. 3) add support for both RGB444 and RGB444X to the driver. I stick with this proposal, except instead of an RGB444X (big endian) version I create a BGR444 format. BTW, I'm using this code written for the OLPC as a reference as to what apps expect: https://bitbucket.org/pygame/pygame/src/db67108d6a8e6064884549f471c22fab21bdbfa6/src/_camera.c?at=default At line 491-495 it is clear what format is expected. Regards, Hans Note that it is not possible to change the RGB444 documentation since this format is used in other drivers as well, and there it is in proper little endian format. I am actually favoring option 2, since that prevents current applications using RGB444 from working with the new kernel, but it is easy to fix by changing RGB444 to RGB444X. OLPC specific apps can even just assume that RGB444 and RGB444X are the same, and so they will work with both the new and old driver. Let me know what you prefer. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 00/21] marvell-ccic: drop and fix formats
On Wed, 11 Mar 2015 09:10:24 +0100 Hans Verkuil hverk...@xs4all.nl wrote: After some more testing I realized that the 422P format produced wrong colors and I couldn't get it to work. Since it never worked and nobody complained about it (and it is a fairly obscure format as well) I've dropped it. I'm not sure how that format came in anymore; I didn't add it. No objections to its removal. I also tested RGB444 format for the first time, and that had wrong colors as well, but that was easy to fix. Finally there was a Bayer format reported, but it was never implemented. So that too was dropped. The RGB444 change worries me somewhat; that was the default format on the XO1 and worked for years. I vaguely remember some discussions about the ordering of the colors there, but that was a while ago. Did you test it with any of the Sugar apps? In the end, correctness is probably the right way to go (it usually is!), but I'd hate to get a regression report from somebody who is crazy enough to put current kernels on those machines. Fortunately, such people should be rare. Bayer sort-of worked once, honest. I added it for some academic who wanted to do stuff, and was never really able to close the loop on getting it working correctly. It might be worth removing the alleged support from ov7670 as well. In any case, for all of them: Acked-by: Jonathan Corbet cor...@lwn.net jon -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 00/21] marvell-ccic: drop and fix formats
Hi Jon, On 03/13/2015 10:28 PM, Jonathan Corbet wrote: On Wed, 11 Mar 2015 09:10:24 +0100 Hans Verkuil hverk...@xs4all.nl wrote: After some more testing I realized that the 422P format produced wrong colors and I couldn't get it to work. Since it never worked and nobody complained about it (and it is a fairly obscure format as well) I've dropped it. I'm not sure how that format came in anymore; I didn't add it. No objections to its removal. It came in with the patches from Marvell. I also tested RGB444 format for the first time, and that had wrong colors as well, but that was easy to fix. Finally there was a Bayer format reported, but it was never implemented. So that too was dropped. The RGB444 change worries me somewhat; that was the default format on the XO1 and worked for years. I vaguely remember some discussions about the ordering of the colors there, but that was a while ago. Did you test it with any of the Sugar apps? I've tested with the 'Record' app, and that picks a YUV format, not RGB444. Are there other apps that I can test with where you can select the capture format? In the end, correctness is probably the right way to go (it usually is!), but I'd hate to get a regression report from somebody who is crazy enough to put current kernels on those machines. Fortunately, such people should be rare. Bayer sort-of worked once, honest. I added it for some academic who wanted to do stuff, and was never really able to close the loop on getting it working correctly. It might be worth removing the alleged support from ov7670 as well. I might give it try to get it to work. I'm in the process of adding Bayer support to the vivid driver, which makes it easier to test. I'll see if I have some time this weekend. In any case, for all of them: Acked-by: Jonathan Corbet cor...@lwn.net Thanks! Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 00/21] marvell-ccic: drop and fix formats
This v2 patch series replaces patch 18 from the first series. After some more testing I realized that the 422P format produced wrong colors and I couldn't get it to work. Since it never worked and nobody complained about it (and it is a fairly obscure format as well) I've dropped it. I also tested RGB444 format for the first time, and that had wrong colors as well, but that was easy to fix. Finally there was a Bayer format reported, but it was never implemented. So that too was dropped. I double checked all remaining formats, and they all work fine. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
