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. 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? > I see no evidence that unsigned data is supported. Yeah, and I really doubt whether unsigned format is supported at all. Better to drop it. > I would really take all these configurations with caution, it's not > clear to me if the documentation reflects the actual implementation. Sure, it's just for fun for now. > 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. But this reminds me that I didn't check the correctness of chmap order in the current driver. Another thing to try tomorrow. > > > > > - The behavior in had_process_buffer_underrun() isn't clear enough. > > Or, more exactly, the behavior of AUD_HDMI_STATUS reg and AUD_CONFIG > > reg look complex, and need a bit more description for readers > > (including me). > > > > Can you guys enlighten a bit? > > For the LPE_AUDIO_Buffer_config register (65020h), there is one > important field > 10:8: DMA Fifo watermark. The audio unit has 8x64 bytes, this register > define when to fetch data. Bottom line it'd be wise to avoid rewinding > below that FIFO size... Right, this may be needed to be exposed. > 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. > > Hope this helps Very appreciated, thank you for prompt answer! Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
