On Fri, Aug 21, 2015 at 4:48 AM, Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> On Thu, Aug 20, 2015 at 05:36:34PM -0400, Alex Deucher wrote:
> > From: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu@xxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxx
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
