Hi,
On Saturday 02 May 2009 00:18:46 Karicheri, Muralidharan wrote:
> Laurent,
>
> Thanks once again for your support. Please see below my response with
> [MK] prefix. If some more outstanding issues are to be discussed, will it
> be possible to talk to you over phone?
Sure. Contact me by private e-mail if you want to arrange a phone meeting.
[snip]
> > > > > The driver uses ccdc_hw_device interface to configure CCDC based on
> > > > > the interface requirement of the slave decoder device.
> > > >
> > > > If I'm not mistaken, the vpfe_capture.ko module depends on either
> > > > ccdc_dm355.ko or ccdc_davinci.ko (which should really be named
> > > > ccdc_dm6446.ko, but you've already been told about that) but not both,
> > > > as those modules conflict. Is there a reason not to combine both the
> > > > vpfe and the ccdc drivers into a single module ?
> > >
> > > [MK] The reason they are developed as independent module is to help in
> > > re-use it across other OS if needed. I am not sure why you want to
> > > combine them. Could you elaborate?
> >
> > See below.
> >
> > > > You should also probably call ccdc functions directly instead of going
> > > > through the ccdc_hw_dev structure, and pass them a pointer to a ccdc
> > > > data structure instead of storing everything as global variables in
> > > > theccdc_dm355.c and ccdc_davinci.c source files.
> > >
> > > [MK] As I see it, vpfe_capture bridge driver needs to use different CCDC
> > > modules on each of the different SoCs that we support. Using function
> > > ptrs help us to assign NULL to function ptrs on some SoCs where the
> > > specific function is not required to be implemented. But we could
> > > consider functions that are called from interrupt context available as
> > > global inline function to make it more efficient. Alternately using the
> > > functions as global, how do I call a function from bridge driver if that
> > > is applicable only to one SoC and not applicable for others?
> > >
> > > I thought following is not acceptable
> > >
> > > #ifdef CONFIG_ARCH_DAVINCI_DM644x (config variable for DM6446 SoC)
> > > call specific function
> > > #endif
> >
> > You're right. I overlooked the fact that some functions were not
> > mandatory. A simple solution, that would allow the VPFE code not to test
> > for NULL function pointers, would be to implement dummy functions
> > (basically just returning a fixed value) for operations not supported by a
> > given CCDC module. I'll let you decide if this is acceptable and
> > desirable.
>
> [MK]. I am more inclined to check for null in the code rather than
> implementing dummy functions. I like how similar thing is done in
> videobuf-core.c where the code checks for mandatory functions by calling
>
> BUG_ON(!q->ops->buf_queue)
> Do you think this is a good choice?
Given that function pointers can not change after initialization, checking
them in the registration function is enough. BUG_ON() will ensure the
developer notices potential issues, I think that's a good solution.
> > I can think of two ways to support different CCDC modules in the VPFE
> > driver. The first one is what you've implemented so far. Only a single
> > CCDC module can be loaded as they all export identically named symbols.
> > This make it impossible to build VPFE support directly in a kernel (as
> > opposed to dynamically loaded modules) that needs to run on both a DM355
> > and a DM6446.
> >
> > The second way is to have the CCDC modules register their ccdc_hw_device
> > (btw, the function pointers might be split into a second structure called
> > ccdc_hw_ops to be consistent with names in the rest of the kernel) with
> > the
>
> [MK] Ok.
>
> > VPFE driver. This would allow different CCDC modules to be compiled in the
> > same kernel. Platform device data passed by board-specific code would be
> > used to select the right CCDC module at runtime.
>
> [MK] If I understand correctly, what you are suggesting is to have the ccdc
> module register with vpfe driver. So vpfe driver implements a register
> function which all available ccdc modules call to register with the vpfe.
> vpfe driver platform data indicate the board type. Based on that
> vpfe_drivers select the correct ccdc and rejects the rest. Is it what you
> mean here ?
Yes that's right. Association between the VPFE and CCDC drivers will be
dynamically handled at runtime.
> > I think the direction currently followed by most architectures is to
> > enable building of kernels supporting different boards/devices. This
> > would make the second solution preferable. I have no strong opinion on
> > this though.
>
> [MK] I feel the same way.
>
> > > > > +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.
[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).
> 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.
> > > > > + }
> > > > > + common = &(channel->common[buf_type_index]);
> > > > > + if (p->memory != V4L2_MEMORY_MMAP) {
> > > >
> > > > p->memory isn't supposed to be set by userspace applications when
> > > > calling VIDIOC_QUERYBUF. Don't check it.
> > >
> > > [MK] Ok. Just curious. What happens when user application request for
> > > USERPTR IO and then calls QUERYBUF?
> >
> > VIDIOC_QUERYBUF is part of the memory mapping I/O method. It doesn't make
> > sense with the user pointer I/O method. The V4L2 specification doesn't
> > explicitly states what should happen, I guess returning an error would be
> > the right thing to do.
>
> [MK]. I guess I can check memory variable saved in common instead for
> validation instead. Right?
Right.
> > > > > + /* 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.
[snip]
> > There are a few more parameters for Bayer mode. Some of them, such as lens
> > shading correction, can probably be converted to V4L2 parameters. We can
> > do that at a later time though.
> >
> > My point is that parameters that can be computed from V4L2 data (image
> > format and size, ...) shouldn't be passed explicitly from userspace, and
> > parameters that are definitely board-specific should come from platform
> > data set by board-specific code. Other parameters can still be set using
> > VPFE_CMD_S_SOC_PARAMS.
> >
> > I like your plan to configure the CCDC, IPIPE and other blocks using
> > different ioctls. VPFE_CMD_S_SOC_PARAMS could be renamed to
> > VPFE_CMD_S_CCDC_PARAMS.
>
> [MK] Better would be to call it VPFE_CMD_S_CCDC_RAW_PARAMS. I plan to
> rename VPFE_CMD_S/G_SOC_PARAMS to VPFE_CMD_S/G_CCDC_RAW_PARAMS and later
> remove the module specific structures for DFC, CSC etc to individual
> control ioctls (using V4L2_CID_PRIVATE_BASE).
Ok.
Best regards,
Laurent Pinchart
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
