Hello,
On Thursday, August 25, 2011 3:27 PM Jonathan Corbet wrote:
> 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?
Such change might have some side effect - the logic in buf_queue usually
assumed that the code from start_streaming has been called earlier, so some
additional checks or changes might be needed.
> > 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.
I would like to get some more comments on this. Usually this matters only to
the drivers that are able to be in streaming state without any buffers
(usually some camera capture interfaces), which might need to enable dma
engine from buf_queue callback. Other drivers only adds the buffer to the
internal list and don't care about streaming state at all.
> 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?
Hmmm. Looks that I missed something. The driver will need to ignore streaming
parameter in such case...
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
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
