On Thu, 25 Aug 2011 12:52:11 +0200
Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
