diff mbox

[V3,15/15,media] marvell-ccic: add 3 frame buffers support in DMA_CONTIG mode

Message ID 1355565484-15791-16-git-send-email-twang13@marvell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Albert Wang Dec. 15, 2012, 9:58 a.m. UTC 
This patch adds support of 3 frame buffers in DMA-contiguous mode.

In current DMA_CONTIG mode, only 2 frame buffers can be supported.
Actually, Marvell CCIC can support at most 3 frame buffers.

Currently 2 frame buffers mode will be used by default.
To use 3 frame buffers mode, can do:
  define MAX_FRAME_BUFS 3
in mcam-core.h

Signed-off-by: Albert Wang <twang13@marvell.com>
Signed-off-by: Libin Yang <lbyang@marvell.com>
---
 drivers/media/platform/marvell-ccic/mcam-core.c |   59 +++++++++++++++++------
 drivers/media/platform/marvell-ccic/mcam-core.h |   11 +++++
 2 files changed, 55 insertions(+), 15 deletions(-)

Comments

Jonathan Corbet Dec. 16, 2012, 4:56 p.m. UTC | #1 
On Sat, 15 Dec 2012 17:58:04 +0800
Albert Wang <twang13@marvell.com> wrote:

> This patch adds support of 3 frame buffers in DMA-contiguous mode.
> 
> In current DMA_CONTIG mode, only 2 frame buffers can be supported.
> Actually, Marvell CCIC can support at most 3 frame buffers.
> 
> Currently 2 frame buffers mode will be used by default.
> To use 3 frame buffers mode, can do:
>   define MAX_FRAME_BUFS 3
> in mcam-core.h

Now that the code supports three buffers properly, is there any reason not
to use that mode by default?

Did you test that it works properly if allocation of the third buffer fails?

Otherwise looks OK except for one thing:

> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h
> index 765d47c..9bf31c8 100755
> --- a/drivers/media/platform/marvell-ccic/mcam-core.h
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.h
> @@ -62,6 +62,13 @@ enum mcam_state {
>  #define MAX_DMA_BUFS 3
>  
>  /*
> + * CCIC can support at most 3 frame buffers in DMA_CONTIG buffer mode
> + * 2 - Use Two Buffers mode
> + * 3 - Use Three Buffers mode
> + */
> +#define MAX_FRAME_BUFS 2 /* Current marvell-ccic used Two Buffers mode */
> +
> +/*
>   * Different platforms work best with different buffer modes, so we
>   * let the platform pick.
>   */
> @@ -99,6 +106,10 @@ struct mcam_frame_state {
>  	unsigned int frames;
>  	unsigned int singles;
>  	unsigned int delivered;
> +	/*
> +	 * Only usebufs == 2 can enter single buffer mode
> +	 */
> +	unsigned int usebufs;
>  };

What is the purpose of the "usebufs" field?  The code maintains it in
various places, but I don't see anywhere that actually uses that value for
anything.

Thanks,

jon
--
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
Albert Wang Dec. 16, 2012, 10:34 p.m. UTC | #2 
Hi, Jonathan


>-----Original Message-----
>From: Jonathan Corbet [mailto:corbet@lwn.net]
>Sent: Monday, 17 December, 2012 00:56
>To: Albert Wang
>Cc: g.liakhovetski@gmx.de; linux-media@vger.kernel.org; Libin Yang
>Subject: Re: [PATCH V3 15/15] [media] marvell-ccic: add 3 frame buffers support in
>DMA_CONTIG mode
>
>On Sat, 15 Dec 2012 17:58:04 +0800
>Albert Wang <twang13@marvell.com> wrote:
>
>> This patch adds support of 3 frame buffers in DMA-contiguous mode.
>>
>> In current DMA_CONTIG mode, only 2 frame buffers can be supported.
>> Actually, Marvell CCIC can support at most 3 frame buffers.
>>
>> Currently 2 frame buffers mode will be used by default.
>> To use 3 frame buffers mode, can do:
>>   define MAX_FRAME_BUFS 3
>> in mcam-core.h
>
>Now that the code supports three buffers properly, is there any reason not
>to use that mode by default?
>
[Albert Wang] Because the original code use the two buffers mode, so we keep it. :)

>Did you test that it works properly if allocation of the third buffer fails?
>
[Albert Wang] Yes, we test it in our Marvell platforms.

