Hi Robert,
On 13/11/15 12:38, Robert Baldyga wrote:
> Hi Felipe,
>
> On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
>> This patch introduces pre-allocation of IN endpoint USB requests. This
>> improves on latency (requires no usb request allocation on transmit) and
>> avoid
>> several potential probles on allocating too many usb requests (which involves
>> DMA pool allocation problems).
>>
>> This implementation also handles better multiple MIDI Gadget ports, always
>> processing the last processed MIDI substream if the last USB request wasn't
>> enought to handle the whole stream.
>>
>> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>> ---
>> drivers/usb/gadget/function/f_midi.c | 174
>> +++++++++++++++++++++++++++--------
>> drivers/usb/gadget/legacy/gmidi.c | 2 +-
>> 2 files changed, 137 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>> b/drivers/usb/gadget/function/f_midi.c
>> index 615d632..6ac39f6 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -23,6 +23,7 @@
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> #include <linux/device.h>
>> +#include <linux/kfifo.h>
>>
>> #include <sound/core.h>
>> #include <sound/initval.h>
>> @@ -88,6 +89,9 @@ struct f_midi {
>> int index;
>> char *id;
>> unsigned int buflen, qlen;
>> + DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>> + unsigned int in_req_num;
>> + unsigned int in_last_port;
>> };
>>
>> static inline struct f_midi *func_to_midi(struct usb_function *f)
>> @@ -95,7 +99,7 @@ static inline struct f_midi *func_to_midi(struct
>> usb_function *f)
>> return container_of(f, struct f_midi, func);
>> }
>>
>> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req);
>> +static void f_midi_transmit(struct f_midi *midi);
>>
>> DECLARE_UAC_AC_HEADER_DESCRIPTOR(1);
>> DECLARE_USB_MIDI_OUT_JACK_DESCRIPTOR(1);
>> @@ -253,7 +257,8 @@ f_midi_complete(struct usb_ep *ep, struct usb_request
>> *req)
>> } 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);
>> + req->length = 0;
>> + f_midi_transmit(midi);
>> return;
>> }
>> break;
>> @@ -264,10 +269,12 @@ f_midi_complete(struct usb_ep *ep, struct usb_request
>> *req)
>> case -ESHUTDOWN: /* disconnect from host */
>> VDBG(cdev, "%s gone (%d), %d/%d\n", ep->name, status,
>> req->actual, req->length);
>> - if (ep == midi->out_ep)
>> + if (ep == midi->out_ep) {
>> f_midi_handle_out_data(ep, req);
>> -
>> - free_ep_req(ep, req);
>> + /* We don't need to free IN requests because it's
>> handled
>> + * by the midi->in_req_fifo. */
>> + free_ep_req(ep, req);
>> + }
>> return;
>>
>> case -EOVERFLOW: /* buffer overrun on read means that
>> @@ -334,6 +341,21 @@ static int f_midi_set_alt(struct usb_function *f,
>> unsigned intf, unsigned alt)
>> if (err)
>> return err;
>>
>> + /* pre-allocate write usb requests to use on f_midi_transmit. */
>> + while (kfifo_avail(&midi->in_req_fifo)) {
>> + struct usb_request *req =
>> + midi_alloc_ep_req(midi->in_ep, midi->buflen);
>> +
>> + if (req == NULL)
>> + return -ENOMEM;
>
> We need to free previously allocated requests in case of failure.
>
>> +
>> + req->length = 0;
>> + req->complete = f_midi_complete;
>> +
>> + kfifo_put(&midi->in_req_fifo, req);
>> + midi->in_req_num++;
>> + }
>> +
>> /* allocate a bunch of read buffers and queue them all at once. */
>> for (i = 0; i < midi->qlen && err == 0; i++) {
>> struct usb_request *req =
>> @@ -366,6 +388,20 @@ static void f_midi_disable(struct usb_function *f)
>> */
>> usb_ep_disable(midi->in_ep);
>> usb_ep_disable(midi->out_ep);
>> +
>> + /* release IN requests */
>> + while (!kfifo_is_empty(&midi->in_req_fifo)) {
>> + struct usb_request *req = NULL;
>> + unsigned int len;
>> +
>> + len = kfifo_get(&midi->in_req_fifo, &req);
>> + if (len == 1)
>> + free_ep_req(midi->in_ep, req);
>> + else
>> + ERROR(midi, "%s couldn't free usb request: something
>> went wrong with kfifo\n",
>> + midi->in_ep->name);
>> + }
>> + midi->in_req_num = 0;
>> }
>>
>> static int f_midi_snd_free(struct snd_device *device)
>> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request
>> *req,
>> }
>> }
>>
>> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
>> +static void f_midi_drop_out_substreams(struct f_midi *midi)
>> {
>> - struct usb_ep *ep = midi->in_ep;
>> - int i;
>> -
>> - if (!ep)
>> - return;
>> -
>> - if (!req)
>> - req = midi_alloc_ep_req(ep, midi->buflen);
>> -
>> - if (!req) {
>> - ERROR(midi, "%s: alloc_ep_request failed\n", __func__);
>> - return;
>> - }
>> - req->length = 0;
>> - req->complete = f_midi_complete;
>> + unsigned int i;
>>
>> for (i = 0; i < MAX_PORTS; i++) {
>> struct gmidi_in_port *port = midi->in_port[i];
>> struct snd_rawmidi_substream *substream = midi->in_substream[i];
>>
>> - if (!port || !port->active || !substream)
>> + if (!port)
>> + break;
>> +
>> + if (!port->active || !substream)
>> continue;
>>
>> - while (req->length + 3 < midi->buflen) {
>> - uint8_t b;
>> - if (snd_rawmidi_transmit(substream, &b, 1) != 1) {
>> - port->active = 0;
>> + snd_rawmidi_drop_output(substream);
>> + }
>> +}
>> +
>> +static void f_midi_transmit(struct f_midi *midi)
>> +{
>> + struct usb_ep *ep = midi->in_ep;
>> + bool active;
>> +
>> + /* We only care about USB requests if IN endpoint is enabled */
>> + if (!ep || !ep->enabled)
>> + goto drop_out;
>> +
>> + do {
>> + struct usb_request *req = NULL;
>> + unsigned int len, i;
>> +
>> + active = false;
>> +
>> + /* We peek the request in order to reuse it if it fails
>> + * to enqueue on its endpoint */
>> + len = kfifo_peek(&midi->in_req_fifo, &req);
>> + if (len != 1) {
>> + ERROR(midi, "%s: Couldn't get usb request\n", __func__);
>> + goto drop_out;
>> + }
>> +
>> + if (req->length > 0) {
>> + WARNING(midi, "%s: All USB requests have been used.
>> Current queue size "
>> + "is %u, consider increasing it.\n", __func__,
>> midi->in_req_num);
>> + goto drop_out;
>> + }
>> +
>> + for (i = midi->in_last_port; i < MAX_PORTS; i++) {
>> + struct gmidi_in_port *port = midi->in_port[i];
>> + struct snd_rawmidi_substream *substream =
>> midi->in_substream[i];
>> +
>> + if (!port) {
>> + /* Reset counter when we reach the last
>> available port */
>> + midi->in_last_port = 0;
>> + break;
>> + }
>> +
>> + if (!port->active || !substream)
>> + continue;
>> +
>> + while (req->length + 3 < midi->buflen) {
>> + uint8_t b;
>> +
>> + if (snd_rawmidi_transmit(substream, &b, 1) !=
>> 1) {
>> + port->active = 0;
>> + break;
>> + }
>> + f_midi_transmit_byte(req, port, b);
>> + }
>> +
>> + active = !!port->active;
>> + /* Check if last port is still active, which means that
>> + * there is still data on that substream but this
>> current
>> + * request run out of space. */
>> + if (active) {
>> + midi->in_last_port = i;
>> + /* There is no need to re-iterate though midi
>> ports. */
>> break;
>> }
>> - f_midi_transmit_byte(req, port, b);
>> }
>> - }
>>
>> - if (req->length > 0 && ep->enabled) {
>> - int err;
>> + if (req->length > 0) {
>> + int err;
>>
>> - err = usb_ep_queue(ep, req, GFP_ATOMIC);
>> - if (err < 0)
>> - ERROR(midi, "%s queue req: %d\n",
>> - midi->in_ep->name, err);
>> - } else {
>> - free_ep_req(ep, req);
>> - }
>> + err = usb_ep_queue(ep, req, GFP_ATOMIC);
>> + if (err < 0) {
>> + ERROR(midi, "%s failed to queue req: %d\n",
>> + midi->in_ep->name, err);
>> + req->length = 0; /* Re-use request next time. */
>> + } else {
>> + /* Upon success, put request at the back of the
>> queue. */
>> + kfifo_skip(&midi->in_req_fifo);
>> + kfifo_put(&midi->in_req_fifo, req);
>
> There are simpler and clearer ways to implement cyclic buffer than using
> kfifo. To be honest, it took ma a while to realize what's going on. As
> long as you mark unused requests by setting req->length to zero you only
> need to store index of last used req to be able to achieve the same effect.
That is true but that's exactly what I am trying to avoid here. I didn't
want to add a counter and deal with counter++ and counter-- all the
time. kfifo is fast and has a clean and nice interface to deal with that.
I can write a comment right next to it just to make things clearer.
>
>> + }
>> + }
>> + } while (active);
>> +
>> +drop_out:
>> + f_midi_drop_out_substreams(midi);
>> }
>>
>> static void f_midi_in_tasklet(unsigned long data)
>> {
>> struct f_midi *midi = (struct f_midi *) data;
>> - f_midi_transmit(midi, NULL);
>> + f_midi_transmit(midi);
>> }
>>
>> static int f_midi_in_open(struct snd_rawmidi_substream *substream)
>> @@ -663,6 +753,7 @@ static int f_midi_register_card(struct f_midi *midi)
>> goto fail;
>> }
>> midi->rmidi = rmidi;
>> + midi->in_last_port = 0;
>> strcpy(rmidi->name, card->shortname);
>> rmidi->info_flags = SNDRV_RAWMIDI_INFO_OUTPUT |
>> SNDRV_RAWMIDI_INFO_INPUT |
>> @@ -1057,6 +1148,7 @@ static void f_midi_free(struct usb_function *f)
>> mutex_lock(&opts->lock);
>> for (i = opts->in_ports - 1; i >= 0; --i)
>> kfree(midi->in_port[i]);
>> + kfifo_free(&midi->in_req_fifo);
>> kfree(midi);
>> --opts->refcnt;
>> mutex_unlock(&opts->lock);
>> @@ -1130,6 +1222,12 @@ static struct usb_function *f_midi_alloc(struct
>> usb_function_instance *fi)
>> midi->index = opts->index;
>> midi->buflen = opts->buflen;
>> midi->qlen = opts->qlen;
>> + midi->in_req_num = 0;
>> + midi->in_last_port = 0;
>> +
>> + if (kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL))
>> + goto setup_fail;
>> +
>> ++opts->refcnt;
>> mutex_unlock(&opts->lock);
>>
>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>> b/drivers/usb/gadget/legacy/gmidi.c
>> index be8e91d..f68c188 100644
>> --- a/drivers/usb/gadget/legacy/gmidi.c
>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>> @@ -53,7 +53,7 @@ MODULE_PARM_DESC(buflen, "MIDI buffer length");
>>
>> static unsigned int qlen = 32;
>> module_param(qlen, uint, S_IRUGO);
>> -MODULE_PARM_DESC(qlen, "USB read request queue length");
>> +MODULE_PARM_DESC(qlen, "USB read and write request queue length");
>>
>> static unsigned int in_ports = 1;
>> module_param(in_ports, uint, S_IRUGO);
>>
>
> Best regards,
> Robert
>
Felipe
0x92698E6A.asc
Description: application/pgp-keys
