Laurent, Thanks.
I am done all my rework except the one discussed here. I have also ported the driver to sub device framework. So I feel I could submit the patch directly to v4l2 mailing list without another round of iteration on this list. For the response to this email, please see below. Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 Phone : 301-515-3736 email: [email protected] >-----Original Message----- >From: Laurent Pinchart [mailto:[email protected]] >Sent: Tuesday, May 12, 2009 6:56 PM >To: Karicheri, Muralidharan >Cc: [email protected] >Subject: Re: [PATCH 1/7] VPFE capture driver for DM355 and DM6446 > >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. > [MK] Ok. I will add spinlock() to protect the list. >> > > > > +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" ? > [MK] I meant 4th argument to videobuf_queue_dma_contig_init(). Anyways, as per the above comment, I will use the spinlock used above for this as well. >Best regards, > >Laurent Pinchart > _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
