Re: [Sound-open-firmware] [PATCH v3 05/14] ASoC: SOF: Add PCM operations support

Pierre-Louis Bossart
2018-12-12T15:29:23Z

On 12/12/18 2:04 AM, Takashi Iwai wrote:
> 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 :)
Yes, will fix.
>
>> + /* 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?
will double-check.
>
>> + /* 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?
Not sure, will double-check. Now that I read the code again this looks
odds, and if it was intentional then we need an explicit comment. Thanks
for the sighting.
>
>> + /* 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.
will double-check this entire sequence.
>
>
>> +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?
Not sure if I fully understand your point here. The trigger does indeed
need an IPC to proceed, but the front-ends are marked as such with the
.nonatomic field set to true. Not sure how different this is from
existing atom/sst or Skylake drivers.