Skip to contentSkip to conversation

em28xx: pre-allocate DVB isoc transfer buffers

PATCH In review Gianluca Gennari 3 participants 10 messages Feb 13, 2012 – Mar 16, 2012 Media (V4L/DVB)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 em28xx: pre-allocate DVB isoc transfer buffers

(was: Re: PCTV 290e page allocation failure)

On MIPS/ARM set-top-boxes, as well as old x86 PCs, memory allocation failures in the em28xx driver are common, due to memory fragmentation over time, that makes impossible to allocate large chunks of coherent memory.
A typical system with 256/512 MB of RAM fails after just 1 day of uptime (see the old thread for detailed reports and crashlogs).

In fact, the em28xx driver allocates memory for USB isoc transfers at runtime, as opposite to the dvb-usb drivers that allocates the USB buffers when the device is initialized, and frees them when the device is disconnected.

Moreover, in digital mode the USB isoc transfer buffers are freed, allocated and cleared every time the user selects a new channel, wasting time and resources.

This patch solves both problems by allocating DVB isoc transfer buffers in em28xx_usb_probe(), and freeing them in em28xx_usb_disconnect(). In fact, the buffers size and number depend only on the max USB packet size that is parsed from the USB descriptors in em28xx_usb_probe(), so it can never change for a given device.

This approach makes no sense in analog mode (as the buffer size depends on the alternate mode selected at runtime), the patch creates two separate sets of buffers for digital and analog modes.

For digital-only devices, USB buffers are created when the device is probed and freed when the device is disconnected.
For analog-only devices, nothing changes: isoc buffers are created at runtime. For hybrid devices, two sets of buffers are maintained: the digital-mode buffers are created when the device is probed, and freed when the device is disconnected; analog-mode buffers are created/destroyed at runtime as before. So, in analog mode, digital and analog buffers coexists at the same time: this can be justified by the fact that digital mode is by far more commonly used nowadays, so it makes sense to optimize the driver for this use case scenario.

The patch has been tested in the last few days on a x86 PC and a MIPS set-top-box, with the PCTV 290e (digital only) and the Terratec Hybrid XS (hybrid device). With the latter, I switched several times between analog and digital mode (Kaffeine/TvTime) with no issue at all.
I unplugged/plugged the devices several times with no problem. Also, after over 3 days of normal usage in the MPIS set-top-box, the PCTV 290e was still up and running.

---
 drivers/media/video/em28xx/em28xx-cards.c |   16 +++-
 drivers/media/video/em28xx/em28xx-core.c  |  145 +++++++++++++++++++----------
 drivers/media/video/em28xx/em28xx-dvb.c   |   14 +--
 drivers/media/video/em28xx/em28xx-video.c |   10 +-
 drivers/media/video/em28xx/em28xx.h       |   23 ++++-
 5 files changed, 142 insertions(+), 66 deletions(-)
drivers/media/video/em28xx/em28xx-cards.c +15 -1
index 2aa772a..5c06bac 100644
@@ -3307,6 +3307,17 @@ static int em28xx_usb_probe(struct usb_interface *interface,
 		goto unlock_and_free;
 	}
 
+	if (has_dvb) {
+		/* pre-allocate DVB isoc transfer buffers */
+		retval = em28xx_alloc_isoc(dev, EM28XX_DIGITAL_MODE,
+					   EM28XX_DVB_MAX_PACKETS,
+					   EM28XX_DVB_NUM_BUFS,
+					   dev->dvb_max_pkt_size);
+		if (retval) {
+			goto unlock_and_free;
+		}
+	}
+
 	request_modules(dev);
 
 	/* Should be the last thing to do, to avoid newer udev's to
@@ -3379,7 +3390,7 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
 		     video_device_node_name(dev->vdev));
 
 		dev->state |= DEV_MISCONFIGURED;
-		em28xx_uninit_isoc(dev);
+		em28xx_uninit_isoc(dev, dev->mode);
 		dev->state |= DEV_DISCONNECTED;
 		wake_up_interruptible(&dev->wait_frame);
 		wake_up_interruptible(&dev->wait_stream);
@@ -3388,6 +3399,9 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
 		em28xx_release_resources(dev);
 	}
 
+	/* free DVB isoc buffers */
+	em28xx_uninit_isoc(dev, EM28XX_DIGITAL_MODE);
+
 	mutex_unlock(&dev->lock);
 
 	em28xx_close_extension(dev);
