Hi Lars,
first of all thanks a lot for your detailled feedback!
On Tue, Apr 26, 2016 at 10:47:05AM +0200, Lars-Peter Clausen wrote:
> >>> + .periods_min = 2,
> >>> + .periods_max = 255,
> >>> + .buffer_bytes_max = 128 * PAGE_SIZE,
> >>> +};
> >>> +
> >>> +static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = {
> >>> + .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
> >>> + .pcm_hardware = &bcm2835_pcm_hardware,
> >>> + .prealloc_buffer_size = 256 * PAGE_SIZE,
> >>> +};
> >>
> >> The generic dmaengine PCM driver auto-discovers these things, no need to
> >> provide them. The code is OK as it is.
> >
> > With the auto-discover code we loose the S16_LE format.
> >
> > If I understood the code in dmaengine_pcm_set_runtime_hwparams
> > correctly, this is because the DMA controller doesn't support
> > 16bit transfers (only multiples of 32bit are allowed).
> >
> > But since the I2S driver needs exactly 2 channels S16_LE actually
> > works fine (one 32bit transfer per frame).
> >
> > Do you know of a better way to get S16_LE support? It could well
> > be that I missed something important...
>
> With your patch we should just get an error when trying to configure a
> stream with 16-bit samples since when setting up the DMA controller the
> generic code will still choose a 16-bit device port size and this will be
> rejected by the DMA controller.
We had that code in downstream RPi kernel for ages (IIRC since
3.10) and so far it worked fine.
I traced the code for the S16_LE case to find out why:
snd_hwparams_to_dma_slave_config() sets src/dst_addr_width to 16bit.
But immediately afterwards snd_dmaengine_pcm_set_config_from_dai_data()
overrides addr_width to 32bit.
bcm2835-i2s only supports 32bit access to the FIFO/data register
and dma_data.addr_width is set to 32bit in the I2S driver - that
code is also in mainline since the initial bcm2835-i2s commit.
Of course all this only works because the number of channels
is always 2.
> When you look at peripherals that have DMA support there are basically two
> types.
>
> Type A expect that each write (same applies for read) to the memory mapped
> FIFO corresponds to exactly one sample. So if the sample width is 16-bit a
> 16-bit write is done, if the sample width is 32-bit a 32-bit write is done.
> In this case it is up to the DMA controller to fragment the byte stream into
> individual samples.
>
> Type B on the other hand has a fixed port width (usually the bus size) and
> expects that each write to the memory mapped FIFO uses the port width. It
> then internally unpacks the data into the sample data. E.g. if the FIFO is
> 32-bit wide and the sample width is 16-bit it will unpack the 32-bit entry
> into two samples.
>
> Currently the generic code only supports type A. It would be great if you
> could add support for type B to support the BCM2835 I2S controller properly.
Do you have a particular solution in mind?
Introducing a flag to just auto-add some packed formats would be
easy, but a generic, robust solution might be tricky. We'd have
to make sure that unsupported configurations (like an odd number
of 16bit channels on 32bit-only setups) get rejected or we might
be in trouble.
so long,
Hias
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
