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 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 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 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
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 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 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: 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 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 Sun, 16 Dec 2012 14:12:11 -0800 Albert Wang 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 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 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
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 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