[Sound-open-firmware] [PATCH] SRC: Fix use of int and uint types and remove unused function parameters
This patch fixes the warning messages shown with gcc option -Wextra. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> --- src/audio/src.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/audio/src.c b/src/audio/src.c index 3fd9614..3723b4e 100644 --- a/src/audio/src.c +++ b/src/audio/src.c @@ -65,8 +65,7 @@ struct comp_data { void (*src_func)(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, - uint32_t sink_frames); + int source_frames); }; /* Common mute function for 2s and 1s SRC. This preserves the same @@ -113,8 +112,7 @@ static void src_muted_s32(struct comp_buffer *source, struct comp_buffer *sink, static void fallback_s32(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, - uint32_t sink_frames) + int source_frames) { struct comp_data *cd = comp_get_drvdata(dev); @@ -129,7 +127,7 @@ static void fallback_s32(struct comp_dev *dev, /* Normal 2 stage SRC */ static void src_2s_s32_default(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, uint32_t sink_frames) + int source_frames) { int i; int j; @@ -203,7 +201,7 @@ static void src_2s_s32_default(struct comp_dev *dev, /* 1 stage SRC for simple conversions */ static void src_1s_s32_default(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, uint32_t sink_frames) + int source_frames) { int i; int j; @@ -440,7 +438,7 @@ static int src_params(struct comp_dev *dev) * be too long. */ q = need.blk_out / dev->frames; - if (q * dev->frames < need.blk_out) + if (q * (int)dev->frames < need.blk_out) ++q; if (q * dev->frames < need.blk_out + dev->frames) @@ -543,9 +541,9 @@ static int src_copy(struct comp_dev *dev) need_sink = blk_out * dev->frame_bytes; /* Run as many times as buffers allow */ - while ((source->avail >= need_source) && (sink->free >= need_sink)) { + while (((int)source->avail >= need_source) && ((int)sink->free >= need_sink)) { /* Run src */ - cd->src_func(dev, source, sink, blk_in, blk_out); + cd->src_func(dev, source, sink, blk_in); /* calc new free and available */ comp_update_buffer_consume(source, 0); -- 2.11.0
On Wed, 2017-09-27 at 16:49 +0300, Seppo Ingalsuo wrote:
This patch fixes the warning messages shown with gcc option -Wextra.
Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> --- src/audio/src.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/audio/src.c b/src/audio/src.c index 3fd9614..3723b4e 100644 --- a/src/audio/src.c +++ b/src/audio/src.c @@ -65,8 +65,7 @@ struct comp_data { void (*src_func)(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, - uint32_t sink_frames); + int source_frames);
Should this be unsigned ?
};
/* Common mute function for 2s and 1s SRC. This preserves the same @@ -113,8 +112,7 @@ static void src_muted_s32(struct comp_buffer *source, struct comp_buffer *sink, static void fallback_s32(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, - uint32_t sink_frames) + int source_frames)
ditto
{
struct comp_data *cd = comp_get_drvdata(dev); @@ -129,7 +127,7 @@ static void fallback_s32(struct comp_dev *dev, /* Normal 2 stage SRC */ static void src_2s_s32_default(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, uint32_t sink_frames) + int source_frames) { int i; int j; @@ -203,7 +201,7 @@ static void src_2s_s32_default(struct comp_dev *dev, /* 1 stage SRC for simple conversions */ static void src_1s_s32_default(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, uint32_t sink_frames) + int source_frames) { int i; int j; @@ -440,7 +438,7 @@ static int src_params(struct comp_dev *dev) * be too long. */ q = need.blk_out / dev->frames; - if (q * dev->frames < need.blk_out) + if (q * (int)dev->frames < need.blk_out)
probably wont need to cast if unsigned too.
++q;
if (q * dev->frames < need.blk_out + dev->frames) @@ -543,9 +541,9 @@ static int src_copy(struct comp_dev *dev) need_sink = blk_out * dev->frame_bytes;
/* Run as many times as buffers allow */ - while ((source->avail >= need_source) && (sink->free >= need_sink)) { + while (((int)source->avail >= need_source) && ((int)sink->free >= need_sink)) { /* Run src */ - cd->src_func(dev, source, sink, blk_in, blk_out); + cd->src_func(dev, source, sink, blk_in);
/* calc new free and available */ comp_update_buffer_consume(source, 0);
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On 27.09.2017 18:52, Liam Girdwood wrote:
On Wed, 2017-09-27 at 16:49 +0300, Seppo Ingalsuo wrote:
This patch fixes the warning messages shown with gcc option -Wextra.
Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> --- src/audio/src.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/audio/src.c b/src/audio/src.c index 3fd9614..3723b4e 100644 --- a/src/audio/src.c +++ b/src/audio/src.c @@ -65,8 +65,7 @@ struct comp_data { void (*src_func)(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, - uint32_t sink_frames); + int source_frames); Should this be unsigned ?
No, it's intentional. The frames count is used for +/- arithmetics ops in the code so it's better to keep it signed integer. With unsigned int type I would need more casts in the code. In this case the lower positive range of int32 is not an issue. Generally for DSPs the signed integer is natural type to use. Multiplications instructions are better available for signed integer type so why not prefer them instead. Cheers, Seppo
};
/* Common mute function for 2s and 1s SRC. This preserves the same @@ -113,8 +112,7 @@ static void src_muted_s32(struct comp_buffer *source, struct comp_buffer *sink, static void fallback_s32(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, - uint32_t sink_frames) + int source_frames) ditto {
struct comp_data *cd = comp_get_drvdata(dev); @@ -129,7 +127,7 @@ static void fallback_s32(struct comp_dev *dev, /* Normal 2 stage SRC */ static void src_2s_s32_default(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, uint32_t sink_frames) + int source_frames) { int i; int j; @@ -203,7 +201,7 @@ static void src_2s_s32_default(struct comp_dev *dev, /* 1 stage SRC for simple conversions */ static void src_1s_s32_default(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, uint32_t sink_frames) + int source_frames) { int i; int j; @@ -440,7 +438,7 @@ static int src_params(struct comp_dev *dev) * be too long. */ q = need.blk_out / dev->frames; - if (q * dev->frames < need.blk_out) + if (q * (int)dev->frames < need.blk_out) probably wont need to cast if unsigned too.
++q;
if (q * dev->frames < need.blk_out + dev->frames) @@ -543,9 +541,9 @@ static int src_copy(struct comp_dev *dev) need_sink = blk_out * dev->frame_bytes;
/* Run as many times as buffers allow */ - while ((source->avail >= need_source) && (sink->free >= need_sink)) { + while (((int)source->avail >= need_source) && ((int)sink->free >= need_sink)) { /* Run src */ - cd->src_func(dev, source, sink, blk_in, blk_out); + cd->src_func(dev, source, sink, blk_in);
/* calc new free and available */ comp_update_buffer_consume(source, 0);
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sorry, new patch version is needed, please ignore this submission. This patch contains a bug that makes this check to fail: /* Run as many times as buffers allow */ - while (((int)source->avail >= need_source) && ((int)sink->free
= need_sink)) { + while (((int)(source->avail) >= need_source) && ((int)(sink->free) >= need_sink)) {
I noticed this from flood of errors into trace. Thanks, Seppo On 27.09.2017 19:02, Seppo Ingalsuo wrote:
On 27.09.2017 18:52, Liam Girdwood wrote:
On Wed, 2017-09-27 at 16:49 +0300, Seppo Ingalsuo wrote:
This patch fixes the warning messages shown with gcc option -Wextra.
Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> --- src/audio/src.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/audio/src.c b/src/audio/src.c index 3fd9614..3723b4e 100644 --- a/src/audio/src.c +++ b/src/audio/src.c @@ -65,8 +65,7 @@ struct comp_data { void (*src_func)(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, - uint32_t sink_frames); + int source_frames); Should this be unsigned ?
No, it's intentional. The frames count is used for +/- arithmetics ops in the code so it's better to keep it signed integer. With unsigned int type I would need more casts in the code. In this case the lower positive range of int32 is not an issue.
Generally for DSPs the signed integer is natural type to use. Multiplications instructions are better available for signed integer type so why not prefer them instead.
Cheers, Seppo
}; /* Common mute function for 2s and 1s SRC. This preserves the same @@ -113,8 +112,7 @@ static void src_muted_s32(struct comp_buffer *source, struct comp_buffer *sink, static void fallback_s32(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, - uint32_t sink_frames) + int source_frames) ditto { struct comp_data *cd = comp_get_drvdata(dev); @@ -129,7 +127,7 @@ static void fallback_s32(struct comp_dev *dev, /* Normal 2 stage SRC */ static void src_2s_s32_default(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, uint32_t sink_frames) + int source_frames) { int i; int j; @@ -203,7 +201,7 @@ static void src_2s_s32_default(struct comp_dev *dev, /* 1 stage SRC for simple conversions */ static void src_1s_s32_default(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, uint32_t sink_frames) + int source_frames) { int i; int j; @@ -440,7 +438,7 @@ static int src_params(struct comp_dev *dev) * be too long. */ q = need.blk_out / dev->frames; - if (q * dev->frames < need.blk_out) + if (q * (int)dev->frames < need.blk_out) probably wont need to cast if unsigned too.
++q; if (q * dev->frames < need.blk_out + dev->frames) @@ -543,9 +541,9 @@ static int src_copy(struct comp_dev *dev) need_sink = blk_out * dev->frame_bytes; /* Run as many times as buffers allow */ - while ((source->avail >= need_source) && (sink->free >= need_sink)) { + while (((int)source->avail >= need_source) && ((int)sink->free
= need_sink)) { /* Run src */ - cd->src_func(dev, source, sink, blk_in, blk_out); + cd->src_func(dev, source, sink, blk_in); /* calc new free and available */ comp_update_buffer_consume(source, 0);
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
_______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Wed, 2017-09-27 at 19:02 +0300, Seppo Ingalsuo wrote:
On 27.09.2017 18:52, Liam Girdwood wrote:
On Wed, 2017-09-27 at 16:49 +0300, Seppo Ingalsuo wrote:
This patch fixes the warning messages shown with gcc option -Wextra.
Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> --- src/audio/src.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/audio/src.c b/src/audio/src.c index 3fd9614..3723b4e 100644 --- a/src/audio/src.c +++ b/src/audio/src.c @@ -65,8 +65,7 @@ struct comp_data { void (*src_func)(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, - uint32_t sink_frames); + int source_frames); Should this be unsigned ?
No, it's intentional. The frames count is used for +/- arithmetics ops in the code so it's better to keep it signed integer. With unsigned int type I would need more casts in the code. In this case the lower positive range of int32 is not an issue.
Ok, so it's better then to also fix the issue in component.h and buffer.h on any structure member that will be used in audio processing. I dont want to mix types or cast :) struct comp_buffer { /* runtime data */ uint32_t size; /* runtime buffer size in bytes (period multiple) */ uint32_t alloc_size; /* allocated size in bytes */ uint32_t avail; /* available bytes for reading */ uint32_t free; /* free bytes for writing */ }; These 4 uints are all used in processing and from struct comp_dev :- uint32_t frames; /* number of frames we copy to sink */ uint32_t frame_bytes; /* frames size copied to sink in bytes */ Also some members in sof_ipc_comp_* from ipc.h too. Liam --------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On 29.09.2017 01:05, Liam Girdwood wrote:
On Wed, 2017-09-27 at 19:02 +0300, Seppo Ingalsuo wrote:
On Wed, 2017-09-27 at 16:49 +0300, Seppo Ingalsuo wrote:
This patch fixes the warning messages shown with gcc option -Wextra.
Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> --- src/audio/src.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/audio/src.c b/src/audio/src.c index 3fd9614..3723b4e 100644 --- a/src/audio/src.c +++ b/src/audio/src.c @@ -65,8 +65,7 @@ struct comp_data { void (*src_func)(struct comp_dev *dev, struct comp_buffer *source, struct comp_buffer *sink, - uint32_t source_frames, - uint32_t sink_frames); + int source_frames); Should this be unsigned ? No, it's intentional. The frames count is used for +/- arithmetics ops in the code so it's better to keep it signed integer. With unsigned int type I would need more casts in the code. In this case the lower
On 27.09.2017 18:52, Liam Girdwood wrote: positive range of int32 is not an issue. Ok, so it's better then to also fix the issue in component.h and buffer.h on any structure member that will be used in audio processing. I dont want to mix types or cast :)
struct comp_buffer {
/* runtime data */
uint32_t size; /* runtime buffer size in bytes (period multiple) */ uint32_t alloc_size; /* allocated size in bytes */ uint32_t avail; /* available bytes for reading */ uint32_t free; /* free bytes for writing */
};
These 4 uints are all used in processing and from struct comp_dev :-
uint32_t frames; /* number of frames we copy to sink */ uint32_t frame_bytes; /* frames size copied to sink in bytes */
Also some members in sof_ipc_comp_* from ipc.h too.
Thanks, I'll see how large impact this would cause. Cheers, Seppo
Liam
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (2)
-
Liam Girdwood -
Seppo Ingalsuo