* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
@ 2009-01-19 14:02 Matthieu CASTET
2009-01-20 4:46 ` Magnus Damm
0 siblings, 1 reply; 14+ messages in thread
From: Matthieu CASTET @ 2009-01-19 14:02 UTC (permalink / raw)
To: Magnus Damm; +Cc: video4linux-list, Guennadi Liakhovetski
Hi,
I am writing a soc camera driver, and I use sh_mobile_ceu_camera as an
example.
But I don't understand how buffer are handled when the application is
doing a streamoff :
streamoff will call videobuf_streamoff and then videobuf_queue_cancel.
videobuf_queue_cancel will call free_buffer.
But we didn't do stop_capture, so as far I understand the controller is
still writing data in memory. What prevent us to free the buffer we are
writing.
Why doesn't we do a stop_capture before videobuf_streamoff ?
I saw that pxa_camera use videobuf_waiton, before freeing the buffer.
That seem more safe, but that mean we need to wait that controller
finish to write all the pending buffer.
Matthieu
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-01-19 14:02 soc-camera : sh_mobile_ceu_camera race on free_buffer ? Matthieu CASTET
@ 2009-01-20 4:46 ` Magnus Damm
2009-01-20 9:27 ` Matthieu CASTET
0 siblings, 1 reply; 14+ messages in thread
From: Magnus Damm @ 2009-01-20 4:46 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: video4linux-list, Guennadi Liakhovetski
Hi Matthieu,
On Mon, Jan 19, 2009 at 11:02 PM, Matthieu CASTET
<matthieu.castet@parrot.com> wrote:
> Hi,
>
> I am writing a soc camera driver, and I use sh_mobile_ceu_camera as an
> example.
>
> But I don't understand how buffer are handled when the application is
> doing a streamoff :
>
> streamoff will call videobuf_streamoff and then videobuf_queue_cancel.
> videobuf_queue_cancel will call free_buffer.
>
> But we didn't do stop_capture, so as far I understand the controller is
> still writing data in memory. What prevent us to free the buffer we are
> writing.
I have not looked into this in great detail, but isn't this handled by
the videobuf state? The videobuf has state VIDEOBUF_ACTIVE while it is
in use. I don't think such a buffer is freed.
> Why doesn't we do a stop_capture before videobuf_streamoff ?
>
> I saw that pxa_camera use videobuf_waiton, before freeing the buffer.
> That seem more safe, but that mean we need to wait that controller
> finish to write all the pending buffer.
Hm, but vivi.c does not use videbuf_waiton(). I guess this depends on
how the frames are queued in the driver.
Cheers,
/ magnus
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-01-20 4:46 ` Magnus Damm
@ 2009-01-20 9:27 ` Matthieu CASTET
2009-02-13 10:14 ` Magnus Damm
0 siblings, 1 reply; 14+ messages in thread
From: Matthieu CASTET @ 2009-01-20 9:27 UTC (permalink / raw)
To: Magnus Damm; +Cc: video4linux-list, Guennadi Liakhovetski
Hi Magnus,
Magnus Damm a écrit :
> Hi Matthieu,
>
> On Mon, Jan 19, 2009 at 11:02 PM, Matthieu CASTET
>> But we didn't do stop_capture, so as far I understand the controller is
>> still writing data in memory. What prevent us to free the buffer we are
>> writing.
>
> I have not looked into this in great detail, but isn't this handled by
> the videobuf state? The videobuf has state VIDEOBUF_ACTIVE while it is
> in use. I don't think such a buffer is freed.
Well from my understanding form videobuf_queue_cancel [1], we call
buf_release on all buffer.
>> I saw that pxa_camera use videobuf_waiton, before freeing the buffer.
>> That seem more safe, but that mean we need to wait that controller
>> finish to write all the pending buffer.
>
> Hm, but vivi.c does not use videbuf_waiton(). I guess this depends on
> how the frames are queued in the driver.
May be.
Matthieu
[1]
void videobuf_queue_cancel(struct videobuf_queue *q)
{
unsigned long flags = 0;
int i;
q->streaming = 0;
q->reading = 0;
wake_up_interruptible_sync(&q->wait);
/* remove queued buffers from list */
spin_lock_irqsave(q->irqlock, flags);
for (i = 0; i < VIDEO_MAX_FRAME; i++) {
if (NULL == q->bufs[i])
continue;
if (q->bufs[i]->state == VIDEOBUF_QUEUED) {
list_del(&q->bufs[i]->queue);
q->bufs[i]->state = VIDEOBUF_ERROR;
wake_up_all(&q->bufs[i]->done);
}
}
spin_unlock_irqrestore(q->irqlock, flags);
/* free all buffers + clear queue */
for (i = 0; i < VIDEO_MAX_FRAME; i++) {
if (NULL == q->bufs[i])
continue;
q->ops->buf_release(q, q->bufs[i]);
}
INIT_LIST_HEAD(&q->stream);
}
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-01-20 9:27 ` Matthieu CASTET
@ 2009-02-13 10:14 ` Magnus Damm
2009-02-16 1:07 ` morimoto.kuninori
2009-02-18 18:56 ` Guennadi Liakhovetski
0 siblings, 2 replies; 14+ messages in thread
From: Magnus Damm @ 2009-02-13 10:14 UTC (permalink / raw)
To: Matthieu CASTET; +Cc: linux-media, Guennadi Liakhovetski, Kuninori Morimoto
[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]
Hi Matthieu,
[CC Morimoto-san]
[Changed list to linux-media]
On Tue, Jan 20, 2009 at 6:27 PM, Matthieu CASTET
<matthieu.castet@parrot.com> wrote:
> Magnus Damm a écrit :
>> On Mon, Jan 19, 2009 at 11:02 PM, Matthieu CASTET
>>> But we didn't do stop_capture, so as far I understand the controller is
>>> still writing data in memory. What prevent us to free the buffer we are
>>> writing.
>>
>> I have not looked into this in great detail, but isn't this handled by
>> the videobuf state? The videobuf has state VIDEOBUF_ACTIVE while it is
>> in use. I don't think such a buffer is freed.
> Well from my understanding form videobuf_queue_cancel [1], we call
> buf_release on all buffer.
Yeah, you are correct. I guess waiting for the buffer before freeing
is the correct way to do this. I guess vivi doesn't have to do this
since it's not using DMA.
Morimoto-san, can you check the attached patch? I've tested it on my
Migo-R board together with mplayer and it seems to work well here. I
don't think using mplayer triggers this error case though, so maybe we
should try some other application.
Cheers,
/ magnus
[-- Attachment #2: linux-2.6.29-media-video-sh_mobile_ceu-videobuf_waiton-20090213.patch --]
[-- Type: text/x-patch, Size: 397 bytes --]
--- 0001/drivers/media/video/sh_mobile_ceu_camera.c
+++ work/drivers/media/video/sh_mobile_ceu_camera.c 2009-02-13 18:55:55.000000000 +0900
@@ -150,6 +150,7 @@ static void free_buffer(struct videobuf_
if (in_interrupt())
BUG();
+ videobuf_waiton(&buf->vb, 0, 0);
videobuf_dma_contig_free(vq, &buf->vb);
dev_dbg(&icd->dev, "%s freed\n", __func__);
buf->vb.state = VIDEOBUF_NEEDS_INIT;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-02-13 10:14 ` Magnus Damm
@ 2009-02-16 1:07 ` morimoto.kuninori
2009-02-18 18:51 ` Guennadi Liakhovetski
2009-02-18 18:56 ` Guennadi Liakhovetski
1 sibling, 1 reply; 14+ messages in thread
From: morimoto.kuninori @ 2009-02-16 1:07 UTC (permalink / raw)
To: Magnus Damm; +Cc: Matthieu CASTET, linux-media, Guennadi Liakhovetski
Hi Magnus
> Morimoto-san, can you check the attached patch? I've tested it on my
> Migo-R board together with mplayer and it seems to work well here. I
> don't think using mplayer triggers this error case though, so maybe we
> should try some other application.
I checked this patch with following case.
Migo-R + ov772x + mplayer
Migo-R + tw9910 + mplayer
AP325 + ov772x + mplayer
It works well on all cases.
And I can agree with your opinion
"so maybe we should try some other application."
Best regards
--
Kuninori Morimoto
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-02-16 1:07 ` morimoto.kuninori
@ 2009-02-18 18:51 ` Guennadi Liakhovetski
2009-02-19 1:01 ` morimoto.kuninori
0 siblings, 1 reply; 14+ messages in thread
From: Guennadi Liakhovetski @ 2009-02-18 18:51 UTC (permalink / raw)
To: morimoto.kuninori; +Cc: Magnus Damm, Matthieu CASTET, Linux Media Mailing List
Hi Morimoto-san
On Mon, 16 Feb 2009, morimoto.kuninori@renesas.com wrote:
>
> Hi Magnus
>
> > Morimoto-san, can you check the attached patch? I've tested it on my
> > Migo-R board together with mplayer and it seems to work well here. I
> > don't think using mplayer triggers this error case though, so maybe we
> > should try some other application.
>
> I checked this patch with following case.
>
> Migo-R + ov772x + mplayer
> Migo-R + tw9910 + mplayer
> AP325 + ov772x + mplayer
>
> It works well on all cases.
So I can add your "Tested-by:"?
> And I can agree with your opinion
> "so maybe we should try some other application."
You can try to trigger the race with the capture.c example. Reduce the
"count" variable in mainloop() and run capture.c in a loop for a while...
Try without this patch and then with this patch. But I think this is a
correct fix, so, unless you say, it crashes without it and with it, I'll
apply it.
Thanks
Guennadi
>
> Best regards
> --
> Kuninori Morimoto
> --
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-02-13 10:14 ` Magnus Damm
2009-02-16 1:07 ` morimoto.kuninori
@ 2009-02-18 18:56 ` Guennadi Liakhovetski
1 sibling, 0 replies; 14+ messages in thread
From: Guennadi Liakhovetski @ 2009-02-18 18:56 UTC (permalink / raw)
To: Magnus Damm; +Cc: Matthieu CASTET, Linux Media Mailing List, Kuninori Morimoto
On Fri, 13 Feb 2009, Magnus Damm wrote:
> Hi Matthieu,
>
> [CC Morimoto-san]
> [Changed list to linux-media]
>
> On Tue, Jan 20, 2009 at 6:27 PM, Matthieu CASTET
> <matthieu.castet@parrot.com> wrote:
> > Magnus Damm a écrit :
> >> On Mon, Jan 19, 2009 at 11:02 PM, Matthieu CASTET
> >>> But we didn't do stop_capture, so as far I understand the controller is
> >>> still writing data in memory. What prevent us to free the buffer we are
> >>> writing.
> >>
> >> I have not looked into this in great detail, but isn't this handled by
> >> the videobuf state? The videobuf has state VIDEOBUF_ACTIVE while it is
> >> in use. I don't think such a buffer is freed.
> > Well from my understanding form videobuf_queue_cancel [1], we call
> > buf_release on all buffer.
>
> Yeah, you are correct. I guess waiting for the buffer before freeing
> is the correct way to do this. I guess vivi doesn't have to do this
> since it's not using DMA.
>
> Morimoto-san, can you check the attached patch? I've tested it on my
> Migo-R board together with mplayer and it seems to work well here. I
> don't think using mplayer triggers this error case though, so maybe we
> should try some other application.
Magnus, can you, please, submit it with an Sob, after Morimoto-san has
tested it with capture.c?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-02-18 18:51 ` Guennadi Liakhovetski
@ 2009-02-19 1:01 ` morimoto.kuninori
2009-02-19 7:29 ` Guennadi Liakhovetski
0 siblings, 1 reply; 14+ messages in thread
From: morimoto.kuninori @ 2009-02-19 1:01 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Magnus Damm, Matthieu CASTET, Linux Media Mailing List
Dear Guennadi
> > It works well on all cases.
> So I can add your "Tested-by:"?
Yes please.
> You can try to trigger the race with the capture.c example. Reduce the
> "count" variable in mainloop() and run capture.c in a loop for a while...
> Try without this patch and then with this patch. But I think this is a
> correct fix, so, unless you say, it crashes without it and with it, I'll
> apply it.
I tried with following command option
# capture_example -d /dev/videoX -f -c 1000
I used -f option, I think you already know the reason =).
I think -c 1000 is enough.
# Because tw9910 can not use YUYV,
# I fixed capture_example to use VUYU for tw9910.
sh_mobile_ceu_camera didn't crashe with
and without Magnus patch.
MigoR + ov772x + capture_example
MigoR + tw9910 + capture_example (VUYU fix)
AP325 + ov772x + capture_example
Best regards
--
Kuninori Morimoto
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-02-19 1:01 ` morimoto.kuninori
@ 2009-02-19 7:29 ` Guennadi Liakhovetski
2009-02-19 8:07 ` morimoto.kuninori
0 siblings, 1 reply; 14+ messages in thread
From: Guennadi Liakhovetski @ 2009-02-19 7:29 UTC (permalink / raw)
To: morimoto.kuninori; +Cc: Magnus Damm, Matthieu CASTET, Linux Media Mailing List
On Thu, 19 Feb 2009, morimoto.kuninori@renesas.com wrote:
> Dear Guennadi
>
> > > It works well on all cases.
> > So I can add your "Tested-by:"?
>
> Yes please.
>
> > You can try to trigger the race with the capture.c example. Reduce the
> > "count" variable in mainloop() and run capture.c in a loop for a while...
> > Try without this patch and then with this patch. But I think this is a
> > correct fix, so, unless you say, it crashes without it and with it, I'll
> > apply it.
>
> I tried with following command option
>
> # capture_example -d /dev/videoX -f -c 1000
>
> I used -f option, I think you already know the reason =).
Yes, and I'm working on that.
> I think -c 1000 is enough.
No, sorry, this is not the test I meant. "-c" doesn't really stress the
path we need. You really have to execute capture_example multiple times
completely. The race we're trying to catch happens on STREAMOFF, and for
that you have to run the example completely through. So, I would suggest
something like
for (( i=0; i<100; i++ )); do capture_example -d /dev/videoX -f -c 4; done
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-02-19 7:29 ` Guennadi Liakhovetski
@ 2009-02-19 8:07 ` morimoto.kuninori
2009-02-19 8:33 ` Guennadi Liakhovetski
0 siblings, 1 reply; 14+ messages in thread
From: morimoto.kuninori @ 2009-02-19 8:07 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Magnus Damm, Matthieu CASTET, Linux Media Mailing List
Dear Guennadi
> No, sorry, this is not the test I meant. "-c" doesn't really stress the
> path we need. You really have to execute capture_example multiple times
> completely. The race we're trying to catch happens on STREAMOFF, and for
> that you have to run the example completely through. So, I would suggest
> something like
>
> for (( i=0; i<100; i++ )); do capture_example -d /dev/videoX -f -c 4; done
wow !!
sorry miss understanding.
OK. I re-tried test with your script (300 times).
sh_mobile_ceu_camera didn't crashe with
and without Magnus patch.
MigoR + ov772x + capture_example
MigoR + tw9910 + capture_example (VUYU fix)
AP325 + ov772x + capture_example
Best regards
--
Kuninori Morimoto
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-02-19 8:07 ` morimoto.kuninori
@ 2009-02-19 8:33 ` Guennadi Liakhovetski
2009-02-19 9:30 ` morimoto.kuninori
0 siblings, 1 reply; 14+ messages in thread
From: Guennadi Liakhovetski @ 2009-02-19 8:33 UTC (permalink / raw)
To: morimoto.kuninori; +Cc: Magnus Damm, Matthieu CASTET, Linux Media Mailing List
On Thu, 19 Feb 2009, morimoto.kuninori@renesas.com wrote:
>
> Dear Guennadi
>
> > No, sorry, this is not the test I meant. "-c" doesn't really stress the
> > path we need. You really have to execute capture_example multiple times
> > completely. The race we're trying to catch happens on STREAMOFF, and for
> > that you have to run the example completely through. So, I would suggest
> > something like
> >
> > for (( i=0; i<100; i++ )); do capture_example -d /dev/videoX -f -c 4; done
>
> wow !!
> sorry miss understanding.
>
> OK. I re-tried test with your script (300 times).
>
> sh_mobile_ceu_camera didn't crashe with
> and without Magnus patch.
>
> MigoR + ov772x + capture_example
> MigoR + tw9910 + capture_example (VUYU fix)
> AP325 + ov772x + capture_example
Hm, ok, maybe I can ask you about one more test, if you don't mind. The
thing is, you only see the problem, if after the ->active buffer has been
freed in free_buffer(), your DMA engine continues writing to the freed
memory, but you only notice this, if some other driver manages to grab and
use it in this time, then its data is going to be overwritten.
To actually see, if the race occurs, please do
diff -u a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c
--- a/drivers/media/video/sh_mobile_ceu_camera.c
+++ b/drivers/media/video/sh_mobile_ceu_camera.c
@@ -326,6 +326,7 @@
spin_lock_irqsave(&pcdev->lock, flags);
vb = pcdev->active;
+ WARN_ON(vb->state == VIDEOBUF_NEEDS_INIT);
list_del_init(&vb->queue);
if (!list_empty(&pcdev->capture))
Also, I think, even better than restarting capture-example completely
would be to edit capture-example.c and put a loop around
init_device();
start_capturing();
mainloop();
stop_capturing();
uninit_device();
or maybe even only
start_capturing();
mainloop();
stop_capturing();
Best would be to first try all three loops without the patch from Magnus
and see if any of them triggers. And use a larger frame (maximum that your
sensor can deliver) to give the DMA engine more time per frame.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-02-19 8:33 ` Guennadi Liakhovetski
@ 2009-02-19 9:30 ` morimoto.kuninori
2009-02-19 9:51 ` Guennadi Liakhovetski
0 siblings, 1 reply; 14+ messages in thread
From: morimoto.kuninori @ 2009-02-19 9:30 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Magnus Damm, Matthieu CASTET, Linux Media Mailing List
Dear Guennadi
> Hm, ok, maybe I can ask you about one more test, if you don't mind. The
> thing is, you only see the problem, if after the ->active buffer has been
> freed in free_buffer(), your DMA engine continues writing to the freed
> memory, but you only notice this, if some other driver manages to grab and
> use it in this time, then its data is going to be overwritten.
(snip)
> Best would be to first try all three loops without the patch from Magnus
> and see if any of them triggers. And use a larger frame (maximum that your
> sensor can deliver) to give the DMA engine more time per frame.
Hmm. maybe I could understand...
OK.
I tested with MigoR + ov772x + following way.
1) apply patch to kernel
> diff -u a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c
> --- a/drivers/media/video/sh_mobile_ceu_camera.c
> +++ b/drivers/media/video/sh_mobile_ceu_camera.c
> @@ -326,6 +326,7 @@
> spin_lock_irqsave(&pcdev->lock, flags);
>
> vb = pcdev->active;
> + WARN_ON(vb->state == VIDEOBUF_NEEDS_INIT);
> list_del_init(&vb->queue);
>
> if (!list_empty(&pcdev->capture))
2) apply patch to capture-example
I think your wanting to say is this...
diff -u capture_example.c capture_example.c.test
--- capture_example.c 2009-01-24 09:35:12.000000000 +0900
+++ capture_example.c.test 2009-02-19 18:09:32.000000000 +0900
@@ -590,6 +590,8 @@
int main(int argc, char **argv)
{
+ int i;
+
dev_name = "/dev/video0";
for (;;) {
@@ -648,11 +650,14 @@
}
open_device();
- init_device();
- start_capturing();
- mainloop();
- stop_capturing();
- uninit_device();
+ for (i=0 ; i<100 ; i++) {
+ printf("%d ", i);
+ init_device();
+ start_capturing();
+ mainloop();
+ stop_capturing();
+ uninit_device();
+ }
close_device();
fprintf(stderr, "\n");
return 0;
3) test this capture_example.
Then, It say
======================================================================
camera 0-0: SuperH Mobile CEU driver attached to camera 0
camera_host0: Format 0 not found
.0 ... camera_host0: Format 0 not found
------------[ cut here ]------------
Badness at 8c141b9e [verbose debug info unavailable]
Pid : 869, Comm: c2
CPU : 0 Not tainted (2.6.29-rc4-00197-g41480ae-dirty #801)
PC is at sh_mobile_ceu_irq+0x2a/0xc8
PR is at handle_IRQ_event+0x2e/0x68
PC : 8c141b9e SP : 8fe15e40 SR : 400001f1 TEA : 2961ba40
R0 : 8ff05afc R1 : 00000000 R2 : 8fe14000 R3 : 00000000
R4 : 8ff05a00 R5 : 8ff05a00 R6 : 00000010 R7 : 00000000
R8 : 8ff416e0 R9 : 000000f0 R10 : 00000000 R11 : 00000034
R12 : 8ff414d0 R13 : 8ff3a1e0 R14 : 8ff89e74
MACH: 00000000 MACL: 00000c30 GBR : 29686450 PR : 8c030e0a
Call trace:
[<8c030e0a>] handle_IRQ_event+0x2e/0x68
[<8c0326a8>] handle_level_irq+0x6c/0x100
[<8c00356c>] do_IRQ+0x34/0x5c
[<8c007118>] ret_from_irq+0x0/0x10
[<8c003538>] do_IRQ+0x0/0x5c
[<8c0f2c44>] memset+0x30/0x4c
[<8c00b25a>] dma_alloc_coherent+0x5e/0x148
[<8c141742>] __videobuf_mmap_mapper+0xbe/0x1a4
[<8c13fb88>] videobuf_mmap_mapper+0x3c/0x70
[<8c050ad8>] mmap_region+0x184/0x34c
[<8c005234>] old_mmap+0x60/0xa8
[<8c007266>] syscall_call+0xc/0x10
[<8c0051d4>] old_mmap+0x0/0xa8
camera 0-0: SuperH Mobile CEU driver detached from camera 0
Trace/breakpoint trap
======================================================================
4) apply Magnus patch
5) and test it again
then It say...
========================================================
-bash-3.00# ./c2 -d /dev/video0 -f -c 4
camera 0-0: SuperH Mobile CEU driver attached to camera 0
camera_host0: Format 0 not found
.0 ... camera_host0: Format 0 not found
.1 ... camera_host0: Format 0 not found
.2 ... camera_host0: Format 0 not found
.3 ... camera_host0: Format 0 not found
(snip)
.96 ... camera_host0: Format 0 not found
.97 ... camera_host0: Format 0 not found
.98 ... camera_host0: Format 0 not found
.99 ...camera 0-0: SuperH Mobile CEU driver detached from camera 0
========================================================
Best regards
--
Kuninori Morimoto
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-02-19 9:30 ` morimoto.kuninori
@ 2009-02-19 9:51 ` Guennadi Liakhovetski
2009-02-19 23:56 ` morimoto.kuninori
0 siblings, 1 reply; 14+ messages in thread
From: Guennadi Liakhovetski @ 2009-02-19 9:51 UTC (permalink / raw)
To: morimoto.kuninori; +Cc: Magnus Damm, Matthieu CASTET, Linux Media Mailing List
On Thu, 19 Feb 2009, morimoto.kuninori@renesas.com wrote:
>
> Dear Guennadi
>
> > Hm, ok, maybe I can ask you about one more test, if you don't mind. The
> > thing is, you only see the problem, if after the ->active buffer has been
> > freed in free_buffer(), your DMA engine continues writing to the freed
> > memory, but you only notice this, if some other driver manages to grab and
> > use it in this time, then its data is going to be overwritten.
> (snip)
> > Best would be to first try all three loops without the patch from Magnus
> > and see if any of them triggers. And use a larger frame (maximum that your
> > sensor can deliver) to give the DMA engine more time per frame.
>
> Hmm. maybe I could understand...
>
> OK.
> I tested with MigoR + ov772x + following way.
>
> 1) apply patch to kernel
>
> > diff -u a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c
> > --- a/drivers/media/video/sh_mobile_ceu_camera.c
> > +++ b/drivers/media/video/sh_mobile_ceu_camera.c
> > @@ -326,6 +326,7 @@
> > spin_lock_irqsave(&pcdev->lock, flags);
> >
> > vb = pcdev->active;
> > + WARN_ON(vb->state == VIDEOBUF_NEEDS_INIT);
> > list_del_init(&vb->queue);
> >
> > if (!list_empty(&pcdev->capture))
>
> 2) apply patch to capture-example
>
> I think your wanting to say is this...
>
> diff -u capture_example.c capture_example.c.test
> --- capture_example.c 2009-01-24 09:35:12.000000000 +0900
> +++ capture_example.c.test 2009-02-19 18:09:32.000000000 +0900
> @@ -590,6 +590,8 @@
>
> int main(int argc, char **argv)
> {
> + int i;
> +
> dev_name = "/dev/video0";
>
> for (;;) {
> @@ -648,11 +650,14 @@
> }
>
> open_device();
> - init_device();
> - start_capturing();
> - mainloop();
> - stop_capturing();
> - uninit_device();
> + for (i=0 ; i<100 ; i++) {
> + printf("%d ", i);
> + init_device();
> + start_capturing();
> + mainloop();
> + stop_capturing();
> + uninit_device();
> + }
> close_device();
> fprintf(stderr, "\n");
> return 0;
>
> 3) test this capture_example.
>
> Then, It say
>
> ======================================================================
> camera 0-0: SuperH Mobile CEU driver attached to camera 0
> camera_host0: Format 0 not found
> .0 ... camera_host0: Format 0 not found
> ------------[ cut here ]------------
> Badness at 8c141b9e [verbose debug info unavailable]
>
> Pid : 869, Comm: c2
> CPU : 0 Not tainted (2.6.29-rc4-00197-g41480ae-dirty #801)
>
> PC is at sh_mobile_ceu_irq+0x2a/0xc8
> PR is at handle_IRQ_event+0x2e/0x68
> PC : 8c141b9e SP : 8fe15e40 SR : 400001f1 TEA : 2961ba40
> R0 : 8ff05afc R1 : 00000000 R2 : 8fe14000 R3 : 00000000
> R4 : 8ff05a00 R5 : 8ff05a00 R6 : 00000010 R7 : 00000000
> R8 : 8ff416e0 R9 : 000000f0 R10 : 00000000 R11 : 00000034
> R12 : 8ff414d0 R13 : 8ff3a1e0 R14 : 8ff89e74
> MACH: 00000000 MACL: 00000c30 GBR : 29686450 PR : 8c030e0a
>
> Call trace:
> [<8c030e0a>] handle_IRQ_event+0x2e/0x68
> [<8c0326a8>] handle_level_irq+0x6c/0x100
> [<8c00356c>] do_IRQ+0x34/0x5c
> [<8c007118>] ret_from_irq+0x0/0x10
> [<8c003538>] do_IRQ+0x0/0x5c
> [<8c0f2c44>] memset+0x30/0x4c
> [<8c00b25a>] dma_alloc_coherent+0x5e/0x148
> [<8c141742>] __videobuf_mmap_mapper+0xbe/0x1a4
> [<8c13fb88>] videobuf_mmap_mapper+0x3c/0x70
> [<8c050ad8>] mmap_region+0x184/0x34c
> [<8c005234>] old_mmap+0x60/0xa8
> [<8c007266>] syscall_call+0xc/0x10
> [<8c0051d4>] old_mmap+0x0/0xa8
Yesss! There you go - this is the race we are hunting.
> camera 0-0: SuperH Mobile CEU driver detached from camera 0
> Trace/breakpoint trap
> ======================================================================
>
> 4) apply Magnus patch
> 5) and test it again
>
> then It say...
>
> ========================================================
> -bash-3.00# ./c2 -d /dev/video0 -f -c 4
> camera 0-0: SuperH Mobile CEU driver attached to camera 0
> camera_host0: Format 0 not found
> .0 ... camera_host0: Format 0 not found
> .1 ... camera_host0: Format 0 not found
> .2 ... camera_host0: Format 0 not found
> .3 ... camera_host0: Format 0 not found
> (snip)
> .96 ... camera_host0: Format 0 not found
> .97 ... camera_host0: Format 0 not found
> .98 ... camera_host0: Format 0 not found
> .99 ...camera 0-0: SuperH Mobile CEU driver detached from camera 0
> ========================================================
Nice and clean. Good, now I just need a formal patch submission with a
"Tested-by" string, please.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: soc-camera : sh_mobile_ceu_camera race on free_buffer ?
2009-02-19 9:51 ` Guennadi Liakhovetski
@ 2009-02-19 23:56 ` morimoto.kuninori
0 siblings, 0 replies; 14+ messages in thread
From: morimoto.kuninori @ 2009-02-19 23:56 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: Linux Media Mailing List
Dear Guennadi
> > ------------[ cut here ]------------
> > Badness at 8c141b9e [verbose debug info unavailable]
> >
> > Pid : 869, Comm: c2
> > CPU : 0 Not tainted (2.6.29-rc4-00197-g41480ae-dirty #801)
(snip)
> Yesss! There you go - this is the race we are hunting.
(snip)
> Nice and clean. Good, now I just need a formal patch submission with a
> "Tested-by" string, please.
Fuuu. good !! Thank you.
Tested-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
Best regards
--
Kuninori Morimoto
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2009-02-19 23:56 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-01-19 14:02 soc-camera : sh_mobile_ceu_camera race on free_buffer ? Matthieu CASTET
2009-01-20 4:46 ` Magnus Damm
2009-01-20 9:27 ` Matthieu CASTET
2009-02-13 10:14 ` Magnus Damm
2009-02-16 1:07 ` morimoto.kuninori
2009-02-18 18:51 ` Guennadi Liakhovetski
2009-02-19 1:01 ` morimoto.kuninori
2009-02-19 7:29 ` Guennadi Liakhovetski
2009-02-19 8:07 ` morimoto.kuninori
2009-02-19 8:33 ` Guennadi Liakhovetski
2009-02-19 9:30 ` morimoto.kuninori
2009-02-19 9:51 ` Guennadi Liakhovetski
2009-02-19 23:56 ` morimoto.kuninori
2009-02-18 18:56 ` Guennadi Liakhovetski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox