Hi Clemens
On Mon, Oct 12, 2015 at 11:16 AM, Clemens Ladisch <clemens@xxxxxxxxxx> wrote:
> Felipe Tonello wrote:
>> On Fri, Oct 9, 2015 at 10:23 AM, Clemens Ladisch <clemens@xxxxxxxxxx> wrote:
>>> Felipe Tonello wrote:
>>>> } else if (ep == midi->in_ep) {
>>>> - /* Our transmit completed. See if there's more to go.
>>>> - * f_midi_transmit eats req, don't queue it again. */
>>>> - f_midi_transmit(midi, req);
>>>> + /* Our transmit completed. Don't queue it again. */
>>>> + free_ep_req(ep, req);
>>>> return;
>>>> }
>>>> break;
>>>
>>> The ALSA framework will give you a notification _once_. If the
>>> resulting data is larger than midi->buflen, then you have to continue
>>> sending packets. This is exactly what the call to f_midi_transmit()
>>> does.
>>
>> That's true. But what it will also happen is that f_midi_transmit()
>> will potentially eat up data from an unrelated ALSA trigger.
>
> The triggers are for some MIDI port of the _same_ USB endpoint, so
> they're not unrelated. f_midi_transmit() collects data from all ports
> anyway; separating the data from different ports into different USB
> packets would just needlessly introduce additional latency.
Ok.
>
>> In the end it is all fine, because the function will realise the
>> request->len == 0 so it will free the request. But logically speaking
>> it is incorrect and error prone.
>
> It is _not_ incorrect if you realize that f_midi_transmit() applies to
> the endpoint, not to any particular port.
>
>>> (To decrease latency, it might be a good idea to queue multiple requests,
>>> but you wouldn't want to continue piling up requests if the host isn't
>>> listening. sound/usb/midi.c just allocates a fixed number of requests,
>>> and always reuses them.)
>>
>> I believe that is the best way to implement. Create multiple requests
>> until the ALSA substreams buffer are empty and free the request on
>> completion.
>
> I believe a better way to implement this is to allocate a fixed number
> of requests, and to always reuse them.
How many?
>
>> The problem of having requests when host isn't listening will happen
>> anyway because there is no way to know that until completion.
>
> But if you have no upper limit on the number of queues requests, you
> will eventually run out of (DMA) memory.
And that's what happening at the moment. One of my patches are to fix
a memory leak when that happens.
But it would be ideal to have a FIFO of requests and perhaps ignore
new requests if the FIFO is full.
So, allocate (pre-allocate?) new requests until the FIFO is full. Upon
completion, remove the request from FIFO, but still reuse it on
f_midi_transmit() and queue it on the FIFO again if there is still
data from ALSA, otherwise just free the request.
What do you think?
Felipe Tonello
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
