RE: [PATCH 15/15] [media] marvell-ccic: add 3 frame buffers support in DMA_CONTIG mode
Hi, Guennadi
Thanks a lot for your review! :)
>-Original Message-
>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>Sent: Wednesday, 28 November, 2012 00:30
>To: Albert Wang
>Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang
>Subject: Re: [PATCH 15/15] [media] marvell-ccic: add 3 frame buffers support in
>DMA_CONTIG mode
>
>On Fri, 23 Nov 2012, Albert Wang 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
>>
>> Signed-off-by: Albert Wang
>> ---
>> 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(-)
>>
>> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
>> b/drivers/media/platform/marvell-ccic/mcam-core.c
>> index 2d200d6..3b75594 100755
>> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
>> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
>> @@ -401,13 +401,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.tribufs == 0)
>> +cam->frame_state.tribufs++;
>
>TBH, I don't understand what the "tribuf" field means and what it is doing.
>Could you
>explain a bit?
>
Yes, in the first version, I just use tribufs in the 3 frame buffer mode.
Then I consolidated the controls of 2 buffers mode and 3 buffers mode according
to Jonathan's suggestion.
But still continue to use the "tribufs", maybe it's confused.
>> +else {
>> +set_bit(CF_SINGLE_BUFFER, &cam->flags);
>> +cam->frame_state.singles++;
>> +if (cam->frame_state.tribufs < 2)
>> +cam->frame_state.tribufs++;
>
>This seems to be the only location, where tribuf affects the control flow.
>So, it looks like, it controls, if no more buffers are on the queue, wheather
>you need to
>set the CF_SINGLE_BUFFER flag and increment the singles count.
>
Yes, the tribufs indicates which conditions we need set the single buffer flag.
>Thanks
>Guennadi
>
>> +}
>> } else {
>> /*
>> * OK, we have a buffer we can use.
>> @@ -416,15 +435,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.tribufs != (3 - MAX_FRAME_BUFS))
>> +cam->frame_state.tribufs--;
>> }
>>
>> 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 ?
>> -
Re: [PATCH 15/15] [media] marvell-ccic: add 3 frame buffers support in DMA_CONTIG mode
On Fri, 23 Nov 2012, Albert Wang 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
>
> Signed-off-by: Albert Wang
> ---
> 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(-)
>
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
> b/drivers/media/platform/marvell-ccic/mcam-core.c
> index 2d200d6..3b75594 100755
> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
> @@ -401,13 +401,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.tribufs == 0)
> + cam->frame_state.tribufs++;
TBH, I don't understand what the "tribuf" field means and what it is
doing. Could you explain a bit?
> + else {
> + set_bit(CF_SINGLE_BUFFER, &cam->flags);
> + cam->frame_state.singles++;
> + if (cam->frame_state.tribufs < 2)
> + cam->frame_state.tribufs++;
This seems to be the only location, where tribuf affects the control flow.
So, it looks like, it controls, if no more buffers are on the queue,
wheather you need to set the CF_SINGLE_BUFFER flag and increment the
singles count.
Thanks
Guennadi
> + }
> } else {
> /*
>* OK, we have a buffer we can use.
> @@ -416,15 +435,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.tribufs != (3 - MAX_FRAME_BUFS))
> + cam->frame_state.tribufs--;
> }
>
> 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);
> }
> }
>
> @@ -433,10 +452,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);
> }
>
> /*
> @@ -1043,6 +1066,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 tribufs == 1
> + * If CCIC use Three Buffers mode, init tribufs == 0
> + */
> + cam->frame_state.tribufs = 3 - MAX_FRAME_BUFS;
>
[PATCH 15/15] [media] marvell-ccic: add 3 frame buffers support in DMA_CONTIG mode
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
---
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(-)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
b/drivers/media/platform/marvell-ccic/mcam-core.c
index 2d200d6..3b75594 100755
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -401,13 +401,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.tribufs == 0)
+ cam->frame_state.tribufs++;
+ else {
+ set_bit(CF_SINGLE_BUFFER, &cam->flags);
+ cam->frame_state.singles++;
+ if (cam->frame_state.tribufs < 2)
+ cam->frame_state.tribufs++;
+ }
} else {
/*
* OK, we have a buffer we can use.
@@ -416,15 +435,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.tribufs != (3 - MAX_FRAME_BUFS))
+ cam->frame_state.tribufs--;
}
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);
}
}
@@ -433,10 +452,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);
}
/*
@@ -1043,6 +1066,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 tribufs == 1
+* If CCIC use Three Buffers mode, init tribufs == 0
+*/
+ cam->frame_state.tribufs = 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 5b2cf6e..6420754 100755
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -68,6 +68,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 Buffer
