From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anssi Hannula Subject: Re: [alsa-devel] [PATCH 12/13] drm: bridge/dw_hdmi-ahb-audio: add audio driver Date: Sat, 09 May 2015 19:49:44 +0300 Message-ID: <554E3AA8.8060601@iki.fi> References: <20150509102501.GO2067@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Russell King Cc: Fabio Estevam , alsa-devel@alsa-project.org, dri-devel@lists.freedesktop.org, Mark Brown , Yakir Yang , linux-arm-kernel@lists.infradead.org List-Id: alsa-devel@alsa-project.org SGksCgpBIGNvdXBsZSBvZiB0aGluZ3MgSSBub3RpY2VkIGJlbG93OgoKMDkuMDUuMjAxNSwgMTM6 MjYsIFJ1c3NlbGwgS2luZyBraXJqb2l0dGk6Cj4gQWRkIEFMU0EgYmFzZWQgSERNSSBBSEIgYXVk aW8gZHJpdmVyIGZvciBkd19oZG1pLiAgVGhlIG9ubHkgYnVmZmVyCj4gZm9ybWF0IHN1cHBvcnRl ZCBieSB0aGUgaGFyZHdhcmUgaXMgaXRzIG93biBzcGVjaWFsIElFQzk1OCBiYXNlZCBmb3JtYXQs Cj4gd2hpY2ggaXMgbm90IGNvbXBhdGlibGUgd2l0aCBhbnkgQUxTQSBmb3JtYXQuICBUbyBhdm9p ZCBkb2luZyB0b28gbXVjaAo+IGRhdGEgbWFuaXB1bGF0aW9uIHdpdGhpbiB0aGUgZHJpdmVyLCB3 ZSBzdXBwb3J0IG9ubHkgQUxTQXMgSUVDOTU4IExFIGFuZAo+IDI0LWJpdCBQQ00gZm9ybWF0cyBm b3IgMiB0byA2IGNoYW5uZWxzLCB3aGljaCB3ZSBjb252ZXJ0IHRvIGl0cyBoYXJkd2FyZQo+IGZv cm1hdC4KPiAKPiBBIG1vcmUgZGVzaXJhYmxlIHNvbHV0aW9uIHdvdWxkIGJlIHRvIGhhdmUgdGhp cyBjb252ZXJzaW9uIGluIHVzZXJzcGFjZSwKPiBidXQgQUxTQSBkb2VzIG5vdCBhcHBlYXIgdG8g YWxsb3cgc3VjaCB0cmFuc2Zvcm1hdGlvbnMgb3V0c2lkZSBvZgo+IGxpYmFzb3VuZCBpdHNlbGYu Cj4gCj4gU2lnbmVkLW9mZi1ieTogUnVzc2VsbCBLaW5nIDxybWsra2VybmVsQGFybS5saW51eC5v cmcudWs+Cj4gLS0tCj4gIGRyaXZlcnMvZ3B1L2RybS9icmlkZ2UvS2NvbmZpZyAgICAgICAgICAg ICB8ICAxMCArCj4gIGRyaXZlcnMvZ3B1L2RybS9icmlkZ2UvTWFrZWZpbGUgICAgICAgICAgICB8 ICAgMSArCj4gIGRyaXZlcnMvZ3B1L2RybS9icmlkZ2UvZHdfaGRtaS1haGItYXVkaW8uYyB8IDU2 MCArKysrKysrKysrKysrKysrKysrKysrKysrKysrKwo+ICBkcml2ZXJzL2dwdS9kcm0vYnJpZGdl L2R3X2hkbWktYWhiLWF1ZGlvLmggfCAgMTMgKwo+ICBkcml2ZXJzL2dwdS9kcm0vYnJpZGdlL2R3 X2hkbWkuYyAgICAgICAgICAgfCAgMjQgKysKPiAgZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kd19o ZG1pLmggICAgICAgICAgIHwgICAzICsKPiAgNiBmaWxlcyBjaGFuZ2VkLCA2MTEgaW5zZXJ0aW9u cygrKQo+ICBjcmVhdGUgbW9kZSAxMDA2NDQgZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kd19oZG1p LWFoYi1hdWRpby5jCj4gIGNyZWF0ZSBtb2RlIDEwMDY0NCBkcml2ZXJzL2dwdS9kcm0vYnJpZGdl L2R3X2hkbWktYWhiLWF1ZGlvLmgKPiAKWy4uLl0KPiArc3RhdGljIHZvaWQgZHdfaGRtaV9jcmVh dGVfY3Moc3RydWN0IHNuZF9kd19oZG1pICpkdywKPiArCXN0cnVjdCBzbmRfcGNtX3J1bnRpbWUg KnJ1bnRpbWUpCj4gK3sKPiArCXU4IGNzWzRdOwo+ICsJdW5zaWduZWQgY2gsIGksIGo7Cj4gKwo+ ICsJc25kX3BjbV9jcmVhdGVfaWVjOTU4X2NvbnN1bWVyKHJ1bnRpbWUsIGNzLCBzaXplb2YoY3Mp KTsKCgpJIHRoaW5rIGdlbmVyYWxseSBkcml2ZXJzIGhhdmUgbGVmdCB0aGUgaWVjOTU4IGJpdHMg Zm9yIHVzZXJzcGFjZSB0bwpoYW5kbGUsIGkuZS4gdmlhIGEgIklFQzk1OCBQbGF5YmFjayBEZWZh dWx0IgooU05EUlZfQ1RMX05BTUVfSUVDOTU4KCIiLFBMQVlCQUNLLERFRkFVTFQpKSBtaXhlciBl bGVtZW50LCB3aXRoCmRlZmF1bHRzIGNvbWluZyBmcm9tIC91c3Ivc2hhcmUvYWxzYS9wY20vaGRt aS5jb25mLi4uCgpMb29raW5nIGF0IHRoZSBzb3VuZCBkcml2ZXIgdHJlZSwgbm93LCB0aG91Z2gs IHRoZXkgYXJlIGFscmVhZHkgc29tZXdoYXQKaW5jb25zaXN0ZW50OgoKMS4gTW9zdCBkcml2ZXJz OiBpZWM5NTggYml0cyBmcm9tIHVzZXJzcGFjZSwgZXhjZXB0IGZvciBody1zZXQgYml0cy4KCjIu IFNvbWUgZHJpdmVycyAoZS5nLiBjdHhmaSwgZnNsX3NwZGlmKTogU29tZSBiaXRzIHN1Y2ggYXMg cmF0ZSBzZXQgYnkKZHJpdmVyLCBidXQgZXZlcnl0aGluZyBpcyBhbHNvIGV4cG9zZWQgdG8gdXNl cnNwYWNlLgpBdCBsZWFzdCBpbiBmc2xfc3BkaWYgY2FzZSB0aGUgZHJpdmVyIHNldHMgdGhlIHN0 dWZmIGluIGh3X3BhcmFtcyB3aGljaAp3b3VsZCB0aGVuIGdldCBvdmVyd3JpdHRlbiBieSB1c2Vy c3BhY2UgKHdoaWNoIHNldHMgdGhlbSBhZnRlciBod19wYXJhbXMpLgoKMy4gU29tZSBkcml2ZXJz IChlLmcuIG9tYXAtaGRtaS1hdWRpbyk6IFNldCBieSBkcml2ZXIsIG5vdCBleHBvc2VkIHRvCnVz ZXJzcGFjZSwgbGlrZSBpbiB0aGlzIHBhdGNoLgoKKE9mIGNvdXJzZSBoYXZpbmcgdXNlcnNwYWNl IHNldCB0aGVtIHJlcXVpcmVzIHRoYXQgdGhlIGRldmljZSBoYXMgYQpwcm9wZXIgZW50cnkgaW4g L3Vzci9zaGFyZS9hbHNhL2NhcmRzIGFuZCB0aGUgcGNtIGRldmljZSBpcyBhY2Nlc3NlZCB2aWEK dGhlIHN0YW5kYXJkICJoZG1pIiBvciAiaWVjOTU4IiBkZXZpY2UgbmFtZXMgd2hpY2ggcGVyZm9y bSB0aGUgY2hhbm5lbApzdGF0dXMgd29yZCBzZXR1cC4gSSBndWVzcyB0aGUgQVJNIFNvQyBzdHVm ZiBnZW5lcmFsbHkgZG9lc24ndCBib3RoZXIKd2l0aCB0aGF0LCBleHBsYWluaW5nIGEgYml0IHdo eSBzb21lIGtlcm5lbCBkcml2ZXJzIHNldCB0aGVtIGJ5IHRoZW1zZWx2ZXMpLgoKVGhlIG1haW4g aW50ZXJlc3QgdG8gaWVjOTU4IGJpdHMgZnJvbSB1c2Vyc3BhY2UgaXMgcHJvYmFibHkgdG93YXJk cyB0aGUKbm9uLWF1ZGlvIGJpdCwgdXNlZCBmb3IgYXVkaW8gcGFzc3Rocm91Z2ggKHRoZSBiaXQg aXMgbm90IG1hbmRhdG9yeQp0aGVyZSwgYnV0IGl0IGhlbHBzKS4KCk90aGVyIGRyaXZlcnMgKHdl bGwsIEkgZ3Vlc3MganVzdCBIREEgaGFzIHRoaXMgc28gZmFyKSBhbHNvIHVzZSB0aGUKdXNlcnNw YWNlLXByb3ZpZGVkIG5vbi1hdWRpbyBiaXQgdG8gYWxzbyBzZWxlY3QgKGluIGNvbmp1bmN0aW9u IHdpdGgKY2hhbm5lbHM9PTgpIHRoZSBIQlIgbW9kZSwgaS5lLiBIRCBhdWRpbyBwYXNzdGhyb3Vn aCAod2hpY2ggZHdfaGRtaSBhbHNvCnN1cHBvcnRzIHZpYSBETUFfQ09ORjAgYnV0IG5vdCBlbmFi bGVkIG9uIHRoaXMgcGF0Y2gpLgoKClNpbmNlIHRoaXMgaXNuJ3QgdGhlIGZpcnN0IGRyaXZlciBk b2luZyB0aGluZ3MgdGhpcyB3YXksIGFuZCB0aGlzCmRvZXNuJ3QgcmVhbGx5IHByZXZlbnQgZXhw b3NpbmcgdGhlbSB0byB1c2Vyc3BhY2UgbGF0ZXIgb24sIEkgZ3Vlc3MgdGhpcwpwYXRjaCBpcyBz dGlsbCBPSyBoZXJlLCB1bmxlc3MgdGhlIEFMU0EgcGVvcGxlIHRoaW5rIG90aGVyd2lzZS4uLgoK ClsuLi5dCj4gK3N0YXRpYyBzdHJ1Y3Qgc25kX3BjbV9oYXJkd2FyZSBkd19oZG1pX2h3ID0gewo+ ICsJLmluZm8gPSBTTkRSVl9QQ01fSU5GT19JTlRFUkxFQVZFRCB8Cj4gKwkJU05EUlZfUENNX0lO Rk9fQkxPQ0tfVFJBTlNGRVIgfAo+ICsJCVNORFJWX1BDTV9JTkZPX01NQVAgfAo+ICsJCVNORFJW X1BDTV9JTkZPX01NQVBfVkFMSUQsCj4gKwkuZm9ybWF0cyA9IFNORFJWX1BDTV9GTVRCSVRfSUVD OTU4X1NVQkZSQU1FX0xFIHwKPiArCQkgICBTTkRSVl9QQ01fRk1UQklUX1MyNF9MRSwKPiArCS5y YXRlcyA9IFNORFJWX1BDTV9SQVRFXzMyMDAwIHwKPiArCQkgU05EUlZfUENNX1JBVEVfNDQxMDAg fAo+ICsJCSBTTkRSVl9QQ01fUkFURV80ODAwMCB8Cj4gKwkJIFNORFJWX1BDTV9SQVRFXzg4MjAw IHwKPiArCQkgU05EUlZfUENNX1JBVEVfOTYwMDAgfAo+ICsJCSBTTkRSVl9QQ01fUkFURV8xNzY0 MDAgfAo+ICsJCSBTTkRSVl9QQ01fUkFURV8xOTIwMDAsCj4gKwkuY2hhbm5lbHNfbWluID0gMiwK PiArCS5jaGFubmVsc19tYXggPSA4LAoKWW91IGFyZSBwcm92aWRpbmcgbXVsdGljaGFubmVsIHN1 cHBvcnQsIGJ1dCBBRkFJQ1MgeW91IGFyZSBub3Qgc2V0dGluZwp0aGUgY2hhbm5lbCBhbGxvY2F0 aW9uIChDQSkgYml0cyBpbiB0aGUgYXVkaW8gaW5mb2ZyYW1lIGFueXdoZXJlCihIRE1JX0ZDX0FV RElDT05GMiByZWdpc3RlcikuCgpIRE1JX0ZDX0FVRElDT05GMiByZWdpc3RlciBkZWZhdWx0IHZh bHVlIGlzIDB4MDAsIHdoaWNoIG1lYW5zIHBsYWluCnN0ZXJlbyAocGVyIENFQS04NjEpLiBJZiB0 aGlzIGlzIHdoYXQgZ29lcyBvbiB0byB0aGUgSERNSSBsaW5rIGFzIHdlbGwsCnRoZSBhdWRpbyBz aW5rIHNob3VsZCBpZ25vcmUgdGhlIG90aGVyIGNoYW5uZWxzLgpEaWQgeW91IGNoZWNrIHRoYXQg bXVsdGljaGFubmVsIFBDTSBhY3R1YWxseSB3b3Jrcz8gKG1heWJlIEknbSBqdXN0Cm1pc3Npbmcg d2hlcmUgQ0EgaXMgc2V0KQoKCk5vdGUgYWxzbyB0aGF0IEkgdGhpbmsgdGhpcyBIVyB1c2VzIHRo ZSBuYXRpdmUgSERNSSBjaGFubmVsIG9yZGVyIChmcm9tCkNFQS04NjEpLCB3aGljaCBpcyBkaWZm ZXJlbnQgZnJvbSB0aGUgQUxTQSBkZWZhdWx0IGNoYW5uZWwgb3JkZXIsIHNvIHlvdQpzaG91bGQg aW5mb3JtIHVzZXJzcGFjZSBvZiB0aGUgY2hhbm5lbCBvcmRlciAodmlhCnNuZF9wY21fYWRkX2No bWFwX2N0bHMoKSkuIFRoZSBjaGFubmVsIG9yZGVyIGlzIHNwZWNpZmllZCBieSB0aGUgQ0EKdmFs dWUgSSBtZW50aW9uZWQgYWJvdmUuCgpBc3N1bWluZyBJJ20gbm90IG1pc3Npbmcgc29tZXRoaW5n IHdoaWNoIG1ha2VzIGV2ZXJ5dGhpbmcgd29yayBhbHJlYWR5LApvbmUgb2YgdGhlc2Ugc2hvdWxk IGJlIGltcGxlbWVudGVkOgoKKGEpIFByb3ZpZGUgYWxsIHRoZSBjaG1hcHMgKGkuZS4gb25lIHBl ciBDQSB2YWx1ZSkgYXMgYSBsaXN0IGZvcgp1c2Vyc3BhY2UgdG8gc2VsZWN0IG9uZSwgYW5kIHBy b3ZpZGUgdGhlIGFjdGl2ZSBjaG1hcCB0byB1c2Vyc3BhY2UgYXMgd2VsbC4KCihiKSBKdXN0IGhh cmRjb2RlIGEgc2luZ2xlIENBIHZhbHVlIHBlciBjaGFubmVsIGNvdW50ICh3aGljaCBjb3ZlcnMg OTklCm9mIHVzZSBjYXNlcyksIGFuZCBwcm92aWRlIHRoZSBjb3JyZXNwb25kaW5nIGFjdGl2ZSBj aG1hcCB0byB1c2Vyc3BhY2UuCgooYykgY2hhbm5lbHNfbWF4ID0gMi4KCkJvdGggKGEpIGFuZCAo YikgYXJlIGdlbmVyaWMgc3R1ZmYgdGhhdCBjb3VsZC9zaG91bGQgYmUgaW4gaGVscGVycyBmb3IK YWxsIGRyaXZlcnMgdG8gdXNlIChpZiAoYiksIHByZWZlcmFibHkgd2l0aCBhbiBpbnRlcmZhY2Ug dGhhdCBhbGxvd3MKZWFzaWx5IGV4dGVuZGluZyBpdCB0byAoYSkgaW4gdGhlIGZ1dHVyZSkuCgpT b21lIG9mIHRoZSBjb2RlIGZyb20gc291bmQvcGNpL2hkYS9wYXRjaF9oZG1pLmMgc2hvdWxkIGJl IHVzZWZ1bCAoYXQKbGVhc3QgdGhlIENBIHRhYmxlIGl0IGhhcyAtIHRoaXMgc3R1ZmYgcmVhbGx5 IHNob3VkIGJlIG91dHNpZGUgdGhlCmRyaXZlcikuIE5vdGUgdGhhdCBtdWNoIG9mIHRoZSBjb21w bGV4aXR5IG9mIHBhdGNoX2hkbWkuYyBjb21lcyBmcm9tIHRoZQpmYWN0IHRoYXQgdGhlIEhXIHRo ZXJlIHN1cHBvcnRzIGNoYW5uZWwgcmVtYXBwaW5nCihTTkRSVl9DVExfVExWVF9DSE1BUF9WQVIg b3IgX1BBSVJFRCksIHdoaWNoIGR3X2hkbWkgZG9lc24ndAooU05EUlZfQ1RMX1RMVlRfQ0hNQVBf RklYRUQpLgoKCi0tIAotLSAKQW5zc2kgSGFubnVsYQpfX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBs aXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFu L2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: anssi.hannula@iki.fi (Anssi Hannula) Date: Sat, 09 May 2015 19:49:44 +0300 Subject: [alsa-devel] [PATCH 12/13] drm: bridge/dw_hdmi-ahb-audio: add audio driver In-Reply-To: References: <20150509102501.GO2067@n2100.arm.linux.org.uk> Message-ID: <554E3AA8.8060601@iki.fi> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > --- > 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). -- -- Anssi Hannula