From: Lars-Peter C. <la...@me...> - 2012-06-25 21:29:59
|
The header and packet struct are only used in the scope of this function and they are freed at the end of it. Also these structs are rather small, so they can safely be allocate on the stack. By doing so memory leaks on the error paths are avoided. Signed-off-by: Lars-Peter Clausen <la...@me...> --- libsigrok/hardware/fx2lafw/fx2lafw.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/libsigrok/hardware/fx2lafw/fx2lafw.c b/libsigrok/hardware/fx2lafw/fx2lafw.c index 76c5019..c3c9e2c 100644 --- a/libsigrok/hardware/fx2lafw/fx2lafw.c +++ b/libsigrok/hardware/fx2lafw/fx2lafw.c @@ -818,8 +818,8 @@ static void receive_transfer(struct libusb_transfer *transfer) static int hw_dev_acquisition_start(int dev_index, void *cb_data) { struct sr_dev_inst *sdi; - struct sr_datafeed_packet *packet; - struct sr_datafeed_header *header; + struct sr_datafeed_packet packet; + struct sr_datafeed_header header; struct sr_datafeed_meta_logic meta; struct context *ctx; struct libusb_transfer *transfer; @@ -833,16 +833,6 @@ static int hw_dev_acquisition_start(int dev_index, void *cb_data) ctx->session_dev_id = cb_data; ctx->num_samples = 0; - if (!(packet = g_try_malloc(sizeof(struct sr_datafeed_packet)))) { - sr_err("fx2lafw: %s: packet malloc failed.", __func__); - return SR_ERR_MALLOC; - } - - if (!(header = g_try_malloc(sizeof(struct sr_datafeed_header)))) { - sr_err("fx2lafw: %s: header malloc failed.", __func__); - return SR_ERR_MALLOC; - } - /* Start with 2K transfer, subsequently increased to 4K. */ size = 2048; for (i = 0; i < NUM_SIMUL_TRANSFERS; i++) { @@ -871,21 +861,18 @@ static int hw_dev_acquisition_start(int dev_index, void *cb_data) 40, receive_data, NULL); free(lupfd); /* NOT g_free()! */ - packet->type = SR_DF_HEADER; - packet->payload = header; - header->feed_version = 1; - gettimeofday(&header->starttime, NULL); - sr_session_send(cb_data, packet); + packet.type = SR_DF_HEADER; + packet.payload = &header; + header.feed_version = 1; + gettimeofday(&header.starttime, NULL); + sr_session_send(cb_data, &packet); /* Send metadata about the SR_DF_LOGIC packets to come. */ - packet->type = SR_DF_META_LOGIC; - packet->payload = &meta; + packet.type = SR_DF_META_LOGIC; + packet.payload = &meta; meta.samplerate = ctx->cur_samplerate; meta.num_probes = ctx->sample_wide ? 16 : 8; - sr_session_send(cb_data, packet); - - g_free(header); - g_free(packet); + sr_session_send(cb_data, &packet); if ((ret = command_start_acquisition (ctx->usb->devhdl, ctx->cur_samplerate, ctx->sample_wide)) != SR_OK) { -- 1.7.10 |
From: Lars-Peter C. <la...@me...> - 2012-06-25 21:30:00
|
In receive_transfer for each completed transfer a new buffer is allocated and the old one is freed. We can avoid this by simply reusing the buffer for the next transfer. This is possible if we only resubmit the transfer after all processing on the data buffer has been done. A new buffer is only allocated if the size of the old one is not 4096 bytes. Signed-off-by: Lars-Peter Clausen <la...@me...> --- libsigrok/hardware/fx2lafw/fx2lafw.c | 47 +++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/libsigrok/hardware/fx2lafw/fx2lafw.c b/libsigrok/hardware/fx2lafw/fx2lafw.c index e001e27..0cfe436 100644 --- a/libsigrok/hardware/fx2lafw/fx2lafw.c +++ b/libsigrok/hardware/fx2lafw/fx2lafw.c @@ -684,6 +684,30 @@ static void free_transfer(struct libusb_transfer *transfer) } +static void resubmit_transfer(struct libusb_transfer *transfer) +{ + uint8_t *new_buf; + + /* Increase buffer size to 4096 */ + if (transfer->length != 4096) { + new_buf = g_try_malloc(4096); + /* If allocation of the new buffer fails, just do not bother and + * continue to use the old one. */ + if (new_bufer) { + g_free(transfer->buffer); + transfer->buffer = new_buf; + transfer->length = 4096; + } + } + + if (libusb_submit_transfer(transfer) != 0) { + free_transfer(transfer); + /* TODO: Stop session? */ + /* TODO: Better error message. */ + sr_err("fx2lafw: %s: libusb_submit_transfer error.", __func__); + } +} + static void receive_transfer(struct libusb_transfer *transfer) { /* TODO: These statics have to move to the ctx struct. */ @@ -692,7 +716,6 @@ static void receive_transfer(struct libusb_transfer *transfer) struct sr_datafeed_logic logic; struct context *ctx = transfer->user_data; int trigger_offset, i; - uint8_t *new_buf; /* * If acquisition has already ended, just free any queued up @@ -711,21 +734,6 @@ static void receive_transfer(struct libusb_transfer *transfer) const int sample_width = ctx->sample_wide ? 2 : 1; const int cur_sample_count = transfer->actual_length / sample_width; - /* Fire off a new request. */ - if (!(new_buf = g_try_malloc(4096))) { - sr_err("fx2lafw: %s: new_buf malloc failed.", __func__); - free_transfer(transfer); - return; /* TODO: SR_ERR_MALLOC */ - } - - transfer->buffer = new_buf; - transfer->length = 4096; - if (libusb_submit_transfer(transfer) != 0) { - /* TODO: Stop session? */ - /* TODO: Better error message. */ - sr_err("fx2lafw: %s: libusb_submit_transfer error.", __func__); - } - if (transfer->actual_length == 0) { empty_transfer_count++; if (empty_transfer_count > MAX_EMPTY_TRANSFERS) { @@ -734,8 +742,10 @@ static void receive_transfer(struct libusb_transfer *transfer) * will work out that the samplecount is short. */ abort_acquisition(ctx); + free_transfer(transfer); + } else { + resubmit_transfer(transfer); } - g_free(cur_buf); return; } else { empty_transfer_count = 0; @@ -820,7 +830,8 @@ static void receive_transfer(struct libusb_transfer *transfer) * ratio-sized buffer. */ } - g_free(cur_buf); + + resubmit_transfer(transfer); } static int hw_dev_acquisition_start(int dev_index, void *cb_data) -- 1.7.10 |
From: Lars-Peter C. <la...@me...> - 2012-06-25 21:30:01
|
While errors are usually already implicitly caught by looking at the packet length field there is one error status which is worth special handling. If the device has been removed there is not really a chance to recover from this error, so data acquisition can be stop immediately. Signed-off-by: Lars-Peter Clausen <la...@me...> --- libsigrok/hardware/fx2lafw/fx2lafw.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/libsigrok/hardware/fx2lafw/fx2lafw.c b/libsigrok/hardware/fx2lafw/fx2lafw.c index 0cfe436..b56f810 100644 --- a/libsigrok/hardware/fx2lafw/fx2lafw.c +++ b/libsigrok/hardware/fx2lafw/fx2lafw.c @@ -712,6 +712,7 @@ static void receive_transfer(struct libusb_transfer *transfer) { /* TODO: These statics have to move to the ctx struct. */ static int empty_transfer_count = 0; + gboolean packet_has_error = FALSE; struct sr_datafeed_packet packet; struct sr_datafeed_logic logic; struct context *ctx = transfer->user_data; @@ -734,7 +735,20 @@ static void receive_transfer(struct libusb_transfer *transfer) const int sample_width = ctx->sample_wide ? 2 : 1; const int cur_sample_count = transfer->actual_length / sample_width; - if (transfer->actual_length == 0) { + switch (transfer->status) { + case LIBUSB_TRANSFER_NO_DEVICE: + abort_acquisition(ctx); + free_transfer(transfer); + return; + case LIBUSB_TRANSFER_COMPLETED: + case LIBUSB_TRANSFER_TIMED_OUT: /* We may have received some data though */ + break; + default: + packet_has_error = TRUE; + break; + } + + if (transfer->actual_length == 0 || packet_has_error) { empty_transfer_count++; if (empty_transfer_count > MAX_EMPTY_TRANSFERS) { /* -- 1.7.10 |
From: Lars-Peter C. <la...@me...> - 2012-06-25 21:29:59
|
When freeing a transfer we have to free the transfer buffer as well. We also have to keep track of the number of allocated transfers and if the freed transfer was the last one stop acquisition. This patch introduces a helper function which takes care of all of this. Signed-off-by: Lars-Peter Clausen <la...@me...> --- libsigrok/hardware/fx2lafw/fx2lafw.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/libsigrok/hardware/fx2lafw/fx2lafw.c b/libsigrok/hardware/fx2lafw/fx2lafw.c index c3c9e2c..e001e27 100644 --- a/libsigrok/hardware/fx2lafw/fx2lafw.c +++ b/libsigrok/hardware/fx2lafw/fx2lafw.c @@ -670,6 +670,20 @@ static void finish_acquisition(struct context *ctx) free(lupfd); /* NOT g_free()! */ } +static void free_transfer(struct libusb_transfer *transfer) +{ + struct context *ctx = transfer->user_data; + + g_free(transfer->buffer); + transfer->buffer = NULL; + libusb_free_transfer(transfer); + + ctx->submitted_transfers--; + if (ctx->submitted_transfers == 0) + finish_acquisition(ctx); + +} + static void receive_transfer(struct libusb_transfer *transfer) { /* TODO: These statics have to move to the ctx struct. */ @@ -685,13 +699,7 @@ static void receive_transfer(struct libusb_transfer *transfer) * transfer that come in. */ if (ctx->num_samples == -1) { - if (transfer) - libusb_free_transfer(transfer); - - ctx->submitted_transfers--; - if (ctx->submitted_transfers == 0) - finish_acquisition(ctx); - + free_transfer(transfer); return; } @@ -706,7 +714,7 @@ static void receive_transfer(struct libusb_transfer *transfer) /* Fire off a new request. */ if (!(new_buf = g_try_malloc(4096))) { sr_err("fx2lafw: %s: new_buf malloc failed.", __func__); - libusb_free_transfer(transfer); + free_transfer(transfer); return; /* TODO: SR_ERR_MALLOC */ } -- 1.7.10 |
From: Lars-Peter C. <la...@me...> - 2012-06-25 21:30:01
|
Currently timeout and buffer size are hard-coded in the fx2lawfw driver which is non-optimal if we want to get good results at both high and low sample rates. The timeout is hard-coded to 40ms, which does not work well when sampling at with a low sample rate. E.g. at 20kHz filling all available buffer space alone takes 6 seconds. So naturally we'll see a lot of transfers timeout in this case. The buffer size is hard-coded to 4096 bytes, which does not work well with high sample rates. E.g. at 24MHz these 4096 bytes are enough space for 0.17 ms of data. The total buffer size is enough for about 5ms of data. Sooner or later the application wont be able to resubmit a transfer within this time span and the device will abort data acquisition. Usually this happens within the first few seconds of sampling. This patch adds a few new helper functions which calculate the buffer size and timeout based on the current sample rate. The buffer size is chosen to be large enough to hold about 10ms of data and it also must be a multiple of 512 bytes since the firmware will send us the data in 512 chunks. The timeout is set to the time it would take to fill the whole available buffer space plus a 25% headroom to accommodate for jitter. This more than enough, but there is no need to make the timeout a tight deadline, since it is only meant as a last resort in case the device stops submitting data. And in this case data acquisition will be aborted anyway. The patch also limits the the number of transfers so that the total buffer space is not much more of 500 ms, this will ensure that we do not have to wait too long when aborting data acquisition. This patch also significantly reduces the number of context switches when sampling at a higher sample rate. On my system for example the CPU load of sigrok-cli when sampling at 24Mhz goes down from ~25% to 3-4%. Signed-off-by: Lars-Peter Clausen <la...@me...> --- libsigrok/hardware/fx2lafw/fx2lafw.c | 67 +++++++++++++++++++++++----------- 1 file changed, 46 insertions(+), 21 deletions(-) diff --git a/libsigrok/hardware/fx2lafw/fx2lafw.c b/libsigrok/hardware/fx2lafw/fx2lafw.c index b56f810..40b9823 100644 --- a/libsigrok/hardware/fx2lafw/fx2lafw.c +++ b/libsigrok/hardware/fx2lafw/fx2lafw.c @@ -686,20 +686,6 @@ static void free_transfer(struct libusb_transfer *transfer) static void resubmit_transfer(struct libusb_transfer *transfer) { - uint8_t *new_buf; - - /* Increase buffer size to 4096 */ - if (transfer->length != 4096) { - new_buf = g_try_malloc(4096); - /* If allocation of the new buffer fails, just vdo not bother and - * continue to use the old one. */ - if (new_bufer) { - g_free(transfer->buffer); - transfer->buffer = new_buf; - transfer->length = 4096; - } - } - if (libusb_submit_transfer(transfer) != 0) { free_transfer(transfer); /* TODO: Stop session? */ @@ -848,6 +834,43 @@ static void receive_transfer(struct libusb_transfer *transfer) resubmit_transfer(transfer); } +static size_t get_buffer_size(struct context *ctx) +{ + size_t s; + + /* The buffer should be large enough to hold 10ms of data and a multiple + * of 512. */ + s = ctx->cur_samplerate / 100; + s = (s + 511) & ~511; + + return s; +} + +static unsigned int get_number_of_transfers(struct context *ctx) +{ + unsigned int n; + + /* Total buffer size should be able to hold about 500ms of data */ + n = ctx->cur_samplerate / get_buffer_size(ctx) / 2; + + if (n > NUM_SIMUL_TRANSFERS) + return NUM_SIMUL_TRANSFERS; + + return n; +} + +static unsigned int get_timeout(struct context *ctx) +{ + size_t total_size; + unsigned int timeout; + + total_size = get_buffer_size(ctx) * get_number_of_transfers(ctx); + timeout = total_size / (ctx->cur_samplerate / 1000); + timeout += timeout / 4; /* Leave a headroom of 25% percent */ + + return timeout; +} + static int hw_dev_acquisition_start(int dev_index, void *cb_data) { struct sr_dev_inst *sdi; @@ -857,7 +880,9 @@ static int hw_dev_acquisition_start(int dev_index, void *cb_data) struct context *ctx; struct libusb_transfer *transfer; const struct libusb_pollfd **lupfd; - int ret, size, i; + unsigned int num_transfers, i; + unsigned int timeout; + int ret, size; unsigned char *buf; if (!(sdi = sr_dev_inst_get(dev_insts, dev_index))) @@ -866,9 +891,10 @@ static int hw_dev_acquisition_start(int dev_index, void *cb_data) ctx->session_dev_id = cb_data; ctx->num_samples = 0; - /* Start with 2K transfer, subsequently increased to 4K. */ - size = 2048; - for (i = 0; i < NUM_SIMUL_TRANSFERS; i++) { + size = get_buffer_size(ctx); + num_transfers = get_number_of_transfers(ctx); + timeout = get_timeout(ctx); + for (i = 0; i < num_transfers; i++) { if (!(buf = g_try_malloc(size))) { sr_err("fx2lafw: %s: buf malloc failed.", __func__); return SR_ERR_MALLOC; @@ -876,7 +902,7 @@ static int hw_dev_acquisition_start(int dev_index, void *cb_data) transfer = libusb_alloc_transfer(0); libusb_fill_bulk_transfer(transfer, ctx->usb->devhdl, 2 | LIBUSB_ENDPOINT_IN, buf, size, - receive_transfer, ctx, 40); + receive_transfer, ctx, timeout); if (libusb_submit_transfer(transfer) != 0) { /* TODO: Free them all. */ libusb_free_transfer(transfer); @@ -885,13 +911,12 @@ static int hw_dev_acquisition_start(int dev_index, void *cb_data) } ctx->submitted_transfers++; - size = 4096; } lupfd = libusb_get_pollfds(usb_context); for (i = 0; lupfd[i]; i++) sr_source_add(lupfd[i]->fd, lupfd[i]->events, - 40, receive_data, NULL); + timeout, receive_data, NULL); free(lupfd); /* NOT g_free()! */ packet.type = SR_DF_HEADER; -- 1.7.10 |
From: Joel H. <jo...@ai...> - 2012-06-26 07:10:00
|
Hi Lars, Thanks for these. They look really good. Just a couple of minor points: > + n = ctx->cur_samplerate / get_buffer_size(ctx) / 2; > + timeout = total_size / (ctx->cur_samplerate / 1000); These lines, particularly the first one would be better with one division not 2. Also please use consts where you can: e.g. size_t s; unsigned int n; size_t total_size; unsigned int timeout; // This could be eliminated altogether - just return unsigned int num_transfers, ; unsigned int timeout; Constifying these removes a few empty lines of code, and ensure that these have a meaningful value throughout their lifetime. Also get_buffer_size(struct context *ctx) could be a one liner, or if you prefer a two liner just to make it clearer, but s doesn't need to be reassigned. I'd like to see these things fixed, but otherwise, great stuff. This patch was badly needed. Thanks Joel On 25/06/12 22:33, Lars-Peter Clausen wrote: > Currently timeout and buffer size are hard-coded in the fx2lawfw driver which is > non-optimal if we want to get good results at both high and low sample rates. > > The timeout is hard-coded to 40ms, which does not work well when sampling at with > a low sample rate. E.g. at 20kHz filling all available buffer space alone takes > 6 seconds. So naturally we'll see a lot of transfers timeout in this case. > > The buffer size is hard-coded to 4096 bytes, which does not work well with high > sample rates. E.g. at 24MHz these 4096 bytes are enough space for 0.17 ms of > data. The total buffer size is enough for about 5ms of data. Sooner or later the > application wont be able to resubmit a transfer within this time span and the > device will abort data acquisition. Usually this happens within the first few > seconds of sampling. > > This patch adds a few new helper functions which calculate the buffer size and > timeout based on the current sample rate. > > The buffer size is chosen to be large enough to hold about 10ms of data and it > also must be a multiple of 512 bytes since the firmware will send us the data in > 512 chunks. > > The timeout is set to the time it would take to fill the whole available buffer > space plus a 25% headroom to accommodate for jitter. This more than enough, but > there is no need to make the timeout a tight deadline, since it is only meant as > a last resort in case the device stops submitting data. And in this case data > acquisition will be aborted anyway. > > The patch also limits the the number of transfers so that the total buffer space > is not much more of 500 ms, this will ensure that we do not have to wait too > long when aborting data acquisition. > > This patch also significantly reduces the number of context switches when > sampling at a higher sample rate. On my system for example the CPU load of > sigrok-cli when sampling at 24Mhz goes down from ~25% to 3-4%. > > Signed-off-by: Lars-Peter Clausen<la...@me...> > --- > libsigrok/hardware/fx2lafw/fx2lafw.c | 67 +++++++++++++++++++++++----------- > 1 file changed, 46 insertions(+), 21 deletions(-) > > diff --git a/libsigrok/hardware/fx2lafw/fx2lafw.c b/libsigrok/hardware/fx2lafw/fx2lafw.c > index b56f810..40b9823 100644 > --- a/libsigrok/hardware/fx2lafw/fx2lafw.c > +++ b/libsigrok/hardware/fx2lafw/fx2lafw.c > @@ -686,20 +686,6 @@ static void free_transfer(struct libusb_transfer *transfer) > > static void resubmit_transfer(struct libusb_transfer *transfer) > { > - uint8_t *new_buf; > - > - /* Increase buffer size to 4096 */ > - if (transfer->length != 4096) { > - new_buf = g_try_malloc(4096); > - /* If allocation of the new buffer fails, just vdo not bother and > - * continue to use the old one. */ > - if (new_bufer) { > - g_free(transfer->buffer); > - transfer->buffer = new_buf; > - transfer->length = 4096; > - } > - } > - > if (libusb_submit_transfer(transfer) != 0) { > free_transfer(transfer); > /* TODO: Stop session? */ > @@ -848,6 +834,43 @@ static void receive_transfer(struct libusb_transfer *transfer) > resubmit_transfer(transfer); > } > > +static size_t get_buffer_size(struct context *ctx) > +{ > + size_t s; > + > + /* The buffer should be large enough to hold 10ms of data and a multiple > + * of 512. */ > + s = ctx->cur_samplerate / 100; > + s = (s + 511)& ~511; > + > + return s; > +} > + > +static unsigned int get_number_of_transfers(struct context *ctx) > +{ > + unsigned int n; > + > + /* Total buffer size should be able to hold about 500ms of data */ > + n = ctx->cur_samplerate / get_buffer_size(ctx) / 2; > + > + if (n> NUM_SIMUL_TRANSFERS) > + return NUM_SIMUL_TRANSFERS; > + > + return n; > +} > + > +static unsigned int get_timeout(struct context *ctx) > +{ > + size_t total_size; > + unsigned int timeout; > + > + total_size = get_buffer_size(ctx) * get_number_of_transfers(ctx); > + timeout = total_size / (ctx->cur_samplerate / 1000); > + timeout += timeout / 4; /* Leave a headroom of 25% percent */ > + > + return timeout; > +} > + > static int hw_dev_acquisition_start(int dev_index, void *cb_data) > { > struct sr_dev_inst *sdi; > @@ -857,7 +880,9 @@ static int hw_dev_acquisition_start(int dev_index, void *cb_data) > struct context *ctx; > struct libusb_transfer *transfer; > const struct libusb_pollfd **lupfd; > - int ret, size, i; > + unsigned int num_transfers, i; > + unsigned int timeout; > + int ret, size; > unsigned char *buf; > > if (!(sdi = sr_dev_inst_get(dev_insts, dev_index))) > @@ -866,9 +891,10 @@ static int hw_dev_acquisition_start(int dev_index, void *cb_data) > ctx->session_dev_id = cb_data; > ctx->num_samples = 0; > > - /* Start with 2K transfer, subsequently increased to 4K. */ > - size = 2048; > - for (i = 0; i< NUM_SIMUL_TRANSFERS; i++) { > + size = get_buffer_size(ctx); > + num_transfers = get_number_of_transfers(ctx); > + timeout = get_timeout(ctx); > + for (i = 0; i< num_transfers; i++) { > if (!(buf = g_try_malloc(size))) { > sr_err("fx2lafw: %s: buf malloc failed.", __func__); > return SR_ERR_MALLOC; > @@ -876,7 +902,7 @@ static int hw_dev_acquisition_start(int dev_index, void *cb_data) > transfer = libusb_alloc_transfer(0); > libusb_fill_bulk_transfer(transfer, ctx->usb->devhdl, > 2 | LIBUSB_ENDPOINT_IN, buf, size, > - receive_transfer, ctx, 40); > + receive_transfer, ctx, timeout); > if (libusb_submit_transfer(transfer) != 0) { > /* TODO: Free them all. */ > libusb_free_transfer(transfer); > @@ -885,13 +911,12 @@ static int hw_dev_acquisition_start(int dev_index, void *cb_data) > } > > ctx->submitted_transfers++; > - size = 4096; > } > > lupfd = libusb_get_pollfds(usb_context); > for (i = 0; lupfd[i]; i++) > sr_source_add(lupfd[i]->fd, lupfd[i]->events, > - 40, receive_data, NULL); > + timeout, receive_data, NULL); > free(lupfd); /* NOT g_free()! */ > > packet.type = SR_DF_HEADER; |
From: Lars-Peter C. <la...@me...> - 2012-06-26 19:16:46
|
On 06/26/2012 08:56 AM, Joel Holdsworth wrote: > Hi Lars, > > Thanks for these. They look really good. > > Just a couple of minor points: >> + n = ctx->cur_samplerate / get_buffer_size(ctx) / 2; >> + timeout = total_size / (ctx->cur_samplerate / 1000); > These lines, particularly the first one would be better with one > division not 2. > > Also please use consts where you can: e.g. > > size_t s; > unsigned int n; > size_t total_size; > unsigned int timeout; // This could be eliminated altogether - just > return > unsigned int num_transfers, ; > unsigned int timeout; > > Constifying these removes a few empty lines of code, and ensure that > these have a meaningful value throughout their lifetime. I'm not quite sure if I understood correctly what you want me to change, so I better ask before I resend. Do you mean like const unsigned int num_transfer = get_number_of_transfers(ctx); .... > > Also get_buffer_size(struct context *ctx) could be a one liner, or if > you prefer a two liner just to make it clearer, but s doesn't need to be > reassigned. I think it is more legible as it is right now, but this is just bikeshedding anyway, so I'll change it. > > I'd like to see these things fixed, but otherwise, great stuff. This > patch was badly needed. > Are you OK with patch 1-4? If yes I'd just resend patch 5 and not the whole series. Thanks, - Lars > Thanks > Joel > > > On 25/06/12 22:33, Lars-Peter Clausen wrote: >> Currently timeout and buffer size are hard-coded in the fx2lawfw >> driver which is >> non-optimal if we want to get good results at both high and low sample >> rates. >> >> The timeout is hard-coded to 40ms, which does not work well when >> sampling at with >> a low sample rate. E.g. at 20kHz filling all available buffer space >> alone takes >> 6 seconds. So naturally we'll see a lot of transfers timeout in this >> case. >> >> The buffer size is hard-coded to 4096 bytes, which does not work well >> with high >> sample rates. E.g. at 24MHz these 4096 bytes are enough space for 0.17 >> ms of >> data. The total buffer size is enough for about 5ms of data. Sooner or >> later the >> application wont be able to resubmit a transfer within this time span >> and the >> device will abort data acquisition. Usually this happens within the >> first few >> seconds of sampling. >> >> This patch adds a few new helper functions which calculate the buffer >> size and >> timeout based on the current sample rate. >> >> The buffer size is chosen to be large enough to hold about 10ms of >> data and it >> also must be a multiple of 512 bytes since the firmware will send us >> the data in >> 512 chunks. >> >> The timeout is set to the time it would take to fill the whole >> available buffer >> space plus a 25% headroom to accommodate for jitter. This more than >> enough, but >> there is no need to make the timeout a tight deadline, since it is >> only meant as >> a last resort in case the device stops submitting data. And in this >> case data >> acquisition will be aborted anyway. >> >> The patch also limits the the number of transfers so that the total >> buffer space >> is not much more of 500 ms, this will ensure that we do not have to >> wait too >> long when aborting data acquisition. >> >> This patch also significantly reduces the number of context switches when >> sampling at a higher sample rate. On my system for example the CPU >> load of >> sigrok-cli when sampling at 24Mhz goes down from ~25% to 3-4%. >> >> Signed-off-by: Lars-Peter Clausen<la...@me...> >> --- >> libsigrok/hardware/fx2lafw/fx2lafw.c | 67 >> +++++++++++++++++++++++----------- >> 1 file changed, 46 insertions(+), 21 deletions(-) >> >> diff --git a/libsigrok/hardware/fx2lafw/fx2lafw.c >> b/libsigrok/hardware/fx2lafw/fx2lafw.c >> index b56f810..40b9823 100644 >> --- a/libsigrok/hardware/fx2lafw/fx2lafw.c >> +++ b/libsigrok/hardware/fx2lafw/fx2lafw.c >> @@ -686,20 +686,6 @@ static void free_transfer(struct libusb_transfer >> *transfer) >> >> static void resubmit_transfer(struct libusb_transfer *transfer) >> { >> - uint8_t *new_buf; >> - >> - /* Increase buffer size to 4096 */ >> - if (transfer->length != 4096) { >> - new_buf = g_try_malloc(4096); >> - /* If allocation of the new buffer fails, just vdo not bother >> and >> - * continue to use the old one. */ >> - if (new_bufer) { >> - g_free(transfer->buffer); >> - transfer->buffer = new_buf; >> - transfer->length = 4096; >> - } >> - } >> - >> if (libusb_submit_transfer(transfer) != 0) { >> free_transfer(transfer); >> /* TODO: Stop session? */ >> @@ -848,6 +834,43 @@ static void receive_transfer(struct >> libusb_transfer *transfer) >> resubmit_transfer(transfer); >> } >> >> +static size_t get_buffer_size(struct context *ctx) >> +{ >> + size_t s; >> + >> + /* The buffer should be large enough to hold 10ms of data and a >> multiple >> + * of 512. */ >> + s = ctx->cur_samplerate / 100; >> + s = (s + 511)& ~511; >> + >> + return s; >> +} >> + >> +static unsigned int get_number_of_transfers(struct context *ctx) >> +{ >> + unsigned int n; >> + >> + /* Total buffer size should be able to hold about 500ms of data */ >> + n = ctx->cur_samplerate / get_buffer_size(ctx) / 2; >> + >> + if (n> NUM_SIMUL_TRANSFERS) >> + return NUM_SIMUL_TRANSFERS; >> + >> + return n; >> +} >> + >> +static unsigned int get_timeout(struct context *ctx) >> +{ >> + size_t total_size; >> + unsigned int timeout; >> + >> + total_size = get_buffer_size(ctx) * get_number_of_transfers(ctx); >> + timeout = total_size / (ctx->cur_samplerate / 1000); >> + timeout += timeout / 4; /* Leave a headroom of 25% percent */ >> + >> + return timeout; >> +} >> + >> static int hw_dev_acquisition_start(int dev_index, void *cb_data) >> { >> struct sr_dev_inst *sdi; >> @@ -857,7 +880,9 @@ static int hw_dev_acquisition_start(int dev_index, >> void *cb_data) >> struct context *ctx; >> struct libusb_transfer *transfer; >> const struct libusb_pollfd **lupfd; >> - int ret, size, i; >> + unsigned int num_transfers, i; >> + unsigned int timeout; >> + int ret, size; >> unsigned char *buf; >> >> if (!(sdi = sr_dev_inst_get(dev_insts, dev_index))) >> @@ -866,9 +891,10 @@ static int hw_dev_acquisition_start(int >> dev_index, void *cb_data) >> ctx->session_dev_id = cb_data; >> ctx->num_samples = 0; >> >> - /* Start with 2K transfer, subsequently increased to 4K. */ >> - size = 2048; >> - for (i = 0; i< NUM_SIMUL_TRANSFERS; i++) { >> + size = get_buffer_size(ctx); >> + num_transfers = get_number_of_transfers(ctx); >> + timeout = get_timeout(ctx); >> + for (i = 0; i< num_transfers; i++) { >> if (!(buf = g_try_malloc(size))) { >> sr_err("fx2lafw: %s: buf malloc failed.", __func__); >> return SR_ERR_MALLOC; >> @@ -876,7 +902,7 @@ static int hw_dev_acquisition_start(int dev_index, >> void *cb_data) >> transfer = libusb_alloc_transfer(0); >> libusb_fill_bulk_transfer(transfer, ctx->usb->devhdl, >> 2 | LIBUSB_ENDPOINT_IN, buf, size, >> - receive_transfer, ctx, 40); >> + receive_transfer, ctx, timeout); >> if (libusb_submit_transfer(transfer) != 0) { >> /* TODO: Free them all. */ >> libusb_free_transfer(transfer); >> @@ -885,13 +911,12 @@ static int hw_dev_acquisition_start(int >> dev_index, void *cb_data) >> } >> >> ctx->submitted_transfers++; >> - size = 4096; >> } >> >> lupfd = libusb_get_pollfds(usb_context); >> for (i = 0; lupfd[i]; i++) >> sr_source_add(lupfd[i]->fd, lupfd[i]->events, >> - 40, receive_data, NULL); >> + timeout, receive_data, NULL); >> free(lupfd); /* NOT g_free()! */ >> >> packet.type = SR_DF_HEADER; > |
From: Uwe H. <uw...@he...> - 2012-06-28 00:37:20
|
On Tue, Jun 26, 2012 at 08:59:32PM +0200, Lars-Peter Clausen wrote: > On 06/26/2012 08:56 AM, Joel Holdsworth wrote: > > Thanks for these. They look really good. Jup, awesome improvements! For archival purposes, we had some more discussions and tests on IRC, then merged the whole patchset. Thanks! Uwe. -- http://hermann-uwe.de | http://sigrok.org http://randomprojects.org | http://unmaintained-free-software.org |