[Sound-open-firmware] [PATCH] host: remove first_copy workaround for HDA DMA

[Lauda, Tomasz]

Keyon,

Have you verified if this patch works correctly?

I think that it can possible has some synchronization issues between 
Host DMA and GPDMA.

First host copy will be called after starting GPDMA, but before 
receiving first interrupt.

After removing first_copy condition you are incrementing Host DMA write 
pointer *before *GPDMA has transferred any data.

Host DMA can potentially overwrite first period of data before it will 
be flushed to the SSP.


On 15.05.2018 17:36, Keyon Jie wrote:
> Here remove the first_copy workaround(it was needed as HDA DMA is not
> available at the trigger start stage).
>
> This require the fix from host driver side:
> ASoC: SOF: hw-apl: start HDA DMA at stream_prepare() stage and remove
> stream_trigger()
>
> Signed-off-by: Keyon Jie <yang.jie at linux.intel.com>
> ---
> Test with:
> Mininow max rt5651 and APL GPMRB and CNL nocodec
> SOF master: 7c5dd8cfdf4f8ca1fa5cfdd7352b81cbfc8edb88
> SOF-Tool master: 86fe688a2b4f68a1ce87e0951686be12a00f1a3c
> https://github.com/plbossart/sound/tree/topic/sof-v4.14:
> 22c8b2b68186e6af83f21c302d8086559b5d2e23
>
> note: this need be merged with driver patch together:
> 'ASoC: SOF: start HDA DMA at hw_params() stage and remove stream_trigger()'
>
> ---
>   src/audio/host.c | 41 ++++++++++++-----------------------------
>   1 file changed, 12 insertions(+), 29 deletions(-)
>
> diff --git a/src/audio/host.c b/src/audio/host.c
> index 998c5ca..a0956b3 100644
> --- a/src/audio/host.c
> +++ b/src/audio/host.c
> @@ -84,9 +84,6 @@ struct host_data {
>   	uint32_t period_bytes;
>   	uint32_t period_count;
>   
> -#if defined CONFIG_DMA_GW
> -	uint32_t first_copy;
> -#endif
>   	/* stream info */
>   	struct sof_ipc_stream_posn posn; /* TODO: update this */
>   };
> @@ -477,12 +474,12 @@ static int host_trigger(struct comp_dev *dev, int cmd)
>   			goto out;
>   		}
>   
> -		/* preload first playback period for preloader task */
> +		/*
> +		 * host dma will copy the first period once it is started,
> +		 * automatically.
> +		 * Here update the pointers to reflect the real case.
> +		 */
>   		if (dev->params.direction == SOF_IPC_STREAM_PLAYBACK) {
> -			/*
> -			 * host dma will not start copy at this point yet,
> -			 * just produce empty period bytes for it.
> -			 */
>   			comp_update_buffer_produce(hd->dma_buffer,
>   						   hd->period_bytes);
>   		}
> @@ -736,10 +733,6 @@ static int host_prepare(struct comp_dev *dev)
>   	hd->split_remaining = 0;
>   	dev->position = 0;
>   
> -#if defined CONFIG_DMA_GW
> -	hd->first_copy = 1;
> -#endif
> -
>   	return 0;
>   }
>   
> @@ -863,19 +856,6 @@ static int host_copy(struct comp_dev *dev)
>   	if (dev->state != COMP_STATE_ACTIVE)
>   		return 0;
>   
> -#if defined CONFIG_DMA_GW
> -	if (dev->params.direction == SOF_IPC_STREAM_PLAYBACK &&
> -	    hd->first_copy) {
> -		/*
> -		 * host dma will not start copy at this point yet, just produce
> -		 * empty period bytes for it.
> -		 */
> -		comp_update_buffer_produce(hd->dma_buffer,
> -					   hd->period_bytes);
> -		hd->first_copy = 0;
> -		return 0;
> -	}
> -#endif
>   	local_elem = list_first_item(&hd->config.elem_list,
>   		struct dma_sg_elem, list);
>   
> @@ -896,14 +876,17 @@ static int host_copy(struct comp_dev *dev)
>   	}
>   
>   #if defined CONFIG_DMA_GW
> -
> -	/* update host pointers from last period */
> -	host_gw_dma_update(dev);
> -
>   	/* tell gateway to copy another period */
>   	ret = dma_copy(hd->dma, hd->chan, hd->period_bytes);
>   	if (ret < 0)
>   		goto out;
> +
> +	/*
> +	 * update host pointers for the new copied period.
> +	 * fixme: do we need wait and check to make sure
> +	 * the new copy is finished here?
> +	 */
> +	host_gw_dma_update(dev);
>   #else
>   	/* do DMA transfer */
>   	ret = dma_set_config(hd->dma, hd->chan, &hd->config);