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

[Mukunda,Vijendar]

On 15/06/23 21:20, Mukunda,Vijendar wrote:
> On 13/06/23 12:30, Mukunda,Vijendar wrote:
>> 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.
    @Bossart: Let us know if you have any further comments for our replies.
>>> .
>>>> +	}
>>>> +	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;
>>>> +}