Re: [PATCH 7 of 9] vivi: Simplify the vivi driver and avoid deadlocks

[Brandon Philips <brandon@ifup.org>]

On 15:34 Fri 28 Mar 2008, Mauro Carvalho Chehab wrote:
> This is under copyright (2006), as if you were one of the authors of the
> original driver. Also, I prefer if you add a short line bellow your copyright
> for the job you've done on the driver. Something like:
> 
> + *
> + *  Copyright (c) 2008 by Brandon Philips <brandon@ifup.org>
> + *       - Fix bad locks and cleans up streaming code

Ok, this is fixed in the patch I just sent.  I didn't add the
"changelog" entry below the copyright because the git-log will show what
I did.

> > -static int restart_video_queue(struct vivi_dmaqueue *dma_q)
> > -{
> ...
> > -}
> 
> While the restart and timeout code is not needed on vivi driver, IMO, we should
> keep it, since the main reason for this driver is to be a reference code. 

I couldn't figure out how it worked well enough to fix it.   In
particular code similar to the following is copied around throughout
several drivers and I have no idea what it does:  

@@ -785,45 +657,12 @@ buffer_queue(struct videobuf_queue *vq, 
-       if (!list_empty(&vidq->queued)) {
-               dprintk(dev, 1, "adding vb queue=0x%08lx\n",
-                       (unsigned long)&buf->vb.queue);
-               list_add_tail(&buf->vb.queue, &vidq->queued);
-               buf->vb.state = VIDEOBUF_QUEUED;
-               dprintk(dev, 2, "[%p/%d] buffer_queue - append to queued\n",
-                       buf, buf->vb.i);
-       } else if (list_empty(&vidq->active)) {
-               list_add_tail(&buf->vb.queue, &vidq->active);
-               buf->vb.state = VIDEOBUF_ACTIVE;
-               mod_timer(&vidq->timeout, jiffies+BUFFER_TIMEOUT);
-               dprintk(dev, 2, "[%p/%d] buffer_queue - first active\n",
-                       buf, buf->vb.i);
-
-               vivi_start_thread(vidq);
-       } else {
-               prev = list_entry(vidq->active.prev,
-                                 struct vivi_buffer, vb.queue);
-               if (prev->vb.width  == buf->vb.width  &&
-                   prev->vb.height == buf->vb.height &&
-                   prev->fmt       == buf->fmt) {
-                       list_add_tail(&buf->vb.queue, &vidq->active);
-                       buf->vb.state = VIDEOBUF_ACTIVE;
-                       dprintk(dev, 2,
-                               "[%p/%d] buffer_queue - append to active\n",
-                               buf, buf->vb.i);
-
-               } else {
-                       list_add_tail(&buf->vb.queue, &vidq->queued);
-                       buf->vb.state = VIDEOBUF_QUEUED;
-                       dprintk(dev, 2,
-                               "[%p/%d] buffer_queue - first queued\n",
-                               buf, buf->vb.i);
-               }
-       }

What is the difference between VIDEOBUF_ACTIVE and VIDEOBUF_QUEUED?

> This kind of code is important on real drivers, since the IRQ's may not be called
> for some reason. On cx88 and on saa7134, this happens on several situations[2]. 

Yes, I agree.  But, as I said I couldn't figure it out.

> Without a timeout, the driver will wait forever to receive a buffer.

Well, yes on real hardware.  But, in the case of vivi we can just create
the frames as they are needed.  It is dead simple for a fake device :D

> This task is also needed by tm6000 driver, for the same reasons.

Huh?  What task?

Thanks,

	Brandon

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list