marvell-ccic: lacks of some features

marvell-ccic: lacks of some features

[Albert Wang]

Hi, Jonathan & Guennadi

We used the marvell-ccic code and found it lacks of some features, but our Marvell Camera driver (mv_camera.c) which based on soc_camera can support all these features:

1. marvell-ccic only support MMP2 (PXA688), it can’t support other Marvell SOC chips
Our mv_camera can support such as MMP3 (PXA2128), TD (PXA910/920) and so on besides MMP2

2. marvell-ccic only support parallel (DVP) mode, can’t support MIPI mode
Our mv_camera can support both DVP mode and MIPI mode, MIPI interface is the trend of current camera sensors with high resolution

3. marvell-ccic only support ccic1 controller, can’t support ccic2 or dual ccic controllers
As you known, both MMP2 and MMP3 have 2 ccic controllers, ccic2 is different with ccic1
Sometimes we need use both 2 ccic controllers for connecting 2 camera sensors
Actually, we have used 2 ccic controllers' cases in our platforms
Our mv_camera can support these cases: only use ccic1, only use ccic2 and use ccic1 + ccic2

4. marvell-ccic only support camera sensor OV7670
It's an old and low resolution parallel sensor, and sensor info also is hard code
But it loooks we should better separate controller and sensor in driver, controller doesn't care sensor type which will communicate with
Our mv_camera can support any camera sensor which based on subdev structure

5. marvell-ccic only support YUYV format which is packed format besides RGB444 and RGB565, it can’t support planar formats
Our mv_camera can support both packed format and planar formats such as YUV420 and YUV422P

6. marvell-ccic didn't support JPEG format for still capture mode
Our mv_camera can support JPEG directly for still capture, most high resolution camera sensor can output JPEG format directly

7. marvell-ccic can’t support dual camera sensors or multi camera sensors cases
Current most platforms can support dual camera sensor case, include front-facing sensor and rear-facing sensor
Even some high end platforms can support 3D mode record; it need support 2+1 camera sensors
Our mv_camera can support these cases:
dual camera sensor connect to ccic1 or ccic2
one camera sensor connect to ccic1 and the other camera sensor connect to ccic2
dual camera sensor connect to ccic1 and one camera sensor connect to ccic2

8. marvell-ccic can’t support external ISP + raw camera sensor mode
As you known, more and more camera sensors with high resolution are raw camera sensors but not smart sensors
It needs external ISP (Image Signal Processor) to generate the desired formats and resolutions with some advanced features based on the raw data from sensor
Our mv_camera can support both smart camera sensors and external ISP + raw camera sensors

9. marvell-ccic still used obsolete method to stop ccic DMA
This method should be inheritted from old cafe-ccic driver, it use CF flag which is trigged by SOF
This method is inefficient, we must wait at least 150ms for stop ccic DMA and it also can result in many issues during thousands resolutions or formats switch stress test
Actually our ccic can handle it if we use the right stop sequence by config some ccic registers
Our mv_camera had applied the new and right stop method and it also passed the thousands resolutions or formats switch stress test


Thanks
Albert Wang
-----Original Message-----
From: Guennadi Liakhovetski [mailto:g.liakhovetski@gmx.de] 
Sent: Monday, 09 April, 2012 20:11
To: Albert
Cc: Jonathan Corbet; Linux Media Mailing List; Mauro Carvalho Chehab; Albert Wang; Chao Xie; Kassey Lee
Subject: Re: [PATCH 2/7] marvell-cam: Remove broken "owner" logic

Hi Albert

On Mon, 9 Apr 2012, Albert wrote:

> Hi, Jonathan & Guennadi
> 
> I'm Albert Wang from Marvell, nice to meet you!

Nice to meet you too.

> We found there is a soc camera framework in open source, and we made a 
> Marvell camera driver which based on Soc camera framework + videobuf2.
> And now it can serve several Marvell SOC chips, such as MMP2 (PXA688), 
> MMP3
> (PXA2128) and TD (PXA910) which has same CCIC IP.
> 
> But it looks Jonathan had write a Marvell ccic camera driver based on 
> café-ccic + videobuf2 for MMP2 on OLPC in open source.
> 
> So do you think it’s still OK to push our Marvell camera driver to 
> open source?

A colleague of yours - Kassey Lee - has been working on this driver:

http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/33775

