On Mon, May 30, 2016 at 04:22:56PM +0200, Martin Pieuchot wrote:
> On 26/05/16(Thu) 16:09, patrick keshishian wrote:
> >
> > Included is my initial effort to port the dual-licensed driver
> > for Fushicai Audio-Video Grabber (vendor 0x1b71 product 0x3002).
>
> Nice. Did you test both bulk and iso? Some comments inline.
>
> > Index: dev/usb/files.usb
> > ===================================================================
> > RCS file: /cvs/obsd/src/sys/dev/usb/files.usb,v
> > retrieving revision 1.126
> > diff -u -p -u -p -r1.126 files.usb
> > --- dev/usb/files.usb 8 Jan 2016 15:54:13 -0000 1.126
> > +++ dev/usb/files.usb 26 May 2016 22:34:38 -0000
> > @@ -35,6 +35,12 @@ device uvideo: video, firmload
> > attach uvideo at uhub
> > file dev/usb/uvideo.c uvideo
> >
> > +# USBTV007 devices
> > +device utvfu: video, audio
> > +attach utvfu at uhub
> > +file dev/usb/utvfu.c utvfu
> > +file dev/usb/utvfu_ops.c utvfu
>
> I'd appreciate if we can keep the driver in one file, when the license
> issue is solved, this is coherent with the rest of our tree.
Me too. Would it be ok to merge utvfu.c and utvfu_ops.c by including
both Copyrights in this file? Should it be
[Copyright 1]
[Code 1]
[Copyright 2]
[Code 2]
or
[Copyright 1]
[Copyright 2]
[Code 1]
[Code 2]
then?
> > +#ifndef _UTVFU_H_
> > +#define _UTVFU_H_
>
> Is there any hardware reference for this device or everything is reverse
> engineered? I mean the values below are specified somewhere?
>
> > +
> > +#include <sys/queue.h>
> > +#include <sys/videoio.h>
> > +
> > +/* Hardware. */
> > +#define UTVFU_VIDEO_ENDP 0x81
> > +#define UTVFU_AUDIO_ENDP 0x83
> > +#define UTVFU_BASE 0xc000
> > +#define UTVFU_REQUEST_REG 12
> > +
> > +/* Number of concurrent isochronous urbs submitted.
> > + * Higher numbers was seen to overly saturate the USB bus. */
>
> This comment does not respect style(9).
>
> > +#define UTVFU_ISOC_TRANSFERS 16
> > +#define UTVFU_ISOC_PACKETS 8
> > +
> > +#define UTVFU_CHUNK_SIZE 256
> > +#define UTVFU_CHUNK 240
> > +
> > +#define UTVFU_AUDIO_URBSIZE 20480
> > +#define UTVFU_AUDIO_HDRSIZE 4
> > +#define UTVFU_AUDIO_BUFFER 65536
> > +
> > +#define UTVFU_COMPOSITE_INPUT 0
> > +#define UTVFU_SVIDEO_INPUT 1
> ^^^^^^
> We try to avoid mixing space and tab after "#define"
>
> > +/* Chunk header. */
> > +#define UTVFU_MAGIC(hdr) (hdr & 0xff000000U)
> > +#define UTVFU_MAGIC_OK(hdr) ((hdr & 0xff000000U) == 0x88000000U)
> > +#define UTVFU_FRAME_ID(hdr) ((hdr & 0x00ff0000U) >> 16)
> > +#define UTVFU_ODD(hdr) ((hdr & 0x0000f000U) >> 15)
> > +#define UTVFU_CHUNK_NO(hdr) (hdr & 0x00000fffU)
> > +
> > +#define UTVFU_TV_STD (V4L2_STD_525_60 | V4L2_STD_PAL)
> > +
> > +/* parameters for supported TV norms */
> > +struct utvfu_norm_params {
> > + v4l2_std_id norm;
> > + int cap_width,
> > + cap_height,
> > + frame_len;
>
> Please one type per line.
>
> > +};
>
> > +extern struct utvfu_norm_params utvfu_norm_params[];
> > +
> > +#define UTVFU_MAX_BUFFERS 32
> > +struct utvfu_mmap {
> > + SIMPLEQ_ENTRY(utvfu_mmap) q_frames;
> > + uint8_t *buf;
> > + struct v4l2_buffer v4l2_buf;
> > +};
> > +typedef SIMPLEQ_HEAD(, utvfu_mmap) q_mmap;
> > +
> > +struct utvfu_frame_buf {
> > + uint off;
> > + uint size;
> > + uint16_t chunks_done;
> > + uint8_t fid;
> > + uint8_t last_odd;
> > + uint8_t *buf;
> > +};
> > +
> > +#define UTVFU_NFRAMES_MAX 40
> > +struct utvfu_isoc_xfer {
> > + struct utvfu_softc *sc;
> > + struct usbd_xfer *xfer;
> > + void *buf;
> > + uint16_t size[UTVFU_NFRAMES_MAX];
> > +};
> > +
> > +struct utvfu_bulk_xfer {
> > + struct usbd_xfer *xfer;
> > + void *buf;
>
> I'd use the simpler approach of using KERNADDR(&xfer->dmabuf, 0) instead
> of defining another structure to abstract an USB xfer. You can look at
> uhidev_get_report_async_cb() for an example.
>
> > +};
> > +
> > +struct utvfu_vs_iface {
> > + struct usbd_pipe *pipeh;
> > + int endpoint;
> > + uint32_t psize;
> > + struct utvfu_isoc_xfer ixfer[UTVFU_ISOC_TRANSFERS];
> > +};
> > +
> > +struct utvfu_as_iface {
> > + struct usbd_pipe *pipeh;
> > + int endpoint;
> > + struct utvfu_bulk_xfer bxfer;
> > +};
> > +
> > +struct utvfu_audio_chan {
> > + uint8_t *start;
> > + uint8_t *end;
> > + uint8_t *cur;
> > + int blksize;
> > + void *intr_arg;
> > + void (*intr)(void *);
> > + struct utvfu_as_iface iface;
> > +};
> > +
> > +/* Per-device structure. */
> > +struct utvfu_softc {
> > + struct device sc_dev;
> > + struct usbd_device *sc_udev;
> > + struct usbd_interface *sc_uifaceh;
> > +
> > + /* audio & video device */
> > + struct device *sc_audiodev;
> > + struct device *sc_videodev;
> > +
> > + int sc_normi;
> > + int sc_nchunks;
> > + int sc_input;
> > + int sc_max_frame_sz;
> > + int sc_nframes;
> > +
> > + struct utvfu_vs_iface sc_iface;
> > + struct utvfu_frame_buf sc_fb;
> > +
> > + int sc_as_running;
> > + struct utvfu_audio_chan sc_audio;
> > +
> > + /* mmap */
> > + struct utvfu_mmap sc_mmap[UTVFU_MAX_BUFFERS];
> > + uint8_t *sc_mmap_buffer;
> > + q_mmap sc_mmap_q;
> > + int sc_mmap_bufsz;
> > + int sc_mmap_count;
> > + int sc_mmap_flag;
> > +
> > + /* uplayer */
> > + void *sc_uplayer_arg;
> > + int *sc_uplayer_fsize;
> > + uint8_t *sc_uplayer_fbuffer;
> > + void (*sc_uplayer_intr)(void *);
> > +};
> > +
> > +int utvfu_max_frame_size(void);
> > +int utvfu_set_regs(struct utvfu_softc *, const uint16_t regs[][2],
> > int);
> > +void utvfu_image_chunk(struct utvfu_softc *, u_char *);
> > +int utvfu_configure_for_norm(struct utvfu_softc *, v4l2_std_id);
> > +int utvfu_start_capture(struct utvfu_softc *);
> > +int utvfu_mmap_queue(struct utvfu_softc *, uint8_t *, int);
> > +void utvfu_read(struct utvfu_softc *, uint8_t *, int);
> > +
> > +void utvfu_audio_decode(struct utvfu_softc *, int);
> > +int utvfu_audio_start(struct utvfu_softc *);
> > +int utvfu_audio_stop(struct utvfu_softc *);
> > +int utvfu_audio_start_chip(struct utvfu_softc *);
> > +int utvfu_audio_stop_chip(struct utvfu_softc *);
> > +
> > +#endif
> > Index: dev/usb/utvfu.c
> > ===================================================================
> > RCS file: dev/usb/utvfu.c
> > diff -N dev/usb/utvfu.c
> > --- /dev/null 1 Jan 1970 00:00:00 -0000
> > +++ dev/usb/utvfu.c 26 May 2016 22:34:38 -0000
> > @@ -0,0 +1,691 @@
> > +/* $OpenBSD$ */
> > +/*
> > + * Fushicai USBTV007 Audio-Video Grabber Driver
> > + *
> > + * Product web site:
> > + *
> > http://www.fushicai.com/products_detail/&productId=d05449ee-b690-42f9-a661-aa7353894bed.html
> > + *
> > + * Following LWN articles were very useful in construction of this driver:
> > + * Video4Linux2 API series: http://lwn.net/Articles/203924/
> > + * videobuf2 API explanation: http://lwn.net/Articles/447435/
> > + * Thanks go to Jonathan Corbet for providing this quality documentation.
> > + * He is awesome.
> > + *
> > + * Copyright (c) 2013 Lubomir Rintel
> > + * Copyright (c) 2013 Federico Simoncelli
> > + * All rights reserved.
> > + * No physical hardware was harmed running Windows during the
> > + * reverse-engineering activity
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above copyright
> > + * notice, this list of conditions, and the following disclaimer,
> > + * without modification.
> > + * 2. The name of the author may not be used to endorse or promote products
> > + * derived from this software without specific prior written permission.
> > + *
> > + * Alternatively, this software may be distributed under the terms of the
> > + * GNU General Public License ("GPL").
> > + */
> > +
> > +#include <sys/param.h>
> > +#include <sys/systm.h>
> > +#include <sys/kernel.h>
> > +#include <sys/malloc.h>
> > +#include <sys/audioio.h>
> > +#include <sys/videoio.h>
> > +
> > +#include <machine/bus.h>
> > +
> > +#include <dev/audio_if.h>
> > +#include <dev/usb/usb.h>
> > +#include <dev/usb/usbdi.h>
> > +#include <dev/usb/usbdivar.h>
> > +#include <dev/usb/usbdi_util.h>
> > +#include <dev/usb/usbdevs.h>
> > +#include <dev/video_if.h>
> > +
> > +#include "utvfu.h"
> > +
> > +#define UTVFU_DEBUG
> > +#ifdef UTVFU_DEBUG
> > +extern int utvfu_debug;
> > +#define DPRINTF(l, x...) do { if ((l) <= utvfu_debug) printf(x); } while
> > (0)
>
> You mostly have 1 level, so do you really want a `D'PRINTF()? By the
> way, we try to remove debug printf before committing code. So if you
> think some of the printfs you added won't help later, just get rid of
> them.
>
> > +#else
> > +#define DPRINTF(l, x...)
> > +#endif
> > +
> > +#define DEVNAME(_s) ((_s)->sc_dev.dv_xname)
> > +
> > +struct utvfu_norm_params utvfu_norm_params[] = {
> > + {
> > + .norm = V4L2_STD_525_60,
> > + .cap_width = 720,
> > + .cap_height = 480,
> > + /* 4 bytes/2 pixel YUYV/YUV 4:2:2 */
> > + .frame_len = (720 * 480 * 2),
> > + },
> > + {
> > + .norm = V4L2_STD_PAL,
> > + .cap_width = 720,
> > + .cap_height = 576,
> > + /* 4 bytes/2 pixel YUYV/YUV 4:2:2 */
> > + .frame_len = (720 * 576 * 2),
> > + }
> > +};
> > +
> > +int
> > +utvfu_set_regs(struct utvfu_softc *sc, const uint16_t regs[][2], int size)
> > +{
> > + int i;
> > + usbd_status error;
> > + usb_device_request_t req;
> > +
> > + DPRINTF(1, "%s: %s: size=%d enter\n", DEVNAME(sc), __func__, size);
> > +
> > + req.bmRequestType = UT_WRITE_VENDOR_DEVICE;
> > + req.bRequest = UTVFU_REQUEST_REG;
> > + USETW(req.wLength, 0);
> > +
> > + for (i = 0; i < size; i++) {
> > + USETW(req.wIndex, regs[i][0]);
> > + USETW(req.wValue, regs[i][1]);
> > +
> > + error = usbd_do_request(sc->sc_udev, &req, NULL);
> > + if (USBD_NORMAL_COMPLETION != error) {
>
> This way of checking for error value might be clever, but I IMHO being
> coherent with the rest of the code base is more important. So I would
> stick to the "if (variable == constant)" style.
Yes, please. Also not mixing up e.g. 'i++' and '++i' would be nice.
