RE: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support
Hi, Jonathan Thanks a lot for explaining it. :) -Original Message- From: Jonathan Corbet [mailto:cor...@lwn.net] Sent: Tuesday, 05 February, 2013 11:14 To: Albert Wang Cc: g.liakhovet...@gmx.de; linux-media@vger.kernel.org; Libin Yang Subject: Re: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support My apologies for the slow response...I'm running far behind. On Thu, 31 Jan 2013 00:29:13 -0800 Albert Wang twan...@marvell.com wrote: As you know, we are working on adding B_DMA_SG support on soc_camera mode. We found there is some code we can't understand in irq handler: if (handled == IRQ_HANDLED) { set_bit(CF_DMA_ACTIVE, cam-flags); if (cam-buffer_mode == B_DMA_sg) mcam_ctlr_stop(cam); } The question is why we need stop ccic in irq handler when buffer mode is B_DMA_sg? That's actually intended to be addressed by this comment in the DMA setup code: /* * Frame completion with S/G is trickier. We can't muck with * a descriptor chain on the fly, since the controller buffers it * internally. So we have to actually stop and restart; Marvell * says this is the way to do it. * ...and, indeed, at the time, I was told by somebody at Marvell that I needed to stop the controller before I could store a new descriptor into the chain. I don't see how it could work otherwise, really? [Albert Wang] Em.., maybe I guess there was some flaw in old version. We indeed can work on current platform without the code. I'd be happy to see this code go, it always felt a bit hacky. But the controller buffers the descriptor chain deep inside its unreachable guts, so one has to mess with it carefully. [Albert Wang] I suppose your platform should work with the original code. So how do you think this time we will keep the original code in the patches? If there is something wrong with it when you test the patches on your platform, maybe we can try to change it. : Thanks, jon Thanks Albert Wang 86-21-61092656 -- 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: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support
My apologies for the slow response...I'm running far behind. On Thu, 31 Jan 2013 00:29:13 -0800 Albert Wang twan...@marvell.com wrote: As you know, we are working on adding B_DMA_SG support on soc_camera mode. We found there is some code we can't understand in irq handler: if (handled == IRQ_HANDLED) { set_bit(CF_DMA_ACTIVE, cam-flags); if (cam-buffer_mode == B_DMA_sg) mcam_ctlr_stop(cam); } The question is why we need stop ccic in irq handler when buffer mode is B_DMA_sg? That's actually intended to be addressed by this comment in the DMA setup code: /* * Frame completion with S/G is trickier. We can't muck with * a descriptor chain on the fly, since the controller buffers it * internally. So we have to actually stop and restart; Marvell * says this is the way to do it. * ...and, indeed, at the time, I was told by somebody at Marvell that I needed to stop the controller before I could store a new descriptor into the chain. I don't see how it could work otherwise, really? I'd be happy to see this code go, it always felt a bit hacky. But the controller buffers the descriptor chain deep inside its unreachable guts, so one has to mess with it carefully. Thanks, 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: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support
Hi, Jonathan As you know, we are working on adding B_DMA_SG support on soc_camera mode. We found there is some code we can't understand in irq handler: if (handled == IRQ_HANDLED) { set_bit(CF_DMA_ACTIVE, cam-flags); if (cam-buffer_mode == B_DMA_sg) mcam_ctlr_stop(cam); } The question is why we need stop ccic in irq handler when buffer mode is B_DMA_sg? if (cam-buffer_mode == B_DMA_sg) mcam_ctlr_stop(cam); Currently we tested B_DMA_sg mode on our platform, and this buffer mode can work only if we comment these 2 lines. Could you please help us take a look if you have time? Thank you very much for your help! :) Thanks Albert Wang 86-21-61092656 -Original Message- From: Albert Wang Sent: Wednesday, 19 December, 2012 04:48 To: 'Jonathan Corbet' Cc: g.liakhovet...@gmx.de; linux-media@vger.kernel.org; Libin Yang Subject: RE: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support Hi, Jonathan -Original Message- From: Jonathan Corbet [mailto:cor...@lwn.net] Sent: Wednesday, 19 December, 2012 03:15 To: Albert Wang Cc: g.liakhovet...@gmx.de; linux-media@vger.kernel.org; Libin Yang Subject: Re: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support On Mon, 17 Dec 2012 19:04:26 -0800 Albert Wang twan...@marvell.com wrote: [Albert Wang] So if we add B_DMA_SG and B_VMALLOC support and OLPC XO 1.0 support in soc_camera mode. Then we can just remove the original mode and only support soc_camera mode in marvell-ccic? That is the idea, yes. Unless there is some real value to supporting both modes (that I've not seen), I think it's far better to support just one of them. Trying to support duplicated modes just leads to pain in the long run, in my experience. [Albert Wang] OK, we will update and submit the remained patches except for the 3 patches related with soc_camera support as the first part. Then we will submit the soc_camera support patches after we rework the patches and add B_DMA_SG and B_VMALLOC support and OLPC XO 1.0 support. I can offer to *try* to find time to help with XO 1.0 testing when the time comes. [Albert Wang] Thank you very much! We were worried about how to get the OLPC XO 1.0 HW. That would be a great help! :) Thanks, jon Thanks Albert Wang 86-21-61092656 -- 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: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support
On Mon, 17 Dec 2012 19:04:26 -0800 Albert Wang twan...@marvell.com wrote: [Albert Wang] So if we add B_DMA_SG and B_VMALLOC support and OLPC XO 1.0 support in soc_camera mode. Then we can just remove the original mode and only support soc_camera mode in marvell-ccic? That is the idea, yes. Unless there is some real value to supporting both modes (that I've not seen), I think it's far better to support just one of them. Trying to support duplicated modes just leads to pain in the long run, in my experience. I can offer to *try* to find time to help with XO 1.0 testing when the time comes. Thanks, 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: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support
Hi, Jonathan -Original Message- From: Jonathan Corbet [mailto:cor...@lwn.net] Sent: Wednesday, 19 December, 2012 03:15 To: Albert Wang Cc: g.liakhovet...@gmx.de; linux-media@vger.kernel.org; Libin Yang Subject: Re: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support On Mon, 17 Dec 2012 19:04:26 -0800 Albert Wang twan...@marvell.com wrote: [Albert Wang] So if we add B_DMA_SG and B_VMALLOC support and OLPC XO 1.0 support in soc_camera mode. Then we can just remove the original mode and only support soc_camera mode in marvell-ccic? That is the idea, yes. Unless there is some real value to supporting both modes (that I've not seen), I think it's far better to support just one of them. Trying to support duplicated modes just leads to pain in the long run, in my experience. [Albert Wang] OK, we will update and submit the remained patches except for the 3 patches related with soc_camera support as the first part. Then we will submit the soc_camera support patches after we rework the patches and add B_DMA_SG and B_VMALLOC support and OLPC XO 1.0 support. I can offer to *try* to find time to help with XO 1.0 testing when the time comes. [Albert Wang] Thank you very much! We were worried about how to get the OLPC XO 1.0 HW. That would be a great help! :) Thanks, jon Thanks Albert Wang 86-21-61092656 -- 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: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support
On Sun, 16 Dec 2012 14:12:11 -0800 Albert Wang twan...@marvell.com wrote: - Is the soc_camera mode necessary? Is there something you're trying to do that can't be done without it? Or, at least, does it add sufficient benefit to be worth this work? It would be nice if the reasoning behind this change were put into the changelog. [Albert Wang] We just want to add one more option for user. :) And we split it to 2 parts because we want to keep the original mode. - If the soc_camera change is deemed to be worthwhile, is there anything preventing you from doing it 100% so it's the only mode used? [Albert Wang] No, but current all Marvell platform have used the soc_camera in camera driver. :) So we just hope the marvell-ccic can have this option. :) OK, so this, I think, is the one remaining point of disagreement here; unfortunately it's a biggish one. Users, I believe, don't really care which underlying framework the driver is using; they just want a camera implementing the V4L2 spec. So, this particular option does not have any real value for them. But it has a real cost in terms of duplicated code, added complexity, and namespace pollution. If you believe I'm wrong, please tell me why, but I think that this option is not worth the cost. The reason for the soc_camera conversion is because that's how your drivers do it — not necessarily the strongest of reasons. Of course, the reason for keeping things as they are is because that's how the in-tree drivers does it; not necessarily a whole lot stronger. I'm not sold on the soc_camera conversion, but neither am I implacably opposed to it. But I *really* dislike the idea of having both, I don't see that leading to good things in the long run. So can I ask one more time: if soc_camera is important to you, could you please just convert the driver over 100% and drop the other mode entirely? It seems that should be easier than trying to support both, and it should certainly be easier to maintain in the future. I'm sorry to be obnoxious about this. Meanwhile, the bulk of this last patch series seems good; most of them have my acks, and I saw acks from Guennadi on some as well. I would recommend that you separate those out into a different series and submit them for merging, presumably for 3.9. That will give you a bit less code to carry going forward as this last part gets worked out. Thanks again for doing this work and persevering with it! 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: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support
Hi, Jonathan -Original Message- From: Jonathan Corbet [mailto:cor...@lwn.net] Sent: Monday, 17 December, 2012 23:29 To: Albert Wang Cc: g.liakhovet...@gmx.de; linux-media@vger.kernel.org; Libin Yang Subject: Re: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support On Sun, 16 Dec 2012 14:12:11 -0800 Albert Wang twan...@marvell.com wrote: - Is the soc_camera mode necessary? Is there something you're trying to do that can't be done without it? Or, at least, does it add sufficient benefit to be worth this work? It would be nice if the reasoning behind this change were put into the changelog. [Albert Wang] We just want to add one more option for user. :) And we split it to 2 parts because we want to keep the original mode. - If the soc_camera change is deemed to be worthwhile, is there anything preventing you from doing it 100% so it's the only mode used? [Albert Wang] No, but current all Marvell platform have used the soc_camera in camera driver. :) So we just hope the marvell-ccic can have this option. :) OK, so this, I think, is the one remaining point of disagreement here; unfortunately it's a biggish one. Users, I believe, don't really care which underlying framework the driver is using; they just want a camera implementing the V4L2 spec. So, this particular option does not have any real value for them. But it has a real cost in terms of duplicated code, added complexity, and namespace pollution. If you believe I'm wrong, please tell me why, but I think that this option is not worth the cost. The reason for the soc_camera conversion is because that's how your drivers do it — not necessarily the strongest of reasons. Of course, the reason for keeping things as they are is because that's how the in-tree drivers does it; not necessarily a whole lot stronger. I'm not sold on the soc_camera conversion, but neither am I implacably opposed to it. But I *really* dislike the idea of having both, I don't see that leading to good things in the long run. So can I ask one more time: if soc_camera is important to you, could you please just convert the driver over 100% and drop the other mode entirely? It seems that should be easier than trying to support both, and it should certainly be easier to maintain in the future. [Albert Wang] So if we add B_DMA_SG and B_VMALLOC support and OLPC XO 1.0 support in soc_camera mode. Then we can just remove the original mode and only support soc_camera mode in marvell-ccic? I'm sorry to be obnoxious about this. Meanwhile, the bulk of this last patch series seems good; most of them have my acks, and I saw acks from Guennadi on some as well. I would recommend that you separate those out into a different series and submit them for merging, presumably for 3.9. That will give you a bit less code to carry going forward as this last part gets worked out. Thanks again for doing this work and persevering with it! jon Thanks Albert Wang 86-21-61092656 N�r��yb�X��ǧv�^�){.n�+{���bj)w*jg����ݢj/���z�ޖ��2�ޙ�)ߡ�a�����G���h��j:+v���w��٥
Re: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support
On Sat, 15 Dec 2012 17:57:59 +0800 Albert Wang twan...@marvell.com wrote: This patch splits mcam-core into 2 parts to prepare for soc_camera support. The first part remains in mcam-core.c. This part includes the HW operations and vb2 callback functions. The second part is moved to mcam-core-standard.c. This part is relevant with the implementation of using V4L2. OK, I'll confess I'm still not 100% sold on this part. Can I repeat the questions I raised before? - Is the soc_camera mode necessary? Is there something you're trying to do that can't be done without it? Or, at least, does it add sufficient benefit to be worth this work? It would be nice if the reasoning behind this change were put into the changelog. - If the soc_camera change is deemed to be worthwhile, is there anything preventing you from doing it 100% so it's the only mode used? The split as you've done it here is an improvement over what came before, but it still results in a lot of duplicated code; it also adds a *lot* of symbols to the global namespace. If this is really the only way then we'll find a way to make it work, but I'd like to be sure that we can't do something better. Thanks, 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: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support
Hi, Jonathan -Original Message- From: Jonathan Corbet [mailto:cor...@lwn.net] Sent: Monday, 17 December, 2012 00:37 To: Albert Wang Cc: g.liakhovet...@gmx.de; linux-media@vger.kernel.org; Libin Yang Subject: Re: [PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support On Sat, 15 Dec 2012 17:57:59 +0800 Albert Wang twan...@marvell.com wrote: This patch splits mcam-core into 2 parts to prepare for soc_camera support. The first part remains in mcam-core.c. This part includes the HW operations and vb2 callback functions. The second part is moved to mcam-core-standard.c. This part is relevant with the implementation of using V4L2. OK, I'll confess I'm still not 100% sold on this part. Can I repeat the questions I raised before? - Is the soc_camera mode necessary? Is there something you're trying to do that can't be done without it? Or, at least, does it add sufficient benefit to be worth this work? It would be nice if the reasoning behind this change were put into the changelog. [Albert Wang] We just want to add one more option for user. :) And we split it to 2 parts because we want to keep the original mode. - If the soc_camera change is deemed to be worthwhile, is there anything preventing you from doing it 100% so it's the only mode used? [Albert Wang] No, but current all Marvell platform have used the soc_camera in camera driver. :) So we just hope the marvell-ccic can have this option. :) The split as you've done it here is an improvement over what came before, but it still results in a lot of duplicated code; it also adds a *lot* of symbols to the global namespace. If this is really the only way then we'll find a way to make it work, but I'd like to be sure that we can't do something better. Thanks, jon Thanks Albert Wang 86-21-61092656 -- 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
[PATCH V3 10/15] [media] marvell-ccic: split mcam-core into 2 parts for soc_camera support
This patch splits mcam-core into 2 parts to prepare for soc_camera support. The first part remains in mcam-core.c. This part includes the HW operations and vb2 callback functions. The second part is moved to mcam-core-standard.c. This part is relevant with the implementation of using V4L2. Signed-off-by: Libin Yang lby...@marvell.com Signed-off-by: Albert Wang twan...@marvell.com --- drivers/media/platform/marvell-ccic/Makefile |4 +- .../platform/marvell-ccic/mcam-core-standard.c | 820 + .../platform/marvell-ccic/mcam-core-standard.h | 26 + drivers/media/platform/marvell-ccic/mcam-core.c| 944 +--- drivers/media/platform/marvell-ccic/mcam-core.h| 54 ++ 5 files changed, 939 insertions(+), 909 deletions(-) create mode 100644 drivers/media/platform/marvell-ccic/mcam-core-standard.c create mode 100644 drivers/media/platform/marvell-ccic/mcam-core-standard.h diff --git a/drivers/media/platform/marvell-ccic/Makefile b/drivers/media/platform/marvell-ccic/Makefile index 05a792c..595ebdf 100755 --- a/drivers/media/platform/marvell-ccic/Makefile +++ b/drivers/media/platform/marvell-ccic/Makefile @@ -1,6 +1,6 @@ obj-$(CONFIG_VIDEO_CAFE_CCIC) += cafe_ccic.o cafe_ccic-y := cafe-driver.o mcam-core.o -obj-$(CONFIG_VIDEO_MMP_CAMERA) += mmp_camera.o -mmp_camera-y := mmp-driver.o mcam-core.o +obj-$(CONFIG_VIDEO_MMP_CAMERA) += mmp_camera_standard.o +mmp_camera_standard-y := mmp-driver.o mcam-core.o mcam-core-standard.o diff --git a/drivers/media/platform/marvell-ccic/mcam-core-standard.c b/drivers/media/platform/marvell-ccic/mcam-core-standard.c new file mode 100644 index 000..5e0d8f1 --- /dev/null +++ b/drivers/media/platform/marvell-ccic/mcam-core-standard.c @@ -0,0 +1,820 @@ +/* + * The Marvell camera core. This device appears in a number of settings, + * so it needs platform-specific support outside of the core. + * + * Copyright 2011 Jonathan Corbet cor...@lwn.net + */ +#include linux/kernel.h +#include linux/module.h +#include linux/fs.h +#include linux/mm.h +#include linux/i2c.h +#include linux/interrupt.h +#include linux/spinlock.h +#include linux/slab.h +#include linux/device.h +#include linux/wait.h +#include linux/list.h +#include linux/dma-mapping.h +#include linux/delay.h +#include linux/vmalloc.h +#include linux/io.h +#include linux/videodev2.h +#include media/v4l2-device.h +#include media/v4l2-ioctl.h +#include media/v4l2-chip-ident.h +#include media/ov7670.h +#include linux/clk.h +#include media/videobuf2-vmalloc.h +#include media/videobuf2-dma-contig.h +#include media/videobuf2-dma-sg.h + +#include mcam-core.h +#include mcam-core-standard.h + +static const enum v4l2_mbus_pixelcode mcam_def_mbus_code = + V4L2_MBUS_FMT_YUYV8_2X8; + +static struct mcam_format_struct { + __u8 *desc; + __u32 pixelformat; + int bpp; /* Bytes per pixel */ + enum v4l2_mbus_pixelcode mbus_code; +} mcam_formats[] = { + { + .desc = YUYV 4:2:2, + .pixelformat= V4L2_PIX_FMT_YUYV, + .mbus_code = V4L2_MBUS_FMT_YUYV8_2X8, + .bpp= 2, + }, + { + .desc = UYVY 4:2:2, + .pixelformat= V4L2_PIX_FMT_UYVY, + .mbus_code = V4L2_MBUS_FMT_UYVY8_2X8, + .bpp= 2, + }, + { + .desc = YUV 4:2:2 PLANAR, + .pixelformat= V4L2_PIX_FMT_YUV422P, + .mbus_code = V4L2_MBUS_FMT_UYVY8_2X8, + .bpp= 2, + }, + { + .desc = YUV 4:2:0 PLANAR, + .pixelformat= V4L2_PIX_FMT_YUV420, + .mbus_code = V4L2_MBUS_FMT_YUYV8_1_5X8, + .bpp= 2, + }, + { + .desc = YVU 4:2:0 PLANAR, + .pixelformat= V4L2_PIX_FMT_YVU420, + .mbus_code = V4L2_MBUS_FMT_YVYU8_1_5X8, + .bpp= 2, + }, + { + .desc = RGB 444, + .pixelformat= V4L2_PIX_FMT_RGB444, + .mbus_code = V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE, + .bpp= 2, + }, + { + .desc = RGB 565, + .pixelformat= V4L2_PIX_FMT_RGB565, + .mbus_code = V4L2_MBUS_FMT_RGB565_2X8_LE, + .bpp= 2, + }, + { + .desc = Raw RGB Bayer, + .pixelformat= V4L2_PIX_FMT_SBGGR8, + .mbus_code = V4L2_MBUS_FMT_SBGGR8_1X8, + .bpp= 1 + }, +}; +#define N_MCAM_FMTS ARRAY_SIZE(mcam_formats) + +static struct mcam_format_struct *mcam_find_format(u32 pixelformat) +{ + unsigned i; + + for (i = 0; i N_MCAM_FMTS; i++) + if