>Otherwise looks OK except for one thing:
>
>> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h
>b/drivers/media/platform/marvell-ccic/mcam-core.h
>> index 765d47c..9bf31c8 100755
>> --- a/drivers/media/platform/marvell-ccic/mcam-core.h
>> +++ b/drivers/media/platform/marvell-ccic/mcam-core.h
>> @@ -62,6 +62,13 @@ enum mcam_state {
>>  #define MAX_DMA_BUFS 3
>>
>>  /*
>> + * CCIC can support at most 3 frame buffers in DMA_CONTIG buffer mode
>> + * 2 - Use Two Buffers mode
>> + * 3 - Use Three Buffers mode
>> + */
>> +#define MAX_FRAME_BUFS 2 /* Current marvell-ccic used Two Buffers mode */
>> +
>> +/*
>>   * Different platforms work best with different buffer modes, so we
>>   * let the platform pick.
>>   */
>> @@ -99,6 +106,10 @@ struct mcam_frame_state {
>>  	unsigned int frames;
>>  	unsigned int singles;
>>  	unsigned int delivered;
>> +	/*
>> +	 * Only usebufs == 2 can enter single buffer mode
>> +	 */
>> +	unsigned int usebufs;
>>  };
>
>What is the purpose of the "usebufs" field?  The code maintains it in
>various places, but I don't see anywhere that actually uses that value for
>anything.
>
[Albert Wang] Two buffers mode doesn't need it.
But Three buffers mode need it indicates which conditions we need set the single buffer flag.
I used "tribufs" as the name in the previous version, but it looks it's a confused name when we merged
Two buffers mode and Three buffers mode with same code by removing #ifdef based on your comments months ago. :)
So we just changed the name with "usebufs".

>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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jonathan Corbet Dec. 16, 2012, 10:55 p.m. UTC | #3 
On Sun, 16 Dec 2012 14:34:31 -0800
Albert Wang <twang13@marvell.com> wrote:

> >What is the purpose of the "usebufs" field?  The code maintains it in
> >various places, but I don't see anywhere that actually uses that value for
> >anything.
> >  
> [Albert Wang] Two buffers mode doesn't need it.
> But Three buffers mode need it indicates which conditions we need set the single buffer flag.
> I used "tribufs" as the name in the previous version, but it looks it's a confused name when we merged
> Two buffers mode and Three buffers mode with same code by removing #ifdef based on your comments months ago. :)
> So we just changed the name with "usebufs".

OK, I misread the code a bit, sorry.  I do find the variable confusing
still, but it clearly does play a role.

I think that using three buffers by default would make sense.  I don't
think that increased overruns are an unbreakable ABI feature :)

Feel free to add my ack to this one.

Thanks,

jon
--
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
Albert Wang Dec. 17, 2012, 5:06 a.m. UTC | #4 
Hi, Jonathan


>-----Original Message-----
>From: Jonathan Corbet [mailto:corbet@lwn.net]
>Sent: Monday, 17 December, 2012 06:55
>To: Albert Wang
>Cc: g.liakhovetski@gmx.de; linux-media@vger.kernel.org; Libin Yang
>Subject: Re: [PATCH V3 15/15] [media] marvell-ccic: add 3 frame buffers support in
>DMA_CONTIG mode
>
>On Sun, 16 Dec 2012 14:34:31 -0800
>Albert Wang <twang13@marvell.com> wrote:
>
>> >What is the purpose of the "usebufs" field?  The code maintains it in
>> >various places, but I don't see anywhere that actually uses that value for
>> >anything.
>> >
>> [Albert Wang] Two buffers mode doesn't need it.
>> But Three buffers mode need it indicates which conditions we need set the single
>buffer flag.
>> I used "tribufs" as the name in the previous version, but it looks it's a confused name
>when we merged
>> Two buffers mode and Three buffers mode with same code by removing #ifdef based
>on your comments months ago. :)
>> So we just changed the name with "usebufs".
>
>OK, I misread the code a bit, sorry.  I do find the variable confusing
>still, but it clearly does play a role.
>
>I think that using three buffers by default would make sense.  I don't
>think that increased overruns are an unbreakable ABI feature :)
>
[Albert Wang] OK, we can change the default to three buffers mode.

>Feel free to add my ack to this one.
>
>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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 2a4d481..d7f124d 100755
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -400,13 +400,32 @@  static void mcam_set_contig_buffer(struct mcam_camera *cam, unsigned int frame)
 	struct mcam_vb_buffer *buf;
 	struct v4l2_pix_format *fmt = &cam->pix_format;
 
