Re: [alsa-devel] [PATCH 02/11] ASoC: amd: dma config parameters changes

Hi Vijendar,

On Thu, Apr 26, 2018 at 5:14 AM Vijendar Mukunda Vijendar.Mukunda@amd.com wrote:

Added dma configuration parameters to rtd structure. Moved dma configuration parameters intialization to hw_params callback. Removed hard coding in prepare and trigger callbacks.

Signed-off-by: Vijendar Mukunda Vijendar.Mukunda@amd.com

sound/soc/amd/acp-pcm-dma.c | 97

+++++++++++++++++----------------------------

sound/soc/amd/acp.h | 5 +++ 2 files changed, 41 insertions(+), 61 deletions(-)

diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c index 9c026c4..f18ed9a 100644 --- a/sound/soc/amd/acp-pcm-dma.c +++ b/sound/soc/amd/acp-pcm-dma.c @@ -321,19 +321,12 @@ static void config_acp_dma(void __iomem *acp_mmio, u32 asic_type) { u32 pte_offset, sram_bank;

•   u16 ch1, ch2, destination, dma_dscr_idx;
  

      if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {
              pte_offset = ACP_PLAYBACK_PTE_OFFSET;

•           ch1 = SYSRAM_TO_ACP_CH_NUM;
  

•           ch2 = ACP_TO_I2S_DMA_CH_NUM;
              sram_bank = ACP_SHARED_RAM_BANK_1_ADDRESS;
  

•           destination = TO_ACP_I2S_1;
  

•     } else {
              pte_offset = ACP_CAPTURE_PTE_OFFSET;
  

•           ch1 = SYSRAM_TO_ACP_CH_NUM;
  

•           ch2 = ACP_TO_I2S_DMA_CH_NUM;
  

Wait... since this is the capture stream, shouldn't the channels have been:

ch1 = ACP_TO_SYSRAM_CH_NUM; /* 14 */ ch2 = I2S_TO_ACP_DMA_CH_NUM; /* 15 */

Is this an existing bug? Why does everything still work if these are wrong?

              switch (asic_type) {
              case CHIP_STONEY:
                      sram_bank = ACP_SHARED_RAM_BANK_3_ADDRESS;

@@ -341,30 +334,19 @@ static void config_acp_dma(void __iomem *acp_mmio, default: sram_bank = ACP_SHARED_RAM_BANK_5_ADDRESS; }

•           destination = FROM_ACP_I2S_1;
      }
  

•     acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
                     pte_offset);
  

•   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
  

•           dma_dscr_idx = PLAYBACK_START_DMA_DESCR_CH12;
  

•   else
  

•           dma_dscr_idx = CAPTURE_START_DMA_DESCR_CH14;
  

•     /* Configure System memory <-> ACP SRAM DMA descriptors */
      set_acp_sysmem_dma_descriptors(acp_mmio, rtd->size,
  

•                                  rtd->direction, pte_offset, ch1,
  

•                                  sram_bank, dma_dscr_idx,
  

asic_type);

•   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
  

•           dma_dscr_idx = PLAYBACK_START_DMA_DESCR_CH13;
  

•   else
  

•           dma_dscr_idx = CAPTURE_START_DMA_DESCR_CH15;
  

•                                  rtd->direction, pte_offset,
  

•                                  rtd->ch1, sram_bank,
  

•                                  rtd->dma_dscr_idx_1, asic_type);
      /* Configure ACP SRAM <-> I2S DMA descriptors */
      set_acp_to_i2s_dma_descriptors(acp_mmio, rtd->size,
                                     rtd->direction, sram_bank,
  

•                                  destination, ch2, dma_dscr_idx,
  

•                                  asic_type);
  

•                                  rtd->destination, rtd->ch2,
  

•                                  rtd->dma_dscr_idx_2, asic_type);
  

  }

/* Start a given DMA channel transfer */ @@ -804,6 +786,21 @@ static int acp_dma_hw_params(struct

snd_pcm_substream *substream,

              acp_reg_write(val, adata->acp_mmio,
                            mmACP_I2S_16BIT_RESOLUTION_EN);
      }

•   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
  

•           rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  

•           rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
  

•           rtd->destination = TO_ACP_I2S_1;
  

•           rtd->dma_dscr_idx_1 = PLAYBACK_START_DMA_DESCR_CH12;
  

•           rtd->dma_dscr_idx_2 = PLAYBACK_START_DMA_DESCR_CH13;
  

•   } else {
  

•           rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  

•           rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
  

•           rtd->destination = FROM_ACP_I2S_1;
  

•           rtd->dma_dscr_idx_1 = CAPTURE_START_DMA_DESCR_CH14;
  

•           rtd->dma_dscr_idx_2 = CAPTURE_START_DMA_DESCR_CH15;
  

•   }
  

I think you should do this initialization in acp_dma_open(), where the audio_substream_data is kzalloc'ed and otherwise initialized to match the provided snd_pcm_substream.

      size = params_buffer_bytes(params);
      status = snd_pcm_lib_malloc_pages(substream, size);
      if (status < 0)

@@ -898,21 +895,15 @@ static int acp_dma_prepare(struct snd_pcm_substream

*substream)

      if (!rtd)
              return -EINVAL;

•   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
  

•           config_acp_dma_channel(rtd->acp_mmio,
  

SYSRAM_TO_ACP_CH_NUM,

•                                  PLAYBACK_START_DMA_DESCR_CH12,
  

•                                  NUM_DSCRS_PER_CHANNEL, 0);
  

