* [PATCH] [media]: s5p-fimc: fix ISR and buffer handling for fimc-capture
@ 2010-12-29 0:30 Sungchun Kang
2010-12-29 15:31 ` Sylwester Nawrocki
0 siblings, 1 reply; 2+ messages in thread
From: Sungchun Kang @ 2010-12-29 0:30 UTC (permalink / raw)
To: linux-media, linux-samsung-soc; +Cc: s.nawrocki, kgene.kim, Sungchun Kang
These patches are related and it may be summarized as follows.
1. Some of the case are fimc H/W did not stop although there are
no available output DMA buffer. So, it is modified check the
routine fimc deactivation. And, the state of ST_CAPT_RUN is cleared
in LAST-IRQ routine.
2. When request buffer count is less than 4, CIOYSAn register did
not set if VIDIOC_QBUF is called repeatedly. So, clear bit the
state of ST_CAPT_STREAM in ISR.
3. Because fimc interrupt generated when the frame start to write,
it is necessary to use LAST-IRQ for processing of the last frame.
So, added the enumeration for LAST-IRQ.
4. After LAST-IRQ is generated, H/W pointer will be skip 1 frame.
(reference by user manual) So, S/W pointer should be increased too.
Reviewed-by Jonghun Han <jonghun.han@samsung.com>
Signed-off-by: Sungchun Kang <sungchun.kang@samsung.com>
---
This patch is depended on:
http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/vb2-mfc-fimc
drivers/media/video/s5p-fimc/fimc-capture.c | 12 +++++++-----
drivers/media/video/s5p-fimc/fimc-core.c | 20 ++++++++++++++------
drivers/media/video/s5p-fimc/fimc-core.h | 1 +
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c
index 4e4441f..0a3b344 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -350,13 +350,15 @@ static void buffer_queue(struct vb2_buffer *vb)
dbg("active_buf_cnt: %d", fimc->vid_cap.active_buf_cnt);
- if (vid_cap->active_buf_cnt >= vid_cap->reqbufs_count ||
- vid_cap->active_buf_cnt >= FIMC_MAX_OUT_BUFS) {
- if (!test_and_set_bit(ST_CAPT_STREAM, &fimc->state)) {
+ if (vid_cap->active_buf_cnt == FIMC_MAX_OUT_BUFS)
+ set_bit(ST_CAPT_STREAM, &fimc->state);
+
+ if (test_bit(ST_CAPT_LAST_IRQ, &fimc->state) ||
+ !test_bit(ST_CAPT_RUN, &fimc->state)) {
fimc_activate_capture(ctx);
- dbg("");
- }
+ clear_bit(ST_CAPT_LAST_IRQ, &fimc->state);
}
+
spin_unlock_irqrestore(&fimc->slock, flags);
}
diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c
index 2374fd8..4a85966 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -334,9 +334,14 @@ static void fimc_capture_handler(struct fimc_dev *fimc)
if (++cap->buf_index >= FIMC_MAX_OUT_BUFS)
cap->buf_index = 0;
- } else if (test_and_clear_bit(ST_CAPT_STREAM, &fimc->state) &&
- cap->active_buf_cnt <= 1) {
- fimc_deactivate_capture(fimc);
+ } else {
+ clear_bit(ST_CAPT_STREAM, &fimc->state);
+ }
+
+ if (cap->active_buf_cnt == 0) {
+ clear_bit(ST_CAPT_RUN, &fimc->state);
+ if (++cap->buf_index >= FIMC_MAX_OUT_BUFS)
+ cap->buf_index = 0;
}
dbg("frame: %d, active_buf_cnt= %d",
@@ -346,6 +351,7 @@ static void fimc_capture_handler(struct fimc_dev *fimc)
static irqreturn_t fimc_isr(int irq, void *priv)
{
struct fimc_dev *fimc = priv;
+ struct fimc_vid_cap *cap = &fimc->vid_cap;
BUG_ON(!fimc);
fimc_hw_clear_irq(fimc);
@@ -372,10 +378,12 @@ static irqreturn_t fimc_isr(int irq, void *priv)
if (test_bit(ST_CAPT_RUN, &fimc->state))
fimc_capture_handler(fimc);
-
- if (test_and_clear_bit(ST_CAPT_PEND, &fimc->state)) {
+ else if (test_bit(ST_CAPT_PEND, &fimc->state))
set_bit(ST_CAPT_RUN, &fimc->state);
- wake_up(&fimc->irq_queue);
+
+ if (cap->active_buf_cnt == 1) {
+ fimc_deactivate_capture(fimc);
+ set_bit(ST_CAPT_LAST_IRQ, &fimc->state);
}
isr_unlock:
diff --git a/drivers/media/video/s5p-fimc/fimc-core.h b/drivers/media/video/s5p-fimc/fimc-core.h
index 1f1beaa..58cb2e0 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.h
+++ b/drivers/media/video/s5p-fimc/fimc-core.h
@@ -56,6 +56,7 @@ enum fimc_dev_flags {
ST_CAPT_RUN,
ST_CAPT_STREAM,
ST_CAPT_SHUT,
+ ST_CAPT_LAST_IRQ,
};
#define fimc_m2m_active(dev) test_bit(ST_OUTDMA_RUN, &(dev)->state)
--
1.6.2.5
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] [media]: s5p-fimc: fix ISR and buffer handling for fimc-capture
2010-12-29 0:30 [PATCH] [media]: s5p-fimc: fix ISR and buffer handling for fimc-capture Sungchun Kang
@ 2010-12-29 15:31 ` Sylwester Nawrocki
0 siblings, 0 replies; 2+ messages in thread
From: Sylwester Nawrocki @ 2010-12-29 15:31 UTC (permalink / raw)
To: Sungchun Kang; +Cc: linux-media, linux-samsung-soc, kgene.kim
On 12/29/2010 01:30 AM, Sungchun Kang wrote:
> These patches are related and it may be summarized as follows.
>
> 1. Some of the case are fimc H/W did not stop although there are
> no available output DMA buffer. So, it is modified check the
> routine fimc deactivation. And, the state of ST_CAPT_RUN is cleared
> in LAST-IRQ routine.
>
> 2. When request buffer count is less than 4, CIOYSAn register did
> not set if VIDIOC_QBUF is called repeatedly. So, clear bit the
> state of ST_CAPT_STREAM in ISR.
>
> 3. Because fimc interrupt generated when the frame start to write,
> it is necessary to use LAST-IRQ for processing of the last frame.
> So, added the enumeration for LAST-IRQ.
>
> 4. After LAST-IRQ is generated, H/W pointer will be skip 1 frame.
> (reference by user manual) So, S/W pointer should be increased too.
>
> Reviewed-by Jonghun Han <jonghun.han@samsung.com>
> Signed-off-by: Sungchun Kang <sungchun.kang@samsung.com>
> ---
> This patch is depended on:
> http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/vb2-mfc-fimc
>
> drivers/media/video/s5p-fimc/fimc-capture.c | 12 +++++++-----
> drivers/media/video/s5p-fimc/fimc-core.c | 20 ++++++++++++++------
> drivers/media/video/s5p-fimc/fimc-core.h | 1 +
> 3 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c
> index 4e4441f..0a3b344 100644
> --- a/drivers/media/video/s5p-fimc/fimc-capture.c
> +++ b/drivers/media/video/s5p-fimc/fimc-capture.c
> @@ -350,13 +350,15 @@ static void buffer_queue(struct vb2_buffer *vb)
>
> dbg("active_buf_cnt: %d", fimc->vid_cap.active_buf_cnt);
>
> - if (vid_cap->active_buf_cnt >= vid_cap->reqbufs_count ||
> - vid_cap->active_buf_cnt >= FIMC_MAX_OUT_BUFS) {
> - if (!test_and_set_bit(ST_CAPT_STREAM, &fimc->state)) {
> + if (vid_cap->active_buf_cnt == FIMC_MAX_OUT_BUFS)
> + set_bit(ST_CAPT_STREAM, &fimc->state);
> +
> + if (test_bit(ST_CAPT_LAST_IRQ, &fimc->state) ||
> + !test_bit(ST_CAPT_RUN, &fimc->state)) {
> fimc_activate_capture(ctx);
> - dbg("");
> - }
> + clear_bit(ST_CAPT_LAST_IRQ, &fimc->state);
> }
> +
> spin_unlock_irqrestore(&fimc->slock, flags);
> }
>
> diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c
> index 2374fd8..4a85966 100644
> --- a/drivers/media/video/s5p-fimc/fimc-core.c
> +++ b/drivers/media/video/s5p-fimc/fimc-core.c
> @@ -334,9 +334,14 @@ static void fimc_capture_handler(struct fimc_dev *fimc)
> if (++cap->buf_index >= FIMC_MAX_OUT_BUFS)
> cap->buf_index = 0;
>
> - } else if (test_and_clear_bit(ST_CAPT_STREAM, &fimc->state) &&
> - cap->active_buf_cnt <= 1) {
> - fimc_deactivate_capture(fimc);
> + } else {
> + clear_bit(ST_CAPT_STREAM, &fimc->state);
> + }
> +
> + if (cap->active_buf_cnt == 0) {
> + clear_bit(ST_CAPT_RUN, &fimc->state);
> + if (++cap->buf_index >= FIMC_MAX_OUT_BUFS)
> + cap->buf_index = 0;
> }
>
> dbg("frame: %d, active_buf_cnt= %d",
> @@ -346,6 +351,7 @@ static void fimc_capture_handler(struct fimc_dev *fimc)
> static irqreturn_t fimc_isr(int irq, void *priv)
> {
> struct fimc_dev *fimc = priv;
> + struct fimc_vid_cap *cap = &fimc->vid_cap;
>
> BUG_ON(!fimc);
> fimc_hw_clear_irq(fimc);
> @@ -372,10 +378,12 @@ static irqreturn_t fimc_isr(int irq, void *priv)
>
> if (test_bit(ST_CAPT_RUN, &fimc->state))
> fimc_capture_handler(fimc);
> -
> - if (test_and_clear_bit(ST_CAPT_PEND, &fimc->state)) {
> + else if (test_bit(ST_CAPT_PEND, &fimc->state))
> set_bit(ST_CAPT_RUN, &fimc->state);
> - wake_up(&fimc->irq_queue);
> +
> + if (cap->active_buf_cnt == 1) {
> + fimc_deactivate_capture(fimc);
> + set_bit(ST_CAPT_LAST_IRQ, &fimc->state);
> }
>
> isr_unlock:
> diff --git a/drivers/media/video/s5p-fimc/fimc-core.h b/drivers/media/video/s5p-fimc/fimc-core.h
> index 1f1beaa..58cb2e0 100644
> --- a/drivers/media/video/s5p-fimc/fimc-core.h
> +++ b/drivers/media/video/s5p-fimc/fimc-core.h
> @@ -56,6 +56,7 @@ enum fimc_dev_flags {
> ST_CAPT_RUN,
> ST_CAPT_STREAM,
> ST_CAPT_SHUT,
> + ST_CAPT_LAST_IRQ,
> };
>
> #define fimc_m2m_active(dev) test_bit(ST_OUTDMA_RUN, &(dev)->state)
Many thanks for this improvement! This issue has been sitting for some time
in my job queue. I have tested your changes on GONI (s5pv210) and s5pv310 based
board. Unfortunately there is still some issue on s5pv210. The driver seems
not to respect the buffer ownership, i.e. it overwrites some times the buffer
which is passed to user space, when there are delays between DQBUF and the
capture needs to be stopped in HW. And that leads to a tearing. There is no
such issue on s5pv310. I'm going to investigate this further.
I have applied your patch. Thank you!
Regards,
--
Sylwester Nawrocki
Samsung Poland R&D Center
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2010-12-29 15:31 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-29 0:30 [PATCH] [media]: s5p-fimc: fix ISR and buffer handling for fimc-capture Sungchun Kang
2010-12-29 15:31 ` Sylwester Nawrocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).