-	/*
-	 * If there are no available buffers, go into single mode
-	 */
 	if (list_empty(&cam->buffers)) {
-		buf = cam->vb_bufs[frame ^ 0x1];
-		set_bit(CF_SINGLE_BUFFER, &cam->flags);
-		cam->frame_state.singles++;
+		/*
+		 * If there are no available buffers
+		 * go into single buffer mode
+		 *
+		 * If CCIC use Two Buffers mode
+		 * will use another remaining frame buffer
+		 * frame 0 -> buf 1
+		 * frame 1 -> buf 0
+		 *
+		 * If CCIC use Three Buffers mode
+		 * will use the 2rd remaining frame buffer
+		 * frame 0 -> buf 2
+		 * frame 1 -> buf 0
+		 * frame 2 -> buf 1
+		 */
+		buf = cam->vb_bufs[(frame + (MAX_FRAME_BUFS - 1))
+						% MAX_FRAME_BUFS];
+		if (cam->frame_state.usebufs == 0)
+			cam->frame_state.usebufs++;
+		else {
+			set_bit(CF_SINGLE_BUFFER, &cam->flags);
+			cam->frame_state.singles++;
+			if (cam->frame_state.usebufs < 2)
+				cam->frame_state.usebufs++;
+		}
 	} else {
 		/*
 		 * OK, we have a buffer we can use.
@@ -415,15 +434,15 @@  static void mcam_set_contig_buffer(struct mcam_camera *cam, unsigned int frame)
 					queue);
 		list_del_init(&buf->queue);
 		clear_bit(CF_SINGLE_BUFFER, &cam->flags);
+		if (cam->frame_state.usebufs != (3 - MAX_FRAME_BUFS))
+			cam->frame_state.usebufs--;
 	}
 
 	cam->vb_bufs[frame] = buf;
-	mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR, buf->yuv_p.y);
+	mcam_reg_write(cam, REG_Y0BAR + (frame << 2), buf->yuv_p.y);
 	if (mcam_fmt_is_planar(fmt->pixelformat)) {
-		mcam_reg_write(cam, frame == 0 ?
-					REG_U0BAR : REG_U1BAR, buf->yuv_p.u);
-		mcam_reg_write(cam, frame == 0 ?
-					REG_V0BAR : REG_V1BAR, buf->yuv_p.v);
+		mcam_reg_write(cam, REG_U0BAR + (frame << 2), buf->yuv_p.u);
+		mcam_reg_write(cam, REG_V0BAR + (frame << 2), buf->yuv_p.v);
 	}
 }
 
@@ -432,10 +451,14 @@  static void mcam_set_contig_buffer(struct mcam_camera *cam, unsigned int frame)
  */
 void mcam_ctlr_dma_contig(struct mcam_camera *cam)
 {
-	mcam_reg_set_bit(cam, REG_CTRL1, C1_TWOBUFS);
-	cam->nbufs = 2;
-	mcam_set_contig_buffer(cam, 0);
-	mcam_set_contig_buffer(cam, 1);
+	unsigned int frame;
+
+	cam->nbufs = MAX_FRAME_BUFS;
+	for (frame = 0; frame < cam->nbufs; frame++)
+		mcam_set_contig_buffer(cam, frame);
+
+	if (cam->nbufs == 2)
+		mcam_reg_set_bit(cam, REG_CTRL1, C1_TWOBUFS);
 }
 
 /*
@@ -978,6 +1001,12 @@  static int mcam_vb_start_streaming(struct vb2_queue *vq, unsigned int count)
 	for (frame = 0; frame < cam->nbufs; frame++)
 		clear_bit(CF_FRAME_SOF0 + frame, &cam->flags);
 
+	/*
+	 *  If CCIC use Two Buffers mode, init usebufs == 1
+	 *  If CCIC use Three Buffers mode, init usebufs == 0
+	 */
+	cam->frame_state.usebufs = 3 - MAX_FRAME_BUFS;
+
 	return mcam_read_setup(cam);
 }
 
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h
index 765d47c..9bf31c8 100755
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -62,6 +62,13 @@  enum mcam_state {
 #define MAX_DMA_BUFS 3
 
 /*
+ * CCIC can support at most 3 frame buffers in DMA_CONTIG buffer mode
+ * 2 - Use Two Buffers mode
+ * 3 - Use Three Buffers mode
+ */
+#define MAX_FRAME_BUFS 2 /* Current marvell-ccic used Two Buffers mode */
+
+/*
  * Different platforms work best with different buffer modes, so we
  * let the platform pick.
  */
@@ -99,6 +106,10 @@  struct mcam_frame_state {
 	unsigned int frames;
 	unsigned int singles;
 	unsigned int delivered;
+	/*
+	 * Only usebufs == 2 can enter single buffer mode
+	 */
+	unsigned int usebufs;
 };
 
 /*