•           config_acp_dma_channel(rtd->acp_mmio,
  

ACP_TO_I2S_DMA_CH_NUM,

•                                  PLAYBACK_START_DMA_DESCR_CH13,
  

•                                  NUM_DSCRS_PER_CHANNEL, 0);
  

•   } else {
  

•           config_acp_dma_channel(rtd->acp_mmio,
  

ACP_TO_SYSRAM_CH_NUM,

•                                  CAPTURE_START_DMA_DESCR_CH14,
  

•                                  NUM_DSCRS_PER_CHANNEL, 0);
  

•           config_acp_dma_channel(rtd->acp_mmio,
  

I2S_TO_ACP_DMA_CH_NUM,

•                                  CAPTURE_START_DMA_DESCR_CH15,
  

•                                  NUM_DSCRS_PER_CHANNEL, 0);
  

•   }
  

•   config_acp_dma_channel(rtd->acp_mmio,
  

•                          rtd->ch1,
  

•                          rtd->dma_dscr_idx_1,
  

•                          NUM_DSCRS_PER_CHANNEL, 0);
  

•   config_acp_dma_channel(rtd->acp_mmio,
  

•                          rtd->ch2,
  

The original code was using ACP_TO_SYSRAM_CH_NUM for the capture case, but now you are using SYSRAM_TO_ACP_CH_NUM as just initialized in acp_dma_hw_params(). I think the old config_acp_dma() was wrong, and it should still be ACP_TO_SYSRAM_CH_NUM. When you make this fix, either do it in a separate preliminary patch (preferred), or at least call it out in the commit message.

Also, instead of "ch1" and "ch2", perhaps we can use the more descriptive "ch_i2s" and "ch_sysram" [and same for dma_descr].

•                          rtd->dma_dscr_idx_2,
  

•                          NUM_DSCRS_PER_CHANNEL, 0);
      return 0;
  

  }

@@ -939,10 +930,9 @@ static int acp_dma_trigger(struct snd_pcm_substream

*substream, int cmd)

              if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
                      if (rtd->i2ssp_renderbytescount == 0)
                              rtd->i2ssp_renderbytescount = bytescount;

•                   acp_dma_start(rtd->acp_mmio,
  

•                                 SYSRAM_TO_ACP_CH_NUM, false);
  

•                   acp_dma_start(rtd->acp_mmio, rtd->ch1, false);
                      while (acp_reg_read(rtd->acp_mmio,
  

mmACP_DMA_CH_STS) &

BIT(SYSRAM_TO_ACP_CH_NUM)) {

•                           BIT(rtd->ch1)) {
                              if (!loops--) {
                                      dev_err(component->dev,
                                              "acp dma start
  

timeout\n");

@@ -950,38 +940,23 @@ static int acp_dma_trigger(struct snd_pcm_substream

*substream, int cmd)

                              }
                              cpu_relax();
                      }

•                   acp_dma_start(rtd->acp_mmio,
  

•                                 ACP_TO_I2S_DMA_CH_NUM, true);
  

•             } else {
                      if (rtd->i2ssp_capturebytescount == 0)
                              rtd->i2ssp_capturebytescount =
  

bytescount;

•                   acp_dma_start(rtd->acp_mmio,
  

•                                 I2S_TO_ACP_DMA_CH_NUM, true);
              }
  

•           acp_dma_start(rtd->acp_mmio, rtd->ch2, true);
              ret = 0;
              break;
      case SNDRV_PCM_TRIGGER_STOP:
      case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
      case SNDRV_PCM_TRIGGER_SUSPEND:
  

•           /*
  

•            * Need to stop only circular DMA channels :
  

•            * ACP_TO_I2S_DMA_CH_NUM / I2S_TO_ACP_DMA_CH_NUM.
  

Non-circular

•            * channels will stopped automatically after its transfer
  

•            * completes : SYSRAM_TO_ACP_CH_NUM / ACP_TO_SYSRAM_CH_NUM
  

•            */
              if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
  

•                   ret = acp_dma_stop(rtd->acp_mmio,
  

•                                      SYSRAM_TO_ACP_CH_NUM);
  

•                   ret = acp_dma_stop(rtd->acp_mmio,
  

•                                      ACP_TO_I2S_DMA_CH_NUM);
  

•                   acp_dma_stop(rtd->acp_mmio, rtd->ch1);
  

•                   ret =  acp_dma_stop(rtd->acp_mmio, rtd->ch2);
                      rtd->i2ssp_renderbytescount = 0;
              } else {
  

•                   ret = acp_dma_stop(rtd->acp_mmio,
  

•                                      I2S_TO_ACP_DMA_CH_NUM);
  

•                   ret = acp_dma_stop(rtd->acp_mmio,
  

•                                      ACP_TO_SYSRAM_CH_NUM);
  

•                   acp_dma_stop(rtd->acp_mmio, rtd->ch2);
  

•                   ret = acp_dma_stop(rtd->acp_mmio, rtd->ch1);
  

Using "ch_i2s" and "ch_sysram" would help here, since then it wouldn't need to do the slightly confusing "stop 2 then stop 1".

-Dan

                      rtd->i2ssp_capturebytescount = 0;
              }
              break;

diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h index 0e6089b..5e25428 100644 --- a/sound/soc/amd/acp.h +++ b/sound/soc/amd/acp.h @@ -85,6 +85,11 @@ struct audio_substream_data { unsigned int order; u16 num_of_pages; u16 direction;

•   u16 ch1;
  

•   u16 ch2;
  

•   u16 destination;
  

•   u16 dma_dscr_idx_1;
  

•   u16 dma_dscr_idx_2;
      uint64_t size;
      u64 i2ssp_renderbytescount;
      u64 i2ssp_capturebytescount;
  

-- 2.7.4