Il 07/02/2012 16:18, Devin Heitmueller ha scritto:
> On Tue, Feb 7, 2012 at 5:44 AM, Gianluca Gennari <gennarone@gmail.com> wrote:
>> 1) dvb-usb drivers allocate the URBs when the device is connected, and
>> they are never freed/reallocated until the device is disconnected; on
>> the other hand, the em28xx driver allocates the URBs only when the data
>> starts streaming and frees them when the stream stops, so the URBs are
>> freed/reallocated every time the user zaps to a new channel;
>
> Correct, the current strategy is to optimized to minimize memory use
> when the device is not active, as opposed to tying up the memory when
> it's not in use (obviously at the cost of the memory possibly not
> being available on certain ARM/MIPS platforms).
Ok, that's clear.
>> 2) dvb-usb drivers typically allocate 10 URBs each with a 4k buffer (but
>> the exact size of the buffer is board dependent); instead, em28xx
>> allocates 5 URBs with buffers of size 64xMaxPacketSize (which is 940
>> byte for the PCTV 290e).
>>
>> This means a typical dvb-usb driver uses about 40k of coherent memory,
>> while the PCTV 290e takes about 300k! And this 300k of coherent memory
>> are freed/reallocated each time the user selects a new channel.
>>
>> I played a bit with the size of the buffers; I found out that both the
>> PCTV 290e and the Terratec Hybrid XS work perfectly fine with 4k
>> buffers, just like the usb-dvb drivers. So the PCTV 290e only needs 20k
>> of coherent memory instead of the 300k currently allocated (this is
>> equivalent to set EM28XX_DVB_MAX_PACKETS to just 4 instead of 64).
>
> This one is a bit harder to speculate on. Are you actually capturing
> all the packets and ensuring there are no packets being dropped (e.g.
> looking for discontinuities)? Have you tried it with modulation types
> that are high bandwidth? Have you tried capturing an entire stream
> with PID filtering disabled? The change you described may "appear" to
> be working when in fact it's only working for a subset of real-world
> use cases.
On the MIPS STB, PID filtering is always disabled. The highest bandwidth
modulation I have available in my area is a QAM256 DVB-T2 multiplex, and
it appears to be working fine.
Lowering the buffer size below 4k, I can see some image corruption
(green stripes of macro-blocks). So 4k must be on the edge.
I have not tried to capture the entire stream yet. So far, I'm basically
just using the devices.
> Also, the buffer management may be appropriate for the em2874/em2884,
> but be broken for other devices such as the em288[0123]. Testing
> would be required to ensure it's not introducing regressions. In
> particular, the 2874/2884 had architecture changes which may result in
> differences in behavior (the register map is significantly different,
> for example).
Of course, I'm not proposing to force all existing drivers to use 4k
buffers instead of 60k. What I want to show is that there is room for
optimization in this area. In fact I like the idea of the kernel module
parameter to set the buffer size: the default behavior is unchanged, but
users can test lower values.
>> Also, I prepared a proof-of-concept patch to mimic the usb-dvb URB
>> management; this means the URBs are allocated when the USB device is
>> probed, and are freed when the device is disconnected (the patch code
>> checks for changes in the requested buffer size, but this can never
>> happen in digital mode).
>
> I don't have any specific problem with such a change assuming it is
> properly vetted against other devices.
Ok.
>> I've tested the patch (with 4k buffers) with both my em28xx sticks and
>> both work perfectly fine on my PC as well as on my MIPS set-top-box.
>> Analog mode is not tested.
>
> Again, the big question here is surrounding your testing methodology.
> If you could expand on how you're testing, that would be helpful in
> assessing how well the patch will really work.
Please note that the buffer size and the buffer allocation strategy (at
runtime or at driver initialization) are two completely different
topics. The first can cause regressions, the second should not produce
any functional change (bugs excluded).
Regards,
Gianluca
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
b/drivers/media/video/em28xx/em28xx-cards.c
@@ -3321,6 +3321,14 @@ static int em28xx_usb_probe(struct usb_interface
*interface,
*/
em28xx_init_extension(dev);
+ if (has_dvb && dev->dvb_max_pkt_size > 0) {
+ /* pre-alloc isoc transfer buffers */
+ em28xx_init_isoc(dev, EM28XX_DVB_MAX_PACKETS,
+ EM28XX_DVB_NUM_BUFS, dev->dvb_max_pkt_size,
+ false, NULL);
+ dev->isoc_ctl.preallocated_transfer_bufs = true;
+ }
+
return 0;
unlock_and_free:
@@ -3372,6 +3380,9 @@ static void em28xx_usb_disconnect(struct
usb_interface *interface)
v4l2_device_disconnect(&dev->v4l2_dev);
+ dev->isoc_ctl.preallocated_transfer_bufs = false;
+ em28xx_uninit_isoc(dev, true);
+
if (dev->users) {
em28xx_warn
("device %s is open! Deregistration and memory "
@@ -3379,7 +3390,6 @@ static void em28xx_usb_disconnect(struct
usb_interface *interface)
video_device_node_name(dev->vdev));
dev->state |= DEV_MISCONFIGURED;
- em28xx_uninit_isoc(dev);
dev->state |= DEV_DISCONNECTED;
wake_up_interruptible(&dev->wait_frame);
wake_up_interruptible(&dev->wait_stream);
b/drivers/media/video/em28xx/em28xx-core.c
@@ -59,6 +59,10 @@ static unsigned int disable_vbi;
module_param(disable_vbi, int, 0644);
MODULE_PARM_DESC(disable_vbi, "disable vbi support");
+static unsigned int transfer_buf_size;
+module_param(transfer_buf_size, int, 0644);
+MODULE_PARM_DESC(transfer_buf_size, "override USB isoc transfer buffer
size");
+
/* FIXME */
#define em28xx_isocdbg(fmt, arg...) do {\
if (core_debug) \
@@ -961,14 +965,18 @@ static void em28xx_irq_callback(struct urb *urb)
/*
* Stop and Deallocate URBs
*/
-void em28xx_uninit_isoc(struct em28xx *dev)
+void em28xx_uninit_isoc(struct em28xx *dev, bool start)
{
struct urb *urb;
int i;
em28xx_isocdbg("em28xx: called em28xx_uninit_isoc\n");
+ if (dev->isoc_ctl.preallocated_transfer_bufs)
+ return;
+
dev->isoc_ctl.nfields = -1;
+ printk ("free %d isoc transfer buffers\n", dev->isoc_ctl.num_bufs);
for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {
urb = dev->isoc_ctl.urb[i];
if (urb) {
@@ -996,7 +1004,8 @@ void em28xx_uninit_isoc(struct em28xx *dev)
dev->isoc_ctl.transfer_buffer = NULL;
dev->isoc_ctl.num_bufs = 0;
- em28xx_capture_start(dev, 0);
+ if (start)
+ em28xx_capture_start(dev, 0);
}
EXPORT_SYMBOL_GPL(em28xx_uninit_isoc);
@@ -1004,7 +1013,7 @@ EXPORT_SYMBOL_GPL(em28xx_uninit_isoc);
* Allocate URBs and start IRQ
*/
int em28xx_init_isoc(struct em28xx *dev, int max_packets,
- int num_bufs, int max_pkt_size,
+ int num_bufs, int max_pkt_size, bool start,
int (*isoc_copy) (struct em28xx *dev, struct urb *urb))
{
struct em28xx_dmaqueue *dma_q = &dev->vidq;
@@ -1017,78 +1026,99 @@ int em28xx_init_isoc(struct em28xx *dev, int
max_packets,
em28xx_isocdbg("em28xx: called em28xx_prepare_isoc\n");
- /* De-allocates all pending stuff */
- em28xx_uninit_isoc(dev);
-
- dev->isoc_ctl.isoc_copy = isoc_copy;
- dev->isoc_ctl.num_bufs = num_bufs;
-
- dev->isoc_ctl.urb = kzalloc(sizeof(void *)*num_bufs, GFP_KERNEL);
- if (!dev->isoc_ctl.urb) {
- em28xx_errdev("cannot alloc memory for usb buffers\n");
- return -ENOMEM;
- }
-
- dev->isoc_ctl.transfer_buffer = kzalloc(sizeof(void *)*num_bufs,
- GFP_KERNEL);
- if (!dev->isoc_ctl.transfer_buffer) {
- em28xx_errdev("cannot allocate memory for usb transfer\n");
- kfree(dev->isoc_ctl.urb);
- return -ENOMEM;
- }
-
- dev->isoc_ctl.max_pkt_size = max_pkt_size;
- dev->isoc_ctl.vid_buf = NULL;
- dev->isoc_ctl.vbi_buf = NULL;
-
- sb_size = max_packets * dev->isoc_ctl.max_pkt_size;
-
- /* allocate urbs and transfer buffers */
- for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {
- urb = usb_alloc_urb(max_packets, GFP_KERNEL);
- if (!urb) {
- em28xx_err("cannot alloc isoc_ctl.urb %i\n", i);
- em28xx_uninit_isoc(dev);
+ if (transfer_buf_size && max_pkt_size > 0) {
+ sb_size = transfer_buf_size;
+ max_packets = sb_size / max_pkt_size;
+ printk ("overridden sb_size:%d and max_packets:%d\n",
+ sb_size, max_packets);
+ } else
+ sb_size = max_packets * max_pkt_size;
+
+ if (dev->isoc_ctl.transfer_buffer == NULL
+ || dev->isoc_ctl.num_bufs != num_bufs
+ || dev->isoc_ctl.max_pkt_size != max_pkt_size
+ || (dev->isoc_ctl.num_bufs > 0
+ && dev->isoc_ctl.urb[0]->number_of_packets != max_packets)) {
+ /* De-allocates all pending stuff */
+ dev->isoc_ctl.preallocated_transfer_bufs = false;
+ em28xx_uninit_isoc(dev, start);
+
+ dev->isoc_ctl.num_bufs = num_bufs;
+
+ dev->isoc_ctl.urb = kzalloc(sizeof(void *)*num_bufs, GFP_KERNEL);
+ if (!dev->isoc_ctl.urb) {
+ em28xx_errdev("cannot alloc memory for usb buffers\n");
return -ENOMEM;
}
- dev->isoc_ctl.urb[i] = urb;
-
- dev->isoc_ctl.transfer_buffer[i] = usb_alloc_coherent(dev->udev,
- sb_size, GFP_KERNEL, &urb->transfer_dma);
- if (!dev->isoc_ctl.transfer_buffer[i]) {
- em28xx_err("unable to allocate %i bytes for transfer"
- " buffer %i%s\n",
- sb_size, i,
- in_interrupt() ? " while in int" : "");
- em28xx_uninit_isoc(dev);
+
+ dev->isoc_ctl.transfer_buffer = kzalloc(sizeof(void *)*num_bufs,
+ GFP_KERNEL);
+ if (!dev->isoc_ctl.transfer_buffer) {
+ em28xx_errdev("cannot allocate memory for usb transfer\n");
+ kfree(dev->isoc_ctl.urb);
return -ENOMEM;
}
- memset(dev->isoc_ctl.transfer_buffer[i], 0, sb_size);
- /* FIXME: this is a hack - should be
- 'desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK'
- should also be using 'desc.bInterval'
- */
- pipe = usb_rcvisocpipe(dev->udev,
- dev->mode == EM28XX_ANALOG_MODE ?
- EM28XX_EP_ANALOG : EM28XX_EP_DIGITAL);
-
- usb_fill_int_urb(urb, dev->udev, pipe,
- dev->isoc_ctl.transfer_buffer[i], sb_size,
- em28xx_irq_callback, dev, 1);
-
- urb->number_of_packets = max_packets;
- urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
-
- k = 0;
- for (j = 0; j < max_packets; j++) {
- urb->iso_frame_desc[j].offset = k;
- urb->iso_frame_desc[j].length =
- dev->isoc_ctl.max_pkt_size;
- k += dev->isoc_ctl.max_pkt_size;
+ dev->isoc_ctl.max_pkt_size = max_pkt_size;
+ dev->isoc_ctl.vid_buf = NULL;
+ dev->isoc_ctl.vbi_buf = NULL;
+
+ printk ("alloc %d buffers of %d bytes (each containing %d packets of
%d bytes)\n",
+ dev->isoc_ctl.num_bufs, sb_size, max_packets,
dev->isoc_ctl.max_pkt_size);
+ /* allocate urbs and transfer buffers */
+ for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {
+ urb = usb_alloc_urb(max_packets, GFP_KERNEL);
+ if (!urb) {
+ em28xx_err("cannot alloc isoc_ctl.urb %i\n", i);
+ em28xx_uninit_isoc(dev, start);
+ return -ENOMEM;
+ }
+ dev->isoc_ctl.urb[i] = urb;
+
+ dev->isoc_ctl.transfer_buffer[i] = usb_alloc_coherent(dev->udev,
+ sb_size, GFP_KERNEL, &urb->transfer_dma);
+ if (!dev->isoc_ctl.transfer_buffer[i]) {
+ em28xx_err("unable to allocate %i bytes for transfer"
+ " buffer %i%s\n",
+ sb_size, i,
+ in_interrupt() ? " while in int" : "");
+ em28xx_uninit_isoc(dev, start);
+ return -ENOMEM;
+ }
+ memset(dev->isoc_ctl.transfer_buffer[i], 0, sb_size);
+
+ /* FIXME: this is a hack - should be
+ 'desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK'
+ should also be using 'desc.bInterval'
+ */
+ pipe = usb_rcvisocpipe(dev->udev,
+ dev->mode == EM28XX_ANALOG_MODE ?
+ EM28XX_EP_ANALOG : EM28XX_EP_DIGITAL);
+
+ usb_fill_int_urb(urb, dev->udev, pipe,
+ dev->isoc_ctl.transfer_buffer[i], sb_size,
+ em28xx_irq_callback, dev, 1);
+
+ urb->number_of_packets = max_packets;
+ urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
+
+ k = 0;
+ for (j = 0; j < max_packets; j++) {
+ urb->iso_frame_desc[j].offset = k;
+ urb->iso_frame_desc[j].length =
+ dev->isoc_ctl.max_pkt_size;
+ k += dev->isoc_ctl.max_pkt_size;
+ }
}
+ } else {
+ printk ("no need to realloc isoc transfer buffers (size not changed)\n");
}
+ dev->isoc_ctl.isoc_copy = isoc_copy;
+
+ if (!start)
+ return 0;
+
init_waitqueue_head(&dma_q->wq);
init_waitqueue_head(&vbi_dma_q->wq);
@@ -1100,7 +1130,8 @@ int em28xx_init_isoc(struct em28xx *dev, int
max_packets,
if (rc) {
em28xx_err("submit of urb %i failed (error=%i)\n", i,
rc);
- em28xx_uninit_isoc(dev);
+ dev->isoc_ctl.preallocated_transfer_bufs = false;
+ em28xx_uninit_isoc(dev, start);
return rc;
}
}
b/drivers/media/video/em28xx/em28xx-dvb.c
@@ -61,9 +61,6 @@ if (debug >= level) \
printk(KERN_DEBUG "%s/2-dvb: " fmt, dev->name, ## arg); \
} while (0)
-#define EM28XX_DVB_NUM_BUFS 5
-#define EM28XX_DVB_MAX_PACKETS 64
-
struct em28xx_dvb {
struct dvb_frontend *fe[2];
@@ -178,15 +175,13 @@ static int em28xx_start_streaming(struct
em28xx_dvb *dvb)
return em28xx_init_isoc(dev, EM28XX_DVB_MAX_PACKETS,
EM28XX_DVB_NUM_BUFS, max_dvb_packet_size,
- em28xx_dvb_isoc_copy);
+ true, em28xx_dvb_isoc_copy);
}
static int em28xx_stop_streaming(struct em28xx_dvb *dvb)
{
struct em28xx *dev = dvb->adapter.priv;
- em28xx_uninit_isoc(dev);
-
em28xx_set_mode(dev, EM28XX_SUSPEND);
return 0;
b/drivers/media/video/em28xx/em28xx-video.c
@@ -764,15 +764,18 @@ buffer_prepare(struct videobuf_queue *vq, struct
videobuf_buffer *vb,
urb_init = 1;
if (urb_init) {
+ dev->isoc_ctl.preallocated_transfer_bufs = false;
if (em28xx_vbi_supported(dev) == 1)
rc = em28xx_init_isoc(dev, EM28XX_NUM_PACKETS,
EM28XX_NUM_BUFS,
dev->max_pkt_size,
+ true,
em28xx_isoc_copy_vbi);
else
rc = em28xx_init_isoc(dev, EM28XX_NUM_PACKETS,
EM28XX_NUM_BUFS,
dev->max_pkt_size,
+ true,
em28xx_isoc_copy);
if (rc < 0)
goto fail;
@@ -2267,7 +2270,7 @@ static int em28xx_v4l2_close(struct file *filp)
v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
/* do this before setting alternate! */
- em28xx_uninit_isoc(dev);
+ em28xx_uninit_isoc(dev, true);
em28xx_set_mode(dev, EM28XX_SUSPEND);
/* set alternate 0 */
b/drivers/media/video/em28xx/em28xx.h
@@ -151,12 +151,14 @@
/* number of buffers for isoc transfers */
#define EM28XX_NUM_BUFS 5
+#define EM28XX_DVB_NUM_BUFS 5
/* number of packets for each buffer
windows requests only 64 packets .. so we better do the same
this is what I found out for all alternate numbers there!
*/
#define EM28XX_NUM_PACKETS 64
+#define EM28XX_DVB_MAX_PACKETS 64
#define EM28XX_INTERLACED_DEFAULT 1
@@ -228,9 +230,11 @@ struct em28xx_usb_isoc_ctl {
/* Stores the number of received fields */
int nfields;
+ /* Signals if the transfer buffers were preallocated */
+ bool preallocated_transfer_bufs;
+
/* isoc urb callback */
int (*isoc_copy) (struct em28xx *dev, struct urb *urb);
-
};
/* Struct to enumberate video formats */
@@ -677,9 +681,9 @@ int em28xx_set_outfmt(struct em28xx *dev);
int em28xx_resolution_set(struct em28xx *dev);
int em28xx_set_alternate(struct em28xx *dev);
int em28xx_init_isoc(struct em28xx *dev, int max_packets,
- int num_bufs, int max_pkt_size,
+ int num_bufs, int max_pkt_size, bool start,
int (*isoc_copy) (struct em28xx *dev, struct urb *urb));
-void em28xx_uninit_isoc(struct em28xx *dev);
+void em28xx_uninit_isoc(struct em28xx *dev, bool start);
int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev);
int em28xx_set_mode(struct em28xx *dev, enum em28xx_mode set_mode);
int em28xx_gpio_set(struct em28xx *dev, struct em28xx_reg_seq *gpio);