and it has been agreed, that the camera driver for PXA910 and other SoCs, mentioned above should re-use the same code-base, as the cafe_ccic driver from Jon. One of obstacles has been, that the cafe driver didn't use a standard videobuf scheme, implementing one of its own. Now this has been fixed too, the driver has also been broken down in parts to simplify such code re-use. All this speaks in favour of implementing your driver by using common parts of the marvell-ccic driver. This is also what Kassey has agreed to. Whether or not your new driver will be also using the soc-camera framework is your decision, I think, both ways are possible. 
Please, try to re-use the marvell-ccic code and report any problems.

Thanks
Guennadi

> Looking for your comments!
> Thanks a lot in advance!
> 
> Thanks
> Albert Wang
> On Sat, Mar 17, 2012 at 7:14 AM, Jonathan Corbet <corbet@lwn.net> wrote:
> 
> > The marvell cam driver retained just enough of the owner-tracking 
> > logic from cafe_ccic to be broken; it could, conceivably, cause the 
> > driver to release DMA memory while the controller is still active.  
> > Simply remove the remaining pieces and ensure that the controller is 
> > stopped before we free things.
> >
> > Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> > ---
> >  drivers/media/video/marvell-ccic/mcam-core.c |    5 +----
> >  drivers/media/video/marvell-ccic/mcam-core.h |    1 -
> >  2 files changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/video/marvell-ccic/mcam-core.c
> > b/drivers/media/video/marvell-ccic/mcam-core.c
> > index 35cd89d..b261182 100644
> > --- a/drivers/media/video/marvell-ccic/mcam-core.c
> > +++ b/drivers/media/video/marvell-ccic/mcam-core.c
> > @@ -1564,11 +1564,8 @@ static int mcam_v4l_release(struct file *filp)
> >                        singles, delivered);
> >        mutex_lock(&cam->s_mutex);
> >        (cam->users)--;
> > -       if (filp == cam->owner) {
> > -               mcam_ctlr_stop_dma(cam);
> > -               cam->owner = NULL;
> > -       }
> >        if (cam->users == 0) {
> > +               mcam_ctlr_stop_dma(cam);
> >                mcam_cleanup_vb2(cam);
> >                mcam_ctlr_power_down(cam);
> >                if (cam->buffer_mode == B_vmalloc && 
> > alloc_bufs_at_read) diff --git 
> > a/drivers/media/video/marvell-ccic/mcam-core.h
> > b/drivers/media/video/marvell-ccic/mcam-core.h
> > index 917200e..bd6acba 100644
> > --- a/drivers/media/video/marvell-ccic/mcam-core.h
> > +++ b/drivers/media/video/marvell-ccic/mcam-core.h
> > @@ -107,7 +107,6 @@ struct mcam_camera {
> >        enum mcam_state state;
> >        unsigned long flags;            /* Buffer status, mainly (dev_lock)
> > */
> >        int users;                      /* How many open FDs */
> > -       struct file *owner;             /* Who has data access (v4l2) */
> >
> >        /*
> >         * Subsystem structures.
> > --
> > 1.7.9.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe 
> > linux-media" in the body of a message to majordomo@vger.kernel.org 
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer http://www.open-technology.de/

Re: marvell-ccic: lacks of some features

[Jonathan Corbet]

On Fri, 11 May 2012 09:02:26 -0700
Albert Wang <twang13@marvell.com> wrote:

> Hi, Jonathan & Guennadi
> 
> We used the marvell-ccic code and found it lacks of some features, but
> our Marvell Camera driver (mv_camera.c) which based on soc_camera can
> support all these features:

The marvell-ccic driver has the features that were needed by the people
doing the work and that I had the documentation and hardware to support.
Of course it's incomplete.

I'll go through your list, but, first: is the purpose of your message to
argue for a replacement of the marvell-ccic driver by your mv_camera
driver?  I am not necessarily opposed to that idea if mv_camera can support
deployed systems back to the OLPC XO 1.0 and if it seems clear that a
replacement makes more sense than adding features to the in-tree driver.  

> 1. marvell-ccic only support MMP2 (PXA688), it can’t support other Marvell SOC chips
> Our mv_camera can support such as MMP3 (PXA2128), TD (PXA910/920) and so
> on besides MMP2

Which is cool.  It is nice that Marvell is finally providing Linux support
for its camera controllers after all these years.  For the last several
years, I've necessarily been limited in the controllers I could support.
Is it Marvell's intention to provide upstream maintenance and support going
forward? 

> 2. marvell-ccic only support parallel (DVP) mode, can’t support MIPI mode
> Our mv_camera can support both DVP mode and MIPI mode, MIPI interface is
> the trend of current camera sensors with high resolution

Adding MIPI doesn't look that hard, I've just never had a reason (or
hardware) to do it.

> 
> 3. marvell-ccic only support ccic1 controller, can’t support ccic2 or
> dual ccic controllers 
> As you known, both MMP2 and MMP3 have 2 ccic controllers, ccic2 is different with ccic1
> Sometimes we need use both 2 ccic controllers for connecting 2 camera sensors
> Actually, we have used 2 ccic controllers' cases in our platforms
> Our mv_camera can support these cases: only use ccic1, only use ccic2 and use ccic1 + ccic2

It doesn't support two because nobody has asked for it, but the driver was
written with that in mind.  I don't see supporting the second controller as
a big job.

> 4. marvell-ccic only support camera sensor OV7670
> It's an old and low resolution parallel sensor, and sensor info also is hard code
> But it loooks we should better separate controller and sensor in driver, controller doesn't care sensor type which will communicate with
> Our mv_camera can support any camera sensor which based on subdev structure

That is a well-known limitation, which, again, isn't that hard to fix.  The
main problem is fixing it without breaking existing users.

> 5. marvell-ccic only support YUYV format which is packed format besides
> RGB444 and RGB565, it can’t support planar formats 
> Our mv_camera can support both packed format and planar formats such as
> YUV420 and YUV422P

Again, not a fundamental driver limitation; definitely worth fixing when
somebody actually needs planar formats.

> 6. marvell-ccic didn't support JPEG format for still capture mode
> Our mv_camera can support JPEG directly for still capture, most high
> resolution camera sensor can output JPEG format directly

That would indeed be a nice feature to have.

> 7. marvell-ccic can’t support dual camera sensors or multi camera sensors cases
> Current most platforms can support dual camera sensor case, include front-facing sensor and rear-facing sensor
> Even some high end platforms can support 3D mode record; it need support 2+1 camera sensors
> Our mv_camera can support these cases:
> dual camera sensor connect to ccic1 or ccic2
> one camera sensor connect to ccic1 and the other camera sensor connect to ccic2
> dual camera sensor connect to ccic1 and one camera sensor connect to ccic2

Sounds like nice stuff.

> 8. marvell-ccic can’t support external ISP + raw camera sensor mode As
> you known, more and more camera sensors with high resolution are raw
> camera sensors but not smart sensors It needs external ISP (Image Signal
> Processor) to generate the desired formats and resolutions with some
> advanced features based on the raw data from sensor Our mv_camera can
> support both smart camera sensors and external ISP + raw camera sensors

Supporting that mode would be a significant bit of work.  But please let's
not confuse "can't" and "doesn't currently."

> 9. marvell-ccic still used obsolete method to stop ccic DMA
> This method should be inheritted from old cafe-ccic driver, it use CF flag which is trigged by SOF
> This method is inefficient, we must wait at least 150ms for stop ccic DMA and it also can result in many issues during thousands resolutions or formats switch stress test
> Actually our ccic can handle it if we use the right stop sequence by config some ccic registers
> Our mv_camera had applied the new and right stop method and it also
> passed the thousands resolutions or formats switch stress test

Which DMA mode are you talking about now?  I've supported DMA to the best
of my ability given the information in the data sheet, plus a couple of
hints from Kassey Lee (who, I believe, no longer works there?).  This can't
be a hard thing to change, anyway.

Anyway, we're seeing the results of Marvell going off and working on its
own private code instead of enhancing the in-tree driver that has been
there since 2006.  Sad, but so it goes.  But if Marvell wants to work
upstream now, I sure don't want to make things harder.  How about we get a
new version of the mv_camera driver for review and we can all think about
what's the best thing to do at this point?

Thanks,

jon

RE: marvell-ccic: lacks of some features

[Albert Wang]

Hi, Jonathan

Nice to meet you!

We must clarify that it's not our target to replace your marvell-ccic by our mv_camera in the tree

We just hope you can help to review our mv_camera patches and discuss if can put it in tree, because it may support more and better for Marvell Soc chips.
There is no conflict between your marvell-ccic which support OLPC and our mv_camera which support most Marvell platforms.
Actually we are willing to add support on OLPC. :)

OK, we will provide the patches for discussing.
Thank you very much!


Thanks
Albert Wang
-----Original Message-----
From: Jonathan Corbet [mailto:corbet@lwn.net] 
Sent: Saturday, 12 May, 2012 00:28
To: Albert Wang
Cc: 'Guennadi Liakhovetski'; Linux Media Mailing List; Mauro Carvalho Chehab; Chao Xie; Angela Wan; Kassey Lee; Albert
Subject: Re: marvell-ccic: lacks of some features

On Fri, 11 May 2012 09:02:26 -0700
Albert Wang <twang13@marvell.com> wrote:

> Hi, Jonathan & Guennadi
> 
> We used the marvell-ccic code and found it lacks of some features, but 
> our Marvell Camera driver (mv_camera.c) which based on soc_camera can 
> support all these features:

The marvell-ccic driver has the features that were needed by the people doing the work and that I had the documentation and hardware to support.
Of course it's incomplete.

I'll go through your list, but, first: is the purpose of your message to argue for a replacement of the marvell-ccic driver by your mv_camera driver?  I am not necessarily opposed to that idea if mv_camera can support deployed systems back to the OLPC XO 1.0 and if it seems clear that a replacement makes more sense than adding features to the in-tree driver.  

> 1. marvell-ccic only support MMP2 (PXA688), it can’t support other 
> Marvell SOC chips Our mv_camera can support such as MMP3 (PXA2128), TD 
> (PXA910/920) and so on besides MMP2

Which is cool.  It is nice that Marvell is finally providing Linux support for its camera controllers after all these years.  For the last several years, I've necessarily been limited in the controllers I could support.
Is it Marvell's intention to provide upstream maintenance and support going forward? 

> 2. marvell-ccic only support parallel (DVP) mode, can’t support MIPI 
> mode Our mv_camera can support both DVP mode and MIPI mode, MIPI 
> interface is the trend of current camera sensors with high resolution

Adding MIPI doesn't look that hard, I've just never had a reason (or
hardware) to do it.

> 
> 3. marvell-ccic only support ccic1 controller, can’t support ccic2 or 
> dual ccic controllers As you known, both MMP2 and MMP3 have 2 ccic 
> controllers, ccic2 is different with ccic1 Sometimes we need use both 
> 2 ccic controllers for connecting 2 camera sensors Actually, we have 
> used 2 ccic controllers' cases in our platforms Our mv_camera can 
> support these cases: only use ccic1, only use ccic2 and use ccic1 + 
> ccic2

It doesn't support two because nobody has asked for it, but the driver was written with that in mind.  I don't see supporting the second controller as a big job.

> 4. marvell-ccic only support camera sensor OV7670 It's an old and low 
> resolution parallel sensor, and sensor info also is hard code But it 
> loooks we should better separate controller and sensor in driver, 
> controller doesn't care sensor type which will communicate with Our 
> mv_camera can support any camera sensor which based on subdev 
> structure

That is a well-known limitation, which, again, isn't that hard to fix.  The main problem is fixing it without breaking existing users.

> 5. marvell-ccic only support YUYV format which is packed format 
> besides
> RGB444 and RGB565, it can’t support planar formats Our mv_camera can 
> support both packed format and planar formats such as
> YUV420 and YUV422P

Again, not a fundamental driver limitation; definitely worth fixing when somebody actually needs planar formats.

> 6. marvell-ccic didn't support JPEG format for still capture mode Our 
> mv_camera can support JPEG directly for still capture, most high 
> resolution camera sensor can output JPEG format directly

That would indeed be a nice feature to have.

> 7. marvell-ccic can’t support dual camera sensors or multi camera 
> sensors cases Current most platforms can support dual camera sensor 
> case, include front-facing sensor and rear-facing sensor Even some 
> high end platforms can support 3D mode record; it need support 2+1 camera sensors Our mv_camera can support these cases:
> dual camera sensor connect to ccic1 or ccic2 one camera sensor connect 
> to ccic1 and the other camera sensor connect to ccic2 dual camera 
> sensor connect to ccic1 and one camera sensor connect to ccic2

Sounds like nice stuff.

> 8. marvell-ccic can’t support external ISP + raw camera sensor mode As 
> you known, more and more camera sensors with high resolution are raw 
> camera sensors but not smart sensors It needs external ISP (Image 
> Signal
> Processor) to generate the desired formats and resolutions with some 
> advanced features based on the raw data from sensor Our mv_camera can 
> support both smart camera sensors and external ISP + raw camera 
> sensors

Supporting that mode would be a significant bit of work.  But please let's not confuse "can't" and "doesn't currently."

> 9. marvell-ccic still used obsolete method to stop ccic DMA This 
> method should be inheritted from old cafe-ccic driver, it use CF flag 
> which is trigged by SOF This method is inefficient, we must wait at 
> least 150ms for stop ccic DMA and it also can result in many issues 
> during thousands resolutions or formats switch stress test Actually 
> our ccic can handle it if we use the right stop sequence by config 
> some ccic registers Our mv_camera had applied the new and right stop 
> method and it also passed the thousands resolutions or formats switch 
> stress test

Which DMA mode are you talking about now?  I've supported DMA to the best of my ability given the information in the data sheet, plus a couple of hints from Kassey Lee (who, I believe, no longer works there?).  This can't be a hard thing to change, anyway.

Anyway, we're seeing the results of Marvell going off and working on its own private code instead of enhancing the in-tree driver that has been there since 2006.  Sad, but so it goes.  But if Marvell wants to work upstream now, I sure don't want to make things harder.  How about we get a new version of the mv_camera driver for review and we can all think about what's the best thing to do at this point?

Thanks,

jon

RE: marvell-ccic: lacks of some features

[Guennadi Liakhovetski]

Hi Albert

On Fri, 11 May 2012, Albert Wang wrote:

> Hi, Jonathan
> 
> Nice to meet you!
> 
> We must clarify that it's not our target to replace your marvell-ccic by 
> our mv_camera in the tree
> 
> We just hope you can help to review our mv_camera patches and discuss if 
> can put it in tree, because it may support more and better for Marvell 
> Soc chips.
> There is no conflict between your marvell-ccic which support OLPC and 
> our mv_camera which support most Marvell platforms.
> Actually we are willing to add support on OLPC. :)

Thanks for continuing your work on the mv_camera driver and for your 
effort to integrate it into the mainline! We certainly want to support new 
hardware types and features. So, we're certainly glad to hear, that your 
driver supports features, not presently available in the mainline.

As you certainly understand, we also want to reuse kernel code as much as 
possible. Drivers for the same IP block in different packaging, with 
different interfaces even with slight differences in functionality is one 
such example. Therefore, we certainly would _very much_ prefer having your 
driver and marvell-ccic share as much code as possible.

It would be interesting to know: have you actually tried to build your 
driver around the marvell-ccic code-base? If yes - how did it go? What 
difficulties did you encounter? If no - have you considered doing so? If 
yes - why have you decided against it? Have you considered a possibility 
of building your driver as an soc-camera driver, while still reusing the 
core functionality from marvell-ccic?

Thanks
Guennadi

> OK, we will provide the patches for discussing.
> Thank you very much!
> 
> 
> Thanks
> Albert Wang
> -----Original Message-----
> From: Jonathan Corbet [mailto:corbet@lwn.net] 
> Sent: Saturday, 12 May, 2012 00:28
> To: Albert Wang
> Cc: 'Guennadi Liakhovetski'; Linux Media Mailing List; Mauro Carvalho Chehab; Chao Xie; Angela Wan; Kassey Lee; Albert
> Subject: Re: marvell-ccic: lacks of some features
> 
> On Fri, 11 May 2012 09:02:26 -0700
> Albert Wang <twang13@marvell.com> wrote:
> 
> > Hi, Jonathan & Guennadi
> > 
> > We used the marvell-ccic code and found it lacks of some features, but 
> > our Marvell Camera driver (mv_camera.c) which based on soc_camera can 
> > support all these features:
> 
> The marvell-ccic driver has the features that were needed by the people doing the work and that I had the documentation and hardware to support.
> Of course it's incomplete.
> 
> I'll go through your list, but, first: is the purpose of your message to argue for a replacement of the marvell-ccic driver by your mv_camera driver?  I am not necessarily opposed to that idea if mv_camera can support deployed systems back to the OLPC XO 1.0 and if it seems clear that a replacement makes more sense than adding features to the in-tree driver.  
> 
> > 1. marvell-ccic only support MMP2 (PXA688), it can’t support other 
> > Marvell SOC chips Our mv_camera can support such as MMP3 (PXA2128), TD 
> > (PXA910/920) and so on besides MMP2
> 
> Which is cool.  It is nice that Marvell is finally providing Linux support for its camera controllers after all these years.  For the last several years, I've necessarily been limited in the controllers I could support.
> Is it Marvell's intention to provide upstream maintenance and support going forward? 
> 
> > 2. marvell-ccic only support parallel (DVP) mode, can’t support MIPI 
> > mode Our mv_camera can support both DVP mode and MIPI mode, MIPI 
> > interface is the trend of current camera sensors with high resolution
> 
> Adding MIPI doesn't look that hard, I've just never had a reason (or
> hardware) to do it.
> 
> > 
> > 3. marvell-ccic only support ccic1 controller, can’t support ccic2 or 
> > dual ccic controllers As you known, both MMP2 and MMP3 have 2 ccic 
> > controllers, ccic2 is different with ccic1 Sometimes we need use both 
> > 2 ccic controllers for connecting 2 camera sensors Actually, we have 
> > used 2 ccic controllers' cases in our platforms Our mv_camera can 
> > support these cases: only use ccic1, only use ccic2 and use ccic1 + 
> > ccic2
> 
> It doesn't support two because nobody has asked for it, but the driver was written with that in mind.  I don't see supporting the second controller as a big job.
> 
> > 4. marvell-ccic only support camera sensor OV7670 It's an old and low 
> > resolution parallel sensor, and sensor info also is hard code But it 
> > loooks we should better separate controller and sensor in driver, 
> > controller doesn't care sensor type which will communicate with Our 
> > mv_camera can support any camera sensor which based on subdev 
> > structure
> 
> That is a well-known limitation, which, again, isn't that hard to fix.  The main problem is fixing it without breaking existing users.
> 
> > 5. marvell-ccic only support YUYV format which is packed format 
> > besides
> > RGB444 and RGB565, it can’t support planar formats Our mv_camera can 
> > support both packed format and planar formats such as
> > YUV420 and YUV422P
> 
> Again, not a fundamental driver limitation; definitely worth fixing when somebody actually needs planar formats.
> 
> > 6. marvell-ccic didn't support JPEG format for still capture mode Our 
> > mv_camera can support JPEG directly for still capture, most high 
> > resolution camera sensor can output JPEG format directly
> 
> That would indeed be a nice feature to have.
> 
> > 7. marvell-ccic can’t support dual camera sensors or multi camera 
> > sensors cases Current most platforms can support dual camera sensor 
> > case, include front-facing sensor and rear-facing sensor Even some 
> > high end platforms can support 3D mode record; it need support 2+1 camera sensors Our mv_camera can support these cases:
> > dual camera sensor connect to ccic1 or ccic2 one camera sensor connect 
> > to ccic1 and the other camera sensor connect to ccic2 dual camera 
> > sensor connect to ccic1 and one camera sensor connect to ccic2
> 
> Sounds like nice stuff.
> 
> > 8. marvell-ccic can’t support external ISP + raw camera sensor mode As 
> > you known, more and more camera sensors with high resolution are raw 
> > camera sensors but not smart sensors It needs external ISP (Image 
> > Signal
> > Processor) to generate the desired formats and resolutions with some 
> > advanced features based on the raw data from sensor Our mv_camera can 
> > support both smart camera sensors and external ISP + raw camera 
> > sensors
> 
> Supporting that mode would be a significant bit of work.  But please let's not confuse "can't" and "doesn't currently."
> 
> > 9. marvell-ccic still used obsolete method to stop ccic DMA This 
> > method should be inheritted from old cafe-ccic driver, it use CF flag 
> > which is trigged by SOF This method is inefficient, we must wait at 
> > least 150ms for stop ccic DMA and it also can result in many issues 
> > during thousands resolutions or formats switch stress test Actually 
> > our ccic can handle it if we use the right stop sequence by config 
> > some ccic registers Our mv_camera had applied the new and right stop 
> > method and it also passed the thousands resolutions or formats switch 
> > stress test
> 
> Which DMA mode are you talking about now?  I've supported DMA to the best of my ability given the information in the data sheet, plus a couple of hints from Kassey Lee (who, I believe, no longer works there?).  This can't be a hard thing to change, anyway.
> 
> Anyway, we're seeing the results of Marvell going off and working on its own private code instead of enhancing the in-tree driver that has been there since 2006.  Sad, but so it goes.  But if Marvell wants to work upstream now, I sure don't want to make things harder.  How about we get a new version of the mv_camera driver for review and we can all think about what's the best thing to do at this point?
> 
> Thanks,
> 
> jon
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Add V4l2 camera driver for Marvell PXA910/PXA688/PXA2128 CCIC

[Albert Wang]

Hi, Guennadi & Jonathan

As we have discussed 2 months ago, we make the updated version of mmp_camera driver which based on kernel 3.4 for support Marvell MMP soc family.

So could you please find time to take a look at this patch?

Thank you very much!


Thanks
Albert Wang
86-21-61092656

-----Original Message-----
From: Guennadi Liakhovetski [mailto:g.liakhovetski@gmx.de] 
Sent: Sunday, 13 May, 2012 03:05
To: Albert Wang
Cc: Jonathan Corbet; Linux Media Mailing List; Mauro Carvalho Chehab; Chao Xie; Angela Wan; Kassey Lee; Albert
Subject: RE: marvell-ccic: lacks of some features

Hi Albert

On Fri, 11 May 2012, Albert Wang wrote:

> Hi, Jonathan
> 
> Nice to meet you!
> 
> We must clarify that it's not our target to replace your marvell-ccic 
> by our mv_camera in the tree
> 
> We just hope you can help to review our mv_camera patches and discuss 
> if can put it in tree, because it may support more and better for 
> Marvell Soc chips.
> There is no conflict between your marvell-ccic which support OLPC and 
> our mv_camera which support most Marvell platforms.
> Actually we are willing to add support on OLPC. :)

Thanks for continuing your work on the mv_camera driver and for your effort to integrate it into the mainline! We certainly want to support new hardware types and features. So, we're certainly glad to hear, that your driver supports features, not presently available in the mainline.

As you certainly understand, we also want to reuse kernel code as much as possible. Drivers for the same IP block in different packaging, with different interfaces even with slight differences in functionality is one such example. Therefore, we certainly would _very much_ prefer having your driver and marvell-ccic share as much code as possible.

It would be interesting to know: have you actually tried to build your driver around the marvell-ccic code-base? If yes - how did it go? What difficulties did you encounter? If no - have you considered doing so? If yes - why have you decided against it? Have you considered a possibility of building your driver as an soc-camera driver, while still reusing the core functionality from marvell-ccic?

Thanks
Guennadi

> OK, we will provide the patches for discussing.
> Thank you very much!
> 
> 
> Thanks
> Albert Wang
> -----Original Message-----
> From: Jonathan Corbet [mailto:corbet@lwn.net]
> Sent: Saturday, 12 May, 2012 00:28
> To: Albert Wang
> Cc: 'Guennadi Liakhovetski'; Linux Media Mailing List; Mauro Carvalho 
> Chehab; Chao Xie; Angela Wan; Kassey Lee; Albert
> Subject: Re: marvell-ccic: lacks of some features
> 
> On Fri, 11 May 2012 09:02:26 -0700
> Albert Wang <twang13@marvell.com> wrote:
> 
> > Hi, Jonathan & Guennadi
> > 
> > We used the marvell-ccic code and found it lacks of some features, 
> > but our Marvell Camera driver (mv_camera.c) which based on 
> > soc_camera can support all these features:
> 
> The marvell-ccic driver has the features that were needed by the people doing the work and that I had the documentation and hardware to support.
> Of course it's incomplete.
> 
> I'll go through your list, but, first: is the purpose of your message to argue for a replacement of the marvell-ccic driver by your mv_camera driver?  I am not necessarily opposed to that idea if mv_camera can support deployed systems back to the OLPC XO 1.0 and if it seems clear that a replacement makes more sense than adding features to the in-tree driver.  
> 
> > 1. marvell-ccic only support MMP2 (PXA688), it can’t support other 
> > Marvell SOC chips Our mv_camera can support such as MMP3 (PXA2128), 
> > TD
> > (PXA910/920) and so on besides MMP2
> 
> Which is cool.  It is nice that Marvell is finally providing Linux support for its camera controllers after all these years.  For the last several years, I've necessarily been limited in the controllers I could support.
> Is it Marvell's intention to provide upstream maintenance and support going forward? 
> 
> > 2. marvell-ccic only support parallel (DVP) mode, can’t support MIPI 
> > mode Our mv_camera can support both DVP mode and MIPI mode, MIPI 
> > interface is the trend of current camera sensors with high 
> > resolution
> 
> Adding MIPI doesn't look that hard, I've just never had a reason (or
> hardware) to do it.
> 
> > 
> > 3. marvell-ccic only support ccic1 controller, can’t support ccic2 
> > or dual ccic controllers As you known, both MMP2 and MMP3 have 2 
> > ccic controllers, ccic2 is different with ccic1 Sometimes we need 
> > use both
> > 2 ccic controllers for connecting 2 camera sensors Actually, we have 
> > used 2 ccic controllers' cases in our platforms Our mv_camera can 
> > support these cases: only use ccic1, only use ccic2 and use ccic1 +
> > ccic2
> 
> It doesn't support two because nobody has asked for it, but the driver was written with that in mind.  I don't see supporting the second controller as a big job.
> 
> > 4. marvell-ccic only support camera sensor OV7670 It's an old and 
> > low resolution parallel sensor, and sensor info also is hard code 
> > But it loooks we should better separate controller and sensor in 
> > driver, controller doesn't care sensor type which will communicate 
> > with Our mv_camera can support any camera sensor which based on 
> > subdev structure
> 
> That is a well-known limitation, which, again, isn't that hard to fix.  The main problem is fixing it without breaking existing users.
> 
> > 5. marvell-ccic only support YUYV format which is packed format 
> > besides
> > RGB444 and RGB565, it can’t support planar formats Our mv_camera can 
> > support both packed format and planar formats such as
> > YUV420 and YUV422P
> 
> Again, not a fundamental driver limitation; definitely worth fixing when somebody actually needs planar formats.
> 
> > 6. marvell-ccic didn't support JPEG format for still capture mode 
> > Our mv_camera can support JPEG directly for still capture, most high 
> > resolution camera sensor can output JPEG format directly
> 
> That would indeed be a nice feature to have.
> 
> > 7. marvell-ccic can’t support dual camera sensors or multi camera 
> > sensors cases Current most platforms can support dual camera sensor 
> > case, include front-facing sensor and rear-facing sensor Even some 
> > high end platforms can support 3D mode record; it need support 2+1 camera sensors Our mv_camera can support these cases:
> > dual camera sensor connect to ccic1 or ccic2 one camera sensor 
> > connect to ccic1 and the other camera sensor connect to ccic2 dual 
> > camera sensor connect to ccic1 and one camera sensor connect to 
> > ccic2
> 
> Sounds like nice stuff.
> 
> > 8. marvell-ccic can’t support external ISP + raw camera sensor mode 
> > As you known, more and more camera sensors with high resolution are 
> > raw camera sensors but not smart sensors It needs external ISP 
> > (Image Signal
> > Processor) to generate the desired formats and resolutions with some 
> > advanced features based on the raw data from sensor Our mv_camera 
> > can support both smart camera sensors and external ISP + raw camera 
> > sensors
> 
> Supporting that mode would be a significant bit of work.  But please let's not confuse "can't" and "doesn't currently."
> 
> > 9. marvell-ccic still used obsolete method to stop ccic DMA This 
> > method should be inheritted from old cafe-ccic driver, it use CF 
> > flag which is trigged by SOF This method is inefficient, we must 
> > wait at least 150ms for stop ccic DMA and it also can result in many 
> > issues during thousands resolutions or formats switch stress test 
> > Actually our ccic can handle it if we use the right stop sequence by 
> > config some ccic registers Our mv_camera had applied the new and 
> > right stop method and it also passed the thousands resolutions or 
> > formats switch stress test
> 
> Which DMA mode are you talking about now?  I've supported DMA to the best of my ability given the information in the data sheet, plus a couple of hints from Kassey Lee (who, I believe, no longer works there?).  This can't be a hard thing to change, anyway.
> 
> Anyway, we're seeing the results of Marvell going off and working on its own private code instead of enhancing the in-tree driver that has been there since 2006.  Sad, but so it goes.  But if Marvell wants to work upstream now, I sure don't want to make things harder.  How about we get a new version of the mv_camera driver for review and we can all think about what's the best thing to do at this point?
> 
> Thanks,
> 
> jon
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer http://www.open-technology.de/