[Sound-open-firmware] [PATCH] intel-ipc: fix host ring buffer size not page aligned issue

[Jie, Yang]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart at linux.intel.com]
>Sent: Tuesday, November 14, 2017 10:20 PM
>To: Keyon Jie <yang.jie at linux.intel.com>; sound-open-firmware at alsa-
>project.org; Girdwood, Liam R <liam.r.girdwood at intel.com>
>Cc: Jie, Yang <yang.jie at intel.com>
>Subject: Re: [Sound-open-firmware] [PATCH] intel-ipc: fix host ring buffer size not
>page aligned issue
>
>On 11/14/17 12:15 AM, Keyon Jie wrote:
>> The host ring buffer size may be not page aligned, but the last page
>> was ulitlized by host component
>
>utilized
>
>> wrongly, which may introduce beating noise.
>>
>> Here change to correct size for the last element, which will fix the
>> issue.
>
>That is plausible, I did notice an audible change when playing with the period size,
>but...
>
>>
>> Signed-off-by: Keyon Jie <yang.jie at linux.intel.com>
>> ---
>>   src/ipc/intel-ipc.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/src/ipc/intel-ipc.c b/src/ipc/intel-ipc.c index
>> 56bd8ba..2e503e1 100644
>> --- a/src/ipc/intel-ipc.c
>> +++ b/src/ipc/intel-ipc.c
>> @@ -192,6 +192,17 @@ static int parse_page_descriptors(struct intel_ipc_data
>*iipc,
>>   		else
>>   			elem.dest = phy_addr;
>>
>> +		if (elem.size * (i + 1) >= ring->size) {
>> +			/* the last page may be not full used */
>> +			if (i == (ring->pages - 1))
>> +				elem.size = ring->size - elem.size * i;
>
>...this code looks error-prone. should we invert the cases and test first if this is
>the last page? actually do you even need the first test?

Here I suppose the ring->size from host is suspect, e.g. it tells the ring->pages is
16, with the ring->size is only 3 pages.

But you are right, let me make this test more readable, and send v2.

Thanks,
~Keyon

>
>> +			else {
>> +				trace_ipc_error("eBs");
>> +				/* should not happen*/
>> +				return -EINVAL;
>> +			}
>> +		}
>> +
>>   		if (is_trace)
>>   			err = dma_trace_host_buffer(d, &elem, ring->size);
>>   		else
>>