Hi,
On Thursday 07 May 2009 17:06:02 Karicheri, Muralidharan wrote:
> Laurent,
>
> I need to re-visit following comments as I found something different
> after going through the videobuf-core.c
>
> > > > > +static void vpfe_videobuf_queue(struct videobuf_queue *vq,
> > > > > + struct videobuf_buffer *vb)
> > > > > +{
> > > > > + /* Get the file handle object and channel object */
> > > > > + struct vpfe_fh *fh = vq->priv_data;
> > > > > + struct channel_obj *channel = fh->channel;
> > > > > + struct common_obj *common = NULL;
> > > > > +
> > > > > + v4l2_dbg(1, debug, vpfe_dev->driver, "<vpfe_buffer_queue>\n");
> > > > > + common = &(channel->common[VPFE_VIDEO_INDEX]);
> > > > > +
> > > > > + /* add the buffer to the DMA queue */
> > > > > + list_add_tail(&vb->queue, &common->dma_queue);
> > > >
> > > > This needs to be protected by a mutex
> > >
> > > [MK] I think in the videobuf-core.c this is already protected
> > > by mutex lock using vb_lock of videobuf_queue structure.
> >
> > Actually it needs to be protected by a spinlock. common->dma_queue is
> > accessed in IRQ handlers.
>
> [MK] in videobuf-core.c, all calls to buffer_queue() is called inside
> spin_lock_irqsave()/spin_lock_restore() which means irq is disabled.
> So this is not required to be locked in the bridge driver code.
If I'm not mistaken, spin_lock_irqsave() will only disable interrupts on the
current processor. As the Davinci processors do not support SMP this shouldn't
be a real issue, but for the sake of correctness you should still protect
access to the list with a spinlock in the IRQ handlers. Beside, some real-time
patches to the Linux kernel run interrupts in kernel threads. Failure to use
proper locking will likely cause problems there.
> > > > > +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] also here when freeing the buffer, driver could use the same irqlock
> passed to the core from bridge driver to lock/unlock since when calling
> this function, core is not using the same. Do you agree ?
The general rule is that you need to protect concurrent access to data using
mutexes and/or spinlocks where applicable. Some data structures don't require
locking, but linked lists do. When manipulating linked lists you either need a
mutex if all access to the list is done in a context where you can sleep, or a
spinlock otherwise.
What do you mean exactly by "irqlock passed to the core from bridge driver" ?
Best regards,
Laurent Pinchart
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
