[alsa-devel] [PATCH V5 3/3] ASoC: AMD: add AMD ASoC ACP-I2S driver

[maruthi srinivas]

On Fri, Aug 21, 2015 at 4:48 AM, Mark Brown <broonie at kernel.org> wrote:
>
> On Thu, Aug 20, 2015 at 05:36:34PM -0400, Alex Deucher wrote:
> > From: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu at amd.com>
> >
> > ACP IP block consists of dedicated DMA and I2S blocks. The PCM driver
> > provides the DMA and CPU DAI components to ALSA core.
>
> This is flagged as patch 3/3 but appears to be sent as a single patch.
> Please don't do this, the purpose of numbering patches is to help people
> tell which order they are in.  Numbering only makes sense when you send
> more than one patch, if you are sending a single patch there is no need
> to number.

Ok, sorry. I will be more careful next time. Actually, two patches are
sent - one for
DRM and other ASoC. It should have been represented as 2/2.

>
>
> > +++ b/sound/soc/amd/Kconfig
> > @@ -0,0 +1,5 @@
> > +config SND_SOC_AMD_ACP
> > +     tristate "AMD Audio Coprocessor support"
> > +     depends on MFD_CORE
>
> What is the dependency on the MFD core?

None. Sorry, I forgot to remove this.
Actually, AMD DRM driver depends on MFD_CORE to create a platform device
for ASoC. This is already present in DRM patch.

>
>
> > +/*
> > + * AMD ALSA SoC PCM Driver
> > + *
> > + * Copyright 2014-2015 Advanced Micro Devices, Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
>
> Still not happy with the licensing.

 I will contact our internal teams and get back on this.

>
> > +static const struct snd_soc_component_driver dw_i2s_component = {
> > +     .name = "dw-i2s",
> > +};
>
> We already have a driver for the DesignWare I2S controller.  To repeat
> the concern I raised in a slightly different bit of the code last time:
>
> | This doesn't appear to be a designware-i2s driver, we already have one
> | of those?
>

Our IP block includes few AMD IPs along with DesignWare I2S IP.  I have reused
code from Designware I2S controller from
sound/soc/dwc/designware_i2s.c and because
of the way IPs are coupled together, I couldn't use existing
Designware I2S driver as is.
I have given credit to the original author in DRM patch copyright
header, where register I2S read/writes
are made. Do I need to add the same header in ASoC driver too ?

>
> Like I say please stop ignoring review comments.  The hw_params()
> certainly seems to bear more than a passing resemblance.
>

Iam sorry,  this mistake won't be repeated.

>
> > +static void acp_pcm_period_elapsed(struct device *dev, u16 play_intr,
> > +                                                     u16 capture_intr)
> > +{
> > +     struct snd_pcm_substream *substream;
> > +     struct audio_drv_data *irq_data = dev_get_drvdata(dev);
> > +
> > +     /* Inform ALSA about the period elapsed (one out of two periods) */
> > +     if (play_intr)
> > +             substream = irq_data->play_stream;
> > +     else
> > +             substream = irq_data->capture_stream;
>
> So you've replaced the two if statements with an if/else which means
> subsstream is now guaranteed to be initialized but perhaps not in the
> way the caller intended...  I did find the caller (which got moved into
> another patch in this version) and it appears that the two u16s are
> actually used to pass boolean flags.  The whole interface here now
> seems odd, what is going on here?
>

Agreed. I will modify this interface to use a bool, in next revision

>
> > +     if (NULL != pg) {
>
> We still normally write these checks more like (pg != NULL) or even just
> (pg).
>

Ok, will modify accordingly in next revision.

>
> > +             /* Save for runtime private data */
> > +             rtd->pg = pg;
> > +             rtd->order = get_order(size);
> > +
> > +             /* Let ACP know the Allocated memory */
> > +             num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> > +
> > +             /* Fill the page table entries in ACP SRAM */
> > +             rtd->pg = pg;
> > +             rtd->size = size;
> > +             rtd->num_of_pages = num_of_pages;
> > +             rtd->direction = substream->stream;
> > +
> > +             acp_dev->config_dma(acp_dev, rtd);
> > +             status = 0;
> > +     } else {
> > +             status = -ENOMEM;
> > +     }
> > +     return status;
> > +}
> > +
> > +static int acp_dma_hw_free(struct snd_pcm_substream *substream)
> > +{
> > +     return snd_pcm_lib_free_pages(substream);
> > +}
>
> hw_free() doesn't seem to undo everything that we did on allocation?


