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
