Re: [PATCH] zr36067: Debugging cleanups (updated again)

Hi Trent,

On May 24, 2007, at 4:37 AM, Trent Piepho wrote:

> On Thu, 24 May 2007, Ronald S. Bultje wrote:
>> Hi Trent,
>>
>> On May 23, 2007, at 11:08 PM, Trent Piepho wrote:
>>> I've attached my patch to this email.
>>
>> That doesn't look right. You should do it in this order:
>> 1) make sure we're the actively capturing process. If not, error out.
>
> How do you check this?  fh->v4l_buffers.active != ZORAN_FREE

Yes.

>> The only thing that you need to change in the original code is this
>> (quasi-patch):
>>
>> 		queue = &zr->v4l_capq;
>> 		frame = zr->v4l_pend[zr->v4l_pend_tail & V4L_MASK_FRAME];
>> +              if (fh->/zr->v4l_buffers.buffer[frame].state !=
>> BUZ_STATE_DONE)
>> -		poll_wait(file, queue, wait);
>> +			poll_wait(file, queue, wait);
>> 		if (fh->v4l_buffers.buffer[frame].state == BUZ_STATE_DONE)
>> ?                ^^^^ change to zr->
>> 			res = POLLIN | POLLRDNORM;
>
> This still has some of the same bugs.  It's entirely possible that  
> there is
> already a frame that can be returned, this function doesn't check  
> that.
> For example when buffer[v4l_sync_tail].state == DONE and pend_head ==
> pend_tail, it returns POLLNVAL, when it should return POLLIN since  
> there is
> a frame ready to call DQBUF on.

There's probably some bugs, I fully realize that. E.g., it indeed  
shouldn't use frame=zr->v4l_pend[pend_tail & MASK_FRAME], it should  
use v4l_pend[sync_tail & MASK_FRAME] instead. Also, it appears that  
with all this, we don't need to check pend_head/pend_tail at all, you  
can probably remove that (a few lines up from here). With those  
changes, I think the code is fine. E.g. in the line "if (zr- 
 >v4l_buffers.buffer[frame].state == BUZ_STATE_DONE)", we then do  
check whether the frame is ready to be returned already.

The funny thing is that I did write a poll-testcase when I  
implemented this function, it must have done something, although I  
can't remember what/how I tested...

> It also doesn't properly protect the accesses with the spinlock.  I  
> moved
> the code around a bit to minimize the code that needs to be inside the
> spinlock protected region

I omitted that to keep it simple, that should be easy enough to  
add... :-).

> I'm not sure if returning POLLNVAL or POLLHUP is right.  Would it be
> possible for one thread to call poll() on the fd and then have another
> thread call QBUF on the same fd while the first thread was sleeping  
> in the
> poll?

Theoretically, although there really isn't any point. Since we use  
locks (resource_lock) in zoran_poll, this should be fine.

Ronald

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/