Re: [PATCH 3/4] media i.MX27 camera: improve discard buffer handling.
Hi Guennadi,
On 27 January 2012 19:02, Guennadi Liakhovetski g.liakhovet...@gmx.de wrote:
(removing bar...@tkos.co.il - it bounces)
On Thu, 26 Jan 2012, javier Martin wrote:
Hi Guennadi,
On 25 January 2012 13:12, Guennadi Liakhovetski g.liakhovet...@gmx.de
wrote:
On Fri, 20 Jan 2012, Javier Martin wrote:
The way discard buffer was previously handled lead
to possible races that made a buffer that was not
yet ready to be overwritten by new video data. This
is easily detected at 25fps just adding #define DEBUG
to enable the memset check and seeing how the image
is corrupted.
A new discard queue and two discard buffers have
been added to make them flow trough the pipeline
of queues and thus provide suitable event ordering.
Hmm, do I understand it right, that the problem is, that while the first
frame went to the discard buffer, the second one went already to a user
buffer, while it wasn't ready yet?
The problem is not only related to the discard buffer but also the way
valid buffers were handled in an unsafe basis.
For instance, the buf-bufnum = !bufnum issue. If you receive and
IRQ from bufnum = 0 you have to update buffer 0, not buffer 1.
And you solve this by adding one more
discard buffer? Wouldn't it be possible to either not start capture until
.start_streaming() is issued, which should also be the case after your
patch 2/4, or, at least, just reuse one discard buffer multiple times
until user buffers are available?
If I understand right, you don't really introduce two discard buffers,
there's still only one data buffer, but you add two discard data objects
and a list to keep them on. TBH, this seems severely over-engineered to
me. What's wrong with just keeping one DMA data buffer and using it as
long, as needed, and checking in your ISR, whether a proper buffer is
present, by looking for list_empty(active)?
I added a couple of comments below, but my biggest question really is -
why are these two buffer objects needed? Please, consider getting rid of
them. So, this is not a full review, if the objects get removed, most of
this patch will change anyway.
1. Why a discard buffer is needed?
When I first took a look at the code it only supported CH1 of the PrP
(i.e. RGB formats) and a discard buffer was used. My first reaction
was trying to get rid of that trick. Both CH1 and CH2 of the PrP have
two pointers that the engine uses to write video frames in a ping-pong
basis. However, there is a big difference between both channels: if
you want to detect frame loss in CH1 you have to continually feed the
two pointers with valid memory areas because the engine is always
writing and you can't stop it, and this is where the discard buffer
function is required, but CH2 has a mechanism to stop capturing and
keep counting loss frames, thus a discard buffer wouldn't be needed.
To sum up: the driver would be much cleaner without this discard
buffer trick but we would have to drop support for CH1 (RGB format).
Being respectful to CH1 support I decided to add CH2 by extending the
discard buffer strategy to both channels, since the code is cleaner
this way and doesn't make sense to manage both channels differently.
2. Why two discard buffer objects are needed?
After enabling the DEBUG functionality that writes the buffers with
0xaa before they are filled with video data, I discovered some of them
were being corrupted. When I tried to find the reason I found that we
really have a pipeline here:
--- ---
capture (n) | active_bufs (2)|
---
where
capture has buffers that are queued and ready to be written into
active_bufs represents those buffers that are assigned to a pointer
in the PrP and has a maximum of 2 since there are two pointers that
are written in a ping-pong fashion
Ok, I understand what the discard memory is used for and why you need to
write it twice to the hardware - to those two pointers. And I can kindof
agree, that using them uniformly with user buffers on the active list
simplifies handling. I just don't think it's a good idea to keep two
struct vb2_buffer instances around with no use. Maybe you could use
something like
struct mx2_buf_internal {
struct list_head queue;
int bufnum;
bool discard;
};
struct mx2_buffer {
struct vb2_buffer vb;
enum mx2_buffer_state state;
struct mx2_buf_internal internal;
};
and only use struct mx2_buf_internal for your discard buffers.
You are right, the approach you suggest is more efficient.
What I purpose is that you accept my following v3 patch series and
allow me to send a further cleanup series with the following changes:
1. Remove goto out from mx2_videobuf_queue.
2. Use
Re: [PATCH 3/4] media i.MX27 camera: improve discard buffer handling.
(removing bar...@tkos.co.il - it bounces)
On Thu, 26 Jan 2012, javier Martin wrote:
Hi Guennadi,
On 25 January 2012 13:12, Guennadi Liakhovetski g.liakhovet...@gmx.de wrote:
On Fri, 20 Jan 2012, Javier Martin wrote:
The way discard buffer was previously handled lead
to possible races that made a buffer that was not
yet ready to be overwritten by new video data. This
is easily detected at 25fps just adding #define DEBUG
to enable the memset check and seeing how the image
is corrupted.
A new discard queue and two discard buffers have
been added to make them flow trough the pipeline
of queues and thus provide suitable event ordering.
Hmm, do I understand it right, that the problem is, that while the first
frame went to the discard buffer, the second one went already to a user
buffer, while it wasn't ready yet?
The problem is not only related to the discard buffer but also the way
valid buffers were handled in an unsafe basis.
For instance, the buf-bufnum = !bufnum issue. If you receive and
IRQ from bufnum = 0 you have to update buffer 0, not buffer 1.
And you solve this by adding one more
discard buffer? Wouldn't it be possible to either not start capture until
.start_streaming() is issued, which should also be the case after your
patch 2/4, or, at least, just reuse one discard buffer multiple times
until user buffers are available?
If I understand right, you don't really introduce two discard buffers,
there's still only one data buffer, but you add two discard data objects
and a list to keep them on. TBH, this seems severely over-engineered to
me. What's wrong with just keeping one DMA data buffer and using it as
long, as needed, and checking in your ISR, whether a proper buffer is
present, by looking for list_empty(active)?
I added a couple of comments below, but my biggest question really is -
why are these two buffer objects needed? Please, consider getting rid of
them. So, this is not a full review, if the objects get removed, most of
this patch will change anyway.
1. Why a discard buffer is needed?
When I first took a look at the code it only supported CH1 of the PrP
(i.e. RGB formats) and a discard buffer was used. My first reaction
was trying to get rid of that trick. Both CH1 and CH2 of the PrP have
two pointers that the engine uses to write video frames in a ping-pong
basis. However, there is a big difference between both channels: if
you want to detect frame loss in CH1 you have to continually feed the
two pointers with valid memory areas because the engine is always
writing and you can't stop it, and this is where the discard buffer
function is required, but CH2 has a mechanism to stop capturing and
keep counting loss frames, thus a discard buffer wouldn't be needed.
To sum up: the driver would be much cleaner without this discard
buffer trick but we would have to drop support for CH1 (RGB format).
Being respectful to CH1 support I decided to add CH2 by extending the
discard buffer strategy to both channels, since the code is cleaner
this way and doesn't make sense to manage both channels differently.
2. Why two discard buffer objects are needed?
After enabling the DEBUG functionality that writes the buffers with
0xaa before they are filled with video data, I discovered some of them
were being corrupted. When I tried to find the reason I found that we
really have a pipeline here:
--- ---
capture (n) | active_bufs (2)|
---
where
capture has buffers that are queued and ready to be written into
active_bufs represents those buffers that are assigned to a pointer
in the PrP and has a maximum of 2 since there are two pointers that
are written in a ping-pong fashion
Ok, I understand what the discard memory is used for and why you need to
write it twice to the hardware - to those two pointers. And I can kindof
agree, that using them uniformly with user buffers on the active list
simplifies handling. I just don't think it's a good idea to keep two
struct vb2_buffer instances around with no use. Maybe you could use
something like
struct mx2_buf_internal {
struct list_headqueue;
int bufnum;
booldiscard;
};
struct mx2_buffer {
struct vb2_buffer vb;
enum mx2_buffer_state state;
struct mx2_buf_internal internal;
};
and only use struct mx2_buf_internal for your discard buffers.
Thanks
Guennadi
However, with the previous approach, the discard buffer is kept
outside this pipeline as if it was a global variable which is usually
a dangerous practice and definitely it's wrong since the buffers are
corrupted.
On the other hand, if we add 2 discard buffer objects we will be able
to pass them through the
Re: [PATCH 3/4] media i.MX27 camera: improve discard buffer handling.
On Fri, 20 Jan 2012, Javier Martin wrote:
The way discard buffer was previously handled lead
to possible races that made a buffer that was not
yet ready to be overwritten by new video data. This
is easily detected at 25fps just adding #define DEBUG
to enable the memset check and seeing how the image
is corrupted.
A new discard queue and two discard buffers have
been added to make them flow trough the pipeline
of queues and thus provide suitable event ordering.
Hmm, do I understand it right, that the problem is, that while the first
frame went to the discard buffer, the second one went already to a user
buffer, while it wasn't ready yet? And you solve this by adding one more
discard buffer? Wouldn't it be possible to either not start capture until
.start_streaming() is issued, which should also be the case after your
patch 2/4, or, at least, just reuse one discard buffer multiple times
until user buffers are available?
If I understand right, you don't really introduce two discard buffers,
there's still only one data buffer, but you add two discard data objects
and a list to keep them on. TBH, this seems severely over-engineered to
me. What's wrong with just keeping one DMA data buffer and using it as
long, as needed, and checking in your ISR, whether a proper buffer is
present, by looking for list_empty(active)?
I added a couple of comments below, but my biggest question really is -
why are these two buffer objects needed? Please, consider getting rid of
them. So, this is not a full review, if the objects get removed, most of
this patch will change anyway.
Signed-off-by: Javier Martin javier.mar...@vista-silicon.com
---
drivers/media/video/mx2_camera.c | 215
+-
1 files changed, 117 insertions(+), 98 deletions(-)
diff --git a/drivers/media/video/mx2_camera.c
b/drivers/media/video/mx2_camera.c
index 4816da6..e0c5dd4 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -224,6 +224,28 @@ struct mx2_fmt_cfg {
struct mx2_prp_cfg cfg;
};
+enum mx2_buffer_state {
+ MX2_STATE_NEEDS_INIT = 0,
+ MX2_STATE_PREPARED = 1,
+ MX2_STATE_QUEUED = 2,
+ MX2_STATE_ACTIVE = 3,
+ MX2_STATE_DONE = 4,
+ MX2_STATE_ERROR = 5,
+ MX2_STATE_IDLE = 6,
+};
+
+/* buffer for one video frame */
+struct mx2_buffer {
+ /* common v4l buffer stuff -- must be first */
+ struct vb2_buffer vb;
+ struct list_headqueue;
+ enum mx2_buffer_state state;
+ enum v4l2_mbus_pixelcodecode;
+
+ int bufnum;
+ booldiscard;
+};
+
When submitting a patch series, it is usually good to avoid
double-patching. E.g., in this case, your first patch adds these enum and
struct and this patch moves them a couple of lines up. Please, place them
at the correct location already with the first patch.
struct mx2_camera_dev {
struct device *dev;
struct soc_camera_host soc_host;
@@ -240,6 +262,7 @@ struct mx2_camera_dev {
struct list_headcapture;
struct list_headactive_bufs;
+ struct list_headdiscard;
spinlock_t lock;
@@ -252,6 +275,7 @@ struct mx2_camera_dev {
u32 csicr1;
+ struct mx2_buffer buf_discard[2];
void*discard_buffer;
dma_addr_t discard_buffer_dma;
size_t discard_size;
@@ -260,27 +284,6 @@ struct mx2_camera_dev {
struct vb2_alloc_ctx*alloc_ctx;
};
-enum mx2_buffer_state {
- MX2_STATE_NEEDS_INIT = 0,
- MX2_STATE_PREPARED = 1,
- MX2_STATE_QUEUED = 2,
- MX2_STATE_ACTIVE = 3,
- MX2_STATE_DONE = 4,
- MX2_STATE_ERROR = 5,
- MX2_STATE_IDLE = 6,
-};
-
-/* buffer for one video frame */
-struct mx2_buffer {
- /* common v4l buffer stuff -- must be first */
- struct vb2_buffer vb;
- struct list_headqueue;
- enum mx2_buffer_state state;
- enum v4l2_mbus_pixelcodecode;
-
- int bufnum;
-};
-
static struct mx2_fmt_cfg mx27_emma_prp_table[] = {
/*
* This is a generic configuration which is valid for most
@@ -334,6 +337,29 @@ static struct mx2_fmt_cfg *mx27_emma_prp_get_format(
return mx27_emma_prp_table[0];
};
+static void mx27_update_emma_buf(struct mx2_camera_dev *pcdev,
+ unsigned long phys, int bufnum)
+{
+ u32 imgsize = pcdev-icd-user_height * pcdev-icd-user_width;
Are only 1-byte-per-pixel formats supported? Ok, it is only used for
YUV420, please, move this variable down in that if.
+ struct mx2_fmt_cfg *prp = pcdev-emma_prp;
+
+ if (prp-cfg.channel == 1) {
+
[PATCH 3/4] media i.MX27 camera: improve discard buffer handling.
The way discard buffer was previously handled lead
to possible races that made a buffer that was not
yet ready to be overwritten by new video data. This
is easily detected at 25fps just adding #define DEBUG
to enable the memset check and seeing how the image
is corrupted.
A new discard queue and two discard buffers have
been added to make them flow trough the pipeline
of queues and thus provide suitable event ordering.
Signed-off-by: Javier Martin javier.mar...@vista-silicon.com
---
drivers/media/video/mx2_camera.c | 215 +-
1 files changed, 117 insertions(+), 98 deletions(-)
diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index 4816da6..e0c5dd4 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -224,6 +224,28 @@ struct mx2_fmt_cfg {
struct mx2_prp_cfg cfg;
};
+enum mx2_buffer_state {
+ MX2_STATE_NEEDS_INIT = 0,
+ MX2_STATE_PREPARED = 1,
+ MX2_STATE_QUEUED = 2,
+ MX2_STATE_ACTIVE = 3,
+ MX2_STATE_DONE = 4,
+ MX2_STATE_ERROR = 5,
+ MX2_STATE_IDLE = 6,
+};
+
+/* buffer for one video frame */
+struct mx2_buffer {
+ /* common v4l buffer stuff -- must be first */
+ struct vb2_buffer vb;
+ struct list_headqueue;
+ enum mx2_buffer_state state;
+ enum v4l2_mbus_pixelcodecode;
+
+ int bufnum;
+ booldiscard;
+};
+
struct mx2_camera_dev {
struct device *dev;
struct soc_camera_host soc_host;
@@ -240,6 +262,7 @@ struct mx2_camera_dev {
struct list_headcapture;
struct list_headactive_bufs;
+ struct list_headdiscard;
spinlock_t lock;
@@ -252,6 +275,7 @@ struct mx2_camera_dev {
u32 csicr1;
+ struct mx2_buffer buf_discard[2];
void*discard_buffer;
dma_addr_t discard_buffer_dma;
size_t discard_size;
@@ -260,27 +284,6 @@ struct mx2_camera_dev {
struct vb2_alloc_ctx*alloc_ctx;
};
-enum mx2_buffer_state {
- MX2_STATE_NEEDS_INIT = 0,
- MX2_STATE_PREPARED = 1,
- MX2_STATE_QUEUED = 2,
- MX2_STATE_ACTIVE = 3,
- MX2_STATE_DONE = 4,
- MX2_STATE_ERROR = 5,
- MX2_STATE_IDLE = 6,
-};
-
-/* buffer for one video frame */
-struct mx2_buffer {
- /* common v4l buffer stuff -- must be first */
- struct vb2_buffer vb;
- struct list_headqueue;
- enum mx2_buffer_state state;
- enum v4l2_mbus_pixelcodecode;
-
- int bufnum;
-};
-
static struct mx2_fmt_cfg mx27_emma_prp_table[] = {
/*
* This is a generic configuration which is valid for most
@@ -334,6 +337,29 @@ static struct mx2_fmt_cfg *mx27_emma_prp_get_format(
return mx27_emma_prp_table[0];
};
+static void mx27_update_emma_buf(struct mx2_camera_dev *pcdev,
+unsigned long phys, int bufnum)
+{
+ u32 imgsize = pcdev-icd-user_height * pcdev-icd-user_width;
+ struct mx2_fmt_cfg *prp = pcdev-emma_prp;
+
+ if (prp-cfg.channel == 1) {
+ writel(phys, pcdev-base_emma +
+ PRP_DEST_RGB1_PTR + 4 * bufnum);
+ } else {
+ writel(phys, pcdev-base_emma +
+ PRP_DEST_Y_PTR -
+ 0x14 * bufnum);
+ if (prp-out_fmt == V4L2_PIX_FMT_YUV420) {
+ writel(phys + imgsize,
+ pcdev-base_emma + PRP_DEST_CB_PTR -
+ 0x14 * bufnum);
+ writel(phys + ((5 * imgsize) / 4), pcdev-base_emma +
+ PRP_DEST_CR_PTR - 0x14 * bufnum);
+ }
+ }
+}
+
static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
{
unsigned long flags;
@@ -382,7 +408,7 @@ static int mx2_camera_add_device(struct soc_camera_device
*icd)
writel(pcdev-csicr1, pcdev-base_csi + CSICR1);
pcdev-icd = icd;
- pcdev-frame_count = -1;
+ pcdev-frame_count = 0;
dev_info(icd-parent, Camera driver attached to camera %d\n,
icd-devnum);
@@ -648,10 +674,9 @@ static void mx2_videobuf_release(struct vb2_buffer *vb)
* types.
*/
spin_lock_irqsave(pcdev-lock, flags);
- if (buf-state == MX2_STATE_QUEUED || buf-state == MX2_STATE_ACTIVE) {
- list_del_init(buf-queue);
- buf-state = MX2_STATE_NEEDS_INIT;
- } else if (cpu_is_mx25() buf-state == MX2_STATE_ACTIVE) {
+ INIT_LIST_HEAD(buf-queue);
+ buf-state = MX2_STATE_NEEDS_INIT;
+ if (cpu_is_mx25() buf-state ==
