Re: [RFC] [Patch] implement video driver for sur40

[Hans Verkuil <hverkuil@xs4all.nl>]

On 01/06/2015 11:17 AM, Florian Echtler wrote:
> On 06.01.2015 10:36, Hans Verkuil wrote:
>> On 01/06/2015 10:29 AM, Florian Echtler wrote:
>>> There's only one failing test left, which is this one:
>>>
>>> Streaming ioctls:
>>>  	test read/write: OK
>>>  		fail: v4l2-test-buffers.cpp(284): g_field() == V4L2_FIELD_ANY
>>
>> You're not filling in the 'field' field of struct v4l2_buffer when returning a
>> frame. It should most likely be FIELD_NONE in your case.
>>>  		fail: v4l2-test-buffers.cpp(611): buf.check(q, last_seq)
>>>  		fail: v4l2-test-buffers.cpp(884): captureBufs(node, q, m2m_q, frame_count, false)
> OK, easy to fix. This will also influence the other two warnings, I assume?

Most likely, yes.

> 
>>> On a different note, I'm getting occasional warnings in syslog when I run 
>>> a regular video streaming application (e.g. cheese):
>>>
>>> ------------[ cut here ]------------
> ...
>>> ---[ end trace 451ed974170f6e44 ]---
>>>
>>> Does this mean the driver consumes too much CPU resources?
>>
>> No, it means that your driver is not returning all buffers to vb2. Most
>> likely this is missing in the vb2 stop_streaming op. When that is called
>> your driver must return all buffers it has back to vb2 by calling
>> vb2_buffer_done with state ERROR. The same can happen in the start_streaming
>> op if that returns an error for some reason. In that case all buffers owned
>> by the driver should be returned to vb2 with state QUEUED. See also
>> Documentation/video4linux/v4l2-pci-skeleton.c as reference code.
> I did actually build my driver code based on v4l2-pci-skeleton.c, and
> I'm calling the exact same return_all_buffers function (see below) with
> VB2_BUF_STATE_ERROR from my stop_streaming ioctl.
> 
> static void return_all_buffers(struct sur40_state *sur40,
> 			       enum vb2_buffer_state state)
> {
> 	struct sur40_buffer *buf, *node;
> 
> 	spin_lock(&sur40->qlock);
> 	list_for_each_entry_safe(buf, node, &sur40->buf_list, list) {
> 		vb2_buffer_done(&buf->vb, state);
> 		list_del(&buf->list);
> 	}
> 	spin_unlock(&sur40->qlock);
> }
> 
> Is there another possible explanation?

No :-)

You are still missing a buffer somewhere. I'd have to see your latest source code
to see what's wrong.

Some drivers (esp. USB drivers) use a separate pointer to the active buffer, so that
buffer is no longer part of the buf_list, but still needs to be returned in stop_streaming.
Could that be the cause perhaps?

Regards,

	Hans