Re: [PATCH v3 05/14] ASoC: SOF: Add PCM operations support

Takashi Iwai
2018-12-12T08:04:38Z

On Tue, 11 Dec 2018 22:23:09 +0100,
Pierre-Louis Bossart wrote:
>
> + /* number of pages should be rounded up */
> + if (runtime->dma_bytes % PAGE_SIZE)
> + pcm.params.buffer.pages = (runtime->dma_bytes / PAGE_SIZE) + 1;
> + else
> + pcm.params.buffer.pages = runtime->dma_bytes / PAGE_SIZE;

There is likely some nice macro for this :)

> + /* firmware already configured host stream */
> + ret = snd_sof_pcm_platform_hw_params(sdev,
> + substream,
> + params,
> + &pcm.params);
> + dev_dbg(sdev->dev, "stream_tag %d", pcm.params.stream_tag);

This error can be ignored?

> + /* send IPC to the DSP */
> + ret = sof_ipc_tx_message(sdev->ipc, pcm.hdr.cmd, &pcm, sizeof(pcm),
> + &ipc_params_reply, sizeof(ipc_params_reply));

This error value isn't evaluated immediately but passed until the
last. Is it intentional?

> + /* validate offset */
> + posn_offset = ipc_params_reply.posn_offset;
> +
> + /* check if offset is overflow or it is not aligned */
> + if (posn_offset > sdev->stream_box.size ||
> + posn_offset % sizeof(struct sof_ipc_stream_posn) != 0) {
> + dev_err(sdev->dev, "error: got wrong posn offset 0x%x for PCM %d\n",
> + posn_offset, ret);
> + return ret;

Here you return an error after checking more things.

> + }
> + spcm->posn_offset[substream->stream] =
> + sdev->stream_box.offset + posn_offset;
> +
> + /* save pcm hw_params */
> + memcpy(&spcm->params[substream->stream], params, sizeof(*params));
> +
> + return ret;

Even here returns an error after saving as if done properly.

> +static int sof_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
....
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +
> + /* check if the stream hw_params needs to be restored */
> + if (spcm->restore_stream[substream->stream]) {
> +
> + /* restore hw_params */
> + ret = sof_restore_hw_params(substream, spcm, sdev);

This calls the hw_params that is supposed to be non-atomic.

> + snd_sof_pcm_platform_trigger(sdev, substream, cmd);
> +
> + /* send IPC to the DSP */
> + ret = sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd, &stream,
> + sizeof(stream), &reply, sizeof(reply));
> +
> + return ret;

... so the whole trigger action is non-atomic PCM only?

thanks,

Takashi