marvell-ccic: lacks of some features

[Albert Wang <twang13@marvell.com>]

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/