drivers/media/video/em28xx/em28xx-core.c +96 -49

248 lines not on this page · raw message

drivers/media/video/em28xx/em28xx-dvb.c +6 -8
index aabbf48..fbd9010 100644
@@ -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];
 
@@ -172,20 +169,21 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
 	max_dvb_packet_size = dev->dvb_max_pkt_size;
 	if (max_dvb_packet_size < 0)
 		return max_dvb_packet_size;
-	dprintk(1, "Using %d buffers each with %d bytes\n",
+	dprintk(1, "Using %d buffers each with %d x %d bytes\n",
 		EM28XX_DVB_NUM_BUFS,
+		EM28XX_DVB_MAX_PACKETS,
 		max_dvb_packet_size);
 
-	return em28xx_init_isoc(dev, EM28XX_DVB_MAX_PACKETS,
-				EM28XX_DVB_NUM_BUFS, max_dvb_packet_size,
-				em28xx_dvb_isoc_copy);
+	return em28xx_init_isoc(dev, EM28XX_DIGITAL_MODE,
+				EM28XX_DVB_MAX_PACKETS, EM28XX_DVB_NUM_BUFS,
+				max_dvb_packet_size, em28xx_dvb_isoc_copy);
 }
 
 static int em28xx_stop_streaming(struct em28xx_dvb *dvb)
 {
 	struct em28xx *dev = dvb->adapter.priv;
 
-	em28xx_uninit_isoc(dev);
+	em28xx_capture_start(dev, 0);
 
 	em28xx_set_mode(dev, EM28XX_SUSPEND);
 
drivers/media/video/em28xx/em28xx-video.c +6 -4
index 613300b..324b695 100644
@@ -760,17 +760,19 @@ buffer_prepare(struct videobuf_queue *vq, struct videobuf_buffer *vb,
 			goto fail;
 	}
 
-	if (!dev->isoc_ctl.num_bufs)
+	if (!dev->isoc_ctl.analog_bufs.num_bufs)
 		urb_init = 1;
 
 	if (urb_init) {
 		if (em28xx_vbi_supported(dev) == 1)
-			rc = em28xx_init_isoc(dev, EM28XX_NUM_PACKETS,
+			rc = em28xx_init_isoc(dev, EM28XX_ANALOG_MODE,
+					      EM28XX_NUM_PACKETS,
 					      EM28XX_NUM_BUFS,
 					      dev->max_pkt_size,
 					      em28xx_isoc_copy_vbi);
 		else
-			rc = em28xx_init_isoc(dev, EM28XX_NUM_PACKETS,
+			rc = em28xx_init_isoc(dev, EM28XX_ANALOG_MODE,
+					      EM28XX_NUM_PACKETS,
 					      EM28XX_NUM_BUFS,
 					      dev->max_pkt_size,
 					      em28xx_isoc_copy);
@@ -2267,7 +2269,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, EM28XX_ANALOG_MODE);
 		em28xx_set_mode(dev, EM28XX_SUSPEND);
 
 		/* set alternate 0 */
drivers/media/video/em28xx/em28xx.h +19 -4
index 22e252b..2ae6815 100644
@@ -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
 
@@ -197,10 +199,13 @@ enum em28xx_mode {
 
 struct em28xx;
 
-struct em28xx_usb_isoc_ctl {
+struct em28xx_usb_isoc_bufs {
 		/* max packet size of isoc transaction */
 	int				max_pkt_size;
 
+		/* number of packets in each buffer */
+	int				num_packets;
+
 		/* number of allocated urbs */
 	int				num_bufs;
 
@@ -209,6 +214,14 @@ struct em28xx_usb_isoc_ctl {
 
 		/* transfer buffers for isoc transfer */
 	char				**transfer_buffer;
+};
+
+struct em28xx_usb_isoc_ctl {
+		/* isoc transfer buffers for analog mode */
+	struct em28xx_usb_isoc_bufs	analog_bufs;
+
+		/* isoc transfer buffers for digital mode */
+	struct em28xx_usb_isoc_bufs	digital_bufs;
 
 		/* Last buffer command and region */
 	u8				cmd;
@@ -676,10 +689,12 @@ int em28xx_vbi_supported(struct em28xx *dev);
 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 em28xx_alloc_isoc(struct em28xx *dev, enum em28xx_mode mode,
+		      int max_packets, int num_bufs, int max_pkt_size);
+int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
+		     int max_packets, int num_bufs, int max_pkt_size,
 		     int (*isoc_copy) (struct em28xx *dev, struct urb *urb));
-void em28xx_uninit_isoc(struct em28xx *dev);
+void em28xx_uninit_isoc(struct em28xx *dev, enum em28xx_mode mode);
 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);
Signature · 1 line
1.7.0.4
#1 lore raw

PATCH 1/1 em28xx: pre-allocate DVB isoc transfer buffers

Gianluca,

One quick comment about your patch; I've noticed that you've declared two new "GPL only" symbols:

EXPORT_SYMBOL_GPL(em28xx_capture_start);
EXPORT_SYMBOL_GPL(em28xx_alloc_isoc);

I'm not sure what the exact policy is with GPL symbols, but I do know what Al Viro posted recently on the subject:

http://thread.gmane.org/gmane.linux.file-systems/61372

Do we really need EXPORT_SYMBOL_GPL() here?

Cheers,
Chris

#2 lore raw

Il 20/02/2012 00:46, Chris Rankin ha scritto:

Quoted 11 lines

Gianluca,

One quick comment about your patch; I've noticed that you've declared two new "GPL only" symbols:

EXPORT_SYMBOL_GPL(em28xx_capture_start);
EXPORT_SYMBOL_GPL(em28xx_alloc_isoc);

I'm not sure what the exact policy is with GPL symbols, but I do know what Al Viro posted recently on the subject:

Quoted 7 lines

http://thread.gmane.org/gmane.linux.file-systems/61372

Do we really need EXPORT_SYMBOL_GPL() here?

Cheers,
Chris

Hi Chris,
thanks for the comment.
The two new symbols are used in place of the old em28xx_init_isoc and em28xx_uninit_isoc in two different modules (em28xx and em28xx-dvb). Since the old symbols are exported through EXPORT_SYMBOL_GPL(), I did the same with the new ones.
This choice should not break any non-GPL module, as this symbols are meant to be used only in the em28xx* modules, just like the old ones.

Regards,
Gianluca

#3 lore raw
In reply to Gianluca Gennari
Quoted 8 lines · Gianluca Gennari

(was: Re: PCTV 290e page allocation failure)

On MIPS/ARM set-top-boxes, as well as old x86 PCs, memory allocation failures in the em28xx driver are common, due to memory fragmentation over time, that makes impossible to allocate large chunks of coherent memory.
A typical system with 256/512 MB of RAM fails after just 1 day of uptime (see the old thread for detailed reports and crashlogs).

Quoted 7 lines

In fact, the em28xx driver allocates memory for USB isoc transfers at runtime, as opposite to the dvb-usb drivers that allocates the USB buffers when the device is initialized, and frees them when the device is disconnected.

Moreover, in digital mode the USB isoc transfer buffers are freed, allocated and cleared every time the user selects a new channel, wasting time and resources.

Does this patch have a chance of getting in?

I am still having to flush caches before use. If you want more testing I can give it a go. I didn't earlier as I didn't have a git to apply it to and thought it was going to get in anyway.

#4 lore raw

Il 16/03/2012 01:25, Andy Furniss ha scritto:

Quoted 5 lines

Does this patch have a chance of getting in?

I am still having to flush caches before use. If you want more testing I can give it a go. I didn't earlier as I didn't have a git to apply it to and thought it was going to get in anyway.

Hi Andy,
the patch is already in the current media_build tree and is queued for kernel 3.4.

Regards,
Gianluca

#5 lore raw
In reply to Andy Furniss

Il 16/03/2012 01:25, Andy Furniss ha scritto:

Quoted 7 lines

Does this patch have a chance of getting in?

I am still having to flush caches before use. If you want more testing I can give it a go. I didn't earlier as I didn't have a git to apply it to and thought it was going to get in anyway.

Hi Andy,
the patch is already in the current media_build tree and is queued for kernel 3.4.

Regards,
Gianluca

#6 lore raw
Quoted 4 lines · Gianluca Gennari

Il 16/03/2012 01:25, Andy Furniss ha scritto:

Quoted 3 lines

Does this patch have a chance of getting in?

Quoted 8 lines
Quoted 4 lines

I am still having to flush caches before use. If you want more testing I can give it a go. I didn't earlier as I didn't have a git to apply it to and thought it was going to get in anyway.

Hi Andy,
the patch is already in the current media_build tree and is queued for kernel 3.4.

Ahh, I'll give that a try, thanks.

#7 lore raw
Quoted 7 lines · Andy Furniss
Quoted 3 lines · Gianluca Gennari

Hi Andy,
the patch is already in the current media_build tree and is queued for kernel 3.4.

Ahh, I'll give that a try, thanks.

Seems to be working OK so far, I had to avoid building radio to get it to compile.

Are these just informational - I get five every time I tune (barring the first time IIRC)

Mar 16 14:56:58 noki kernel: ehci_hcd 0000:01:0a.2: shutdown urb ced2d000 ep4in-iso
Mar 16 14:56:58 noki kernel: ehci_hcd 0000:01:0a.2: shutdown urb ced2d800 ep4in-iso
Mar 16 14:56:58 noki kernel: ehci_hcd 0000:01:0a.2: shutdown urb cec3f800 ep4in-iso
Mar 16 14:56:58 noki kernel: ehci_hcd 0000:01:0a.2: shutdown urb ced2c000 ep4in-iso
Mar 16 14:56:58 noki kernel: ehci_hcd 0000:01:0a.2: shutdown urb ced2c800 ep4in-iso
#8 lore raw

Il 16/03/2012 16:05, Andy Furniss ha scritto:

Quoted 24 lines
Quoted 7 lines · Andy Furniss
Quoted 3 lines · Gianluca Gennari

Hi Andy,
the patch is already in the current media_build tree and is queued for kernel 3.4.

Ahh, I'll give that a try, thanks.

Seems to be working OK so far, I had to avoid building radio to get it to compile.

Are these just informational - I get five every time I tune (barring the first time IIRC)

Mar 16 14:56:58 noki kernel: ehci_hcd 0000:01:0a.2: shutdown urb
ced2d000 ep4in-iso
Mar 16 14:56:58 noki kernel: ehci_hcd 0000:01:0a.2: shutdown urb
ced2d800 ep4in-iso
Mar 16 14:56:58 noki kernel: ehci_hcd 0000:01:0a.2: shutdown urb
cec3f800 ep4in-iso
Mar 16 14:56:58 noki kernel: ehci_hcd 0000:01:0a.2: shutdown urb
ced2c000 ep4in-iso
Mar 16 14:56:58 noki kernel: ehci_hcd 0000:01:0a.2: shutdown urb
Quoted 1 line

ced2c800 ep4in-iso

Looks like some innocuous logging from the ehci_hcd driver. I've never seen it because I'm not using the ehci_hcd module on my systems. When you tune a new channel, the USB transfer is stopped (with the URBs still alive, so the driver "shuts down" them) and a new one is started. Then the URBs are reused (instead of being deallocated/allocated again/cleared as before) so they are resubmitted.

Regards,
Gianluca

#9 lore raw
Quoted 10 lines · Gianluca Gennari
Quoted 3 lines

Andy Furniss wrote:

Mar 16 14:56:58 noki kernel: ehci_hcd 0000:01:0a.2: shutdown urb

ced2c800 ep4in-iso

Looks like some innocuous logging from the ehci_hcd driver. I've never seen it because I'm not using the ehci_hcd module on my systems. When you tune a new channel, the USB transfer is stopped (with the URBs still alive, so the driver "shuts down" them) and a new one is started. Then the URBs are reused (instead of being deallocated/allocated again/cleared as before) so they are resubmitted.

Ok, I don't have any problems so far - I also probably have some verbose/debugging usb options enabled in my kernel so I suppose others may never see these.