Laurent, See my response below. I think we are closing in on most of the comments.
Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 Phone : 301-515-3736 email: [email protected] >> >> > > > > +static int debug; >> > > > > +static char *ch0_decoder = "TVP514X"; >> > > > > +static u32 ch0_numbuffers = 3; >> > > > > +static u32 ch0_bufsize = (720 * 576 * 2); >> > > > >> > > > The buffer size and the number of buffers should be controlled by >the >> > > > userspace application. You can restrict the value to a driver- >specific >> > > > range, but don't use module parameters to select them. >> > > >> > > I think this is used across all our drivers (including the dm6467 >> > > driver that is being reviewed in the list now) >> > > >> > > The reason for these variables as module parameter is as follows:- >> > > >> > > Our drivers supports capture resolutions that varies from D1 to >> > > HD (1920x1080). On image sensor capture, we may support capture of >> > > even higher resolution. We support both MMAP and PTR based IO. >> > > If MMAP is used, we would like to pre-allocate buffers for >application >> > > since allocating it on the fly result in fragmentation. So setting >> > > number of buffers as a module parameters make sense. Also if a >> > > particular implementation doesn't want to use these pre-allocated >> > > buffers (use ptr IO instead), the parameter can be set to zero to >> > > force the driver not allocate any buffers and thereby reduce system >> > > memory usage. I have currently removed PTR based IO in this patch >> > > since our implementation was a hack based on an older version of the >> > > kernel. If more buffers than pre-allocated are requested, then >> > > driver tries to allocate more and free it when device >> > > is closed (This is how we do it in our internal driver which will >> > > get ported to open source later. Proriority now is to get the driver >> > > in the open source with minimum set of features and add more features >> > > later). Similar reasoning applies to buffer size. If user application >> > > is using max D1 resolution capture, there is no need to pre-allocate >> > > big size buffers and can be controlled at boot/insmode time. >> > >> > So the buffer count and size parameters are only used to preallocate >the >> > buffers ? In that case maybe you could use a single parameter to set >the >> > preallocated memory size. >> >> [MK] Are you saying allocate a single big buffer? How do we then handle >> individual buffers as required by v4l2 driver?. Also there is more chance >> to fail the allocation if we are requesting a single large buffer. In our >> implementation, we allocate 3 or more separate buffers. I don't quite get >> what you are saying here. > >The single buffer could be split into smaller buffers when needed. You're >right, this will put more pressure on the memory subsystem. On the other >hand, >allocating 'numbuffers' buffers of 'bufsize' bytes will make it impossible >to >use the preallocated memory if the user requests 'numbuffers / 2' buffers >of >'bufsize * 2' bytes. There's a trade-off here, I don't have a strong >opinion >on this, feel free to use your favorite solution. > [MK]. Anyway I don't have pre-allocation in the driver as of now. So this will be re-visited later when pre-allocation is done. Right now, I will not change it. May be I will add a comment in the code to indicate so. >[snip] > >> > > > > +static void vpfe_videobuf_release(struct videobuf_queue *vq, >> > > > > + struct videobuf_buffer *vb) >> > > > > +{ >> > > > > + v4l2_dbg(1, debug, vpfe_dev->driver, >> > > > > "vpfe_videobuf_release\n"); + videobuf_dma_contig_free(vq, >vb); >> > > > > + vb->state = VIDEOBUF_NEEDS_INIT; >> > > > >> > > > I'm not sure about this, but aren't you supposed to remove the >buffer >> > > > from the dma_queue here ? >> > > >> > > [MK] I believe when REQUEST_BUF is called the second time, >> > > INIT_LIST_HEAD will re-initialize the queue. >> > >> > The buf_release callback is not necessarily followed by a >VIDIOC_REQBUFS. >> > For instance, if the user calls VIDIOC_STREAMOFF, vpfe_videobuf_release >> > will be called. It would be perfectly valid to call VIDIOC_STREAMON >after >> > that with any VIDIOC_REQBUFS in between. >> >> [MK]. What you are saying is the following scenario will create problem >> since I don't remove the buffer from the dma queue resulting in driver >> using an invalid buffer (already released by videobuf_dma_contig_free) if >> STREAMON is called again >> >> VIDIOC_STREAMON >> VIDIOC_STREAMOFF >> VIDIOC_STREAMON. > >Yes that's right. > >> So I guess I can call INIT_LIST_HEAD() to remove the buffer so that >> STREAMON will fail gracefully if application calls STREAMON immediately >> after that, it will fail. > >You should instead remove the buffer from the DMA queue when the >buf_release >callback is invoked. In addition to releasing the buffer itself, >buf_release() >must release all resources associated with the buffer. This means it must >remove the buffer from any list it sits on (using appropriate locking of >course). > [MK] Ok. >> Can I assume, after STREAMOFF, to successfully start streaming again, >> application needs to call a) >> REQBUF ->(optional QUERYBUF -> optional mmap) -> QBUF -> STREAMON ? >> or >> QBUF -> STREAMON >> ? > >I don't think you can. STREAMOFF -> STREAMON seems valid. > [MK]. The way we have implemented the driver is on the assumption that application always call QBUF to enqueue buffer before calling STREAMON. That is why I had the question if STREAMOFF->STREAMON is a valid scenario. I have referred the capture example in v4l2 spec (rev 0.24) and I see start_capturing() always call QBUF and then call STREAMON. So at this time, I would like to keep this limitation and re-visit it later. I will add a comment in the code. > >> > > > > + /* If buffer queue is empty, return error */ >> > > > > + if (list_empty(&common->dma_queue)) { >> > > > > + v4l2_err(vpfe_dev->driver, "buffer queue is >empty\n"); >> > > > > + ret = -EIO; >> > > > > + goto lock_out; >> > > > > + } >> > > > > + /* Get the next frame from the buffer queue */ >> > > > > + common->nextFrm = common->curFrm = >> > > > > + list_entry(common->dma_queue.next, >> > > > > + struct videobuf_buffer, queue); >> > > > > + /* Remove buffer from the buffer queue */ >> > > > > + list_del(&common->curFrm->queue); >> > > > > + /* Mark state of the current frame to active */ >> > > > > + common->curFrm->state = VIDEOBUF_ACTIVE; >> > > > > + /* Initialize field_id and started member */ >> > > > > + channel->field_id = 0; >> > > > > + common->started = 1; >> > > > > + >> > > > > + addr = videobuf_to_dma_contig(common->curFrm); >> > > > > + >> > > > > + /* Calculate field offset */ >> > > > > + vpfe_calculate_offsets(channel); >> > > > > + >> > > > > + if (vpfe_attach_irq(channel) < 0) { >> > > > >> > > > Is there a reason not to request IRQs when the device is probed by >the >> > > > driver and leave them requested until the driver is unloaded or the >> > > > device removed ? >> > > >> > > [MK] As I have mentioned in a earlier response, when we port the >latest >> > > version of the driver, it will attach isr on the fly either for >> > > handling irq from ccdc or from resizer based on IPIPE & Resizers >> > > are in the path or not. So we will have to re-do this if we change >> > > this now. I prefer to leave it like this. Let me know if you feel >> > > different. >> > >> > Can't you register both IRQ handlers ? Only handlers for enabled IRQ >will >> > get called anyway. >> >> [MK]. In our SoCs, there are limited number of IRQ lines (7 or 8 >depending >> on the SoC) from the video processing peripheral to ARM. These vary from >> SoC to SoC, ranging from 15 to 31 IRQs available to be muxed over these >> lines. So doing it statically at probe is not possible. In our internal >> linux kernel we have a vpss module in the arch/arm/mach-davinci folder >that >> mux these vpss irq to arm irq. So we need to do this anyway in our system. >> Right now it might be possible to do it in probe. How do you want to >handle >> this? > >Good point. As the number of IRQ lines to the processor is smaller than the >number of events that can be generated, you can't pre-allocate an event -> >interrupt mapping for all of them. > >How do you currently handle those mappings, or how do you plan to handle >them >? If some interrupts are always required, I think it would make sense to >register the handlers in the probe function. Optional interrupts need to be >registered only when used. > [MK]. You are right. Only optional interrupts are requested dynamically. Others are requested during probe(). > >Best regards, > >Laurent Pinchart > > _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
