On Thu, 25 Aug 2011 12:52:11 +0200
Marek Szyprowski [off-list ref] wrote:

This patch changes the order of operations during stream on call. Now the
buffers are first queued to the driver and then the start_streaming method
is called.

This seems good to me (I guess it should, since I'm the guy who griped
about it before :).

This resolves the most common case when the driver needs to know buffer
addresses to enable dma engine and start streaming. Additional parameters
to start_streaming and buffer_queue methods have been added to simplify
drivers code. The driver are now obliged to check if the number of queued
buffers is enough to enable hardware streaming. If not - it should return
an error. In such case all the buffers that have been pre-queued are
invalidated.

I'd suggest that drivers that worked in the old scheme, where the buffers
arrived after the start_streaming() call, should continue to work.  Why
not? 
 

Drivers that are able to start/stop streaming on-fly, can control dma
engine directly in buf_queue callback. In this case start_streaming
callback can be considered as optional. The driver can also assume that
after a few first buf_queue calls with zero 'streaming' parameter, the core
will finally call start_streaming callback.

This part I like a bit less.  In your patch, almost none of the changed
drivers use that parameter.  start_streaming() is a significant state
change, I don't think it's asking a lot of a driver to provide a callback
and actually remember whether it's supposed to be streaming or not.

Beyond that, what happens to a driver without a start_streaming() callback
if the application first queues all its buffers, then does its
VIDIOC_STREAMON call?  I see:

+	list_for_each_entry(vb, &q->queued_list, queued_entry)
+		__enqueue_in_driver(vb, 0);

(So we get streaming=0 for all queued buffers).

 	/*
 	 * Let driver notice that streaming state has been enabled.
 	 */
-	ret = call_qop(q, start_streaming, q);
+	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
 	if (ret) {
 		dprintk(1, "streamon: driver refused to start streaming\n");
+		__vb2_queue_cancel(q);
 		return ret;
 	}

The driver will have gotten all of the buffers with streaming=0, then will
never get a call again; I don't think that will lead to the desired result.
Am I missing something?

Thanks,

jon