Re: [PATCH 5/5] tm6000: rewrite copy_streams

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 27.05.2010 23:23, schrieb Mauro Carvalho Chehab:
> Em Sun, 23 May 2010 20:31:45 +0200
> stefan.ringel@xxxxxxxx escreveu:
>
>   
>> From: Stefan Ringel <stefan.ringel@xxxxxxxx>
>>
>> fusion function copy streams and copy_packets to new function copy_streams.
>>
>> Signed-off-by: Stefan Ringel <stefan.ringel@xxxxxxxx>
>> ---
>>  drivers/staging/tm6000/tm6000-usb-isoc.h |    5 +-
>>  drivers/staging/tm6000/tm6000-video.c    |  337 +++++++++++-------------------
>>  2 files changed, 127 insertions(+), 215 deletions(-)
>>
>> diff --git a/drivers/staging/tm6000/tm6000-usb-isoc.h b/drivers/staging/tm6000/tm6000-usb-isoc.h
>> index 5a5049a..138716a 100644
>> --- a/drivers/staging/tm6000/tm6000-usb-isoc.h
>> +++ b/drivers/staging/tm6000/tm6000-usb-isoc.h
>> @@ -39,7 +39,7 @@ struct usb_isoc_ctl {
>>  	int				pos, size, pktsize;
>>  
>>  		/* Last field: ODD or EVEN? */
>> -	int				field;
>> +	int				vfield;
>>  
>>  		/* Stores incomplete commands */
>>  	u32				tmp_buf;
>> @@ -47,7 +47,4 @@ struct usb_isoc_ctl {
>>  
>>  		/* Stores already requested buffers */
>>  	struct tm6000_buffer    	*buf;
>> -
>> -		/* Stores the number of received fields */
>> -	int				nfields;
>>  };
>> diff --git a/drivers/staging/tm6000/tm6000-video.c b/drivers/staging/tm6000/tm6000-video.c
>> index 2a61cc3..31c574f 100644
>> --- a/drivers/staging/tm6000/tm6000-video.c
>> +++ b/drivers/staging/tm6000/tm6000-video.c
>> @@ -186,234 +186,153 @@ const char *tm6000_msg_type[] = {
>>  /*
>>   * Identify the tm5600/6000 buffer header type and properly handles
>>   */
>> -static int copy_packet(struct urb *urb, u32 header, u8 **ptr, u8 *endp,
>> -			u8 *out_p, struct tm6000_buffer **buf)
>> -{
>> -	struct tm6000_dmaqueue  *dma_q = urb->context;
>> -	struct tm6000_core *dev = container_of(dma_q, struct tm6000_core, vidq);
>> -	u8 c;
>> -	unsigned int cmd, cpysize, pktsize, size, field, block, line, pos = 0;
>> -	int rc = 0;
>> -	/* FIXME: move to tm6000-isoc */
>> -	static int last_line = -2, start_line = -2, last_field = -2;
>> -
>> -	/* FIXME: this is the hardcoded window size
>> -	 */
>> -	unsigned int linewidth = (*buf)->vb.width << 1;
>> -
>> -	if (!dev->isoc_ctl.cmd) {
>> -		c = (header >> 24) & 0xff;
>> -
>> -		/* split the header fields */
>> -		size  = ((header & 0x7e) << 1);
>> -
>> -		if (size > 0)
>> -			size -= 4;
>> -
>> -		block = (header >> 7) & 0xf;
>> -		field = (header >> 11) & 0x1;
>> -		line  = (header >> 12) & 0x1ff;
>> -		cmd   = (header >> 21) & 0x7;
>> -
>> -		/* Validates header fields */
>> -		if(size > TM6000_URB_MSG_LEN)
>> -			size = TM6000_URB_MSG_LEN;
>> -
>> -		if (cmd == TM6000_URB_MSG_VIDEO) {
>> -			if ((block+1)*TM6000_URB_MSG_LEN>linewidth)
>> -				cmd = TM6000_URB_MSG_ERR;
>> -
>> -			/* FIXME: Mounts the image as field0+field1
>> -			 * It should, instead, check if the user selected
>> -			 * entrelaced or non-entrelaced mode
>> -			 */
>> -			pos= ((line<<1)+field)*linewidth +
>> -				block*TM6000_URB_MSG_LEN;
>> -
>> -			/* Don't allow to write out of the buffer */
>> -			if (pos+TM6000_URB_MSG_LEN > (*buf)->vb.size) {
>> -				dprintk(dev, V4L2_DEBUG_ISOC,
>> -					"ERR: size=%d, num=%d, line=%d, "
>> -					"field=%d\n",
>> -					size, block, line, field);
>> -
>> -				cmd = TM6000_URB_MSG_ERR;
>> -			}
>> -		} else {
>> -			pos=0;
>> -		}
>> -
>> -		/* Prints debug info */
>> -		dprintk(dev, V4L2_DEBUG_ISOC, "size=%d, num=%d, "
>> -				" line=%d, field=%d\n",
>> -				size, block, line, field);
>> -
>> -		if ((last_line!=line)&&(last_line+1!=line) &&
>> -		    (cmd != TM6000_URB_MSG_ERR) )  {
>> -			if (cmd != TM6000_URB_MSG_VIDEO)  {
>> -				dprintk(dev, V4L2_DEBUG_ISOC,  "cmd=%d, "
>> -					"size=%d, num=%d, line=%d, field=%d\n",
>> -					cmd, size, block, line, field);
>> -			}
>> -			if (start_line<0)
>> -				start_line=last_line;
>> -			/* Prints debug info */
>> -			dprintk(dev, V4L2_DEBUG_ISOC, "lines= %d-%d, "
>> -					"field=%d\n",
>> -					start_line, last_line, field);
>> -
>> -			if ((start_line<6 && last_line>200) &&
>> -				(last_field != field) ) {
>> -
>> -				dev->isoc_ctl.nfields++;
>> -				if (dev->isoc_ctl.nfields>=2) {
>> -					dev->isoc_ctl.nfields=0;
>> -
>> -					/* Announces that a new buffer were filled */
>> -					buffer_filled (dev, dma_q, *buf);
>> -					dprintk(dev, V4L2_DEBUG_ISOC,
>> -							"new buffer filled\n");
>> -					get_next_buf (dma_q, buf);
>> -					if (!*buf)
>> -						return rc;
>> -					out_p = videobuf_to_vmalloc(&((*buf)->vb));
>> -					if (!out_p)
>> -						return rc;
>> -
>> -					pos = dev->isoc_ctl.pos = 0;
>> -				}
>> -			}
>> -
>> -			start_line=line;
>> -			last_field=field;
>> -		}
>> -		if (cmd == TM6000_URB_MSG_VIDEO)
>> -			last_line = line;
>> -
>> -		pktsize = TM6000_URB_MSG_LEN;
>> -	} else {
>> -		/* Continue the last copy */
>> -		cmd = dev->isoc_ctl.cmd;
>> -		size= dev->isoc_ctl.size;
>> -		pos = dev->isoc_ctl.pos;
>> -		pktsize = dev->isoc_ctl.pktsize;
>> -	}
>> -
>> -	cpysize = (endp-(*ptr) > size) ? size : endp - *ptr;
>> -
>> -	if (cpysize) {
>> -		/* handles each different URB message */
>> -		switch(cmd) {
>> -		case TM6000_URB_MSG_VIDEO:
>> -			/* Fills video buffer */
>> -			memcpy(&out_p[pos], *ptr, cpysize);
>> -			break;
>> -		case TM6000_URB_MSG_PTS:
>> -			break;
>> -		case TM6000_URB_MSG_AUDIO:
>> -			/* Need some code to process audio */
>> -			printk ("%ld: cmd=%s, size=%d\n", jiffies,
>> -				tm6000_msg_type[cmd],size);
>> -			break;
>> -		case TM6000_URB_MSG_VBI:
>> -			break;
>> -		default:
>> -			dprintk (dev, V4L2_DEBUG_ISOC, "cmd=%s, size=%d\n",
>> -						tm6000_msg_type[cmd],size);
>> -		}
>> -	}
>> -	if (cpysize<size) {
>> -		/* End of URB packet, but cmd processing is not
>> -		 * complete. Preserve the state for a next packet
>> -		 */
>> -		dev->isoc_ctl.pos = pos+cpysize;
>> -		dev->isoc_ctl.size= size-cpysize;
>> -		dev->isoc_ctl.cmd = cmd;
>> -		dev->isoc_ctl.pktsize = pktsize-cpysize;
>> -		(*ptr)+=cpysize;
>> -	} else {
>> -		dev->isoc_ctl.cmd = 0;
>> -		(*ptr)+=pktsize;
>> -	}
>> -
>> -	return rc;
>> -}
>> -
>>  static int copy_streams(u8 *data, unsigned long len,
>>  			struct urb *urb)
>>  {
>>  	struct tm6000_dmaqueue  *dma_q = urb->context;
>>  	struct tm6000_core *dev= container_of(dma_q,struct tm6000_core,vidq);
>> -	u8 *ptr=data, *endp=data+len;
>> +	u8 *ptr=data, *endp=data+len, c;
>>  	unsigned long header=0;
>>  	int rc=0;
>> -	struct tm6000_buffer *buf;
>> -	char *outp = NULL;
>> -
>> -	get_next_buf(dma_q, &buf);
>> -	if (buf)
>> -		outp = videobuf_to_vmalloc(&buf->vb);
>> +	unsigned int cmd, cpysize, pktsize, size, field, block, line, pos = 0;
>> +	struct tm6000_buffer *vbuf;
>> +	char *voutp = NULL;
>> +	unsigned int linewidth;
>>  
>> -	if (!outp)
>> +	/* get video buffer */
>> +	get_next_buf (dma_q, &vbuf);
>> +	if (!vbuf)
>> +		return rc;
>> +	voutp = videobuf_to_vmalloc(&vbuf->vb);
>> +	if (!voutp)
>>  		return 0;
>>  
>> -	for (ptr=data; ptr<endp;) {
>> +	for (ptr = data; ptr < endp;) {
>>  		if (!dev->isoc_ctl.cmd) {
>> -			u8 *p=(u8 *)&dev->isoc_ctl.tmp_buf;
>> -			/* FIXME: This seems very complex
>> -			 * It just recovers up to 3 bytes of the header that
>> -			 * might be at the previous packet
>> -			 */
>> -			if (dev->isoc_ctl.tmp_buf_len) {
>> -				while (dev->isoc_ctl.tmp_buf_len) {
>> -					if ( *(ptr+3-dev->isoc_ctl.tmp_buf_len) == 0x47) {
>> -						break;
>> -					}
>> -					p++;
>> -					dev->isoc_ctl.tmp_buf_len--;
>> -				}
>> -				if (dev->isoc_ctl.tmp_buf_len) {
>> -					memcpy(&header, p,
>> -						dev->isoc_ctl.tmp_buf_len);
>> -					memcpy((u8 *)&header +
>> +			/* Header */
>> +			if (dev->isoc_ctl.tmp_buf_len > 0) {
>> +				/* from last urb or packet */
>> +				header = dev->isoc_ctl.tmp_buf;
>> +				if (4 - dev->isoc_ctl.tmp_buf_len > 0)
>> +					memcpy ((u8 *)&header +
>>  						dev->isoc_ctl.tmp_buf_len,
>>  						ptr,
>>  						4 - dev->isoc_ctl.tmp_buf_len);
>>  					ptr += 4 - dev->isoc_ctl.tmp_buf_len;
>> -					goto HEADER;
>>  				}
>> +				dev->isoc_ctl.tmp_buf_len = 0;
>> +			} else {
>> +				if (ptr + 3 >= endp) {
>> +					/* have incomplete header */
>> +					dev->isoc_ctl.tmp_buf_len = endp - ptr;
>> +					memcpy (&dev->isoc_ctl.tmp_buf, ptr,
>> +						dev->isoc_ctl.tmp_buf_len);
>> +					return rc;
>> +				}
>> +				/* Seek for sync */
>> +				for (; ptr < endp - 3; ptr++) {
>> +					if (ptr < endp - 187) {
>> +						if (*(ptr + 3) == 0x47 &&
>> +							(*(ptr + 187) == 0x47)
>> +							break;
>>     
> Hmm... are you sure you need to do this? Just checking for the current sync seems
> to be enough, except if the URB is corrupted. In the latter case, it would be better
> to do the opposite: test for the sync at either ptr or ptr + 187:
>
> 		if (*(ptr + 3) == 0x47 || (*(ptr + 187) == 0x47)
>   
Yes 0x47 can be both
1. data
2. sync byte
or will you that it sync into data? It can ignored copy data and lose
data (2 blocks)

for example:

                                    block 1                    block 2
                                 it are cmd 1            it are cmd 1
 norm.          :              header data             header data
                                |          |                       |   
    |
 0x47 is data :                            header  data
                                                    |        |
                                                  
> I don't like the above neither, but this device requires so many workarounds to work
> with a bad hardware/firmware that one more hack wouldn't hurt, but, if this is really
> needed, a proper comment explaining the reason for the hack should be added.
>
>   
>> +					} else {
>> +						if (*(ptr + 3) == 0x47)
>> +							break;
>> +					}
>> +					if (ptr + 3 >= endp)
>> +						return rc;
>>     
> Huh? This test look strange: if ptr + 3 >= endp, the loop would be end before
> calling this code. So, it seems to be a dead code.
>
>   
for example:
it goes with 6 into for ... and doesn't found a sync byte so it must
return from this function and not go out from for ... -> it's not sync
and going to lose data blocks.
>> +				}
>> +				/* Get message header */
>> +				header = *(unsigned long *)ptr;
>> +				ptr += 4;
>>  			}
>>     
>   


-- 
Stefan Ringel <stefan.ringel@xxxxxxxx>

begin:vcard
fn:Stefan Ringel
n:Ringel;Stefan
email;internet:stefan.ringel@xxxxxxxx
note:web: www.stefanringel.de
x-mozilla-html:FALSE
version:2.1
end:vcard


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux