Re: [PATCH V4 4/9] ASoC: amd: ps: add SoundWire dma driver dma ops

[Mukunda,Vijendar]

On 12/06/23 23:36, Pierre-Louis Bossart wrote:
>> +#define SDW_PLAYBACK_MIN_NUM_PERIODS    2
>> +#define SDW_PLAYBACK_MAX_NUM_PERIODS    8
>> +#define SDW_PLAYBACK_MAX_PERIOD_SIZE    8192
> that's a fairly small max period. That's 21ms for 2ch S32_LE 48kHz.
>
> Does this come from specific limitations or is this an arbitrary number?
>
> A comment on this wouldn't hurt.
This is the initial version. We haven't exercised different sample
rates/bit depth combinations much. Currently, targeted for 2Ch, 48Khz,
16bit audio streams only with 64k as buffer size.

We will extend support for different sample rates/bit depths combinations
in the future.
>> +static u32 sdw0_dma_enable_reg[ACP63_SDW0_DMA_MAX_STREAMS] = {
>> +	ACP_SW0_AUDIO0_TX_EN,
>> +	ACP_SW0_AUDIO1_TX_EN,
>> +	ACP_SW0_AUDIO2_TX_EN,
>> +	ACP_SW0_AUDIO0_RX_EN,
>> +	ACP_SW0_AUDIO1_RX_EN,
>> +	ACP_SW0_AUDIO2_RX_EN,
>> +};
>> +
>> +static u32 sdw1_dma_enable_reg[ACP63_SDW1_DMA_MAX_STREAMS] = {
>> +	ACP_SW1_AUDIO1_TX_EN,
>> +	ACP_SW1_AUDIO1_RX_EN,
>> +};
> Still no explanation as to why SDW0 indices start at zero and SDW1
> indices start at one?
We have already provided reply in previous thread, i.e. for v3 patch set.

https://lore.kernel.org/alsa-devel/de3c86cc-6cba-0cbd-0e04-43711b4c9bc2@xxxxxxx/




>
>> +
>> +static const struct snd_pcm_hardware acp63_sdw_hardware_playback = {
>> +	.info = SNDRV_PCM_INFO_INTERLEAVED |
>> +		SNDRV_PCM_INFO_BLOCK_TRANSFER |
>> +		SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
>> +		SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
>> +	.formats = SNDRV_PCM_FMTBIT_S16_LE |  SNDRV_PCM_FMTBIT_S8 |
>> +		   SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
>> +	.channels_min = 2,
>> +	.channels_max = 2,
>> +	.rates = SNDRV_PCM_RATE_48000,
>> +	.rate_min = 48000,
>> +	.rate_max = 48000,
>> +	.buffer_bytes_max = SDW_PLAYBACK_MAX_NUM_PERIODS * SDW_PLAYBACK_MAX_PERIOD_SIZE,
>> +	.period_bytes_min = SDW_PLAYBACK_MIN_PERIOD_SIZE,
>> +	.period_bytes_max = SDW_PLAYBACK_MAX_PERIOD_SIZE,
>> +	.periods_min = SDW_PLAYBACK_MIN_NUM_PERIODS,
>> +	.periods_max = SDW_PLAYBACK_MAX_NUM_PERIODS,
>> +};
>> +
>> +static const struct snd_pcm_hardware acp63_sdw_hardware_capture = {
>> +	.info = SNDRV_PCM_INFO_INTERLEAVED |
>> +		SNDRV_PCM_INFO_BLOCK_TRANSFER |
>> +		SNDRV_PCM_INFO_MMAP |
>> +		SNDRV_PCM_INFO_MMAP_VALID |
>> +		SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
>> +	.formats = SNDRV_PCM_FMTBIT_S16_LE |  SNDRV_PCM_FMTBIT_S8 |
>> +		   SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
>> +	.channels_min = 2,
>> +	.channels_max = 2,
>> +	.rates = SNDRV_PCM_RATE_48000,
>> +	.rate_min = 48000,
>> +	.rate_max = 48000,
>> +	.buffer_bytes_max = SDW_CAPTURE_MAX_NUM_PERIODS * SDW_CAPTURE_MAX_PERIOD_SIZE,
>> +	.period_bytes_min = SDW_CAPTURE_MIN_PERIOD_SIZE,
>> +	.period_bytes_max = SDW_CAPTURE_MAX_PERIOD_SIZE,
>> +	.periods_min = SDW_CAPTURE_MIN_NUM_PERIODS,
>> +	.periods_max = SDW_CAPTURE_MAX_NUM_PERIODS,
>> +};
>> +static int acp63_sdw_dma_open(struct snd_soc_component *component,
>> +			      struct snd_pcm_substream *substream)
>> +{
>> +	struct snd_pcm_runtime *runtime;
>> +	struct acp_sdw_dma_stream *stream;
>> +	struct snd_soc_dai *cpu_dai;
>> +	struct amd_sdw_manager *amd_manager;
>> +	struct snd_soc_pcm_runtime *prtd = substream->private_data;
>> +	int ret;
>> +
>> +	runtime = substream->runtime;
>> +	cpu_dai = asoc_rtd_to_cpu(prtd, 0);
>> +	amd_manager = snd_soc_dai_get_drvdata(cpu_dai);
>> +	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
>> +	if (!stream)
>> +		return -ENOMEM;
>> +
>> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +		runtime->hw = acp63_sdw_hardware_playback;
>> +	else
>> +		runtime->hw = acp63_sdw_hardware_capture;
>> +	ret = snd_pcm_hw_constraint_integer(runtime,
>> +					    SNDRV_PCM_HW_PARAM_PERIODS);
>> +	if (ret < 0) {
>> +		dev_err(component->dev, "set integer constraint failed\n");
>> +		kfree(stream);
>> +		return ret;
>> +	}
> it's not clear to me why you have to add this specific constraint, isn't
> this checked already with the sdw_hardware_playback information.
In above code, first we are assigning runtime->hw structures.
As per our understanding, we are not assigning any hw_constraints.

This snd_pcm_hw_constraint_integer(runtime,
SNDRV_PCM_HW_PARAM_PERIODS) constraint assures that the number
of periods is integer, hence the buffer size is aligned with the period size.

>
>> +static u64 acp63_sdw_get_byte_count(struct acp_sdw_dma_stream *stream, void __iomem *acp_base)
>> +{
>> +	union acp_sdw_dma_count byte_count;
>> +	u32 pos_low_reg, pos_high_reg;
>> +
>> +	byte_count.bytescount = 0;
>> +	switch (stream->instance) {
>> +	case ACP_SDW0:
>> +		pos_low_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>> +		pos_high_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>> +		break;
>> +	case ACP_SDW1:
>> +		pos_low_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>> +		pos_high_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>> +		break;
>> +	default:
>> +		return -EINVAL;
> returning -EINVAL as a u64 doesn't seem quite right to me?
Agreed. Default case needs to be corrected. In case of invalid
SDW instance, it should return default byte count which is zero
instead of returning -EINVAL.

We have identified similar fix has to be implemented in our other
dma driver as well.
https://elixir.bootlin.com/linux/v6.4-rc6/source/sound/soc/amd/acp/amd.h#L174

Will push a supplement patch to fix it at one go.
>
>> +	}
>> +	if (pos_low_reg) {
>> +		byte_count.bcount.high = readl(acp_base + pos_high_reg);
>> +		byte_count.bcount.low = readl(acp_base + pos_low_reg);
>> +	}
>> +	return byte_count.bytescount;
>> +}