diff mbox

[12/13] drm: bridge/dw_hdmi-ahb-audio: add audio driver

Message ID E1Yr1y8-0006lG-Mw@rmk-PC.arm.linux.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Russell King May 9, 2015, 10:26 a.m. UTC
Add ALSA based HDMI AHB audio driver for dw_hdmi.  The only buffer
format supported by the hardware is its own special IEC958 based format,
which is not compatible with any ALSA format.  To avoid doing too much
data manipulation within the driver, we support only ALSAs IEC958 LE and
24-bit PCM formats for 2 to 6 channels, which we convert to its hardware
format.

A more desirable solution would be to have this conversion in userspace,
but ALSA does not appear to allow such transformations outside of
libasound itself.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/bridge/Kconfig             |  10 +
 drivers/gpu/drm/bridge/Makefile            |   1 +
 drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c | 560 +++++++++++++++++++++++++++++
 drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h |  13 +
 drivers/gpu/drm/bridge/dw_hdmi.c           |  24 ++
 drivers/gpu/drm/bridge/dw_hdmi.h           |   3 +
 6 files changed, 611 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c
 create mode 100644 drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h

Comments

Anssi Hannula May 9, 2015, 4:49 p.m. UTC | #1
Hi,

A couple of things I noticed below:

09.05.2015, 13:26, Russell King kirjoitti:
> Add ALSA based HDMI AHB audio driver for dw_hdmi.  The only buffer
> format supported by the hardware is its own special IEC958 based format,
> which is not compatible with any ALSA format.  To avoid doing too much
> data manipulation within the driver, we support only ALSAs IEC958 LE and
> 24-bit PCM formats for 2 to 6 channels, which we convert to its hardware
> format.
> 
> A more desirable solution would be to have this conversion in userspace,
> but ALSA does not appear to allow such transformations outside of
> libasound itself.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/gpu/drm/bridge/Kconfig             |  10 +
>  drivers/gpu/drm/bridge/Makefile            |   1 +
>  drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c | 560 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h |  13 +
>  drivers/gpu/drm/bridge/dw_hdmi.c           |  24 ++
>  drivers/gpu/drm/bridge/dw_hdmi.h           |   3 +
>  6 files changed, 611 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c
>  create mode 100644 drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h
> 
[...]
> +static void dw_hdmi_create_cs(struct snd_dw_hdmi *dw,
> +	struct snd_pcm_runtime *runtime)
> +{
> +	u8 cs[4];
> +	unsigned ch, i, j;
> +
> +	snd_pcm_create_iec958_consumer(runtime, cs, sizeof(cs));


I think generally drivers have left the iec958 bits for userspace to
handle, i.e. via a "IEC958 Playback Default"
(SNDRV_CTL_NAME_IEC958("",PLAYBACK,DEFAULT)) mixer element, with
defaults coming from /usr/share/alsa/pcm/hdmi.conf...

Looking at the sound driver tree, now, though, they are already somewhat
inconsistent:

1. Most drivers: iec958 bits from userspace, except for hw-set bits.

2. Some drivers (e.g. ctxfi, fsl_spdif): Some bits such as rate set by
driver, but everything is also exposed to userspace.
At least in fsl_spdif case the driver sets the stuff in hw_params which
would then get overwritten by userspace (which sets them after hw_params).

3. Some drivers (e.g. omap-hdmi-audio): Set by driver, not exposed to
userspace, like in this patch.

(Of course having userspace set them requires that the device has a
proper entry in /usr/share/alsa/cards and the pcm device is accessed via
the standard "hdmi" or "iec958" device names which perform the channel
status word setup. I guess the ARM SoC stuff generally doesn't bother
with that, explaining a bit why some kernel drivers set them by themselves).

The main interest to iec958 bits from userspace is probably towards the
non-audio bit, used for audio passthrough (the bit is not mandatory
there, but it helps).

Other drivers (well, I guess just HDA has this so far) also use the
userspace-provided non-audio bit to also select (in conjunction with
channels==8) the HBR mode, i.e. HD audio passthrough (which dw_hdmi also
supports via DMA_CONF0 but not enabled on this patch).


Since this isn't the first driver doing things this way, and this
doesn't really prevent exposing them to userspace later on, I guess this
patch is still OK here, unless the ALSA people think otherwise...


[...]
> +static struct snd_pcm_hardware dw_hdmi_hw = {
> +	.info = SNDRV_PCM_INFO_INTERLEAVED |
> +		SNDRV_PCM_INFO_BLOCK_TRANSFER |
> +		SNDRV_PCM_INFO_MMAP |
> +		SNDRV_PCM_INFO_MMAP_VALID,
> +	.formats = SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE |
> +		   SNDRV_PCM_FMTBIT_S24_LE,
> +	.rates = SNDRV_PCM_RATE_32000 |
> +		 SNDRV_PCM_RATE_44100 |
> +		 SNDRV_PCM_RATE_48000 |
> +		 SNDRV_PCM_RATE_88200 |
> +		 SNDRV_PCM_RATE_96000 |
> +		 SNDRV_PCM_RATE_176400 |
> +		 SNDRV_PCM_RATE_192000,
> +	.channels_min = 2,
> +	.channels_max = 8,

You are providing multichannel support, but AFAICS you are not setting
the channel allocation (CA) bits in the audio infoframe anywhere
(HDMI_FC_AUDICONF2 register).

HDMI_FC_AUDICONF2 register default value is 0x00, which means plain
stereo (per CEA-861). If this is what goes on to the HDMI link as well,
the audio sink should ignore the other channels.
Did you check that multichannel PCM actually works? (maybe I'm just
missing where CA is set)


Note also that I think this HW uses the native HDMI channel order (from
CEA-861), which is different from the ALSA default channel order, so you
should inform userspace of the channel order (via
snd_pcm_add_chmap_ctls()). The channel order is specified by the CA
value I mentioned above.

Assuming I'm not missing something which makes everything work already,
one of these should be implemented:

(a) Provide all the chmaps (i.e. one per CA value) as a list for
userspace to select one, and provide the active chmap to userspace as well.

(b) Just hardcode a single CA value per channel count (which covers 99%
of use cases), and provide the corresponding active chmap to userspace.

(c) channels_max = 2.

Both (a) and (b) are generic stuff that could/should be in helpers for
all drivers to use (if (b), preferably with an interface that allows
easily extending it to (a) in the future).

Some of the code from sound/pci/hda/patch_hdmi.c should be useful (at
least the CA table it has - this stuff really shoud be outside the
driver). Note that much of the complexity of patch_hdmi.c comes from the
fact that the HW there supports channel remapping
(SNDRV_CTL_TLVT_CHMAP_VAR or _PAIRED), which dw_hdmi doesn't
(SNDRV_CTL_TLVT_CHMAP_FIXED).
Russell King - ARM Linux May 9, 2015, 4:55 p.m. UTC | #2
On Sat, May 09, 2015 at 07:49:44PM +0300, Anssi Hannula wrote:
> (Of course having userspace set them requires that the device has a
> proper entry in /usr/share/alsa/cards and the pcm device is accessed via
> the standard "hdmi" or "iec958" device names which perform the channel
> status word setup. I guess the ARM SoC stuff generally doesn't bother
> with that, explaining a bit why some kernel drivers set them by themselves).

I'm not sure that's sufficient - I haven't yet found where in the ALSA
userspace, the AES bits are appropriately set according to the sample
rate.

> HDMI_FC_AUDICONF2 register default value is 0x00, which means plain
> stereo (per CEA-861). If this is what goes on to the HDMI link as well,
> the audio sink should ignore the other channels.
> Did you check that multichannel PCM actually works? (maybe I'm just
> missing where CA is set)

I have no way to test multichannel support - not everyone has a plethora
of HDMI devices readily accessible.
Anssi Hannula May 9, 2015, 5:07 p.m. UTC | #3
09.05.2015, 19:55, Russell King - ARM Linux kirjoitti:
> On Sat, May 09, 2015 at 07:49:44PM +0300, Anssi Hannula wrote:
>> (Of course having userspace set them requires that the device has a
>> proper entry in /usr/share/alsa/cards and the pcm device is accessed via
>> the standard "hdmi" or "iec958" device names which perform the channel
>> status word setup. I guess the ARM SoC stuff generally doesn't bother
>> with that, explaining a bit why some kernel drivers set them by themselves).
> 
> I'm not sure that's sufficient - I haven't yet found where in the ALSA
> userspace, the AES bits are appropriately set according to the sample
> rate.

Right, that is left to the applications (e.g. VLC and Kodi do that). I'm
under the impression that sinks do not normally care about this value,
though, but that could just be because most desktop HW sets it by
themselves.

But indeed, that is one reason to have the kernel set it.
Russell King - ARM Linux May 9, 2015, 5:40 p.m. UTC | #4
On Sat, May 09, 2015 at 08:07:45PM +0300, Anssi Hannula wrote:
> 09.05.2015, 19:55, Russell King - ARM Linux kirjoitti:
> > On Sat, May 09, 2015 at 07:49:44PM +0300, Anssi Hannula wrote:
> >> (Of course having userspace set them requires that the device has a
> >> proper entry in /usr/share/alsa/cards and the pcm device is accessed via
> >> the standard "hdmi" or "iec958" device names which perform the channel
> >> status word setup. I guess the ARM SoC stuff generally doesn't bother
> >> with that, explaining a bit why some kernel drivers set them by themselves).
> > 
> > I'm not sure that's sufficient - I haven't yet found where in the ALSA
> > userspace, the AES bits are appropriately set according to the sample
> > rate.
> 
> Right, that is left to the applications (e.g. VLC and Kodi do that). I'm
> under the impression that sinks do not normally care about this value,
> though, but that could just be because most desktop HW sets it by
> themselves.

No, that seems totally wrong to me.

What if you open the device using aplay?  Or pulseaudio?  Or madplay?
Or another audio application which thinks it's addressing a standard
PCM device.

Why should every audio user have some code in it to generate the IEC
bits?

Even VLC _doesn't_ if it's outputting to a standard audio - in other
words, if you don't tick the SPDIF direct output option which defaults
to disabled (which, when enabled, opens the device passing the AES
bits _and_ permits it to send a compressed audio stream.)  I've looked
at this in VLC many times...

I think you're a little confused about what userspace does here.
Russell King - ARM Linux May 9, 2015, 5:53 p.m. UTC | #5
On Sat, May 09, 2015 at 06:40:54PM +0100, Russell King - ARM Linux wrote:
> Even VLC _doesn't_ if it's outputting to a standard audio - in other
> words, if you don't tick the SPDIF direct output option which defaults
> to disabled (which, when enabled, opens the device passing the AES
> bits _and_ permits it to send a compressed audio stream.)  I've looked
> at this in VLC many times...

FYI, here's the code:

    vlc_fourcc_t fourcc = aout->format.i_format;
    bool spdif = false;
    switch (fourcc)
    {
... other linear float/integer codec IDs ...
        case VLC_CODEC_U16B:
            pcm_format = SND_PCM_FORMAT_U16_BE;
            break;
        case VLC_CODEC_U16L:
            pcm_format = SND_PCM_FORMAT_U16_LE;
            break;
...
        default:
            if (AOUT_FMT_SPDIF(&aout->format))
                spdif = var_InheritBool (aout, "spdif");
            if (spdif)
            {
                fourcc = VLC_CODEC_SPDIFL;
                pcm_format = SND_PCM_FORMAT_S16;
            }
            else
            if (HAVE_FPU)
            {
                fourcc = VLC_CODEC_FL32;
                pcm_format = SND_PCM_FORMAT_FLOAT;
            }
            else
            {
                fourcc = VLC_CODEC_S16N;
                pcm_format = SND_PCM_FORMAT_S16;
            }
    }

    /* Choose the IEC device for S/PDIF output:
       if the device is overridden by the user then it will be the one.
       Otherwise we compute the default device based on the output format. */
    if (spdif && !strcmp (device, "default"))
    {
        unsigned aes3;

        switch (aout->format.i_rate)
        {
#define FS(freq) \
            case freq: aes3 = IEC958_AES3_CON_FS_ ## freq; break;
            FS( 44100) /* def. */ FS( 48000) FS( 32000)
            FS( 22050)            FS( 24000)
            FS( 88200) FS(768000) FS( 96000)
            FS(176400)            FS(192000)
#undef FS
            default:
                aes3 = IEC958_AES3_CON_FS_NOTID;
                break;
        }

        free (device);
        if (asprintf (&device,
                      "iec958:AES0=0x%x,AES1=0x%x,AES2=0x%x,AES3=0x%x",
                      IEC958_AES0_CON_EMPHASIS_NONE | IEC958_AES0_NONAUDIO,
                      IEC958_AES1_CON_ORIGINAL | IEC958_AES1_CON_PCM_CODER,
                      0, aes3) == -1)
            return VLC_ENOMEM;
    }
...
    /* VLC always has a resampler. No need for ALSA's. */
    const int mode = SND_PCM_NO_AUTO_RESAMPLE;

    int val = snd_pcm_open (&pcm, device, SND_PCM_STREAM_PLAYBACK, mode);

So, the result is:

* VLC opens the ALSA device _without_ AES information if the fourcc
  being passed if the format is a linear PCM type.

* If the format is not a linear PCM type _and_ it is a format which
  satisfies AOUT_FMT_SPDIF(), _and_ the SPDIF passthrough option is
  enabled, _then_ we open the IEC958 audio output device with the
  first four AES data bytes specified with SND_PCM_FORMAT_S16.
  Moreover, the first data byte always indicates that the stream is
  non-audio.

So, in the case of linear PCM, the AES data bytes are *not* specified
by VLC.

So, if we don't have the kernel driver specifying the IEC958 information
for HDMI/SPDIF outputs, and users of ALSA's userspace APIs don't generate
them for linear PCM, how are the required AES data bytes generated?
Anssi Hannula May 9, 2015, 5:55 p.m. UTC | #6
09.05.2015, 20:40, Russell King - ARM Linux kirjoitti:
> On Sat, May 09, 2015 at 08:07:45PM +0300, Anssi Hannula wrote:
>> 09.05.2015, 19:55, Russell King - ARM Linux kirjoitti:
>>> On Sat, May 09, 2015 at 07:49:44PM +0300, Anssi Hannula wrote:
>>>> (Of course having userspace set them requires that the device has a
>>>> proper entry in /usr/share/alsa/cards and the pcm device is accessed via
>>>> the standard "hdmi" or "iec958" device names which perform the channel
>>>> status word setup. I guess the ARM SoC stuff generally doesn't bother
>>>> with that, explaining a bit why some kernel drivers set them by themselves).
>>>
>>> I'm not sure that's sufficient - I haven't yet found where in the ALSA
>>> userspace, the AES bits are appropriately set according to the sample
>>> rate.
>>
>> Right, that is left to the applications (e.g. VLC and Kodi do that). I'm
>> under the impression that sinks do not normally care about this value,
>> though, but that could just be because most desktop HW sets it by
>> themselves.
> 
> No, that seems totally wrong to me.
> 
> What if you open the device using aplay?  Or pulseaudio?  Or madplay?
> Or another audio application which thinks it's addressing a standard
> PCM device.

Then, unless the driver or HW sets it, it is not set.

> Why should every audio user have some code in it to generate the IEC
> bits?

Yeah, it makes zero sense of course, I wasn't claiming otherwise (sorry
if it seemed like it).

> Even VLC _doesn't_ if it's outputting to a standard audio - in other
> words, if you don't tick the SPDIF direct output option which defaults
> to disabled (which, when enabled, opens the device passing the AES
> bits _and_ permits it to send a compressed audio stream.)  I've looked
> at this in VLC many times...

That is my understanding as well. Same for pulseaudio, it doesn't set
any AES bits except for passthrough (and most other applications never
set them).

> I think you're a little confused about what userspace does here.
Russell King - ARM Linux May 9, 2015, 6:11 p.m. UTC | #7
On Sat, May 09, 2015 at 08:55:10PM +0300, Anssi Hannula wrote:
> 09.05.2015, 20:40, Russell King - ARM Linux kirjoitti:
> > Even VLC _doesn't_ if it's outputting to a standard audio - in other
> > words, if you don't tick the SPDIF direct output option which defaults
> > to disabled (which, when enabled, opens the device passing the AES
> > bits _and_ permits it to send a compressed audio stream.)  I've looked
> > at this in VLC many times...
> 
> That is my understanding as well. Same for pulseaudio, it doesn't set
> any AES bits except for passthrough (and most other applications never
> set them).

Right, so when you're dealing with HDMI, where it's required that the
AES bits contain accurate information, the only real option is to set
it appropriately in the driver if userspace doesn't specify the AES
data bits.

Now, with the dw-hdmi-ahb driver, I'm doing something sensible - and
yes, I do have a card file in /usr/share/alsa (see below).

What this does is ensure that linear PCM is converted to 24-bit PCM
(which makes the kernel conversion to the required hardware format
easier - I hate this, I'd much prefer it to be done in userspace.)

However, in the case of VLC, if it wants to send non-audio, it will
open the IEC958 device, which will use the iec958 plugin to configure
the AES bits for non-audio, and pass IEC958 data to the kernel (which
still needs to be reformatted to the hardware's special format.)

dw-hdmi-ahb-aud.pcm.default {
	@args [ CARD ]
	@args.CARD { type string }
	type asym
	playback.pcm {
		type softvol
		slave.pcm {
			type plug
			slave.pcm {
				@func concat
				strings [ "dmix:" $CARD ",FORMAT=S24_LE" ]
			}
		}
		control {
			name "PCM Playback Volume"
			card $CARD
		}
	}
}

<confdir:pcm/iec958.conf>

dw-hdmi-ahb-aud.pcm.iec958.0 {
	@args [ CARD AES0 AES1 AES2 AES3 ]
	@args.CARD { type string }
	@args.AES0 { type integer }
	@args.AES1 { type integer }
	@args.AES2 { type integer }
	@args.AES3 { type integer }
	type iec958
	slave.pcm {
		type hw
		card $CARD
		device 0
	}
	slave.format IEC958_SUBFRAME_LE
	status [ $AES0 $AES1 $AES2 $AES3 ]
}
Anssi Hannula May 10, 2015, 6:59 p.m. UTC | #8
09.05.2015, 21:11, Russell King - ARM Linux kirjoitti:
> On Sat, May 09, 2015 at 08:55:10PM +0300, Anssi Hannula wrote:
>> 09.05.2015, 20:40, Russell King - ARM Linux kirjoitti:
>>> Even VLC _doesn't_ if it's outputting to a standard audio - in other
>>> words, if you don't tick the SPDIF direct output option which defaults
>>> to disabled (which, when enabled, opens the device passing the AES
>>> bits _and_ permits it to send a compressed audio stream.)  I've looked
>>> at this in VLC many times...
>>
>> That is my understanding as well. Same for pulseaudio, it doesn't set
>> any AES bits except for passthrough (and most other applications never
>> set them).
> 
> Right, so when you're dealing with HDMI, where it's required that the
> AES bits contain accurate information, the only real option is to set
> it appropriately in the driver if userspace doesn't specify the AES
> data bits.

I wonder whether receivers actually care with HDMI (they generally don't
with S/PDIF) - that's one tidbit for me to test later... But of course
it doesn't change much with the matter at hand, in any case we should
strive to get the bits correct if only because the HDMI spec requires
them to be (I don't think they were optional in IEC 60958-3 either, though).

> Now, with the dw-hdmi-ahb driver, I'm doing something sensible

Right.

What I'd like to see is arrive at some sort of general consensus on how
the AES bits should be handled (i.e. should the driver always set them
themselves and disallow/allow the userspace to override the rate bits),
which could then be applied to other drivers as well.

But maybe that is for another time, or just a futile effort altogether...

> - and
> yes, I do have a card file in /usr/share/alsa (see below).
> 
> What this does is ensure that linear PCM is converted to 24-bit PCM
> (which makes the kernel conversion to the required hardware format
> easier - I hate this, I'd much prefer it to be done in userspace.)

Indeed. I did notice there is a SND(RV)_PCM_FORMAT_SPECIAL but I guess
it might not be easily used for this purpose since it doesn't have a
specific sample width etc (but I am not familiar enough with this to say
whether it could work or not)...

> However, in the case of VLC, if it wants to send non-audio, it will
> open the IEC958 device, which will use the iec958 plugin to configure
> the AES bits for non-audio, and pass IEC958 data to the kernel (which
> still needs to be reformatted to the hardware's special format.)

Ah, so the AES bits are actually overridable by userspace, which is what
I was initially concerned with :)

Of course, this means that applications opening "iec958" but not setting
rate bits (which is common) will get the default 48kHz bits from
/usr/share/alsa/pcm/(iec958|hdmi).conf). Not sure how big an issue that
is, though. The "iec958" ALSA plugin does seem to have a FIXME comment
about setting AES bits according to sample rate.


[...]
> 
> <confdir:pcm/iec958.conf>
> 
> dw-hdmi-ahb-aud.pcm.iec958.0 {

I think you should s/iec958/hdmi/ for the above two lines. HDMI devices
should be using "hdmi" instead of "iec958" by convention (the latter is
used for optical/coaxial S/PDIF).

> 	@args [ CARD AES0 AES1 AES2 AES3 ]
> 	@args.CARD { type string }
> 	@args.AES0 { type integer }
> 	@args.AES1 { type integer }
> 	@args.AES2 { type integer }
> 	@args.AES3 { type integer }
> 	type iec958
> 	slave.pcm {
> 		type hw
> 		card $CARD
> 		device 0
> 	}
> 	slave.format IEC958_SUBFRAME_LE
> 	status [ $AES0 $AES1 $AES2 $AES3 ]
> }
> 
>
Russell King - ARM Linux May 10, 2015, 7:33 p.m. UTC | #9
On Sun, May 10, 2015 at 09:59:42PM +0300, Anssi Hannula wrote:
> I wonder whether receivers actually care with HDMI (they generally don't
> with S/PDIF) - that's one tidbit for me to test later... But of course
> it doesn't change much with the matter at hand, in any case we should
> strive to get the bits correct if only because the HDMI spec requires
> them to be (I don't think they were optional in IEC 60958-3 either, though).

I suspect they don't care too much, but the HDMI spec does require them
to be correct.  If we're trying to aim for best compatibility, setting
them correctly is something we should strive to do.

> What I'd like to see is arrive at some sort of general consensus on how
> the AES bits should be handled (i.e. should the driver always set them
> themselves and disallow/allow the userspace to override the rate bits),
> which could then be applied to other drivers as well.
> 
> But maybe that is for another time, or just a futile effort altogether...

My personal view is that where we're dealing with PCM audio, the driver
needs to set these bits correctly as there is nothing in userspace to
do this.  This provides an identical interface between each audio device
which accepts PCM samples - whether it's a SPDIF or non-SPDIF based
device.

For non-audio data sent via an audio device, the AES bits need to be
conveyed from userspace, and we should respect what userspace gives us.
(If it's wrong, it's a userspace bug, and userspace should be fixed,
rather than trying to work around the bug by patching the kernel.)

> Indeed. I did notice there is a SND(RV)_PCM_FORMAT_SPECIAL but I guess
> it might not be easily used for this purpose since it doesn't have a
> specific sample width etc (but I am not familiar enough with this to say
> whether it could work or not)...

I spent quite a while looking at alsa-lib, wondering whether I could
move all the conversions out to userspace, but I couldn't without
building them _into_ alsa-lib.  This was a while back now, but from
what I remember, plugins to alsa-lib which aren't built as part of
alsa-lib are not able to do format conversions.

> > However, in the case of VLC, if it wants to send non-audio, it will
> > open the IEC958 device, which will use the iec958 plugin to configure
> > the AES bits for non-audio, and pass IEC958 data to the kernel (which
> > still needs to be reformatted to the hardware's special format.)
> 
> Ah, so the AES bits are actually overridable by userspace, which is what
> I was initially concerned with :)
> 
> Of course, this means that applications opening "iec958" but not setting
> rate bits (which is common) will get the default 48kHz bits from
> /usr/share/alsa/pcm/(iec958|hdmi).conf). Not sure how big an issue that
> is, though. The "iec958" ALSA plugin does seem to have a FIXME comment
> about setting AES bits according to sample rate.

Note that VLC does set the "sample" rate appropriately:

        switch (aout->format.i_rate)
        {
#define FS(freq) \
            case freq: aes3 = IEC958_AES3_CON_FS_ ## freq; break;
            FS( 44100) /* def. */ FS( 48000) FS( 32000)
            FS( 22050)            FS( 24000)
            FS( 88200) FS(768000) FS( 96000)
            FS(176400)            FS(192000)
#undef FS
            default:
                aes3 = IEC958_AES3_CON_FS_NOTID;
                break;

> > <confdir:pcm/iec958.conf>
> > 
> > dw-hdmi-ahb-aud.pcm.iec958.0 {
> 
> I think you should s/iec958/hdmi/ for the above two lines. HDMI devices
> should be using "hdmi" instead of "iec958" by convention (the latter is
> used for optical/coaxial S/PDIF).

Except doing that kills VLC's passthrough option (denoted by "spdif"),
which explicitly wants the iec958 device:

    /* Choose the IEC device for S/PDIF output:
       if the device is overridden by the user then it will be the one.
       Otherwise we compute the default device based on the output format. */
    if (spdif && !strcmp (device, "default"))
    {
...
        if (asprintf (&device,
                      "iec958:AES0=0x%x,AES1=0x%x,AES2=0x%x,AES3=0x%x",
                      IEC958_AES0_CON_EMPHASIS_NONE | IEC958_AES0_NONAUDIO,
                      IEC958_AES1_CON_ORIGINAL | IEC958_AES1_CON_PCM_CODER,
                      0, aes3) == -1)

Yes, technically an application bug, since VLC should allow the device
to be selectable and/or detect hdmi devices.  I wonder if that's
something which has changed between 2.0.8 and the latest vlc.

I did consider having the hdmi output device, but also alias that to
the iec958 device name, which I think can be done via:

<confdir:pcm/hdmi.conf>

dw-hdmi-ahb-aud.pcm.hdmi.0 {
...
}

<confdir:pcm/iec958.conf>

dw-hdmi-ahb-aud.pcm.iec958.0 cards.dw-hdmi-ahb-aud.pcm.hdmi.0

However, for HDMI sinks, I haven't seen any way in Linux to allow
userspace to know the audio capabilities of the attached HDMI sink,
and thus whether the video player can output compressed MPEG audio.
Anyone know?
Anssi Hannula May 10, 2015, 8:47 p.m. UTC | #10
10.05.2015, 22:33, Russell King - ARM Linux kirjoitti:
> On Sun, May 10, 2015 at 09:59:42PM +0300, Anssi Hannula wrote:
>> What I'd like to see is arrive at some sort of general consensus on how
>> the AES bits should be handled (i.e. should the driver always set them
>> themselves and disallow/allow the userspace to override the rate bits),
>> which could then be applied to other drivers as well.
>>
>> But maybe that is for another time, or just a futile effort altogether...
> 
> My personal view is that where we're dealing with PCM audio, the driver
> needs to set these bits correctly as there is nothing in userspace to
> do this.  This provides an identical interface between each audio device
> which accepts PCM samples - whether it's a SPDIF or non-SPDIF based
> device.
>
> For non-audio data sent via an audio device, the AES bits need to be
> conveyed from userspace, and we should respect what userspace gives us.
> (If it's wrong, it's a userspace bug, and userspace should be fixed,
> rather than trying to work around the bug by patching the kernel.)

Just to make sure I didn't misunderstand. You propose looking at the
"non-pcm" (aka "non-audio") bit in the AES to see whether driver/kernel
should force rate (and maybe other) AES bits?
Because I think that is currently the only way for the kernel/driver to
know if the data is "non-audio".


>> Indeed. I did notice there is a SND(RV)_PCM_FORMAT_SPECIAL but I guess
>> it might not be easily used for this purpose since it doesn't have a
>> specific sample width etc (but I am not familiar enough with this to say
>> whether it could work or not)...
> 
> I spent quite a while looking at alsa-lib, wondering whether I could
> move all the conversions out to userspace, but I couldn't without
> building them _into_ alsa-lib.  This was a while back now, but from
> what I remember, plugins to alsa-lib which aren't built as part of
> alsa-lib are not able to do format conversions.

Sounds a bit strange (assuming one'd plainly reference the plugin from
<hw>.conf directly), but OK. I'm not suggesting to look into it again
unless you really want to yourself :)

>>> However, in the case of VLC, if it wants to send non-audio, it will
>>> open the IEC958 device, which will use the iec958 plugin to configure
>>> the AES bits for non-audio, and pass IEC958 data to the kernel (which
>>> still needs to be reformatted to the hardware's special format.)
>>
>> Ah, so the AES bits are actually overridable by userspace, which is what
>> I was initially concerned with :)
>>
>> Of course, this means that applications opening "iec958" but not setting
>> rate bits (which is common) will get the default 48kHz bits from
>> /usr/share/alsa/pcm/(iec958|hdmi).conf). Not sure how big an issue that
>> is, though. The "iec958" ALSA plugin does seem to have a FIXME comment
>> about setting AES bits according to sample rate.
> 
> Note that VLC does set the "sample" rate appropriately:
> 
>         switch (aout->format.i_rate)
>         {
> #define FS(freq) \
>             case freq: aes3 = IEC958_AES3_CON_FS_ ## freq; break;
>             FS( 44100) /* def. */ FS( 48000) FS( 32000)
>             FS( 22050)            FS( 24000)
>             FS( 88200) FS(768000) FS( 96000)
>             FS(176400)            FS(192000)
> #undef FS
>             default:
>                 aes3 = IEC958_AES3_CON_FS_NOTID;
>                 break;

Yep, one of the few that do.

>>> <confdir:pcm/iec958.conf>
>>>
>>> dw-hdmi-ahb-aud.pcm.iec958.0 {
>>
>> I think you should s/iec958/hdmi/ for the above two lines. HDMI devices
>> should be using "hdmi" instead of "iec958" by convention (the latter is
>> used for optical/coaxial S/PDIF).
> 
> Except doing that kills VLC's passthrough option (denoted by "spdif"),
> which explicitly wants the iec958 device:
> 
>     /* Choose the IEC device for S/PDIF output:
>        if the device is overridden by the user then it will be the one.
>        Otherwise we compute the default device based on the output format. */
>     if (spdif && !strcmp (device, "default"))
>     {
> ...
>         if (asprintf (&device,
>                       "iec958:AES0=0x%x,AES1=0x%x,AES2=0x%x,AES3=0x%x",
>                       IEC958_AES0_CON_EMPHASIS_NONE | IEC958_AES0_NONAUDIO,
>                       IEC958_AES1_CON_ORIGINAL | IEC958_AES1_CON_PCM_CODER,
>                       0, aes3) == -1)
> 
> Yes, technically an application bug, since VLC should allow the device
> to be selectable and/or detect hdmi devices.  I wonder if that's
> something which has changed between 2.0.8 and the latest vlc.

Yes, in the current version it appends the parameters to the configured
device if it contains "iec958" or "hdmi" (git log says iec958 was
un-hardcoded on the above line in Jan 2014).

So if you have selected the "HDMI Output" device from the audio dropdown
in settings, it would work.

However, the default device "default" is still mangled to "iec958", (the
code has a "TODO: hdmi" comment).


> I did consider having the hdmi output device, but also alias that to
> the iec958 device name, which I think can be done via:
> 
> <confdir:pcm/hdmi.conf>
> 
> dw-hdmi-ahb-aud.pcm.hdmi.0 {
> ...
> }
> 
> <confdir:pcm/iec958.conf>
> 
> dw-hdmi-ahb-aud.pcm.iec958.0 cards.dw-hdmi-ahb-aud.pcm.hdmi.0

If a userspace workaround is wanted, I think I'd prefer this one myself.

Personally, I don't believe a workaround is of much use here, because
using "iec958" is already broken for majority of HDMI users anyway (i.e.
those that use HDMI outputs on x86 PCs)... but I don't see the
workaround as strictly unacceptable, either.


Also, if this used "hdmi" and no workaround, i.MX6 devices with both
S/PDIF and HDMI connectors would then have just a single iec958* device
and a single hdmi* device. But I guess that doesn't matter *that* much...

> However, for HDMI sinks, I haven't seen any way in Linux to allow
> userspace to know the audio capabilities of the attached HDMI sink,
> and thus whether the video player can output compressed MPEG audio.
> Anyone know?

HDA driver exports the ELD as a mixer control, which the application can
then use to determine the capabilities.

Not the best idea IMHO (I think ELD should've preferably remained
internal since I think it should be considered an implementation detail,
plus it is not very application friendly), but it is what it is and I
guess it was the easiest way to do it from ALSA side.

I know pulseaudio uses the ELD control to:
- get receiver name
- trigger a device change event when it changes

Kodi uses it to:
- get receiver name
- get default passthrough (compressed audio) capabilities
- trigger re-enumeration and device reopening when it changes
(e.g. if the user starts audio playback before turning on their
amplifier/receiver, we might've started the playback with
less-than-optimal parameters due to ALSA driver ELD rate/channel
restrictions, since the amplifier/receiver might've passed us the
probably-more-limited monitor/TV SADs instead of its own SADs when the
receiver was in standby mode).

But I wouldn't be too surprised if those were the only users.
Mark Brown May 11, 2015, 3:58 p.m. UTC | #11
On Sun, May 10, 2015 at 08:33:20PM +0100, Russell King - ARM Linux wrote:

> My personal view is that where we're dealing with PCM audio, the driver
> needs to set these bits correctly as there is nothing in userspace to
> do this.  This provides an identical interface between each audio device
> which accepts PCM samples - whether it's a SPDIF or non-SPDIF based
> device.

> For non-audio data sent via an audio device, the AES bits need to be
> conveyed from userspace, and we should respect what userspace gives us.
> (If it's wrong, it's a userspace bug, and userspace should be fixed,
> rather than trying to work around the bug by patching the kernel.)

For what it's worth this is exactly what I'd be expecting to happen -
the kernel should do something by default and then let applications
override it if they so desire.
diff mbox

Patch

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index acef3223772c..56ed35fe0734 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -3,6 +3,16 @@  config DRM_DW_HDMI
 	depends on DRM
 	select DRM_KMS_HELPER
 
+config DRM_DW_HDMI_AHB_AUDIO
+	tristate "Synopsis Designware AHB Audio interface"
+	depends on DRM_DW_HDMI && SND
+	select SND_PCM
+	select SND_PCM_IEC958
+	help
+	  Support the AHB Audio interface which is part of the Synopsis
+	  Designware HDMI block.  This is used in conjunction with
+	  the i.MX6 HDMI driver.
+
 config DRM_PTN3460
 	tristate "PTN3460 DP/LVDS bridge"
 	depends on DRM
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 8dfebd984370..eb80dbbb8365 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -3,3 +3,4 @@  ccflags-y := -Iinclude/drm
 obj-$(CONFIG_DRM_PS8622) += ps8622.o
 obj-$(CONFIG_DRM_PTN3460) += ptn3460.o
 obj-$(CONFIG_DRM_DW_HDMI) += dw_hdmi.o
+obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw_hdmi-ahb-audio.o
diff --git a/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c
new file mode 100644
index 000000000000..e98c291268f4
--- /dev/null
+++ b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.c
@@ -0,0 +1,560 @@ 
+/*
+ * DesignWare HDMI audio driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Written and tested against the Designware HDMI Tx found in iMX6.
+ */
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <drm/bridge/dw_hdmi.h>
+
+#include <sound/asoundef.h>
+#include <sound/core.h>
+#include <sound/initval.h>
+#include <sound/pcm.h>
+#include <sound/pcm_iec958.h>
+
+#include "dw_hdmi-ahb-audio.h"
+
+#define DRIVER_NAME "dw-hdmi-ahb-audio"
+
+/* Provide some bits rather than bit offsets */
+enum {
+	HDMI_AHB_DMA_CONF0_SW_FIFO_RST = BIT(7),
+	HDMI_AHB_DMA_CONF0_EN_HLOCK = BIT(3),
+	HDMI_AHB_DMA_START_START = BIT(0),
+	HDMI_AHB_DMA_STOP_STOP = BIT(0),
+	HDMI_IH_MUTE_AHBDMAAUD_STAT0_ERROR = BIT(5),
+	HDMI_IH_MUTE_AHBDMAAUD_STAT0_LOST = BIT(4),
+	HDMI_IH_MUTE_AHBDMAAUD_STAT0_RETRY = BIT(3),
+	HDMI_IH_MUTE_AHBDMAAUD_STAT0_DONE = BIT(2),
+	HDMI_IH_MUTE_AHBDMAAUD_STAT0_BUFFFULL = BIT(1),
+	HDMI_IH_MUTE_AHBDMAAUD_STAT0_BUFFEMPTY = BIT(0),
+	HDMI_IH_MUTE_AHBDMAAUD_STAT0_ALL =
+		HDMI_IH_MUTE_AHBDMAAUD_STAT0_ERROR |
+		HDMI_IH_MUTE_AHBDMAAUD_STAT0_LOST |
+		HDMI_IH_MUTE_AHBDMAAUD_STAT0_RETRY |
+		HDMI_IH_MUTE_AHBDMAAUD_STAT0_DONE |
+		HDMI_IH_MUTE_AHBDMAAUD_STAT0_BUFFFULL |
+		HDMI_IH_MUTE_AHBDMAAUD_STAT0_BUFFEMPTY,
+	HDMI_IH_AHBDMAAUD_STAT0_ERROR = BIT(5),
+	HDMI_IH_AHBDMAAUD_STAT0_LOST = BIT(4),
+	HDMI_IH_AHBDMAAUD_STAT0_RETRY = BIT(3),
+	HDMI_IH_AHBDMAAUD_STAT0_DONE = BIT(2),
+	HDMI_IH_AHBDMAAUD_STAT0_BUFFFULL = BIT(1),
+	HDMI_IH_AHBDMAAUD_STAT0_BUFFEMPTY = BIT(0),
+	HDMI_IH_AHBDMAAUD_STAT0_ALL =
+		HDMI_IH_AHBDMAAUD_STAT0_ERROR |
+		HDMI_IH_AHBDMAAUD_STAT0_LOST |
+		HDMI_IH_AHBDMAAUD_STAT0_RETRY |
+		HDMI_IH_AHBDMAAUD_STAT0_DONE |
+		HDMI_IH_AHBDMAAUD_STAT0_BUFFFULL |
+		HDMI_IH_AHBDMAAUD_STAT0_BUFFEMPTY,
+	HDMI_AHB_DMA_CONF0_INCR16 = 2 << 1,
+	HDMI_AHB_DMA_CONF0_INCR8 = 1 << 1,
+	HDMI_AHB_DMA_CONF0_INCR4 = 0,
+	HDMI_AHB_DMA_CONF0_BURST_MODE = BIT(0),
+	HDMI_AHB_DMA_MASK_DONE = BIT(7),
+	HDMI_REVISION_ID = 0x0001,
+	HDMI_IH_AHBDMAAUD_STAT0 = 0x0109,
+	HDMI_IH_MUTE_AHBDMAAUD_STAT0 = 0x0189,
+	HDMI_AHB_DMA_CONF0 = 0x3600,
+	HDMI_AHB_DMA_START = 0x3601,
+	HDMI_AHB_DMA_STOP = 0x3602,
+	HDMI_AHB_DMA_THRSLD = 0x3603,
+	HDMI_AHB_DMA_STRADDR0 = 0x3604,
+	HDMI_AHB_DMA_STPADDR0 = 0x3608,
+	HDMI_AHB_DMA_MASK = 0x3614,
+	HDMI_AHB_DMA_POL = 0x3615,
+	HDMI_AHB_DMA_CONF1 = 0x3616,
+	HDMI_AHB_DMA_BUFFPOL = 0x361a,
+};
+
+struct snd_dw_hdmi {
+	struct snd_card *card;
+	struct snd_pcm *pcm;
+	struct dw_hdmi_audio_data data;
+	struct snd_pcm_substream *substream;
+	void (*reformat)(struct snd_dw_hdmi *, size_t, size_t);
+	void *buf_src;
+	void *buf_dst;
+	dma_addr_t buf_addr;
+	unsigned buf_offset;
+	unsigned buf_period;
+	unsigned buf_size;
+	unsigned channels;
+	u8 revision;
+	u8 iec_offset;
+	u8 cs[192][8];
+};
+
+static void dw_hdmi_writel(unsigned long val, void __iomem *ptr)
+{
+	writeb_relaxed(val, ptr);
+	writeb_relaxed(val >> 8, ptr + 1);
+	writeb_relaxed(val >> 16, ptr + 2);
+	writeb_relaxed(val >> 24, ptr + 3);
+}
+
+/*
+ * Convert to hardware format: The userspace buffer contains IEC958 samples,
+ * with the PCUV bits in bits 31..28 and audio samples in bits 27..4.  We
+ * need these to be in bits 27..24, with the IEC B bit in bit 28, and audio
+ * samples in 23..0.
+ *
+ * Default preamble in bits 3..0: 8 = block start, 4 = even 2 = odd
+ *
+ * Ideally, we could do with having the data properly formatted in userspace.
+ */
+static void dw_hdmi_reformat_iec958(struct snd_dw_hdmi *dw,
+	size_t offset, size_t bytes)
+{
+	u32 *src = dw->buf_src + offset;
+	u32 *dst = dw->buf_dst + offset;
+	u32 *end = dw->buf_src + offset + bytes;
+
+	do {
+		u32 b, sample = *src++;
+
+		b = (sample & 8) << (28 - 3);
+
+		sample >>= 4;
+
+		*dst++ = sample | b;
+	} while (src < end);
+}
+
+static u32 parity(u32 sample)
+{
+	sample ^= sample >> 16;
+	sample ^= sample >> 8;
+	sample ^= sample >> 4;
+	sample ^= sample >> 2;
+	sample ^= sample >> 1;
+	return (sample & 1) << 27;
+}
+
+static void dw_hdmi_reformat_s24(struct snd_dw_hdmi *dw,
+	size_t offset, size_t bytes)
+{
+	u32 *src = dw->buf_src + offset;
+	u32 *dst = dw->buf_dst + offset;
+	u32 *end = dw->buf_src + offset + bytes;
+
+	do {
+		unsigned i;
+		u8 *cs;
+
+		cs = dw->cs[dw->iec_offset++];
+		if (dw->iec_offset >= 192)
+			dw->iec_offset = 0;
+
+		i = dw->channels;
+		do {
+			u32 sample = *src++;
+
+			sample &= ~0xff000000;
+			sample |= *cs++ << 24;
+			sample |= parity(sample & ~0xf8000000);
+
+			*dst++ = sample;
+		} while (--i);
+	} while (src < end);
+}
+
+static void dw_hdmi_create_cs(struct snd_dw_hdmi *dw,
+	struct snd_pcm_runtime *runtime)
+{
+	u8 cs[4];
+	unsigned ch, i, j;
+
+	snd_pcm_create_iec958_consumer(runtime, cs, sizeof(cs));
+
+	memset(dw->cs, 0, sizeof(dw->cs));
+
+	for (ch = 0; ch < 8; ch++) {
+		cs[2] &= ~IEC958_AES2_CON_CHANNEL;
+		cs[2] |= (ch + 1) << 4;
+
+		for (i = 0; i < ARRAY_SIZE(cs); i++) {
+			unsigned c = cs[i];
+
+			for (j = 0; j < 8; j++, c >>= 1)
+				dw->cs[i * 8 + j][ch] = (c & 1) << 2;
+		}
+	}
+	dw->cs[0][0] |= BIT(4);
+}
+
+static void dw_hdmi_start_dma(struct snd_dw_hdmi *dw)
+{
+	void __iomem *base = dw->data.base;
+	unsigned offset = dw->buf_offset;
+	unsigned period = dw->buf_period;
+	u32 start, stop;
+
+	dw->reformat(dw, offset, period);
+
+	/* Clear all irqs before enabling irqs and starting DMA */
+	writeb_relaxed(HDMI_IH_AHBDMAAUD_STAT0_ALL,
+		       base + HDMI_IH_AHBDMAAUD_STAT0);
+
+	start = dw->buf_addr + offset;
+	stop = start + period - 1;
+
+	/* Setup the hardware start/stop addresses */
+	dw_hdmi_writel(start, base + HDMI_AHB_DMA_STRADDR0);
+	dw_hdmi_writel(stop, base + HDMI_AHB_DMA_STPADDR0);
+
+	writeb_relaxed((u8)~HDMI_AHB_DMA_MASK_DONE, base + HDMI_AHB_DMA_MASK);
+	writeb(HDMI_AHB_DMA_START_START, base + HDMI_AHB_DMA_START);
+
+	offset += period;
+	if (offset >= dw->buf_size)
+		offset = 0;
+	dw->buf_offset = offset;
+}
+
+static void dw_hdmi_stop_dma(struct snd_dw_hdmi *dw)
+{
+	dw->substream = NULL;
+
+	/* Disable interrupts before disabling DMA */
+	writeb_relaxed(~0, dw->data.base + HDMI_AHB_DMA_MASK);
+	writeb_relaxed(HDMI_AHB_DMA_STOP_STOP, dw->data.base + HDMI_AHB_DMA_STOP);
+}
+
+static irqreturn_t snd_dw_hdmi_irq(int irq, void *data)
+{
+	struct snd_dw_hdmi *dw = data;
+	struct snd_pcm_substream *substream;
+	unsigned stat;
+
+	stat = readb_relaxed(dw->data.base + HDMI_IH_AHBDMAAUD_STAT0);
+	if (!stat)
+		return IRQ_NONE;
+
+	writeb_relaxed(stat, dw->data.base + HDMI_IH_AHBDMAAUD_STAT0);
+
+	substream = dw->substream;
+	if (stat & HDMI_IH_AHBDMAAUD_STAT0_DONE && substream) {
+		snd_pcm_period_elapsed(substream);
+		if (dw->substream)
+			dw_hdmi_start_dma(dw);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static struct snd_pcm_hardware dw_hdmi_hw = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_BLOCK_TRANSFER |
+		SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_MMAP_VALID,
+	.formats = SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE |
+		   SNDRV_PCM_FMTBIT_S24_LE,
+	.rates = SNDRV_PCM_RATE_32000 |
+		 SNDRV_PCM_RATE_44100 |
+		 SNDRV_PCM_RATE_48000 |
+		 SNDRV_PCM_RATE_88200 |
+		 SNDRV_PCM_RATE_96000 |
+		 SNDRV_PCM_RATE_176400 |
+		 SNDRV_PCM_RATE_192000,
+	.channels_min = 2,
+	.channels_max = 8,
+	.buffer_bytes_max = 64 * 1024,
+	.period_bytes_min = 256,
+	.period_bytes_max = 8192,	/* ERR004323: must limit to 8k */
+	.periods_min = 2,
+	.periods_max = 16,
+	.fifo_size = 0,
+};
+
+static int dw_hdmi_open(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_dw_hdmi *dw = substream->private_data;
+	void __iomem *base = dw->data.base;
+	int ret;
+
+	runtime->hw = dw_hdmi_hw;
+
+	ret = snd_pcm_limit_hw_rates(runtime);
+	if (ret < 0)
+		return ret;
+
+	ret = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
+	if (ret < 0)
+		return ret;
+
+	/* Clear FIFO */
+	writeb_relaxed(HDMI_AHB_DMA_CONF0_SW_FIFO_RST,
+		       base + HDMI_AHB_DMA_CONF0);
+
+	/* Configure interrupt polarities */
+	writeb_relaxed(~0, base + HDMI_AHB_DMA_POL);
+	writeb_relaxed(~0, base + HDMI_AHB_DMA_BUFFPOL);
+
+	/* Keep interrupts masked, and clear any pending */
+	writeb_relaxed(~0, base + HDMI_AHB_DMA_MASK);
+	writeb_relaxed(~0, base + HDMI_IH_AHBDMAAUD_STAT0);
+
+	ret = request_irq(dw->data.irq, snd_dw_hdmi_irq, IRQF_SHARED,
+			  "dw-hdmi-audio", dw);
+	if (ret)
+		return ret;
+
+	/* Un-mute done interrupt */
+	writeb_relaxed(HDMI_IH_MUTE_AHBDMAAUD_STAT0_ALL &
+		       ~HDMI_IH_MUTE_AHBDMAAUD_STAT0_DONE,
+		       base + HDMI_IH_MUTE_AHBDMAAUD_STAT0);
+
+	return 0;
+}
+
+static int dw_hdmi_close(struct snd_pcm_substream *substream)
+{
+	struct snd_dw_hdmi *dw = substream->private_data;
+
+	/* Mute all interrupts */
+	writeb_relaxed(HDMI_IH_MUTE_AHBDMAAUD_STAT0_ALL,
+		       dw->data.base + HDMI_IH_MUTE_AHBDMAAUD_STAT0);
+
+	free_irq(dw->data.irq, dw);
+
+	return 0;
+}
+
+static int dw_hdmi_hw_free(struct snd_pcm_substream *substream)
+{
+	return snd_pcm_lib_free_vmalloc_buffer(substream);
+}
+
+static int dw_hdmi_hw_params(struct snd_pcm_substream *substream,
+	struct snd_pcm_hw_params *params)
+{
+	return snd_pcm_lib_alloc_vmalloc_buffer(substream,
+						params_buffer_bytes(params));
+}
+
+static int dw_hdmi_prepare(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_dw_hdmi *dw = substream->private_data;
+	u8 threshold, conf0, conf1;
+
+	/* Setup as per 3.0.5 FSL 4.1.0 BSP */
+	switch (dw->revision) {
+	case 0x0a:
+		conf0 = HDMI_AHB_DMA_CONF0_BURST_MODE |
+			HDMI_AHB_DMA_CONF0_INCR4;
+		if (runtime->channels == 2)
+			threshold = 126;
+		else
+			threshold = 124;
+		break;
+	case 0x1a:
+		conf0 = HDMI_AHB_DMA_CONF0_BURST_MODE |
+			HDMI_AHB_DMA_CONF0_INCR8;
+		threshold = 128;
+		break;
+	default:
+		/* NOTREACHED */
+		return -EINVAL;
+	}
+
+	dw_hdmi_set_sample_rate(dw->data.hdmi, runtime->rate);
+
+	/* Minimum number of bytes in the fifo. */
+	runtime->hw.fifo_size = threshold * 32;
+
+	conf0 |= HDMI_AHB_DMA_CONF0_EN_HLOCK;
+	conf1 = (1 << runtime->channels) - 1;
+
+	writeb_relaxed(threshold, dw->data.base + HDMI_AHB_DMA_THRSLD);
+	writeb_relaxed(conf0, dw->data.base + HDMI_AHB_DMA_CONF0);
+	writeb_relaxed(conf1, dw->data.base + HDMI_AHB_DMA_CONF1);
+
+	switch (runtime->format) {
+	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
+		dw->reformat = dw_hdmi_reformat_iec958;
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		dw_hdmi_create_cs(dw, runtime);
+		dw->reformat = dw_hdmi_reformat_s24;
+		break;
+	}
+	dw->iec_offset = 0;
+	dw->channels = runtime->channels;
+	dw->buf_src  = runtime->dma_area;
+	dw->buf_dst  = substream->dma_buffer.area;
+	dw->buf_addr = substream->dma_buffer.addr;
+	dw->buf_period = snd_pcm_lib_period_bytes(substream);
+	dw->buf_size = snd_pcm_lib_buffer_bytes(substream);
+
+	return 0;
+}
+
+static int dw_hdmi_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+	struct snd_dw_hdmi *dw = substream->private_data;
+	int ret = 0;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		dw->buf_offset = 0;
+		dw->substream = substream;
+		dw_hdmi_start_dma(dw);
+		dw_hdmi_audio_enable(dw->data.hdmi);
+		substream->runtime->delay = substream->runtime->period_size;
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+		dw_hdmi_stop_dma(dw);
+		dw_hdmi_audio_disable(dw->data.hdmi);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static snd_pcm_uframes_t dw_hdmi_pointer(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_dw_hdmi *dw = substream->private_data;
+
+	return bytes_to_frames(runtime, dw->buf_offset);
+}
+
+static struct snd_pcm_ops snd_dw_hdmi_ops = {
+	.open = dw_hdmi_open,
+	.close = dw_hdmi_close,
+	.ioctl = snd_pcm_lib_ioctl,
+	.hw_params = dw_hdmi_hw_params,
+	.hw_free = dw_hdmi_hw_free,
+	.prepare = dw_hdmi_prepare,
+	.trigger = dw_hdmi_trigger,
+	.pointer = dw_hdmi_pointer,
+	.page = snd_pcm_lib_get_vmalloc_page,
+};
+
+static int snd_dw_hdmi_probe(struct platform_device *pdev)
+{
+	const struct dw_hdmi_audio_data *data = pdev->dev.platform_data;
+	struct device *dev = pdev->dev.parent;
+	struct snd_dw_hdmi *dw;
+	struct snd_card *card;
+	struct snd_pcm *pcm;
+	unsigned revision;
+	int ret;
+
+	writeb_relaxed(HDMI_IH_MUTE_AHBDMAAUD_STAT0_ALL,
+		       data->base + HDMI_IH_MUTE_AHBDMAAUD_STAT0);
+	revision = readb_relaxed(data->base + HDMI_REVISION_ID);
+	if (revision != 0x0a && revision != 0x1a) {
+		dev_err(dev, "dw-hdmi-audio: unknown revision 0x%02x\n",
+			revision);
+		return -ENXIO;
+	}
+
+	ret = snd_card_new(dev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,
+			      THIS_MODULE, sizeof(struct snd_dw_hdmi), &card);
+	if (ret < 0)
+		return ret;
+
+	strlcpy(card->driver, DRIVER_NAME, sizeof(card->driver));
+	strlcpy(card->shortname, "DW-HDMI", sizeof(card->shortname));
+	snprintf(card->longname, sizeof(card->longname),
+		 "%s rev 0x%02x, irq %d", card->shortname, revision,
+		 data->irq);
+
+	dw = card->private_data;
+	dw->card = card;
+	dw->data = *data;
+	dw->revision = revision;
+
+	ret = snd_pcm_new(card, "DW HDMI", 0, 1, 0, &pcm);
+	if (ret < 0)
+		goto err;
+
+	dw->pcm = pcm;
+	pcm->private_data = dw;
+	strlcpy(pcm->name, DRIVER_NAME, sizeof(pcm->name));
+	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &snd_dw_hdmi_ops);
+
+	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
+			dev, 64 * 1024, 64 * 1024);
+
+	ret = snd_card_register(card);
+	if (ret < 0)
+		goto err;
+
+	platform_set_drvdata(pdev, dw);
+
+	return 0;
+
+err:
+	snd_card_free(card);
+	return ret;
+}
+
+static int snd_dw_hdmi_remove(struct platform_device *pdev)
+{
+	struct snd_dw_hdmi *dw = platform_get_drvdata(pdev);
+
+	snd_card_free(dw->card);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int snd_dw_hdmi_suspend(struct device *dev)
+{
+	struct snd_dw_hdmi *dw = dev_get_drvdata(dev);
+
+	snd_power_change_state(dw->card, SNDRV_CTL_POWER_D3cold);
+	snd_pcm_suspend_all(dw->pcm);
+
+	return 0;
+}
+
+static int snd_dw_hdmi_resume(struct device *dev)
+{
+	struct snd_dw_hdmi *dw = dev_get_drvdata(dev);
+
+	snd_power_change_state(dw->card, SNDRV_CTL_POWER_D0);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(snd_dw_hdmi_pm, snd_dw_hdmi_suspend,
+			 snd_dw_hdmi_resume);
+#define PM_OPS &snd_dw_hdmi_pm
+#else
+#define PM_OPS NULL
+#endif
+
+static struct platform_driver snd_dw_hdmi_driver = {
+	.probe	= snd_dw_hdmi_probe,
+	.remove	= snd_dw_hdmi_remove,
+	.driver	= {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+		.pm = PM_OPS,
+	},
+};
+
+module_platform_driver(snd_dw_hdmi_driver);
+
+MODULE_AUTHOR("Russell King <rmk+kernel@arm.linux.org.uk>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS(PLATFORM_MODULE_PREFIX DRIVER_NAME);
diff --git a/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h
new file mode 100644
index 000000000000..1e840118d90a
--- /dev/null
+++ b/drivers/gpu/drm/bridge/dw_hdmi-ahb-audio.h
@@ -0,0 +1,13 @@ 
+#ifndef DW_HDMI_AUDIO_H
+#define DW_HDMI_AUDIO_H
+
+struct dw_hdmi;
+
+struct dw_hdmi_audio_data {
+	phys_addr_t phys;
+	void __iomem *base;
+	int irq;
+	struct dw_hdmi *hdmi;
+};
+
+#endif
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index adda3a988f36..1cb427935ed2 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -28,6 +28,7 @@ 
 #include <drm/bridge/dw_hdmi.h>
 
 #include "dw_hdmi.h"
+#include "dw_hdmi-ahb-audio.h"
 
 #define HDMI_EDID_LEN		512
 
@@ -105,6 +106,7 @@  struct dw_hdmi {
 	struct drm_encoder *encoder;
 	struct drm_bridge *bridge;
 
+	struct platform_device *audio;
 	enum dw_hdmi_devtype dev_type;
 	struct device *dev;
 	struct clk *isfr_clk;
@@ -1592,7 +1594,9 @@  int dw_hdmi_bind(struct device *dev, struct device *master,
 {
 	struct drm_device *drm = data;
 	struct device_node *np = dev->of_node;
+	struct platform_device_info pdevinfo;
 	struct device_node *ddc_node;
+	struct dw_hdmi_audio_data audio;
 	struct dw_hdmi *hdmi;
 	int ret;
 	u32 val = 1;
@@ -1712,6 +1716,23 @@  int dw_hdmi_bind(struct device *dev, struct device *master,
 	/* Unmute interrupts */
 	hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0);
 
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+	pdevinfo.parent = dev;
+	pdevinfo.id = PLATFORM_DEVID_AUTO;
+
+	if (hdmi_readb(hdmi, HDMI_CONFIG1_ID) & HDMI_CONFIG1_AHB) {
+		audio.phys = iores->start;
+		audio.base = hdmi->regs;
+		audio.irq = irq;
+		audio.hdmi = hdmi;
+
+		pdevinfo.name = "dw-hdmi-ahb-audio";
+		pdevinfo.data = &audio;
+		pdevinfo.size_data = sizeof(audio);
+		pdevinfo.dma_mask = DMA_BIT_MASK(32);
+		hdmi->audio = platform_device_register_full(&pdevinfo);
+	}
+
 	dev_set_drvdata(dev, hdmi);
 
 	return 0;
@@ -1729,6 +1750,9 @@  void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
 {
 	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
 
+	if (hdmi->audio && !IS_ERR(hdmi->audio))
+		platform_device_unregister(hdmi->audio);
+
 	/* Disable all interrupts */
 	hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
 
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h
index 175dbc89a824..78e54e813212 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.h
+++ b/drivers/gpu/drm/bridge/dw_hdmi.h
@@ -545,6 +545,9 @@ 
 #define HDMI_I2CM_FS_SCL_LCNT_0_ADDR            0x7E12
 
 enum {
+/* CONFIG1_ID field values */
+	HDMI_CONFIG1_AHB = 0x01,
+
 /* IH_FC_INT2 field values */
 	HDMI_IH_FC_INT2_OVERFLOW_MASK = 0x03,
 	HDMI_IH_FC_INT2_LOW_PRIORITY_OVERFLOW = 0x02,