[alsa-devel] [PATCH 1/3] ALSA: compress_core: Update calc_avail to use cumulative values
The app_pointer is managed locally by the compress core for memory mapped DSPs but for DSPs that are not memory mapped this would have to be manually updated from within the DSP driver itself, which is hardly very idiomatic.
This patch switches to using the cumulative values to calculate the available buffer space because these are already gracefully passed out of the DSP driver to the compress core and otherwise should be functionally equivalent.
Signed-off-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com --- sound/core/compress_offload.c | 23 +++++------------------ 1 files changed, 5 insertions(+), 18 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index c84abc8..27bd81a 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -160,8 +160,6 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream, static size_t snd_compr_calc_avail(struct snd_compr_stream *stream, struct snd_compr_avail *avail) { - long avail_calc; /*this needs to be signed variable */ - memset(avail, 0, sizeof(*avail)); snd_compr_update_tstamp(stream, &avail->tstamp); /* Still need to return avail even if tstamp can't be filled in */ @@ -184,22 +182,11 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream, return stream->runtime->buffer_size; }
- /* FIXME: this routine isn't consistent, in one test we use - * cumulative values and in the other byte offsets. Do we - * really need the byte offsets if the cumulative values have - * been updated? In the PCM interface app_ptr and hw_ptr are - * already cumulative */ - - avail_calc = stream->runtime->buffer_size - - (stream->runtime->app_pointer - stream->runtime->hw_pointer); - pr_debug("calc avail as %ld, app_ptr %lld, hw+ptr %lld\n", avail_calc, - stream->runtime->app_pointer, - stream->runtime->hw_pointer); - if (avail_calc >= stream->runtime->buffer_size) - avail_calc -= stream->runtime->buffer_size; - pr_debug("ret avail as %ld\n", avail_calc); - avail->avail = avail_calc; - return avail_calc; + avail->avail = stream->runtime->buffer_size - + (stream->runtime->total_bytes_available - + stream->runtime->total_bytes_transferred); + pr_debug("ret avail as %lld\n", avail->avail); + return avail->avail; }
static inline size_t snd_compr_get_avail(struct snd_compr_stream *stream)
The compress API for non-memory mapped DSPs shares a copy callback for both read and write, however the file operation of write passes a const buffer. Thus we can't maintain const correctness for the copy callback and support both read and write.
This patch seperates the read and write callbacks in the compressed API.
Signed-off-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com --- include/sound/compress_driver.h | 8 ++++++-- sound/core/compress_offload.c | 6 +++--- 2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h index ff6c741..4c64dd1 100644 --- a/include/sound/compress_driver.h +++ b/include/sound/compress_driver.h @@ -99,7 +99,9 @@ struct snd_compr_stream { * @trigger: Trigger operations like start, pause, resume, drain, stop. * This callback is mandatory * @pointer: Retrieve current h/w pointer information. Mandatory - * @copy: Copy the compressed data to/from userspace, Optional + * @write: Copy the compressed data from userspace, Optional + * Can't be implemented if DSP supports mmap + * @read: Copy the compressed data to userspace, Optional * Can't be implemented if DSP supports mmap * @mmap: DSP mmap method to mmap DSP memory * @ack: Ack for DSP when data is written to audio buffer, Optional @@ -121,7 +123,9 @@ struct snd_compr_ops { int (*trigger)(struct snd_compr_stream *stream, int cmd); int (*pointer)(struct snd_compr_stream *stream, struct snd_compr_tstamp *tstamp); - int (*copy)(struct snd_compr_stream *stream, const char __user *buf, + int (*write)(struct snd_compr_stream *stream, const char __user *buf, + size_t count); + int (*read)(struct snd_compr_stream *stream, char __user *buf, size_t count); int (*mmap)(struct snd_compr_stream *stream, struct vm_area_struct *vma); diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 27bd81a..78c6f97 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -265,8 +265,8 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf, if (avail > count) avail = count;
- if (stream->ops->copy) - retval = stream->ops->copy(stream, buf, avail); + if (stream->ops->write) + retval = stream->ops->write(stream, buf, avail); else retval = snd_compr_write_data(stream, buf, avail); if (retval > 0) @@ -403,7 +403,7 @@ static int snd_compr_allocate_buffer(struct snd_compr_stream *stream, void *buffer;
buffer_size = params->buffer.fragment_size * params->buffer.fragments; - if (stream->ops->copy) { + if (stream->ops->write) { buffer = NULL; /* if copy is defined the driver will be required to copy * the data from core
On 04/03/2013 07:13 AM, Charles Keepax wrote:
The compress API for non-memory mapped DSPs shares a copy callback for both read and write, however the file operation of write passes a const buffer. Thus we can't maintain const correctness for the copy callback and support both read and write.
This patch seperates the read and write callbacks in the compressed API.
Why not remove the const to make this consistent with the PCM interface? -Pierre
On Thu, Apr 04, 2013 at 01:01:57PM -0500, Pierre-Louis Bossart wrote:
Why not remove the const to make this consistent with the PCM interface?
I had been trying to avoid loosing the const from the write file operation, however I am happy to just loose this through casting instead. It is certainly a simpler change I will respin these to do so.
Charles
On Fri, Apr 05, 2013 at 09:23:45AM +0100, Charles Keepax wrote:
On Thu, Apr 04, 2013 at 01:01:57PM -0500, Pierre-Louis Bossart wrote:
Why not remove the const to make this consistent with the PCM interface?
I had been trying to avoid loosing the const from the write file operation, however I am happy to just loose this through casting instead. It is certainly a simpler change I will respin these to do so.
Keeping const of write seems to be a better idea... Ah i cant decide :( I will asker wiser folks... Takashi, Mark?
-- ~Vinod
On Tue, Apr 09, 2013 at 04:45:07PM +0530, Vinod Koul wrote:
On Fri, Apr 05, 2013 at 09:23:45AM +0100, Charles Keepax wrote:
On Thu, Apr 04, 2013 at 01:01:57PM -0500, Pierre-Louis Bossart wrote:
Why not remove the const to make this consistent with the PCM interface?
I had been trying to avoid loosing the const from the write file operation, however I am happy to just loose this through casting instead. It is certainly a simpler change I will respin these to do so.
Keeping const of write seems to be a better idea... Ah i cant decide :( I will asker wiser folks... Takashi, Mark?
I generally prefer the const correctness, but on the other hand Pierre is right about the PCM interface and it's probably more trouble than it's worth to change that. So meh, dunno :/
At Tue, 9 Apr 2013 12:47:28 +0100, Mark Brown wrote:
[1 <text/plain; us-ascii (7bit)>] On Tue, Apr 09, 2013 at 04:45:07PM +0530, Vinod Koul wrote:
On Fri, Apr 05, 2013 at 09:23:45AM +0100, Charles Keepax wrote:
On Thu, Apr 04, 2013 at 01:01:57PM -0500, Pierre-Louis Bossart wrote:
Why not remove the const to make this consistent with the PCM interface?
I had been trying to avoid loosing the const from the write file operation, however I am happy to just loose this through casting instead. It is certainly a simpler change I will respin these to do so.
Keeping const of write seems to be a better idea... Ah i cant decide :( I will asker wiser folks... Takashi, Mark?
I generally prefer the const correctness, but on the other hand Pierre is right about the PCM interface and it's probably more trouble than it's worth to change that. So meh, dunno :/
I'm inclined to take an easier approach, just removing const, in such a case.
If we do split to read & write, it should be done for PCM as well. There are only handful users, so it shouldn't be too hard.
Takashi
The compress API for non-memory mapped DSPs shares a copy callback for both read and write, however the file operation of write passes a const buffer. Thus we can't maintain const correctness for the copy callback and support both read and write.
This patch seperates the read and write callbacks in the ASoC compressed API.
Signed-off-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com --- sound/soc/soc-compress.c | 34 +++++++++++++++++++++++++++------- 1 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index f9b2197..0148d82 100644 --- a/sound/soc/soc-compress.c +++ b/sound/soc/soc-compress.c @@ -306,8 +306,8 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream, return 0; }
-static int soc_compr_copy(struct snd_compr_stream *cstream, - const char __user *buf, size_t count) +static int soc_compr_write(struct snd_compr_stream *cstream, + const char __user *buf, size_t count) { struct snd_soc_pcm_runtime *rtd = cstream->private_data; struct snd_soc_platform *platform = rtd->platform; @@ -315,8 +315,24 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
- if (platform->driver->compr_ops && platform->driver->compr_ops->copy) - ret = platform->driver->compr_ops->copy(cstream, buf, count); + if (platform->driver->compr_ops && platform->driver->compr_ops->write) + ret = platform->driver->compr_ops->write(cstream, buf, count); + + mutex_unlock(&rtd->pcm_mutex); + return ret; +} + +static int soc_compr_read(struct snd_compr_stream *cstream, + char __user *buf, size_t count) +{ + struct snd_soc_pcm_runtime *rtd = cstream->private_data; + struct snd_soc_platform *platform = rtd->platform; + int ret = 0; + + mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass); + + if (platform->driver->compr_ops && platform->driver->compr_ops->read) + ret = platform->driver->compr_ops->read(cstream, buf, count);
mutex_unlock(&rtd->pcm_mutex); return ret; @@ -392,9 +408,13 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num) } memcpy(compr->ops, &soc_compr_ops, sizeof(soc_compr_ops));
- /* Add copy callback for not memory mapped DSPs */ - if (platform->driver->compr_ops && platform->driver->compr_ops->copy) - compr->ops->copy = soc_compr_copy; + /* Add write/read callback for not memory mapped DSPs */ + if (platform->driver->compr_ops) { + if (platform->driver->compr_ops->write) + compr->ops->write = soc_compr_write; + if (platform->driver->compr_ops->read) + compr->ops->read = soc_compr_read; + }
mutex_init(&compr->lock); ret = snd_compress_new(rtd->card->snd_card, num, direction, compr);
On Wed, Apr 03, 2013 at 01:13:29PM +0100, Charles Keepax wrote:
The compress API for non-memory mapped DSPs shares a copy callback for both read and write, however the file operation of write passes a const buffer. Thus we can't maintain const correctness for the copy callback and support both read and write.
This should be part of 2nd patch, you break bisect by not doing so...
This patch seperates the read and write callbacks in the ASoC compressed API.
Signed-off-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com
sound/soc/soc-compress.c | 34 +++++++++++++++++++++++++++------- 1 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index f9b2197..0148d82 100644 --- a/sound/soc/soc-compress.c +++ b/sound/soc/soc-compress.c @@ -306,8 +306,8 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream, return 0; }
-static int soc_compr_copy(struct snd_compr_stream *cstream,
const char __user *buf, size_t count)+static int soc_compr_write(struct snd_compr_stream *cstream,
const char __user *buf, size_t count){ struct snd_soc_pcm_runtime *rtd = cstream->private_data; struct snd_soc_platform *platform = rtd->platform; @@ -315,8 +315,24 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
- if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
ret = platform->driver->compr_ops->copy(cstream, buf, count);
- if (platform->driver->compr_ops && platform->driver->compr_ops->write)
ret = platform->driver->compr_ops->write(cstream, buf, count);- mutex_unlock(&rtd->pcm_mutex);
- return ret;
+}
+static int soc_compr_read(struct snd_compr_stream *cstream,
char __user *buf, size_t count)+{
struct snd_soc_pcm_runtime *rtd = cstream->private_data;
struct snd_soc_platform *platform = rtd->platform;
int ret = 0;
mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
if (platform->driver->compr_ops && platform->driver->compr_ops->read)
ret = platform->driver->compr_ops->read(cstream, buf, count);mutex_unlock(&rtd->pcm_mutex); return ret;
@@ -392,9 +408,13 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num) } memcpy(compr->ops, &soc_compr_ops, sizeof(soc_compr_ops));
- /* Add copy callback for not memory mapped DSPs */
- if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
compr->ops->copy = soc_compr_copy;
/* Add write/read callback for not memory mapped DSPs */
if (platform->driver->compr_ops) {
if (platform->driver->compr_ops->write)compr->ops->write = soc_compr_write;if (platform->driver->compr_ops->read)compr->ops->read = soc_compr_read;}
mutex_init(&compr->lock); ret = snd_compress_new(rtd->card->snd_card, num, direction, compr);
-- 1.7.2.5
On 04/03/2013 07:13 AM, Charles Keepax wrote:
The app_pointer is managed locally by the compress core for memory mapped DSPs but for DSPs that are not memory mapped this would have to be manually updated from within the DSP driver itself, which is hardly very idiomatic.
This patch switches to using the cumulative values to calculate the available buffer space because these are already gracefully passed out of the DSP driver to the compress core and otherwise should be functionally equivalent.
This isn't very elegant. In your implementation you bypass app_ptr and hw_ptr to use cumulative values, for 'memory-mapped' DSPs we use app_ptr and hw_ptr everywhere else. This patch seems to make things more confused when they could be simpler without all these redundant fields? I am probably partly responsible for the introduction of these cumulative values, now I think the time has come to simplify things. -Pierre
On Thu, Apr 04, 2013 at 01:22:27PM -0500, Pierre-Louis Bossart wrote:
This isn't very elegant. In your implementation you bypass app_ptr and hw_ptr to use cumulative values, for 'memory-mapped' DSPs we use app_ptr and hw_ptr everywhere else. This patch seems to make things more confused when they could be simpler without all these redundant fields? I am probably partly responsible for the introduction of these cumulative values, now I think the time has come to simplify things.
I am not sure I agree this is less elegant it greatly simplifies the calculation of the available data for one, also half of the avail function was using them anyway. The cumulative values make less assumptions about the underlying representation (although admittedly it is rather unlikely this will be anything other than a ring buffer) and contain more information (ie. how much data has been transferred so far).
You say we use app_ptr and hw_ptr everywhere else but only in places relating to situations where compress_offload is managing the buffer (ie. memory mapped DSPs). They feel more like internal buffer state, where the cumulative values surely reflect the stream state better.
If anything if we were looking to simplify I would be inclined to keep the cumulative values?
On 4/5/13 3:36 AM, Charles Keepax wrote:
On Thu, Apr 04, 2013 at 01:22:27PM -0500, Pierre-Louis Bossart wrote:
This isn't very elegant. In your implementation you bypass app_ptr and hw_ptr to use cumulative values, for 'memory-mapped' DSPs we use app_ptr and hw_ptr everywhere else. This patch seems to make things more confused when they could be simpler without all these redundant fields? I am probably partly responsible for the introduction of these cumulative values, now I think the time has come to simplify things.
I am not sure I agree this is less elegant it greatly simplifies the calculation of the available data for one, also half of the avail function was using them anyway. The cumulative values make less assumptions about the underlying representation (although admittedly it is rather unlikely this will be anything other than a ring buffer) and contain more information (ie. how much data has been transferred so far).
You say we use app_ptr and hw_ptr everywhere else but only in places relating to situations where compress_offload is managing the buffer (ie. memory mapped DSPs). They feel more like internal buffer state, where the cumulative values surely reflect the stream state better.
If anything if we were looking to simplify I would be inclined to keep the cumulative values?
That is my proposal as well, app_ptr and hw_ptr are defined as offsets but can't really be used to make the difference between buffer full and buffer empty and won't work for your implementation. I believe in the pcm case only cumulative values are used in the core. Vinod, please chime in... -Pierre
On Fri, Apr 05, 2013 at 09:51:08AM -0500, Pierre-Louis Bossart wrote:
On 4/5/13 3:36 AM, Charles Keepax wrote:
If anything if we were looking to simplify I would be inclined to keep the cumulative values?
That is my proposal as well, app_ptr and hw_ptr are defined as offsets but can't really be used to make the difference between buffer full and buffer empty and won't work for your implementation. I believe in the pcm case only cumulative values are used in the core. Vinod, please chime in...
Ah ok, I misunderstood. I will start having a look at what this would take but wait for Vinod to give some feedback before I upstream a new version.
Thanks, Charles
On Fri, Apr 05, 2013 at 04:18:45PM +0100, Charles Keepax wrote:
On Fri, Apr 05, 2013 at 09:51:08AM -0500, Pierre-Louis Bossart wrote:
On 4/5/13 3:36 AM, Charles Keepax wrote:
If anything if we were looking to simplify I would be inclined to keep the cumulative values?
That is my proposal as well, app_ptr and hw_ptr are defined as offsets but can't really be used to make the difference between buffer full and buffer empty and won't work for your implementation. I believe in the pcm case only cumulative values are used in the core. Vinod, please chime in...
Ah ok, I misunderstood. I will start having a look at what this would take but wait for Vinod to give some feedback before I upstream a new version.
Okay, historical note :)
we can normally use offsets to work and do all the calculation in the core. But this check fails when app_ptr = hw_ptr. Here we dont know if the buffer is full or empty :) So we need to look at cummulative values for those checks, hence the core maintains both of these and IIRC looks at cummulative values when calucating avail only when we hit the above condition or probably in case when we wrap over the ring buffer.
In both memory mapped and non mapped these cases are true so I dont see why this caluclation is specfic for memory maped DSPs. There is nothing in this case which relies. We only use the values reported by DSP and internal counters...
-- ~Vinod
we can normally use offsets to work and do all the calculation in the core. But this check fails when app_ptr = hw_ptr. Here we dont know if the buffer is full or empty :) So we need to look at cummulative values for those checks, hence the core maintains both of these and IIRC looks at cummulative values when calucating avail only when we hit the above condition or probably in case when we wrap over the ring buffer.
In both memory mapped and non mapped these cases are true so I dont see why this caluclation is specfic for memory maped DSPs. There is nothing in this case which relies. We only use the values reported by DSP and internal counters...
the question was whether we need these offsets at all. Can't we just use cumulative values everywhere? -Pierre
On Tue, Apr 09, 2013 at 08:59:48PM -0700, Pierre-Louis Bossart wrote:
the question was whether we need these offsets at all. Can't we just use cumulative values everywhere? -Pierre
Removing them does introduce a modulus operation which is not very nice given we are dealing with all 64-bit values. Although limiting the buffer size to 32-bit would make that slightly neater.
Aside from that the change is trivial to make and does match how the PCM interface is handled.
Charles
On Wed, Apr 03, 2013 at 01:13:27PM +0100, Charles Keepax wrote:
The app_pointer is managed locally by the compress core for memory mapped DSPs but for DSPs that are not memory mapped this would have to be manually updated from within the DSP driver itself, which is hardly very idiomatic.
app pointer means what has been written into DSP buffer, no matter if its memory mapped or not. This is updated after .copy returns so if you have been asked to copy 100bytes, and return of .copy, app pointer is incremented by 100, so why is that not true for you?
This patch switches to using the cumulative values to calculate the available buffer space because these are already gracefully passed out of the DSP driver to the compress core and otherwise should be functionally equivalent.
Signed-off-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com
sound/core/compress_offload.c | 23 +++++------------------ 1 files changed, 5 insertions(+), 18 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index c84abc8..27bd81a 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -160,8 +160,6 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream, static size_t snd_compr_calc_avail(struct snd_compr_stream *stream, struct snd_compr_avail *avail) {
- long avail_calc; /*this needs to be signed variable */
- memset(avail, 0, sizeof(*avail)); snd_compr_update_tstamp(stream, &avail->tstamp); /* Still need to return avail even if tstamp can't be filled in */
@@ -184,22 +182,11 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream, return stream->runtime->buffer_size; }
- /* FIXME: this routine isn't consistent, in one test we use
* cumulative values and in the other byte offsets. Do we* really need the byte offsets if the cumulative values have* been updated? In the PCM interface app_ptr and hw_ptr are* already cumulative */
What is this patch based off< certainly not asoc/sound-next as I dont remember this comment. IIRC comment was to test this for capture case!
- avail_calc = stream->runtime->buffer_size -
(stream->runtime->app_pointer - stream->runtime->hw_pointer);- pr_debug("calc avail as %ld, app_ptr %lld, hw+ptr %lld\n", avail_calc,
stream->runtime->app_pointer,stream->runtime->hw_pointer);- if (avail_calc >= stream->runtime->buffer_size)
avail_calc -= stream->runtime->buffer_size;- pr_debug("ret avail as %ld\n", avail_calc);
- avail->avail = avail_calc;
- return avail_calc;
- avail->avail = stream->runtime->buffer_size -
(stream->runtime->total_bytes_available -stream->runtime->total_bytes_transferred);- pr_debug("ret avail as %lld\n", avail->avail);
Neverthless, I agree that is simpler calculation...
- return avail->avail;
}
static inline size_t snd_compr_get_avail(struct snd_compr_stream *stream)
1.7.2.5
On Tue, Apr 09, 2013 at 04:32:30PM +0530, Vinod Koul wrote:
On Wed, Apr 03, 2013 at 01:13:27PM +0100, Charles Keepax wrote:
The app_pointer is managed locally by the compress core for memory mapped DSPs but for DSPs that are not memory mapped this would have to be manually updated from within the DSP driver itself, which is hardly very idiomatic.
app pointer means what has been written into DSP buffer, no matter if its memory mapped or not. This is updated after .copy returns so if you have been asked to copy 100bytes, and return of .copy, app pointer is incremented by 100, so why is that not true for you?
Currently though app_pointer is not updated after a copy callback the cumulative values are updated, but not app_pointer. app_pointer is only updated in the case were we don't have a copy callback and the buffer is managed by the compressed core.
I am quite happy to change that or remove this comment from the change log if we prefer but as it currently stands app_pointer will not be updated for systems that use a copy callback by the compress core.
Charles
On Tue, Apr 09, 2013 at 04:32:30PM +0530, Vinod Koul wrote:
What is this patch based off< certainly not asoc/sound-next as I dont remember this comment. IIRC comment was to test this for capture case!
This patch is based off Mark's asoc/for-next tree, apologies I guess I should have based this of Takashi's for-next tree. Although I wouldn't have thought they would have differed here as there haven't been any updates recently appart from the meta-data stuff.
Charles
On Wed, Apr 10, 2013 at 11:07:57AM +0100, Charles Keepax wrote:
On Tue, Apr 09, 2013 at 04:32:30PM +0530, Vinod Koul wrote:
What is this patch based off< certainly not asoc/sound-next as I dont remember this comment. IIRC comment was to test this for capture case!
This patch is based off Mark's asoc/for-next tree, apologies I guess I should have based this of Takashi's for-next tree. Although I wouldn't have thought they would have differed here as there haven't been any updates recently appart from the meta-data stuff.
Thie FIXMEs appear to come from your initial commit, must've been dealt with since.
participants (5)
-
Charles Keepax -
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai -
Vinod Koul