RE: [PATCH 07/14] ASoC: amd: add acp3x pdm driver dma ops

From: "Mukunda, Vijendar" <Vijendar.Mukunda@amd.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	Alex Deucher <alexdeucher@gmail.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"tiwai@suse.de" <tiwai@suse.de>
Cc: "Deucher, Alexander" <Alexander.Deucher@amd.com>
Subject: RE: [PATCH 07/14] ASoC: amd: add acp3x pdm driver dma ops
Date: Wed, 6 May 2020 17:30:08 +0000	[thread overview]
Message-ID: <DM6PR12MB26333FCDC930138DC47CC20397A40@DM6PR12MB2633.namprd12.prod.outlook.com> (raw)
In-Reply-To: <fa130cbb-412d-768e-db71-15463fa58ab1@linux.intel.com>

[AMD Official Use Only - Internal Distribution Only]


> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Wednesday, May 6, 2020 3:29 AM
> To: Alex Deucher <alexdeucher@gmail.com>; alsa-devel@alsa-project.org;
> broonie@kernel.org; Mukunda, Vijendar <Vijendar.Mukunda@amd.com>;
> tiwai@suse.de
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH 07/14] ASoC: amd: add acp3x pdm driver dma ops
> 
> 
> 
> On 5/5/20 3:53 PM, Alex Deucher wrote:
> > From: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> >
> > This patch adds PDM driver DMA operations.
> >
> > Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >   sound/soc/amd/renoir/acp3x-pdm-dma.c | 199
> +++++++++++++++++++++++++++
> >   sound/soc/amd/renoir/rn_acp3x.h      |  29 ++++
> >   2 files changed, 228 insertions(+)
> >
> > diff --git a/sound/soc/amd/renoir/acp3x-pdm-dma.c
> b/sound/soc/amd/renoir/acp3x-pdm-dma.c
> > index 4ee47a85e37e..0b5dc49f42c3 100644
> > --- a/sound/soc/amd/renoir/acp3x-pdm-dma.c
> > +++ b/sound/soc/amd/renoir/acp3x-pdm-dma.c
> > @@ -16,6 +16,25 @@
> >
> >   #define DRV_NAME "acp_rn_pdm_dma"
> >
> > +static const struct snd_pcm_hardware acp_pdm_hardware_capture = {
> > +	.info = SNDRV_PCM_INFO_INTERLEAVED |
> > +		SNDRV_PCM_INFO_BLOCK_TRANSFER |
> > +		SNDRV_PCM_INFO_BATCH | SNDRV_PCM_INFO_MMAP |
> > +		SNDRV_PCM_INFO_MMAP_VALID |
> > +	    SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
> 
> Can you actually resume from the same position? this seems odd when
> combined with INFO_BATCH which means the position is only precise at
> period boundaries.
> 
We used similar flag in Raven APU acp dma driver well.
As per my understanding INFO_BATCH is more about providing period granularity when hw_ptr is queried.
But PDM driver DMA pointer callback returns precise hw_ptr when queried.
Correct me, if understanding is wrong.


> > +	.formats = 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 = CAPTURE_MAX_NUM_PERIODS *
> CAPTURE_MAX_PERIOD_SIZE,
> > +	.period_bytes_min = CAPTURE_MIN_PERIOD_SIZE,
> > +	.period_bytes_max = CAPTURE_MAX_PERIOD_SIZE,
> > +	.periods_min = CAPTURE_MIN_NUM_PERIODS,
> > +	.periods_max = CAPTURE_MAX_NUM_PERIODS,
> > +};
> > +
> 
> [...]
> 
> > +static snd_pcm_uframes_t acp_pdm_dma_pointer(struct
> snd_soc_component *comp,
> > +					     struct snd_pcm_substream
> *stream)
> > +{
> > +	struct pdm_stream_instance *rtd;
> > +	u32 pos, buffersize;
> > +	u64 bytescount;
> > +
> > +	rtd = stream->runtime->private_data;
> > +	pos = 0;
> > +	buffersize = 0;
> > +	bytescount = 0;
> 
> these 3 inits seem unnecessary?

Will remove it.
> > +
> > +	buffersize = frames_to_bytes(stream->runtime,
> > +				     stream->runtime->buffer_size);
> > +	bytescount = acp_pdm_get_byte_count(rtd, stream->stream);
> > +	if (bytescount > rtd->bytescount)
> > +		bytescount -= rtd->bytescount;
> > +	pos = do_div(bytescount, buffersize);
> > +	return bytes_to_frames(stream->runtime, pos);
> > +}
> > +