On 2/3/17 10:43 AM, Takashi Iwai wrote:

The PCM engine on LPE audio isn't like a batch-style process, but rather it deals with the standard ring buffer. Passing the BATCH info flag is inappropriate.

Humm, this is controversial. There are 4 DMA descriptors and getting a precise position in the stream is not straightforward. Rewind is also not supported so if you remove the BATCH flag you will push PulseAudio into doing timer-based scheduling and rewinds that probably don't work.

Similarly, the DOUBLE flag is also superfluous. Drop both bits.

Signed-off-by: Takashi Iwai tiwai@suse.de

sound/x86/intel_hdmi_audio.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index c83f02c2593e..32a21422e6f5 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -131,10 +131,8 @@ static const struct channel_map_table map_tables[] = { /* hardware capability structure */ static const struct snd_pcm_hardware snd_intel_hadstream = { .info = (SNDRV_PCM_INFO_INTERLEAVED |

• SNDRV_PCM_INFO_DOUBLE |
  

  SNDRV_PCM_INFO_MMAP|

• SNDRV_PCM_INFO_MMAP_VALID |
  

• SNDRV_PCM_INFO_BATCH),
  

• SNDRV_PCM_INFO_MMAP_VALID),
  

  .formats = (SNDRV_PCM_FMTBIT_S24 | SNDRV_PCM_FMTBIT_U24), .rates = SNDRV_PCM_RATE_32000 |

On 02/03/2017 02:13 PM, Takashi Iwai wrote:

On Fri, 03 Feb 2017 20:39:49 +0100, Pierre-Louis Bossart wrote:

...

On 2/3/17 10:43 AM, Takashi Iwai wrote:

...

The PCM engine on LPE audio isn't like a batch-style process, but rather it deals with the standard ring buffer. Passing the BATCH info flag is inappropriate.

Humm, this is controversial. There are 4 DMA descriptors and getting a precise position in the stream is not straightforward.

Well, as far as I've tested, it is. The buffer length register keeps the remaining bytes, and you can easily read out and compute the current position in fairly exact manner. Even the old driver has that information -- the patch David added does it.

Yes, and I don't think anyone on the Intel side quite understood what David did there. The code didn't seem quite right.

...

Rewind is also not supported so if you remove the BATCH flag you will push PulseAudio into doing timer-based scheduling and rewinds that probably don't work.

I don't think there is much difference regarding this. Please check the buffer manage description in my previous post

We've never tested the hardware in those configurations so I'd like a bit more time to double check how well this might work. None of us has a clear view of how much buffering and pre-fetch is done by the DMA engine so if you rewind 'too much' you may be out of luck.

In anyway, it'd be appreciated if you can test on your hardware. I could test only on a single machine.

I can test more but only in 10 days from now so if we could delay this patch a bit it'd be better.

thanks,

Takashi

...

...

Similarly, the DOUBLE flag is also superfluous. Drop both bits.

Signed-off-by: Takashi Iwai tiwai@suse.de

sound/x86/intel_hdmi_audio.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index c83f02c2593e..32a21422e6f5 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -131,10 +131,8 @@ static const struct channel_map_table map_tables[] = { /* hardware capability structure */ static const struct snd_pcm_hardware snd_intel_hadstream = { .info = (SNDRV_PCM_INFO_INTERLEAVED |

• SNDRV_PCM_INFO_DOUBLE |
  

  SNDRV_PCM_INFO_MMAP|

• SNDRV_PCM_INFO_MMAP_VALID |
  

• SNDRV_PCM_INFO_BATCH),
  

• SNDRV_PCM_INFO_MMAP_VALID),
  

  .formats = (SNDRV_PCM_FMTBIT_S24 | SNDRV_PCM_FMTBIT_U24), .rates = SNDRV_PCM_RATE_32000 |

---

Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

On Sat, 04 Feb 2017 08:57:08 +0100, Takashi Iwai wrote:

...

...

In anyway, it'd be appreciated if you can test on your hardware. I could test only on a single machine.

I can test more but only in 10 days from now so if we could delay this patch a bit it'd be better.

OK, I can postpone this change and keep BATCH and DOUBLE.

I've tested more intensively, and this seems working well, so far. Hopefully the DMA FIFO or fetch timing doesn't play a big role.

BTW, with the combination of this and the latest my PCM rewrite patch, a more interesting experiment can be done: extend to (a kind of) no-period-wakeup mode. Of course, it doesn't work like HD-audio, as we can't the ring buffer persistently on LPE audio but have to refresh the buffer descriptors. But the refresh of BDs is also done at PCM pointer callback, so it would work as is. For supporting the case period=1, though, another trick is needed: namely, we set up 4 BDs pointing to the same address, so that it won't go out to underrun.

A totally untested patch is below.

thanks,

Takashi

--- diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 0bd77acb9689..be9cabc9f58b 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -133,7 +133,8 @@ static const struct channel_map_table map_tables[] = { static const struct snd_pcm_hardware had_pcm_hardware = { .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_MMAP| - SNDRV_PCM_INFO_MMAP_VALID), + SNDRV_PCM_INFO_MMAP_VALID| + SNDRV_PCM_INFO_NO_PERIOD_WAKEUP), .formats = (SNDRV_PCM_FMTBIT_S24 | SNDRV_PCM_FMTBIT_U24), .rates = SNDRV_PCM_RATE_32000 | @@ -853,7 +854,9 @@ static void had_prog_bd(struct snd_pcm_substream *substream, int ofs = intelhaddata->pcmbuf_filled * intelhaddata->period_bytes; u32 addr = substream->runtime->dma_addr + ofs;

- addr |= AUD_BUF_VALID | AUD_BUF_INTR_EN; + addr |= AUD_BUF_VALID; + if (!subsream->runtime->no_period_wakeup) + addr |= AUD_BUF_INTR_EN; had_write_register(intelhaddata, AUD_BUF_ADDR(idx), addr); had_write_register(intelhaddata, AUD_BUF_LEN(idx), intelhaddata->period_bytes); @@ -881,7 +884,10 @@ static void had_init_ringbuf(struct snd_pcm_substream *substream, int i, num_periods;

num_periods = runtime->periods; - intelhaddata->num_bds = min(num_periods, HAD_NUM_OF_RING_BUFS); + if (num_periods == 1) + intelhaddata->num_bds = HAD_NUM_OF_RING_BUFS; + else + intelhaddata->num_bds = min(num_periods, HAD_NUM_OF_RING_BUFS); intelhaddata->period_bytes = frames_to_bytes(runtime, runtime->period_size); WARN_ON(intelhaddata->period_bytes & 0x3f); @@ -891,7 +897,7 @@ static void had_init_ringbuf(struct snd_pcm_substream *substream, intelhaddata->pcmbuf_filled = 0;

for (i = 0; i < HAD_NUM_OF_RING_BUFS; i++) { - if (i < num_periods) + if (i < num_periods || num_periods == 1) had_prog_bd(substream, intelhaddata); else /* invalidate the rest */ had_invalidate_bd(intelhaddata, i); diff --git a/sound/x86/intel_hdmi_lpe_audio.h b/sound/x86/intel_hdmi_lpe_audio.h index ca4212dca94e..3c2befa721a4 100644 --- a/sound/x86/intel_hdmi_lpe_audio.h +++ b/sound/x86/intel_hdmi_lpe_audio.h @@ -32,7 +32,7 @@ #define HAD_MAX_BUFFER ((1024 * 1024 - 1) & ~0x3f) #define HAD_DEFAULT_BUFFER (600 * 1024) /* default prealloc size */ #define HAD_MAX_PERIODS 256 /* arbitrary, but should suffice */ -#define HAD_MIN_PERIODS 2 +#define HAD_MIN_PERIODS 1 #define HAD_MAX_PERIOD_BYTES ((HAD_MAX_BUFFER / HAD_MIN_PERIODS) & ~0x3f) #define HAD_MIN_PERIOD_BYTES 1024 /* might be smaller */ #define HAD_FIFO_SIZE 0 /* fifo not being used */

On 2/3/17 10:44 AM, Takashi Iwai wrote:

This is again a big rewrite of the driver; now it touches the code to process PCM stream transfers.

The most fundamental change is that now the driver supports more than four periods. Instead of keeping the same index between the ring buffers (from A to D) and the PCM buffer periods, now we keep difference indices for both. Also, for the cases with less periods than four, we track the head index, too. That is, we now have four indices: ringbuf_head, pcm_head, ringbuf_filled, and pcm_filled.

Well that's not completely right. The DMA can only generate an interrupt once the buffer you submit was played, and with 4 descriptors you can't have more than 4 points where interrupts are generated. If you program different values in different descriptors then the notion of periodic hardware interrupts will be lost.

By this flexibility, we can use even dmix, which requires 16 periods as default.

The buffer size could be up to 20bit, so it's set to that value. But the pre-allocation is limited to 128k as default. It's because the chip requires the continuous pages (unfortunately no-SG possible), thus we don't want too much continuous pages.

No, that's not true. You need contiguous regions for each descriptor, but the entire buffer doesn't need to be contiguous.

Signed-off-by: Takashi Iwai tiwai@suse.de

sound/x86/intel_hdmi_audio.c | 510 ++++++++++++++------------------------- sound/x86/intel_hdmi_audio.h | 23 +- sound/x86/intel_hdmi_lpe_audio.h | 26 +- 3 files changed, 204 insertions(+), 355 deletions(-)

diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index c503f0de3975..2c3dcdcb43f5 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -619,82 +619,6 @@ static void snd_intelhad_prog_dip(struct snd_pcm_substream *substream, had_write_register(intelhaddata, AUD_CNTL_ST, ctrl_state.regval); }

-/*

• • Programs buffer address and length registers
• • This function programs ring buffer address and length into registers.
• */

-static int snd_intelhad_prog_buffer(struct snd_pcm_substream *substream,

• 		    struct snd_intelhad *intelhaddata,
  

• 		    int start, int end)
  

-{

• u32 ring_buf_addr, ring_buf_size, period_bytes;
• u8 i, num_periods;
• ring_buf_addr = substream->runtime->dma_addr;
• ring_buf_size = snd_pcm_lib_buffer_bytes(substream);
• intelhaddata->stream_info.ring_buf_size = ring_buf_size;
• period_bytes = frames_to_bytes(substream->runtime,

• 		substream->runtime->period_size);
  

• num_periods = substream->runtime->periods;
• /*

• * buffer addr should  be 64 byte aligned, period bytes
  

• * will be used to calculate addr offset
  

• */
  

• period_bytes &= ~0x3F;
• /* Hardware supports MAX_PERIODS buffers */
• if (end >= HAD_MAX_PERIODS)

• return -EINVAL;
  

• for (i = start; i <= end; i++) {

• /* Program the buf registers with addr and len */
  

• intelhaddata->buf_info[i].buf_addr = ring_buf_addr +
  

• 					 (i * period_bytes);
  

• if (i < num_periods-1)
  

• 	intelhaddata->buf_info[i].buf_size = period_bytes;
  

• else
  

• 	intelhaddata->buf_info[i].buf_size = ring_buf_size -
  

• 					(i * period_bytes);
  

• had_write_register(intelhaddata,
  

• 		   AUD_BUF_A_ADDR + (i * HAD_REG_WIDTH),
  

• 			intelhaddata->buf_info[i].buf_addr |
  

• 			BIT(0) | BIT(1));
  

• had_write_register(intelhaddata,
  

• 		   AUD_BUF_A_LENGTH + (i * HAD_REG_WIDTH),
  

• 			period_bytes);
  

• intelhaddata->buf_info[i].is_valid = true;
  

• }
• dev_dbg(intelhaddata->dev, "%s:buf[%d-%d] addr=%#x and size=%d\n",

• __func__, start, end,
  

• intelhaddata->buf_info[start].buf_addr,
  

• intelhaddata->buf_info[start].buf_size);
  

• intelhaddata->valid_buf_cnt = num_periods;
• return 0;

-}

-static int snd_intelhad_read_len(struct snd_intelhad *intelhaddata) -{

• int i, retval = 0;
• u32 len[4];
• for (i = 0; i < 4 ; i++) {

• had_read_register(intelhaddata,
  

• 		  AUD_BUF_A_LENGTH + (i * HAD_REG_WIDTH),
  

• 		  &len[i]);
  

• if (!len[i])
  

• 	retval++;
  

• }
• if (retval != 1) {

• for (i = 0; i < 4 ; i++)
  

• 	dev_dbg(intelhaddata->dev, "buf[%d] size=%d\n",
  

• 		i, len[i]);
  

• }
• return retval;

-}

static int had_calculate_maud_value(u32 aud_samp_freq, u32 link_rate) { u32 maud_val; @@ -882,34 +806,192 @@ static int snd_intelhad_prog_n(u32 aud_samp_freq, u32 *n_param, return 0; }

+/*

• • PCM ring buffer handling
• */

+#define AUD_BUF_ADDR(x) (AUD_BUF_A_ADDR + (x) * HAD_REG_WIDTH) +#define AUD_BUF_LEN(x) (AUD_BUF_A_LENGTH + (x) * HAD_REG_WIDTH)

+/* Set up a ring buffer at the "filled" position */ +static void had_prog_ringbuf(struct snd_pcm_substream *substream,

• 	     struct snd_intelhad *intelhaddata)
  

+{

• int idx = intelhaddata->ringbuf_filled;
• int ofs = intelhaddata->pcmbuf_filled * intelhaddata->period_bytes;
• u32 addr = substream->runtime->dma_addr + ofs;
• addr |= AUD_BUF_VALID | AUD_BUF_INTR_EN;
• had_write_register(intelhaddata, AUD_BUF_ADDR(idx), addr);
• had_write_register(intelhaddata, AUD_BUF_LEN(idx),

• 	   intelhaddata->period_bytes);
  

• /* advance the indices to the next */
• intelhaddata->ringbuf_filled++;
• intelhaddata->ringbuf_filled %= HAD_NUM_RINGBUFS;
• intelhaddata->pcmbuf_filled++;
• intelhaddata->pcmbuf_filled %= substream->runtime->periods;

+}

+/* invalidate a ring buffer with the given index */ +static void had_invalidate_ringbuf(struct snd_intelhad *intelhaddata,

• 		   int idx)
  

+{

• had_write_register(intelhaddata, AUD_BUF_ADDR(idx), 0);
• had_write_register(intelhaddata, AUD_BUF_LEN(idx), 0);

+}

+/* Initial programming of ring buffers */ +static void had_init_ringbufs(struct snd_pcm_substream *substream,

• 	      struct snd_intelhad *intelhaddata)
  

+{

• struct snd_pcm_runtime *runtime = substream->runtime;
• int i, period_bytes, num_periods;
• num_periods = runtime->periods;
• intelhaddata->period_bytes = period_bytes =

• frames_to_bytes(runtime, runtime->period_size);
  

• WARN_ON(period_bytes & 0x3f);
• intelhaddata->ringbuf_head = 0;
• intelhaddata->pcmbuf_head = 0;
• intelhaddata->ringbuf_filled = 0;
• intelhaddata->pcmbuf_filled = 0;
• for (i = 0; i < HAD_NUM_RINGBUFS; i++) {

• if (i < num_periods)
  

• 	had_prog_ringbuf(substream, intelhaddata);
  

• else /* invalidate the rest */
  

• 	had_invalidate_ringbuf(intelhaddata, i);
  

• }

+}

+/* process a ring buf, advance to the next */ +static void had_advance_ringbuf(struct snd_pcm_substream *substream,

• 		struct snd_intelhad *intelhaddata)
  

+{

• int num_periods = substream->runtime->periods;
• /* reprogram the next buffer */
• had_prog_ringbuf(substream, intelhaddata);
• /* invalidate the processed buf for shorter PCM buffer */
• if (num_periods < HAD_NUM_RINGBUFS)

• had_invalidate_ringbuf(intelhaddata,
  

• 		       intelhaddata->ringbuf_head);
  

• /* proceed to next */
• intelhaddata->ringbuf_head++;
• intelhaddata->ringbuf_head %= HAD_NUM_RINGBUFS;
• intelhaddata->pcmbuf_head++;
• intelhaddata->pcmbuf_head %= num_periods;

+}

+/* process the current ring buffer(s);

• • returns the current PCM buffer byte position, or -EPIPE for underrun.
• */

+static int had_process_ringbufs(struct snd_pcm_substream *substream,

• 		struct snd_intelhad *intelhaddata)
  

+{

• int len, processed;
• unsigned long flags;
• processed = 0;
• spin_lock_irqsave(&intelhaddata->had_spinlock, flags);
• for (;;) {

• /* get the remaining bytes on the buffer */
  

• had_read_register(intelhaddata,
  

• 		  AUD_BUF_LEN(intelhaddata->ringbuf_head),
  

• 		  &len);
  

• if (len < 0 || len > intelhaddata->period_bytes) {
  

• 	dev_dbg(intelhaddata->dev, "Invalid buf length %d\n",
  

• 		len);
  

• 	len = -EPIPE;
  

• 	goto out;
  

• }
  

• if (len > 0) /* OK, this is the current buffer */
  

• 	break;
  

• /* len=0 => already empty, check the next buffer */
  

• if (++processed >= HAD_NUM_RINGBUFS) {
  

• 	len = -EPIPE; /* all empty? - report underrun */
  

• 	goto out;
  

• }
  

• had_advance_ringbuf(substream, intelhaddata);
  

• }
• len = intelhaddata->period_bytes - len;
• len += intelhaddata->period_bytes * intelhaddata->pcmbuf_head;
• out:
• spin_unlock_irqrestore(&intelhaddata->had_spinlock, flags);
• return len;

+}

+/* called from irq handler */ +static void had_process_buffer_done(struct snd_intelhad *intelhaddata) +{

• struct snd_pcm_substream *substream;
• if (!intelhaddata->connected)

• return; /* disconnected? - bail out */
  

• substream = had_substream_get(intelhaddata);
• if (!substream)

• return; /* no stream? - bail out */
  

• /* process or stop the stream */
• if (had_process_ringbufs(substream, intelhaddata) < 0)

• snd_pcm_stop_xrun(substream);
  

• else

• snd_pcm_period_elapsed(substream);
  

• had_substream_put(intelhaddata);

+}

#define MAX_CNT 0xFF

-static void snd_intelhad_handle_underrun(struct snd_intelhad *intelhaddata) +/*

• • The interrupt status 'sticky' bits might not be cleared by
• • setting '1' to that bit once...
• */

+static void wait_clear_underrun_bit(struct snd_intelhad *intelhaddata) {

• u32 hdmi_status = 0, i = 0;

• int i;
• u32 val;
• for (i = 0; i < MAX_CNT; i++) {

• /* clear bit30, 31 AUD_HDMI_STATUS */
  

• had_read_register(intelhaddata, AUD_HDMI_STATUS, &val);
  

• if (!(val & AUD_CONFIG_MASK_UNDERRUN))
  

• 	return;
  

• had_write_register(intelhaddata, AUD_HDMI_STATUS, val);
  

• }
• dev_err(intelhaddata->dev, "Unable to clear UNDERRUN bits\n");

+}

+/* called from irq handler */ +static void had_process_buffer_underrun(struct snd_intelhad *intelhaddata) +{

• struct snd_pcm_substream *substream;

  /* Handle Underrun interrupt within Audio Unit */ had_write_register(intelhaddata, AUD_CONFIG, 0); /* Reset buffer pointers */ had_write_register(intelhaddata, AUD_HDMI_STATUS, 1); had_write_register(intelhaddata, AUD_HDMI_STATUS, 0);

• /*

• * The interrupt status 'sticky' bits might not be cleared by
  

• * setting '1' to that bit once...
  

• */
  

• do { /* clear bit30, 31 AUD_HDMI_STATUS */

• had_read_register(intelhaddata, AUD_HDMI_STATUS,
  

• 		  &hdmi_status);
  

• dev_dbg(intelhaddata->dev, "HDMI status =0x%x\n", hdmi_status);
  

• if (hdmi_status & AUD_CONFIG_MASK_UNDERRUN) {
  

• 	i++;
  

• 	had_write_register(intelhaddata,
  

• 			   AUD_HDMI_STATUS, hdmi_status);
  

• } else
  

• 	break;
  

• } while (i < MAX_CNT);
• if (i >= MAX_CNT)

• dev_err(intelhaddata->dev, "Unable to clear UNDERRUN bits\n");
  

• wait_clear_underrun_bit(intelhaddata);
• if (!intelhaddata->connected)

• return; /* disconnected? - bail out */
  

• /* Report UNDERRUN error to above layers */
• substream = had_substream_get(intelhaddata);
• if (substream) {

• snd_pcm_stop_xrun(substream);
  

• had_substream_put(intelhaddata);
  

• }

}

/* @@ -955,11 +1037,6 @@ static int snd_intelhad_open(struct snd_pcm_substream *substream) intelhaddata->stream_info.substream_refcount++; spin_unlock_irq(&intelhaddata->had_spinlock);

• /* these are cleared in prepare callback, but just to be sure */
• intelhaddata->curr_buf = 0;
• intelhaddata->underrun_count = 0;
• intelhaddata->stream_info.buffer_rendered = 0;
• return retval; error: pm_runtime_put(intelhaddata->dev);

@@ -1123,10 +1200,6 @@ static int snd_intelhad_pcm_prepare(struct snd_pcm_substream *substream) dev_dbg(intelhaddata->dev, "rate=%d\n", runtime->rate); dev_dbg(intelhaddata->dev, "channels=%d\n", runtime->channels);

• intelhaddata->curr_buf = 0;
• intelhaddata->underrun_count = 0;
• intelhaddata->stream_info.buffer_rendered = 0;
• /* Get N value in KHz */ disp_samp_freq = intelhaddata->tmds_clock_speed;

@@ -1150,8 +1223,7 @@ static int snd_intelhad_pcm_prepare(struct snd_pcm_substream *substream) retval = snd_intelhad_audio_ctrl(substream, intelhaddata);

/* Prog buffer address */

• retval = snd_intelhad_prog_buffer(substream, intelhaddata,

• 	HAD_BUF_TYPE_A, HAD_BUF_TYPE_D);
  

• had_init_ringbufs(substream, intelhaddata);

  /*

  • Program channel mapping in following order:

@@ -1171,48 +1243,17 @@ static snd_pcm_uframes_t snd_intelhad_pcm_pointer(struct snd_pcm_substream *substream) { struct snd_intelhad *intelhaddata;

• u32 bytes_rendered = 0;
• u32 t;
• int buf_id;

• int len;

  intelhaddata = snd_pcm_substream_chip(substream);

  if (!intelhaddata->connected) return SNDRV_PCM_POS_XRUN;

• /* Use a hw register to calculate sub-period position reports.

• * This makes PulseAudio happier.
  

• */
  

• buf_id = intelhaddata->curr_buf % 4;
• had_read_register(intelhaddata,

• 	  AUD_BUF_A_LENGTH + (buf_id * HAD_REG_WIDTH), &t);
  

• if ((t == 0) || (t == ((u32)-1L))) {

• intelhaddata->underrun_count++;
  

• dev_dbg(intelhaddata->dev,
  

• 	"discovered buffer done for buf %d, count = %d\n",
  

• 	 buf_id, intelhaddata->underrun_count);
  

• if (intelhaddata->underrun_count > (HAD_MIN_PERIODS/2)) {
  

• 	dev_dbg(intelhaddata->dev,
  

• 		"assume audio_codec_reset, underrun = %d - do xrun\n",
  

• 		 intelhaddata->underrun_count);
  

• 	return SNDRV_PCM_POS_XRUN;
  

• }
  

• } else {

• /* Reset Counter */
  

• intelhaddata->underrun_count = 0;
  

• }
• t = intelhaddata->buf_info[buf_id].buf_size - t;
• if (intelhaddata->stream_info.buffer_rendered)

• div_u64_rem(intelhaddata->stream_info.buffer_rendered,
  

• 	intelhaddata->stream_info.ring_buf_size,
  

• 	&(bytes_rendered));
  

• return bytes_to_frames(substream->runtime, bytes_rendered + t);

• len = had_process_ringbufs(substream, intelhaddata);
• if (len < 0)

• return SNDRV_PCM_POS_XRUN;
  

• return bytes_to_frames(substream->runtime, len);

}

/* @@ -1283,179 +1324,9 @@ static int hdmi_audio_mode_change(struct snd_intelhad *intelhaddata) return retval; }

-static inline int had_chk_intrmiss(struct snd_intelhad *intelhaddata,

• enum intel_had_aud_buf_type buf_id)
  

-{

• int i, intr_count = 0;
• enum intel_had_aud_buf_type buff_done;
• u32 buf_size, buf_addr;
• buff_done = buf_id;
• intr_count = snd_intelhad_read_len(intelhaddata);
• if (intr_count > 1) {

• /* In case of active playback */
  

• dev_err(intelhaddata->dev,
  

• 	"Driver detected %d missed buffer done interrupt(s)\n",
  

• 	(intr_count - 1));
  

• if (intr_count > 3)
  

• 	return intr_count;
  

• buf_id += (intr_count - 1);
  

• /* Reprogram registers*/
  

• for (i = buff_done; i < buf_id; i++) {
  

• 	int j = i % 4;
  

• 	buf_size = intelhaddata->buf_info[j].buf_size;
  

• 	buf_addr = intelhaddata->buf_info[j].buf_addr;
  

• 	had_write_register(intelhaddata,
  

• 			   AUD_BUF_A_LENGTH +
  

• 			   (j * HAD_REG_WIDTH), buf_size);
  

• 	had_write_register(intelhaddata,
  

• 			   AUD_BUF_A_ADDR+(j * HAD_REG_WIDTH),
  

• 			   (buf_addr | BIT(0) | BIT(1)));
  

• }
  

• buf_id = buf_id % 4;
  

• intelhaddata->buff_done = buf_id;
  

• }
• return intr_count;

-}

-/* called from irq handler */ -static int had_process_buffer_done(struct snd_intelhad *intelhaddata) -{

• u32 len = 1;
• enum intel_had_aud_buf_type buf_id;
• enum intel_had_aud_buf_type buff_done;
• struct pcm_stream_info *stream;
• struct snd_pcm_substream *substream;
• u32 buf_size;
• int intr_count;
• unsigned long flags;
• stream = &intelhaddata->stream_info;
• intr_count = 1;
• spin_lock_irqsave(&intelhaddata->had_spinlock, flags);
• if (!intelhaddata->connected) {

• spin_unlock_irqrestore(&intelhaddata->had_spinlock, flags);
  

• dev_dbg(intelhaddata->dev,
  

• 	"%s:Device already disconnected\n", __func__);
  

• return 0;
  

• }
• buf_id = intelhaddata->curr_buf;
• intelhaddata->buff_done = buf_id;
• buff_done = intelhaddata->buff_done;
• buf_size = intelhaddata->buf_info[buf_id].buf_size;
• /* Every debug statement has an implication

• * of ~5msec. Thus, avoid having >3 debug statements
  

• * for each buffer_done handling.
  

• */
  

• /* Check for any intr_miss in case of active playback */
• if (stream->running) {

• intr_count = had_chk_intrmiss(intelhaddata, buf_id);
  

• if (!intr_count || (intr_count > 3)) {
  

• 	spin_unlock_irqrestore(&intelhaddata->had_spinlock,
  

• 			       flags);
  

• 	dev_err(intelhaddata->dev,
  

• 		"HAD SW state in non-recoverable mode\n");
  

• 	return 0;
  

• }
  

• buf_id += (intr_count - 1);
  

• buf_id = buf_id % 4;
  

• }
• intelhaddata->buf_info[buf_id].is_valid = true;
• if (intelhaddata->valid_buf_cnt-1 == buf_id) {

• if (stream->running)
  

• 	intelhaddata->curr_buf = HAD_BUF_TYPE_A;
  

• } else

• intelhaddata->curr_buf = buf_id + 1;
  

• spin_unlock_irqrestore(&intelhaddata->had_spinlock, flags);
• if (!intelhaddata->connected) {

• dev_dbg(intelhaddata->dev, "HDMI cable plugged-out\n");
  

• return 0;
  

• }
• /* Reprogram the registers with addr and length */
• had_write_register(intelhaddata,

• 	   AUD_BUF_A_LENGTH + (buf_id * HAD_REG_WIDTH),
  

• 	   buf_size);
  

• had_write_register(intelhaddata,

• 	   AUD_BUF_A_ADDR + (buf_id * HAD_REG_WIDTH),
  

• 	   intelhaddata->buf_info[buf_id].buf_addr |
  

• 	   BIT(0) | BIT(1));
  

• had_read_register(intelhaddata,

• 	  AUD_BUF_A_LENGTH + (buf_id * HAD_REG_WIDTH),
  

• 	  &len);
  

• dev_dbg(intelhaddata->dev, "%s:Enabled buf[%d]\n", __func__, buf_id);
• /* In case of actual data,

• * report buffer_done to above ALSA layer
  

• */
  

• substream = had_substream_get(intelhaddata);
• if (substream) {

• buf_size = intelhaddata->buf_info[buf_id].buf_size;
  

• intelhaddata->stream_info.buffer_rendered +=
  

• 	(intr_count * buf_size);
  

• snd_pcm_period_elapsed(substream);
  

• had_substream_put(intelhaddata);
  

• }
• return 0;

-}

-/* called from irq handler */ -static int had_process_buffer_underrun(struct snd_intelhad *intelhaddata) -{

• enum intel_had_aud_buf_type buf_id;
• struct pcm_stream_info *stream;
• struct snd_pcm_substream *substream;
• unsigned long flags;
• int connected;
• stream = &intelhaddata->stream_info;
• spin_lock_irqsave(&intelhaddata->had_spinlock, flags);
• buf_id = intelhaddata->curr_buf;
• intelhaddata->buff_done = buf_id;
• connected = intelhaddata->connected;
• if (stream->running)

• intelhaddata->curr_buf = HAD_BUF_TYPE_A;
  

• spin_unlock_irqrestore(&intelhaddata->had_spinlock, flags);
• dev_dbg(intelhaddata->dev, "Enter:%s buf_id=%d, stream_running=%d\n",

• 	__func__, buf_id, stream->running);
  

• snd_intelhad_handle_underrun(intelhaddata);
• if (!connected) {

• dev_dbg(intelhaddata->dev,
  

• 	"%s:Device already disconnected\n", __func__);
  

• return 0;
  

• }
• /* Report UNDERRUN error to above layers */
• substream = had_substream_get(intelhaddata);
• if (substream) {

• snd_pcm_stop_xrun(substream);
  

• had_substream_put(intelhaddata);
  

• }
• return 0;

-}

/* process hot plug, called from wq with mutex locked */ static void had_process_hot_plug(struct snd_intelhad *intelhaddata) {

• enum intel_had_aud_buf_type buf_id; struct snd_pcm_substream *substream;

  spin_lock_irq(&intelhaddata->had_spinlock);

@@ -1465,17 +1336,12 @@ static void had_process_hot_plug(struct snd_intelhad *intelhaddata) return; }

• buf_id = intelhaddata->curr_buf;

• intelhaddata->buff_done = buf_id; intelhaddata->connected = true; dev_dbg(intelhaddata->dev, "%s @ %d:DEBUG PLUG/UNPLUG : HAD_DRV_CONNECTED\n", __func__, __LINE__); spin_unlock_irq(&intelhaddata->had_spinlock);

• dev_dbg(intelhaddata->dev, "Processing HOT_PLUG, buf_id = %d\n",

• buf_id);
  

• /* Safety check */ substream = had_substream_get(intelhaddata); if (substream) {

@@ -1492,11 +1358,8 @@ static void had_process_hot_plug(struct snd_intelhad *intelhaddata) /* process hot unplug, called from wq with mutex locked */ static void had_process_hot_unplug(struct snd_intelhad *intelhaddata) {

• enum intel_had_aud_buf_type buf_id; struct snd_pcm_substream *substream;

• buf_id = intelhaddata->curr_buf;

• substream = had_substream_get(intelhaddata);

  spin_lock_irq(&intelhaddata->had_spinlock);

@@ -1868,13 +1731,12 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));

• /* allocate dma pages for ALSA stream operations

• * memory allocated is based on size, not max value
  

• * thus using same argument for max & size
  

• /* allocate dma pages;

• * try to allocate 128k buffer as default which is large enough
  

  */ snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, NULL,

• 	HAD_MAX_BUFFER, HAD_MAX_BUFFER);
  

• 	128 * 1024, HAD_MAX_BUFFER);
  
  

  /* create controls */ for (i = 0; i < ARRAY_SIZE(had_controls); i++) {

diff --git a/sound/x86/intel_hdmi_audio.h b/sound/x86/intel_hdmi_audio.h index 9f713a8a88bc..6178b09801e7 100644 --- a/sound/x86/intel_hdmi_audio.h +++ b/sound/x86/intel_hdmi_audio.h @@ -64,24 +64,15 @@

struct pcm_stream_info { struct snd_pcm_substream *substream;

• u64 buffer_rendered;
• u32 ring_buf_size; int substream_refcount; bool running;

};

-struct ring_buf_info {

• u32 buf_addr;
• u32 buf_size;
• u8 is_valid;

-};

/*

• struct snd_intelhad - intelhad driver structure
• @card: ptr to hold card details
• @connected: the monitor connection status

• • @buf_info: ring buffer info
  • @stream_info: stream information
  • @eld: holds ELD info
  • @curr_buf: pointer to hold current active ring buf

@@ -91,26 +82,28 @@ struct ring_buf_info {

• @buff_done: id of current buffer done intr
• @dev: platoform device handle
• @chmap: holds channel map info

• • @underrun_count: PCM stream underrun counter
  */

struct snd_intelhad { struct snd_card *card; bool connected;

• struct ring_buf_info buf_info[HAD_NUM_OF_RING_BUFS]; struct pcm_stream_info stream_info; unsigned char eld[HDMI_MAX_ELD_BYTES]; bool dp_output;
• enum intel_had_aud_buf_type curr_buf;
• int valid_buf_cnt; unsigned int aes_bits; spinlock_t had_spinlock;
• enum intel_had_aud_buf_type buff_done; struct device *dev; struct snd_pcm_chmap *chmap;
• int underrun_count; int tmds_clock_speed; int link_rate;

• /* positions (indices) for ring buffer [A-D] */
• unsigned int ringbuf_head; /* being processed */
• unsigned int ringbuf_filled; /* to be filled */
• /* positions (indices) for PCM buffer */
• unsigned int pcmbuf_head; /* being processed */
• unsigned int pcmbuf_filled; /* to be filled */
• unsigned int period_bytes; /* PCM period size in bytes */
• /* internal stuff */ int irq; void __iomem *mmio_start;

diff --git a/sound/x86/intel_hdmi_lpe_audio.h b/sound/x86/intel_hdmi_lpe_audio.h index be9783910a3a..612714eecaff 100644 --- a/sound/x86/intel_hdmi_lpe_audio.h +++ b/sound/x86/intel_hdmi_lpe_audio.h @@ -26,15 +26,14 @@ #define HAD_MAX_DEVICES 1 #define HAD_MIN_CHANNEL 2 #define HAD_MAX_CHANNEL 8 -#define HAD_NUM_OF_RING_BUFS 4 +#define HAD_NUM_RINGBUFS 4

-/* Assume 192KHz, 8channel, 25msec period */ -#define HAD_MAX_BUFFER (600*1024) -#define HAD_MIN_BUFFER (32*1024) -#define HAD_MAX_PERIODS 4 -#define HAD_MIN_PERIODS 4 -#define HAD_MAX_PERIOD_BYTES (HAD_MAX_BUFFER/HAD_MIN_PERIODS) -#define HAD_MIN_PERIOD_BYTES 256 +/* max 20bit address, aligned to 64 */ +#define HAD_MAX_BUFFER ((1024 * 1024 - 1) & ~0x3f) +#define HAD_MAX_PERIODS 256 /* arbitrary, but should suffice */ +#define HAD_MIN_PERIODS 2 +#define HAD_MAX_PERIOD_BYTES ((HAD_MAX_BUFFER / HAD_MIN_PERIODS) & ~0x3f) +#define HAD_MIN_PERIOD_BYTES 1024 /* might be smaller */ #define HAD_FIFO_SIZE 0 /* fifo not being used */ #define MAX_SPEAKERS 8

@@ -82,14 +81,6 @@ /* Naud Value */ #define DP_NAUD_VAL 32768

-/* enum intel_had_aud_buf_type - HDMI controller ring buffer types */ -enum intel_had_aud_buf_type {

• HAD_BUF_TYPE_A = 0,
• HAD_BUF_TYPE_B = 1,
• HAD_BUF_TYPE_C = 2,
• HAD_BUF_TYPE_D = 3,

-};

/* HDMI Controller register offsets - audio domain common */ /* Base address for below regs = 0x65000 */ enum hdmi_ctrl_reg_offset_common { @@ -274,6 +265,9 @@ union aud_buf_addr { u32 regval; };

+#define AUD_BUF_VALID (1U << 0) +#define AUD_BUF_INTR_EN (1U << 1)

/* Length of Audio Buffer */ union aud_buf_len { struct {

On Sat, 04 Feb 2017 08:51:54 +0100, Takashi Iwai wrote:

...

...

and so on.

For the case periods < 4, it works other way round:

t=0 PCM | 0 | 1 | BD | A | B | - | - |

t=1 PCM | 1 | 0 | BD | - | B | C | - |

t=2 PCM | 0 | 1 | BD | - | - | C | D |

it can be done this way but I don't believe this is required. I think you can mark 2 descriptors as invalid and use only two, i.e. to follow your example you'd loop on A-B. The hardware will loop on the 4 descriptors, ignore the invalid ones and stop if it can't find a valid one.

Ah, that's good to know. So I just keep to set the rest invalid. (The code change would be likely only one or two line reduction, though :)

Below is the revised patch. I ended up adding more comments, refactoring the codes with one more field removal.

Also, the default buffer pre-allocation size isn't changed; no need to reduce the size if it worked beforehand.

Takashi

-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH v2] ALSA: x86: Refactor PCM process engine

This is again a big rewrite of the driver; now it touches the code to process PCM stream transfers.

The most fundamental change is that the driver may support more than four periods. Instead of keeping the same index between both the ring buffer (with the fixed four buffer descriptors) and the PCM buffer periods, we keep difference indices for both (bd_head and pcm_head fields). In addition, when the periods are more than four, we need to track both head and next indices. That is, we now have three indices: bd_head, pcm_head and pcm_filled.

Also, the driver works better for periods < 4, too: the remaining BDs out of four are marked as invalid, so that the hardware skips those BDs in its loop.

By this flexibility, we can use even ALSA-lib dmix plugin, which requires 16 periods as default.

The buffer size could be up to 20bit, so the max buffer size was increased accordingly. However, the buffer pre-allocation is kept as the old value (600kB) as default. The reason is the limited number of BDs: since it doesn't suffice for the useful SG page management that can fit with the usual page allocator like some other drivers, we have to still allocate continuous pages, hence we shouldn't take too big memories there.

Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 536 ++++++++++++++++----------------------- sound/x86/intel_hdmi_audio.h | 24 +- sound/x86/intel_hdmi_lpe_audio.h | 25 +- 3 files changed, 231 insertions(+), 354 deletions(-)

diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 57042ef3a480..0e30e03404ea 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -622,82 +622,6 @@ static void had_prog_dip(struct snd_pcm_substream *substream, had_write_register(intelhaddata, AUD_CNTL_ST, ctrl_state.regval); }

-/* - * Programs buffer address and length registers - * This function programs ring buffer address and length into registers. - */ -static int snd_intelhad_prog_buffer(struct snd_pcm_substream *substream, - struct snd_intelhad *intelhaddata, - int start, int end) -{ - u32 ring_buf_addr, ring_buf_size, period_bytes; - u8 i, num_periods; - - ring_buf_addr = substream->runtime->dma_addr; - ring_buf_size = snd_pcm_lib_buffer_bytes(substream); - intelhaddata->stream_info.ring_buf_size = ring_buf_size; - period_bytes = frames_to_bytes(substream->runtime, - substream->runtime->period_size); - num_periods = substream->runtime->periods; - - /* - * buffer addr should be 64 byte aligned, period bytes - * will be used to calculate addr offset - */ - period_bytes &= ~0x3F; - - /* Hardware supports MAX_PERIODS buffers */ - if (end >= HAD_MAX_PERIODS) - return -EINVAL; - - for (i = start; i <= end; i++) { - /* Program the buf registers with addr and len */ - intelhaddata->buf_info[i].buf_addr = ring_buf_addr + - (i * period_bytes); - if (i < num_periods-1) - intelhaddata->buf_info[i].buf_size = period_bytes; - else - intelhaddata->buf_info[i].buf_size = ring_buf_size - - (i * period_bytes); - - had_write_register(intelhaddata, - AUD_BUF_A_ADDR + (i * HAD_REG_WIDTH), - intelhaddata->buf_info[i].buf_addr | - BIT(0) | BIT(1)); - had_write_register(intelhaddata, - AUD_BUF_A_LENGTH + (i * HAD_REG_WIDTH), - period_bytes); - intelhaddata->buf_info[i].is_valid = true; - } - dev_dbg(intelhaddata->dev, "%s:buf[%d-%d] addr=%#x and size=%d\n", - __func__, start, end, - intelhaddata->buf_info[start].buf_addr, - intelhaddata->buf_info[start].buf_size); - intelhaddata->valid_buf_cnt = num_periods; - return 0; -} - -static int snd_intelhad_read_len(struct snd_intelhad *intelhaddata) -{ - int i, retval = 0; - u32 len[4]; - - for (i = 0; i < 4 ; i++) { - had_read_register(intelhaddata, - AUD_BUF_A_LENGTH + (i * HAD_REG_WIDTH), - &len[i]); - if (!len[i]) - retval++; - } - if (retval != 1) { - for (i = 0; i < 4 ; i++) - dev_dbg(intelhaddata->dev, "buf[%d] size=%d\n", - i, len[i]); - } - - return retval; -} - static int had_calculate_maud_value(u32 aud_samp_freq, u32 link_rate) { u32 maud_val; @@ -885,33 +809,217 @@ static int had_prog_n(u32 aud_samp_freq, u32 *n_param, return 0; }

+/* + * PCM ring buffer handling + * + * The hardware provides a ring buffer with the fixed 4 buffer descriptors + * (BDs). The driver maps these 4 BDs onto the PCM ring buffer. The mapping + * moves at each period elapsed. The below illustrates how it works: + * + * At time=0 + * PCM | 0 | 1 | 2 | 3 | 4 | 5 | .... |n-1| + * BD | 0 | 1 | 2 | 3 | + * + * At time=1 (period elapsed) + * PCM | 0 | 1 | 2 | 3 | 4 | 5 | .... |n-1| + * BD | 1 | 2 | 3 | 0 | + * + * At time=2 (second period elapsed) + * PCM | 0 | 1 | 2 | 3 | 4 | 5 | .... |n-1| + * BD | 2 | 3 | 0 | 1 | + * + * The bd_head field points to the index of the BD to be read. It's also the + * position to be filled at next. The pcm_head and the pcm_filled fields + * point to the indices of the current position and of the next position to + * be filled, respectively. For PCM buffer there are both _head and _filled + * because they may be difference when nperiods > 4. For example, in the + * example above at t=1, bd_head=1 and pcm_head=1 while pcm_filled=5: + * + * pcm_head (=1) --v v-- pcm_filled (=5) + * PCM | 0 | 1 | 2 | 3 | 4 | 5 | .... |n-1| + * BD | 1 | 2 | 3 | 0 | + * bd_head (=1) --^ ^-- next to fill (= bd_head) + * + * For nperiods < 4, the remaining BDs out of 4 are marked as invalid, so that + * the hardware skips those BDs in the loop. + */ + +#define AUD_BUF_ADDR(x) (AUD_BUF_A_ADDR + (x) * HAD_REG_WIDTH) +#define AUD_BUF_LEN(x) (AUD_BUF_A_LENGTH + (x) * HAD_REG_WIDTH) + +/* Set up a buffer descriptor at the "filled" position */ +static void had_prog_bd(struct snd_pcm_substream *substream, + struct snd_intelhad *intelhaddata) +{ + int idx = intelhaddata->bd_head; + int ofs = intelhaddata->pcmbuf_filled * intelhaddata->period_bytes; + u32 addr = substream->runtime->dma_addr + ofs; + + addr |= AUD_BUF_VALID | AUD_BUF_INTR_EN; + had_write_register(intelhaddata, AUD_BUF_ADDR(idx), addr); + had_write_register(intelhaddata, AUD_BUF_LEN(idx), + intelhaddata->period_bytes); + + /* advance the indices to the next */ + intelhaddata->bd_head++; + intelhaddata->bd_head %= intelhaddata->num_bds; + intelhaddata->pcmbuf_filled++; + intelhaddata->pcmbuf_filled %= substream->runtime->periods; +} + +/* invalidate a buffer descriptor with the given index */ +static void had_invalidate_bd(struct snd_intelhad *intelhaddata, + int idx) +{ + had_write_register(intelhaddata, AUD_BUF_ADDR(idx), 0); + had_write_register(intelhaddata, AUD_BUF_LEN(idx), 0); +} + +/* Initial programming of ring buffer */ +static void had_init_ringbuf(struct snd_pcm_substream *substream, + struct snd_intelhad *intelhaddata) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + int i, num_periods; + + num_periods = runtime->periods; + intelhaddata->num_bds = min(num_periods, HAD_NUM_OF_RING_BUFS); + intelhaddata->period_bytes = + frames_to_bytes(runtime, runtime->period_size); + WARN_ON(intelhaddata->period_bytes & 0x3f); + + intelhaddata->bd_head = 0; + intelhaddata->pcmbuf_head = 0; + intelhaddata->pcmbuf_filled = 0; + + for (i = 0; i < HAD_NUM_OF_RING_BUFS; i++) { + if (i < num_periods) + had_prog_bd(substream, intelhaddata); + else /* invalidate the rest */ + had_invalidate_bd(intelhaddata, i); + } + + intelhaddata->bd_head = 0; /* reset at head again before starting */ +} + +/* process a bd, advance to the next */ +static void had_advance_ringbuf(struct snd_pcm_substream *substream, + struct snd_intelhad *intelhaddata) +{ + int num_periods = substream->runtime->periods; + + /* reprogram the next buffer */ + had_prog_bd(substream, intelhaddata); + + /* proceed to next */ + intelhaddata->pcmbuf_head++; + intelhaddata->pcmbuf_head %= num_periods; +} + +/* process the current BD(s); + * returns the current PCM buffer byte position, or -EPIPE for underrun. + */ +static int had_process_ringbuf(struct snd_pcm_substream *substream, + struct snd_intelhad *intelhaddata) +{ + int len, processed; + unsigned long flags; + + processed = 0; + spin_lock_irqsave(&intelhaddata->had_spinlock, flags); + for (;;) { + /* get the remaining bytes on the buffer */ + had_read_register(intelhaddata, + AUD_BUF_LEN(intelhaddata->bd_head), + &len); + if (len < 0 || len > intelhaddata->period_bytes) { + dev_dbg(intelhaddata->dev, "Invalid buf length %d\n", + len); + len = -EPIPE; + goto out; + } + + if (len > 0) /* OK, this is the current buffer */ + break; + + /* len=0 => already empty, check the next buffer */ + if (++processed >= intelhaddata->num_bds) { + len = -EPIPE; /* all empty? - report underrun */ + goto out; + } + had_advance_ringbuf(substream, intelhaddata); + } + + len = intelhaddata->period_bytes - len; + len += intelhaddata->period_bytes * intelhaddata->pcmbuf_head; + out: + spin_unlock_irqrestore(&intelhaddata->had_spinlock, flags); + return len; +} + +/* called from irq handler */ +static void had_process_buffer_done(struct snd_intelhad *intelhaddata) +{ + struct snd_pcm_substream *substream; + + if (!intelhaddata->connected) + return; /* disconnected? - bail out */ + + substream = had_substream_get(intelhaddata); + if (!substream) + return; /* no stream? - bail out */ + + /* process or stop the stream */ + if (had_process_ringbuf(substream, intelhaddata) < 0) + snd_pcm_stop_xrun(substream); + else + snd_pcm_period_elapsed(substream); + + had_substream_put(intelhaddata); +} + #define MAX_CNT 0xFF

-static void snd_intelhad_handle_underrun(struct snd_intelhad *intelhaddata) +/* + * The interrupt status 'sticky' bits might not be cleared by + * setting '1' to that bit once... + */ +static void wait_clear_underrun_bit(struct snd_intelhad *intelhaddata) +{ + int i; + u32 val; + + for (i = 0; i < MAX_CNT; i++) { + /* clear bit30, 31 AUD_HDMI_STATUS */ + had_read_register(intelhaddata, AUD_HDMI_STATUS, &val); + if (!(val & AUD_CONFIG_MASK_UNDERRUN)) + return; + had_write_register(intelhaddata, AUD_HDMI_STATUS, val); + } + dev_err(intelhaddata->dev, "Unable to clear UNDERRUN bits\n"); +} + +/* called from irq handler */ +static void had_process_buffer_underrun(struct snd_intelhad *intelhaddata) { - u32 hdmi_status = 0, i = 0; + struct snd_pcm_substream *substream;

/* Handle Underrun interrupt within Audio Unit */ had_write_register(intelhaddata, AUD_CONFIG, 0); /* Reset buffer pointers */ had_reset_audio(intelhaddata); - /* - * The interrupt status 'sticky' bits might not be cleared by - * setting '1' to that bit once... - */ - do { /* clear bit30, 31 AUD_HDMI_STATUS */ - had_read_register(intelhaddata, AUD_HDMI_STATUS, - &hdmi_status); - dev_dbg(intelhaddata->dev, "HDMI status =0x%x\n", hdmi_status); - if (hdmi_status & AUD_CONFIG_MASK_UNDERRUN) { - i++; - had_write_register(intelhaddata, - AUD_HDMI_STATUS, hdmi_status); - } else - break; - } while (i < MAX_CNT); - if (i >= MAX_CNT) - dev_err(intelhaddata->dev, "Unable to clear UNDERRUN bits\n"); + + wait_clear_underrun_bit(intelhaddata); + + if (!intelhaddata->connected) + return; /* disconnected? - bail out */ + + /* Report UNDERRUN error to above layers */ + substream = had_substream_get(intelhaddata); + if (substream) { + snd_pcm_stop_xrun(substream); + had_substream_put(intelhaddata); + } }

/* @@ -957,11 +1065,6 @@ static int had_pcm_open(struct snd_pcm_substream *substream) intelhaddata->stream_info.substream_refcount++; spin_unlock_irq(&intelhaddata->had_spinlock);

- /* these are cleared in prepare callback, but just to be sure */ - intelhaddata->curr_buf = 0; - intelhaddata->underrun_count = 0; - intelhaddata->stream_info.buffer_rendered = 0; - return retval; error: pm_runtime_put(intelhaddata->dev); @@ -1123,10 +1226,6 @@ static int had_pcm_prepare(struct snd_pcm_substream *substream) dev_dbg(intelhaddata->dev, "rate=%d\n", runtime->rate); dev_dbg(intelhaddata->dev, "channels=%d\n", runtime->channels);

- intelhaddata->curr_buf = 0; - intelhaddata->underrun_count = 0; - intelhaddata->stream_info.buffer_rendered = 0; - /* Get N value in KHz */ disp_samp_freq = intelhaddata->tmds_clock_speed;

@@ -1148,8 +1247,7 @@ static int had_pcm_prepare(struct snd_pcm_substream *substream) retval = had_init_audio_ctrl(substream, intelhaddata);

/* Prog buffer address */ - retval = snd_intelhad_prog_buffer(substream, intelhaddata, - HAD_BUF_TYPE_A, HAD_BUF_TYPE_D); + had_init_ringbuf(substream, intelhaddata);

/* * Program channel mapping in following order: @@ -1168,48 +1266,17 @@ static int had_pcm_prepare(struct snd_pcm_substream *substream) static snd_pcm_uframes_t had_pcm_pointer(struct snd_pcm_substream *substream) { struct snd_intelhad *intelhaddata; - u32 bytes_rendered = 0; - u32 t; - int buf_id; + int len;

intelhaddata = snd_pcm_substream_chip(substream);

if (!intelhaddata->connected) return SNDRV_PCM_POS_XRUN;

- /* Use a hw register to calculate sub-period position reports. - * This makes PulseAudio happier. - */ - - buf_id = intelhaddata->curr_buf % 4; - had_read_register(intelhaddata, - AUD_BUF_A_LENGTH + (buf_id * HAD_REG_WIDTH), &t); - - if ((t == 0) || (t == ((u32)-1L))) { - intelhaddata->underrun_count++; - dev_dbg(intelhaddata->dev, - "discovered buffer done for buf %d, count = %d\n", - buf_id, intelhaddata->underrun_count); - - if (intelhaddata->underrun_count > (HAD_MIN_PERIODS/2)) { - dev_dbg(intelhaddata->dev, - "assume audio_codec_reset, underrun = %d - do xrun\n", - intelhaddata->underrun_count); - return SNDRV_PCM_POS_XRUN; - } - } else { - /* Reset Counter */ - intelhaddata->underrun_count = 0; - } - - t = intelhaddata->buf_info[buf_id].buf_size - t; - - if (intelhaddata->stream_info.buffer_rendered) - div_u64_rem(intelhaddata->stream_info.buffer_rendered, - intelhaddata->stream_info.ring_buf_size, - &(bytes_rendered)); - - return bytes_to_frames(substream->runtime, bytes_rendered + t); + len = had_process_ringbuf(substream, intelhaddata); + if (len < 0) + return SNDRV_PCM_POS_XRUN; + return bytes_to_frames(substream->runtime, len); }

/* @@ -1278,179 +1345,9 @@ static int had_process_mode_change(struct snd_intelhad *intelhaddata) return retval; }

-static inline int had_chk_intrmiss(struct snd_intelhad *intelhaddata, - enum intel_had_aud_buf_type buf_id) -{ - int i, intr_count = 0; - enum intel_had_aud_buf_type buff_done; - u32 buf_size, buf_addr; - - buff_done = buf_id; - - intr_count = snd_intelhad_read_len(intelhaddata); - if (intr_count > 1) { - /* In case of active playback */ - dev_err(intelhaddata->dev, - "Driver detected %d missed buffer done interrupt(s)\n", - (intr_count - 1)); - if (intr_count > 3) - return intr_count; - - buf_id += (intr_count - 1); - /* Reprogram registers*/ - for (i = buff_done; i < buf_id; i++) { - int j = i % 4; - - buf_size = intelhaddata->buf_info[j].buf_size; - buf_addr = intelhaddata->buf_info[j].buf_addr; - had_write_register(intelhaddata, - AUD_BUF_A_LENGTH + - (j * HAD_REG_WIDTH), buf_size); - had_write_register(intelhaddata, - AUD_BUF_A_ADDR+(j * HAD_REG_WIDTH), - (buf_addr | BIT(0) | BIT(1))); - } - buf_id = buf_id % 4; - intelhaddata->buff_done = buf_id; - } - - return intr_count; -} - -/* called from irq handler */ -static int had_process_buffer_done(struct snd_intelhad *intelhaddata) -{ - u32 len = 1; - enum intel_had_aud_buf_type buf_id; - enum intel_had_aud_buf_type buff_done; - struct pcm_stream_info *stream; - struct snd_pcm_substream *substream; - u32 buf_size; - int intr_count; - unsigned long flags; - - stream = &intelhaddata->stream_info; - intr_count = 1; - - spin_lock_irqsave(&intelhaddata->had_spinlock, flags); - if (!intelhaddata->connected) { - spin_unlock_irqrestore(&intelhaddata->had_spinlock, flags); - dev_dbg(intelhaddata->dev, - "%s:Device already disconnected\n", __func__); - return 0; - } - buf_id = intelhaddata->curr_buf; - intelhaddata->buff_done = buf_id; - buff_done = intelhaddata->buff_done; - buf_size = intelhaddata->buf_info[buf_id].buf_size; - - /* Every debug statement has an implication - * of ~5msec. Thus, avoid having >3 debug statements - * for each buffer_done handling. - */ - - /* Check for any intr_miss in case of active playback */ - if (stream->running) { - intr_count = had_chk_intrmiss(intelhaddata, buf_id); - if (!intr_count || (intr_count > 3)) { - spin_unlock_irqrestore(&intelhaddata->had_spinlock, - flags); - dev_err(intelhaddata->dev, - "HAD SW state in non-recoverable mode\n"); - return 0; - } - buf_id += (intr_count - 1); - buf_id = buf_id % 4; - } - - intelhaddata->buf_info[buf_id].is_valid = true; - if (intelhaddata->valid_buf_cnt-1 == buf_id) { - if (stream->running) - intelhaddata->curr_buf = HAD_BUF_TYPE_A; - } else - intelhaddata->curr_buf = buf_id + 1; - - spin_unlock_irqrestore(&intelhaddata->had_spinlock, flags); - - if (!intelhaddata->connected) { - dev_dbg(intelhaddata->dev, "HDMI cable plugged-out\n"); - return 0; - } - - /* Reprogram the registers with addr and length */ - had_write_register(intelhaddata, - AUD_BUF_A_LENGTH + (buf_id * HAD_REG_WIDTH), - buf_size); - had_write_register(intelhaddata, - AUD_BUF_A_ADDR + (buf_id * HAD_REG_WIDTH), - intelhaddata->buf_info[buf_id].buf_addr | - BIT(0) | BIT(1)); - - had_read_register(intelhaddata, - AUD_BUF_A_LENGTH + (buf_id * HAD_REG_WIDTH), - &len); - dev_dbg(intelhaddata->dev, "%s:Enabled buf[%d]\n", __func__, buf_id); - - /* In case of actual data, - * report buffer_done to above ALSA layer - */ - substream = had_substream_get(intelhaddata); - if (substream) { - buf_size = intelhaddata->buf_info[buf_id].buf_size; - intelhaddata->stream_info.buffer_rendered += - (intr_count * buf_size); - snd_pcm_period_elapsed(substream); - had_substream_put(intelhaddata); - } - - return 0; -} - -/* called from irq handler */ -static int had_process_buffer_underrun(struct snd_intelhad *intelhaddata) -{ - enum intel_had_aud_buf_type buf_id; - struct pcm_stream_info *stream; - struct snd_pcm_substream *substream; - unsigned long flags; - int connected; - - stream = &intelhaddata->stream_info; - - spin_lock_irqsave(&intelhaddata->had_spinlock, flags); - buf_id = intelhaddata->curr_buf; - intelhaddata->buff_done = buf_id; - connected = intelhaddata->connected; - if (stream->running) - intelhaddata->curr_buf = HAD_BUF_TYPE_A; - - spin_unlock_irqrestore(&intelhaddata->had_spinlock, flags); - - dev_dbg(intelhaddata->dev, "Enter:%s buf_id=%d, stream_running=%d\n", - __func__, buf_id, stream->running); - - snd_intelhad_handle_underrun(intelhaddata); - - if (!connected) { - dev_dbg(intelhaddata->dev, - "%s:Device already disconnected\n", __func__); - return 0; - } - - /* Report UNDERRUN error to above layers */ - substream = had_substream_get(intelhaddata); - if (substream) { - snd_pcm_stop_xrun(substream); - had_substream_put(intelhaddata); - } - - return 0; -} - /* process hot plug, called from wq with mutex locked */ static void had_process_hot_plug(struct snd_intelhad *intelhaddata) { - enum intel_had_aud_buf_type buf_id; struct snd_pcm_substream *substream;

spin_lock_irq(&intelhaddata->had_spinlock); @@ -1460,17 +1357,12 @@ static void had_process_hot_plug(struct snd_intelhad *intelhaddata) return; }

- buf_id = intelhaddata->curr_buf; - intelhaddata->buff_done = buf_id; intelhaddata->connected = true; dev_dbg(intelhaddata->dev, "%s @ %d:DEBUG PLUG/UNPLUG : HAD_DRV_CONNECTED\n", __func__, __LINE__); spin_unlock_irq(&intelhaddata->had_spinlock);

- dev_dbg(intelhaddata->dev, "Processing HOT_PLUG, buf_id = %d\n", - buf_id); - /* Safety check */ substream = had_substream_get(intelhaddata); if (substream) { @@ -1487,11 +1379,8 @@ static void had_process_hot_plug(struct snd_intelhad *intelhaddata) /* process hot unplug, called from wq with mutex locked */ static void had_process_hot_unplug(struct snd_intelhad *intelhaddata) { - enum intel_had_aud_buf_type buf_id; struct snd_pcm_substream *substream;

- buf_id = intelhaddata->curr_buf; - substream = had_substream_get(intelhaddata);

spin_lock_irq(&intelhaddata->had_spinlock); @@ -1862,13 +1751,12 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));

- /* allocate dma pages for ALSA stream operations - * memory allocated is based on size, not max value - * thus using same argument for max & size + /* allocate dma pages; + * try to allocate 600k buffer as default which is large enough */ snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, NULL, - HAD_MAX_BUFFER, HAD_MAX_BUFFER); + HAD_DEFAULT_BUFFER, HAD_MAX_BUFFER);

/* create controls */ for (i = 0; i < ARRAY_SIZE(had_controls); i++) { diff --git a/sound/x86/intel_hdmi_audio.h b/sound/x86/intel_hdmi_audio.h index 9f713a8a88bc..7e2546b853ca 100644 --- a/sound/x86/intel_hdmi_audio.h +++ b/sound/x86/intel_hdmi_audio.h @@ -64,24 +64,15 @@

struct pcm_stream_info { struct snd_pcm_substream *substream; - u64 buffer_rendered; - u32 ring_buf_size; int substream_refcount; bool running; };

-struct ring_buf_info { - u32 buf_addr; - u32 buf_size; - u8 is_valid; -}; - /* * struct snd_intelhad - intelhad driver structure * * @card: ptr to hold card details * @connected: the monitor connection status - * @buf_info: ring buffer info * @stream_info: stream information * @eld: holds ELD info * @curr_buf: pointer to hold current active ring buf @@ -91,26 +82,29 @@ struct ring_buf_info { * @buff_done: id of current buffer done intr * @dev: platoform device handle * @chmap: holds channel map info - * @underrun_count: PCM stream underrun counter */ struct snd_intelhad { struct snd_card *card; bool connected; - struct ring_buf_info buf_info[HAD_NUM_OF_RING_BUFS]; struct pcm_stream_info stream_info; unsigned char eld[HDMI_MAX_ELD_BYTES]; bool dp_output; - enum intel_had_aud_buf_type curr_buf; - int valid_buf_cnt; unsigned int aes_bits; spinlock_t had_spinlock; - enum intel_had_aud_buf_type buff_done; struct device *dev; struct snd_pcm_chmap *chmap; - int underrun_count; int tmds_clock_speed; int link_rate;

+ /* ring buffer (BD) position index */ + unsigned int bd_head; + /* PCM buffer position indices */ + unsigned int pcmbuf_head; /* being processed */ + unsigned int pcmbuf_filled; /* to be filled */ + + unsigned int num_bds; /* number of BDs */ + unsigned int period_bytes; /* PCM period size in bytes */ + /* internal stuff */ int irq; void __iomem *mmio_start; diff --git a/sound/x86/intel_hdmi_lpe_audio.h b/sound/x86/intel_hdmi_lpe_audio.h index be9783910a3a..ca4212dca94e 100644 --- a/sound/x86/intel_hdmi_lpe_audio.h +++ b/sound/x86/intel_hdmi_lpe_audio.h @@ -28,13 +28,13 @@ #define HAD_MAX_CHANNEL 8 #define HAD_NUM_OF_RING_BUFS 4

-/* Assume 192KHz, 8channel, 25msec period */ -#define HAD_MAX_BUFFER (600*1024) -#define HAD_MIN_BUFFER (32*1024) -#define HAD_MAX_PERIODS 4 -#define HAD_MIN_PERIODS 4 -#define HAD_MAX_PERIOD_BYTES (HAD_MAX_BUFFER/HAD_MIN_PERIODS) -#define HAD_MIN_PERIOD_BYTES 256 +/* max 20bit address, aligned to 64 */ +#define HAD_MAX_BUFFER ((1024 * 1024 - 1) & ~0x3f) +#define HAD_DEFAULT_BUFFER (600 * 1024) /* default prealloc size */ +#define HAD_MAX_PERIODS 256 /* arbitrary, but should suffice */ +#define HAD_MIN_PERIODS 2 +#define HAD_MAX_PERIOD_BYTES ((HAD_MAX_BUFFER / HAD_MIN_PERIODS) & ~0x3f) +#define HAD_MIN_PERIOD_BYTES 1024 /* might be smaller */ #define HAD_FIFO_SIZE 0 /* fifo not being used */ #define MAX_SPEAKERS 8

@@ -82,14 +82,6 @@ /* Naud Value */ #define DP_NAUD_VAL 32768

-/* enum intel_had_aud_buf_type - HDMI controller ring buffer types */ -enum intel_had_aud_buf_type { - HAD_BUF_TYPE_A = 0, - HAD_BUF_TYPE_B = 1, - HAD_BUF_TYPE_C = 2, - HAD_BUF_TYPE_D = 3, -}; - /* HDMI Controller register offsets - audio domain common */ /* Base address for below regs = 0x65000 */ enum hdmi_ctrl_reg_offset_common { @@ -274,6 +266,9 @@ union aud_buf_addr { u32 regval; };

+#define AUD_BUF_VALID (1U << 0) +#define AUD_BUF_INTR_EN (1U << 1) + /* Length of Audio Buffer */ union aud_buf_len { struct {