Oh! I freed  in dma_close(), that were  allocated in dma_open(). Rest
of them  used devm_*
>
>
> > +             /* Now configure DMA to transfer only first half of System RAM
> > +              * buffer before playback is triggered. This will overwrite
> > +              * zero-ed second half of SRAM buffer */
> > +             acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM,
> > +                                     PLAYBACK_START_DMA_DESCR_CH12,
> > +                                     1, 0);
>
> | Why?  The comments describe what's happening but it's not clear why it's
> | happening.
>

|------------|----------|
|    A        |       B   |   DRAM
|------------|----------|
           \    /
            \  /
             \/  DMA
             /\
|---------/-|-\--------|
|    C   /   |  \    D  |   SRAM ---DMA ---------> I2S
|-----------|----------|

buffer in System memory(DRAM) is divided into 2 periods - say A,B.
internal memory
inside ACP IP (SRAM) is also divided in similar way - say C,D. In
hw_params()  A and B
are filled with zeros. In prepare() , content of A and B are DMA'ed to
D and C respectively
(as per DMA configuration).  When ALSA core fills A and B with audio content
(start threshold = A+B size), trigger() is called and content of A is
transferred to D.
C still holds zeros by now. The internal buffer (SRAM) which holds C+D
will be DMA'ed to
I2S in cyclic way starting from C.

At the end of C or D's DMA to I2S, irq is generated. In irq handler
DMA is done from B to C
or A to D depending on C or D' DMA completion respectively and
snd_pcm_period_elapsed()
is called.

The reason for doing this is :  When C completes rendering, calls
period_elapsed() and informs
ALSA core there is free space to fill new data to system memory. In
the same irq handler, B is
DMA'ed to C to be ready by the time D completes rendering with
prefetched data (in trigger()).
when D completes rendering, new data fetched by ALSA core can be
DMA'ed  from A to D and
rendering is continued with C. This is done cyclically.

If I don’t do A->D, B->C and instead do A->C, B->D,  there would be
timing problem to have new
data ready to be rendered. The alternate approach would be implement
'copy' callback of
'snd_pcm_ops' in pcm driver. In that callback, I can use
copy_from_user()  to get new data and
then only issue DMA transfers (A->C / B->D). This can solve the timing
issue mentioned, but then
I can't handle MMAP based playback/capture. Existing design can handle
non-mmap and mmap based
transfers, but the only issue(?) would be 1 period size of zeros will
be played initially for any new
playback usecase.

> > +static struct snd_soc_dai_driver i2s_dai_driver = {
> > +     .playback = {
> > +                  .stream_name = "I2S Playback",
> > +                  .channels_min = 2,
> > +                  .channels_max = 2,
>
> Elsewhere support for 8 channels was declared and handled.

The board for which driver is developed, doesn't support more than 2 channels.

>
> > +                  .rates = SNDRV_PCM_RATE_8000_96000,
> > +                  .formats = SNDRV_PCM_FMTBIT_S24_LE |
> > +                             SNDRV_PCM_FMTBIT_S32_LE,
>
> Elsewhere there was 16 bit support declared and handled.

There is a bug in our I2S IP, which wrongly handles 16 bit and lower
resolutions.
So, I removed the support. All the players using the driver, will
handle unsupported
formats with the help of pulseaudio conversions.

>
> > +     pm_rts = pm_runtime_status_suspended(dev);
> > +     if (pm_rts == true) {
>
> There seem to be a lot of variables in here that have only one
> assignment and one user immediately next to them.  I'm not sure it's
> adding much.
>

Ok, I will reuse  variables, where required and remove extra ones in
the next version.

> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>