On Tue, 07 Feb 2017 00:56:54 +0100, Pierre-Louis Bossart wrote:
On 2/6/17 3:45 PM, Takashi Iwai wrote:
On Mon, 06 Feb 2017 22:16:22 +0100, Pierre-Louis Bossart wrote:
On 2/6/17 1:07 PM, Takashi Iwai wrote:
On Fri, 03 Feb 2017 17:43:56 +0100, Takashi Iwai wrote:
Hi,
this is the final part of my cleanup / refactoring patchsets for Intel LPE audio. Now the PCM stream engine has been rewritten. It supports more flexible configuration, not only fixed 4 periods, for example, yet with a lot of code reduction.
As usual, the patches are found in topic/intel-lpe-audio-cleanup branch.
So far, from my side, now most of changes have been submitted / merged. But I still have some unclear things in the code.
- The PCM format: the code contains the handling of S16_LE, but it doesn't seem work when I enabled the info bit. What's missing? Also U24_LE format is ever supported...? How?
In the initial version the samples were assumed to be using the 24 lsb of a 32-bit word. 16-bit data can be supported as long as it was left-shifted before being written to the ring buffer.
For stereo the channels are assumed to be interleaved in Layout 0 (left, right). For more than 2ch, all channels for the same sample are inserted consecutively (ie. no blocks of channels). In the initial version the number of channels is assumed to be even and one bogus channel had to be inserted for odd number of channels.
in more recent versions the samples could also be represented as using the 24 msb and the requirement for the bogus channel was removed but I never worked on this personally and the documentation is far from clear.
OK, that's interesting. So we may support even S32_LE format, too.
well, yes but the 8 LSBs would be ignored, you can only transmit 24 bits with HDMI (it's 28 bits total per sample with 4 status bits).
Yes, S32_LE indicates only the format, and the effective bits are specified in hwparam.msbits field additionally.
The odd channel restriction isn't seen in the current code, indeed. It's worth to try 3 channels tomorrow :)
At any rate, here's what I see in the description of the STREAM_X_LPE_AUD_CONFIG registers for CHT:
Bit 15: 1: DP mode 0: HDMI (default)
Bit14: 1: no bogus sample for odd channels 0: bogus sample for off channels (default)
Bit13: 1: MSB is bit 31 of 32-bit container 0: MSB is bit 23 of 32-bit container (default)
Bit 12: 1: 16-bit container 0: 32-bit container (default)
Bit 11: 1: send silence stream 0: send null packets (default)
So this corresponds to the hw_silence stuff you mentioned?
yes. I am still trying to figure out how it's supposed to be used (you need a valid configuration first).
Judging from the field name, this influences on the underrun behavior. If so, just setting all BDs to invalid (and set AUD_CONFIG enable bit) should suffice?
Note that the hardware can swap channels, I don't remember if this is currently enabled.
We have the following code:
#define SWAP_LFE_CENTER 0x00fac4c8
/* * Program channel mapping in following order: * FL, FR, C, LFE, RL, RR */
had_write_register(intelhaddata, AUD_BUF_CH_SWAP, SWAP_LFE_CENTER);
0x00fac4c8 is octal 74543210, so it's jus a plain order.
Yes, the 24 LBSs (23:0) in this register are split in 8 3-bit subfields, e.g if you program 7 (111b) in bits 2:0 and 0 in bits 23:21 you would swap first and last channels. This only works for multichannel (layout 1), no support for l/r swap.
OK, if such a restriction is present, maybe it's no good idea to allow the flexible mapping via chmap API. Safer to keep the chmap as read-only.
There are two possible interrupt sources in the STREAM_X_LPE_AUD_HDMI_STATUS (65064h) Bit 31: sample buffer underrun. HDMI audio unit did not have enough samples to insert in video frame (cleared by writing a one) Bit 29: buffer done status, write 1 to clear Bit 15: sample buffer underrun interrupt enable
OK, then the current code seems naming the function wrongly. It names had_enable_audio_int() but actually this should be named as had_clear_interrupts() or such.
I think enable and clear are two separate steps, you only clear what you've enabled, no?
Not really. In the original code, it corresponds to hdmi_audio_set_caps():
int hdmi_audio_set_caps(enum had_caps_list set_element, void *capabilties) { ..... switch (set_element) {
case HAD_SET_ENABLE_AUDIO_INT: { u32 status_reg;
hdmi_audio_read(AUD_HDMI_STATUS_v2 + ctx->had_config_offset, &status_reg); status_reg |= HDMI_AUDIO_BUFFER_DONE | HDMI_AUDIO_UNDERRUN; hdmi_audio_write(AUD_HDMI_STATUS_v2 + ctx->had_config_offset, status_reg); hdmi_audio_read(AUD_HDMI_STATUS_v2 + ctx->had_config_offset, &status_reg);
}
And there is no counterpart, HAD_SET_DISABLE_AUDIO_INT. I wasn't sure about the code above, but now I understand that these are simply ACKing both BUFFER_DONE and UNDERRUN bits unconditionally, and do a pre- and a post-read. Effectively, it forcibly clears the interrupts.
thanks,
Takashi