Am 08.01.2014 15:10, schrieb Mauro Carvalho Chehab:
> Em Tue, 07 Jan 2014 18:03:50 +0100
> Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu:
>
>> Am 05.01.2014 22:25, schrieb Mauro Carvalho Chehab:
>>> Em Sun, 05 Jan 2014 22:02:40 +0100
>>> Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu:
>>>
>>>> Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab:
>>>>> From: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
>>>> Is this line still correct ? ;)
>>>>
>>>>> Instead of allocating/deallocating URBs and transfer buffers
>>>>> every time stream is started/stopped, just do it once.
>>>>>
>>>>> That reduces the memory allocation pressure and makes the
>>>>> code that start/stop streaming a way simpler.
>>>>>
>>>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
>>>>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@xxxxxxxxxxx>
>>>> Two Signed-off-by lines ? ;)
>>>>
>>>>> ---
>>>>> drivers/media/usb/em28xx/em28xx-audio.c | 128 ++++++++++++++++++--------------
>>>>> 1 file changed, 73 insertions(+), 55 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
>>>>> index e5120430ec80..30ee389a07f0 100644
>>>>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
>>>>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
>>>>> @@ -3,7 +3,7 @@
>>>>> *
>>>>> * Copyright (C) 2006 Markus Rechberger <mrechberger@xxxxxxxxx>
>>>>> *
>>>>> - * Copyright (C) 2007-2011 Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
>>>>> + * Copyright (C) 2007-2014 Mauro Carvalho Chehab
>>>>> * - Port to work with the in-kernel driver
>>>>> * - Cleanups, fixes, alsa-controls, etc.
>>>>> *
>>>>> @@ -70,16 +70,6 @@ static int em28xx_deinit_isoc_audio(struct em28xx *dev)
>>>>> usb_kill_urb(urb);
>>>>> else
>>>>> usb_unlink_urb(urb);
>>>>> -
>>>>> - usb_free_coherent(dev->udev,
>>>>> - urb->transfer_buffer_length,
>>>>> - dev->adev.transfer_buffer[i],
>>>>> - urb->transfer_dma);
>>>>> -
>>>>> - dev->adev.transfer_buffer[i] = NULL;
>>>>> -
>>>>> - usb_free_urb(urb);
>>>>> - dev->adev.urb[i] = NULL;
>>>>> }
>>>>>
>>>>> return 0;
>>>>> @@ -174,53 +164,14 @@ static void em28xx_audio_isocirq(struct urb *urb)
>>>>> static int em28xx_init_audio_isoc(struct em28xx *dev)
>>>>> {
>>>>> int i, errCode;
>>>>> - const int sb_size = EM28XX_NUM_AUDIO_PACKETS *
>>>>> - EM28XX_AUDIO_MAX_PACKET_SIZE;
>>>>>
>>>>> dprintk("Starting isoc transfers\n");
>>>>>
>>>>> + /* Start streaming */
>>>>> for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
>>>>> - struct urb *urb;
>>>>> - int j, k;
>>>>> - void *buf;
>>>>> -
>>>>> - urb = usb_alloc_urb(EM28XX_NUM_AUDIO_PACKETS, GFP_ATOMIC);
>>>>> - if (!urb) {
>>>>> - em28xx_errdev("usb_alloc_urb failed!\n");
>>>>> - for (j = 0; j < i; j++) {
>>>>> - usb_free_urb(dev->adev.urb[j]);
>>>>> - kfree(dev->adev.transfer_buffer[j]);
>>>>> - }
>>>>> - return -ENOMEM;
>>>>> - }
>>>>> -
>>>>> - buf = usb_alloc_coherent(dev->udev, sb_size, GFP_ATOMIC,
>>>>> - &urb->transfer_dma);
>>>>> - if (!buf)
>>>>> - return -ENOMEM;
>>>>> - dev->adev.transfer_buffer[i] = buf;
>>>>> - memset(buf, 0x80, sb_size);
>>>>> -
>>>>> - urb->dev = dev->udev;
>>>>> - urb->context = dev;
>>>>> - urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
>>>>> - urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
>>>>> - urb->transfer_buffer = dev->adev.transfer_buffer[i];
>>>>> - urb->interval = 1;
>>>>> - urb->complete = em28xx_audio_isocirq;
>>>>> - urb->number_of_packets = EM28XX_NUM_AUDIO_PACKETS;
>>>>> - urb->transfer_buffer_length = sb_size;
>>>>> -
>>>>> - for (j = k = 0; j < EM28XX_NUM_AUDIO_PACKETS;
>>>>> - j++, k += EM28XX_AUDIO_MAX_PACKET_SIZE) {
>>>>> - urb->iso_frame_desc[j].offset = k;
>>>>> - urb->iso_frame_desc[j].length =
>>>>> - EM28XX_AUDIO_MAX_PACKET_SIZE;
>>>>> - }
>>>>> - dev->adev.urb[i] = urb;
>>>>> - }
>>>>> + memset(dev->adev.transfer_buffer[i], 0x80,
>>>>> + dev->adev.urb[i]->transfer_buffer_length);
>>>>>
>>>>> - for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
>>>>> errCode = usb_submit_urb(dev->adev.urb[i], GFP_ATOMIC);
>>>>> if (errCode) {
>>>>> em28xx_errdev("submit of audio urb failed\n");
>>>>> @@ -643,13 +594,36 @@ static struct snd_pcm_ops snd_em28xx_pcm_capture = {
>>>>> .page = snd_pcm_get_vmalloc_page,
>>>>> };
>>>>>
>>>>> +static void em28xx_audio_free_urb(struct em28xx *dev)
>>>>> +{
>>>>> + int i;
>>>>> +
>>>>> + for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
>>>>> + struct urb *urb = dev->adev.urb[i];
>>>>> +
>>>>> + if (!dev->adev.urb[i])
>>>>> + continue;
>>>>> +
>>>>> + usb_free_coherent(dev->udev,
>>>>> + urb->transfer_buffer_length,
>>>>> + dev->adev.transfer_buffer[i],
>>>>> + urb->transfer_dma);
>>>>> +
>>>>> + usb_free_urb(urb);
>>>>> + dev->adev.urb[i] = NULL;
>>>>> + dev->adev.transfer_buffer[i] = NULL;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> static int em28xx_audio_init(struct em28xx *dev)
>>>>> {
>>>>> struct em28xx_audio *adev = &dev->adev;
>>>>> struct snd_pcm *pcm;
>>>>> struct snd_card *card;
>>>>> static int devnr;
>>>>> - int err;
>>>>> + int err, i;
>>>>> + const int sb_size = EM28XX_NUM_AUDIO_PACKETS *
>>>>> + EM28XX_AUDIO_MAX_PACKET_SIZE;
>>>>>
>>>>> if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
>>>>> /* This device does not support the extension (in this case
>>>>> @@ -662,7 +636,8 @@ static int em28xx_audio_init(struct em28xx *dev)
>>>>>
>>>>> printk(KERN_INFO "em28xx-audio.c: Copyright (C) 2006 Markus "
>>>>> "Rechberger\n");
>>>>> - printk(KERN_INFO "em28xx-audio.c: Copyright (C) 2007-2011 Mauro Carvalho Chehab\n");
>>>>> + printk(KERN_INFO
>>>>> + "em28xx-audio.c: Copyright (C) 2007-2014 Mauro Carvalho Chehab\n");
>>>>>
>>>>> err = snd_card_create(index[devnr], "Em28xx Audio", THIS_MODULE, 0,
>>>>> &card);
>>>>> @@ -704,6 +679,47 @@ static int em28xx_audio_init(struct em28xx *dev)
>>>>> em28xx_cvol_new(card, dev, "Surround", AC97_SURROUND_MASTER);
>>>>> }
>>>>>
>>>>> + /* Alloc URB and transfer buffers */
>>>>> + for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
>>>>> + struct urb *urb;
>>>>> + int j, k;
>>>>> + void *buf;
>>>>> +
>>>>> + urb = usb_alloc_urb(EM28XX_NUM_AUDIO_PACKETS, GFP_ATOMIC);
>>>>> + if (!urb) {
>>>>> + em28xx_errdev("usb_alloc_urb failed!\n");
>>>>> + em28xx_audio_free_urb(dev);
>>>>> + return -ENOMEM;
>>>>> + }
>>>>> + dev->adev.urb[i] = urb;
>>>>> +
>>>>> + buf = usb_alloc_coherent(dev->udev, sb_size, GFP_ATOMIC,
>>>>> + &urb->transfer_dma);
>>>>> + if (!buf) {
>>>>> + em28xx_errdev("usb_alloc_coherent failed!\n");
>>>>> + em28xx_audio_free_urb(dev);
>>>>> + return -ENOMEM;
>>>>> + }
>>>>> + dev->adev.transfer_buffer[i] = buf;
>>>>> +
>>>>> + urb->dev = dev->udev;
>>>>> + urb->context = dev;
>>>>> + urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
>>>>> + urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
>>>>> + urb->transfer_buffer = dev->adev.transfer_buffer[i];
>>>>> + urb->interval = 1;
>>>>> + urb->complete = em28xx_audio_isocirq;
>>>>> + urb->number_of_packets = EM28XX_NUM_AUDIO_PACKETS;
>>>>> + urb->transfer_buffer_length = sb_size;
>>>>> +
>>>>> + for (j = k = 0; j < EM28XX_NUM_AUDIO_PACKETS;
>>>>> + j++, k += EM28XX_AUDIO_MAX_PACKET_SIZE) {
>>>>> + urb->iso_frame_desc[j].offset = k;
>>>>> + urb->iso_frame_desc[j].length =
>>>>> + EM28XX_AUDIO_MAX_PACKET_SIZE;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> err = snd_card_register(card);
>>>>> if (err < 0) {
>>>>> snd_card_free(card);
>>>>> @@ -728,6 +744,8 @@ static int em28xx_audio_fini(struct em28xx *dev)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> + em28xx_audio_free_urb(dev);
>>>>> +
>>>>> if (dev->adev.sndcard) {
>>>>> snd_card_free(dev->adev.sndcard);
>>>>> dev->adev.sndcard = NULL;
>>>> I don't get it.
>>>> How does this patch reduce the memory allocation pressure ?
>>>> You are still allocating the same amount of memory.
>>> True, but it is not de-allocating/reallocating the same amount of
>>> memory every time, for every start/stop trigger. Depending on the
>>> userspace and the amount of available RAM, this could cause memory
>>> fragmentation.
>>>
>>> If you take a look at xHCI logs, you'll see that those operations
>>> are very expensive, and that it occurs too often.
>> How often is the device started/stopped ?
>> It's nothing that happens often/with a high freuqency, so is memeory
>> fragmentation really a problem here ?
> The trigger is called every time an underrun occurs. The thing is that
> the memory allocation, plus the start URB logic takes too long.
>
> With my test machine (an i7core with 4 cores, 8 threads, running at
> 2.4 GHz) and Kworld 305U (em2861), the time between receiving the trigger
> and having the first audio buffer is long enough to produce another underrun.
>
> At underrun, the driver will kill all existing URBs (with takes some time),
> deallocate memory. Then a new trigger will reallocate memory and re-submit
> URBs.
>
> That actually means that underrun->trigger off/trigger on->underrun loop
> happens several times per second, producing a crappy audio.
Ah, ok, I didn't expect it to happen that often.
No question then, the patch makes definitely sense.
Thanks for your explanations.
> At least on this machine, removing memory allocation from the trigger loop
> fixes the issue with Kworld 305U.
>
> Btw, I noticed the same bug in the past on an older test hardware and
> HVR-950.
>
> PS.: on a slower machine, this may eventually not be enough, and we may
> need to remove also the URB cancel/resubmit from the trigger loop.
> I have some patches for this too, as such patch were needed to isolate
> bugs when connected to a xHCI port (where URB kill doesn't seem to be
> working properly), but I'll submit such patches only if I can make em28xx
> work when connected to an xHCI 1.0 port, and while the USB core is not fixed.
I really need to buy a em28xx device with vendor audio interface.
My HVR930 has vendor audio, but - as you know - we don't support the
analog part of this device yet. :(
>
>>>> The only differences is that you already do this when the device isn't
>>>> used yet and don't free it when gets unused again.
>>>> IMHO that makes things worse, not better.
>>> Why is it worse?
>> Because you increase the memory usage of closed devices.
> So what? If someone plugged an em28xx device, it is because it is supposed
> to be used. Besides that, the amount of allocated memory is not big.
Sure, it's not a big problem.
>
>>> FYI, we're currently allocating DVB buffers at the device init too,
>>> due to the memory fragmentation problems. This is actually critical
>>> if you try to use it on an ARM with limited amount of RAM.
>> I was always wondering why.
>>
>> Ok, if this really solves problems on ARM, do it.
>> I assume it makes sense fix it for analog video, too. ;)
> Yes, it makes sense for analog video too. There are some additional steps
> to make em28xx v4l to work with ARM though. I'll eventually work on it if
> I have some time during this month.
Ok, great. :)
>
>>>> And yes, it makes the code that starts/stops streaming a way simpler.
>>>> But at the same time it makes the module initialization code the same
>>>> amount more complicated.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
