diff mbox

soc-camera : sh_mobile_ceu_camera race on free_buffer ?

Message ID aec7e5c30902130214k6a0fc8ck74b412f41fa63385@mail.gmail.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Magnus Damm Feb. 13, 2009, 10:14 a.m. UTC 
  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

Comments

Kuninori Morimoto Feb. 16, 2009, 1:07 a.m. UTC | #1 
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
--
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 Feb. 18, 2009, 6:51 p.m. UTC | #2 
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
--
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 Feb. 18, 2009, 6:56 p.m. UTC | #3 
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
--
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
Kuninori Morimoto Feb. 19, 2009, 1:01 a.m. UTC | #4 
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
 
--
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 Feb. 19, 2009, 7:29 a.m. UTC | #5 
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
--
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
Kuninori Morimoto Feb. 19, 2009, 8:07 a.m. UTC | #6 
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
 
--
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

--- 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;