ALSA-LIB: Support for format IEC958_SUBFRAME_LE in the plug plugin?
Hi,
RPi has recently moved to the VC4 driver which accepts only IEC958_SUBFRAME_LE format https://github.com/torvalds/linux/blob/ecb1b8288dc7ccbdcb3b9df005fa1c0e0c038... . Since then people started to have issues with their previous configs which use only the plug plugin (plughw:X).
Wrapping the device with the hdmi plugin solves the problem, as it should. But e.g. mainline java support for alsa offers only hw cards wrapped with the plug plugin (hardcoded, not possible to specify the device name directly). Stock Java apps then do not work with RPi HDMI.
I was wondering if it made sense to add support for the IEC958 formats (using the iec958 plugin methods) to the plug plugin.
It may be complicated with the hdmi_mode hint though, I do not know if there is any API to recognize HDMI vs. SPDIF. Maybe a different format specifically for HDMI could have been perhaps useful but it may be too late for that.
Thank you very much for consideration.
With regards,
Pavel.
Dne 26. 01. 24 v 9:00 Pavel Hofman napsal(a):
RPi has recently moved to the VC4 driver which accepts only IEC958_SUBFRAME_LE format https://github.com/torvalds/linux/blob/ecb1b8288dc7ccbdcb3b9df005fa1c0e0c038... . Since then people started to have issues with their previous configs which use only the plug plugin (plughw:X).
Wrapping the device with the hdmi plugin solves the problem, as it should. But e.g. mainline java support for alsa offers only hw cards wrapped with the plug plugin (hardcoded, not possible to specify the device name directly). Stock Java apps then do not work with RPi HDMI.
I was wondering if it made sense to add support for the IEC958 formats (using the iec958 plugin methods) to the plug plugin.
It may be complicated with the hdmi_mode hint though, I do not know if there is any API to recognize HDMI vs. SPDIF. Maybe a different format specifically for HDMI could have been perhaps useful but it may be too late for that.
Thank you very much for consideration.
Perhaps something trivial like this could do (header file changes not included):
=================================================================== diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c --- a/src/pcm/pcm_plug.c (revision ffed4f342678c31bf0b9edfe184be5f3de41603a) +++ b/src/pcm/pcm_plug.c (date 1706546414549) @@ -490,6 +490,14 @@ } #endif
+int snd_pcm_plug_generic_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sformat, snd_pcm_t *slave, int close_slave) { + unsigned char preamble_vals[3] = { + 0x08, 0x02, 0x04 /* Z, X, Y */ + }; + int hdmi_mode; // ???? + return snd_pcm_iec958_open(pcmp, name, sformat, slave, close_slave, NULL, preamble_vals, hdmi_mode); +} + static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_plug_params_t *clt, snd_pcm_plug_params_t *slv) { snd_pcm_plug_t *plug = pcm->private_data; @@ -567,6 +575,10 @@ f = snd_pcm_adpcm_open; break; #endif + case SND_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SND_PCM_FORMAT_IEC958_SUBFRAME_BE: + f = pcm_snd_general_iec958_open; + break; default: return -EINVAL; }
===================================================================
IMO preamble_vals could be left at the same default as defined in pcm_iec958.c:_snd_pcm_iec958_open().
But the correct hdmi_mode parameter is necessary. Alsa configs specify it explicitly (like vc4-hdmi.conf). But the plug plugin has no such information. Adding a parameter like that to the plug plugin would cancel the effect of this change (to support hardcoded plughw devices e.g. in java, with no support for custom PCM devices).
Maybe an environment variable could be defined for the whole application which could be used for the hdmi_mode, an ugly hack though...
Thanks a lot for any hints and suggestions.
With regards,
Pavel.
On 29. 01. 24 18:06, Pavel Hofman wrote:
Dne 26. 01. 24 v 9:00 Pavel Hofman napsal(a):
RPi has recently moved to the VC4 driver which accepts only IEC958_SUBFRAME_LE format https://github.com/torvalds/linux/blob/ecb1b8288dc7ccbdcb3b9df005fa1c0e0c038... . Since then people started to have issues with their previous configs which use only the plug plugin (plughw:X).
Wrapping the device with the hdmi plugin solves the problem, as it should. But e.g. mainline java support for alsa offers only hw cards wrapped with the plug plugin (hardcoded, not possible to specify the device name directly). Stock Java apps then do not work with RPi HDMI.
I was wondering if it made sense to add support for the IEC958 formats (using the iec958 plugin methods) to the plug plugin.
It may be complicated with the hdmi_mode hint though, I do not know if there is any API to recognize HDMI vs. SPDIF. Maybe a different format specifically for HDMI could have been perhaps useful but it may be too late for that.
Thank you very much for consideration.
Perhaps something trivial like this could do (header file changes not included):
=================================================================== diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c --- a/src/pcm/pcm_plug.c (revision ffed4f342678c31bf0b9edfe184be5f3de41603a) +++ b/src/pcm/pcm_plug.c (date 1706546414549) @@ -490,6 +490,14 @@ } #endif
+int snd_pcm_plug_generic_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sformat, snd_pcm_t *slave, int close_slave) {
- unsigned char preamble_vals[3] = {
0x08, 0x02, 0x04 /* Z, X, Y */};- int hdmi_mode; // ????
- return snd_pcm_iec958_open(pcmp, name, sformat, slave, close_slave,
NULL, preamble_vals, hdmi_mode); +}
- static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new,
snd_pcm_plug_params_t *clt, snd_pcm_plug_params_t *slv) { snd_pcm_plug_t *plug = pcm->private_data; @@ -567,6 +575,10 @@ f = snd_pcm_adpcm_open; break; #endif
case SND_PCM_FORMAT_IEC958_SUBFRAME_LE:case SND_PCM_FORMAT_IEC958_SUBFRAME_BE:f = pcm_snd_general_iec958_open;break; default: return -EINVAL; }===================================================================
IMO preamble_vals could be left at the same default as defined in pcm_iec958.c:_snd_pcm_iec958_open().
But the correct hdmi_mode parameter is necessary. Alsa configs specify it explicitly (like vc4-hdmi.conf). But the plug plugin has no such information. Adding a parameter like that to the plug plugin would cancel the effect of this change (to support hardcoded plughw devices e.g. in java, with no support for custom PCM devices).
Maybe an environment variable could be defined for the whole application which could be used for the hdmi_mode, an ugly hack though...
Thanks a lot for any hints and suggestions.
It looks like a way to go. The hdmi_mode can be set using ALSA configuration use snd_config_search (with global config - snd_config pointer) and snd_config_get_bool functions for that. The variable may be named like 'defaults.pcm.plug.iec958.hdmi_mode' or so (see alsa.conf).
Jaroslav
Dne 29. 01. 24 v 18:27 Jaroslav Kysela napsal(a):
On 29. 01. 24 18:06, Pavel Hofman wrote:
I was wondering if it made sense to add support for the IEC958 formats (using the iec958 plugin methods) to the plug plugin.
It may be complicated with the hdmi_mode hint though, I do not know if there is any API to recognize HDMI vs. SPDIF. Maybe a different format specifically for HDMI could have been perhaps useful but it may be too late for that.
Thank you very much for consideration.
Perhaps something trivial like this could do (header file changes not included):
=================================================================== diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c --- a/src/pcm/pcm_plug.c (revision ffed4f342678c31bf0b9edfe184be5f3de41603a) +++ b/src/pcm/pcm_plug.c (date 1706546414549) @@ -490,6 +490,14 @@ } #endif
+int snd_pcm_plug_generic_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sformat, snd_pcm_t *slave, int close_slave) { + unsigned char preamble_vals[3] = { + 0x08, 0x02, 0x04 /* Z, X, Y */ + }; + int hdmi_mode; // ???? + return snd_pcm_iec958_open(pcmp, name, sformat, slave, close_slave, NULL, preamble_vals, hdmi_mode); +}
static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_plug_params_t *clt, snd_pcm_plug_params_t *slv) { snd_pcm_plug_t *plug = pcm->private_data; @@ -567,6 +575,10 @@ f = snd_pcm_adpcm_open; break; #endif + case SND_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SND_PCM_FORMAT_IEC958_SUBFRAME_BE: + f = pcm_snd_general_iec958_open; + break; default: return -EINVAL; }
===================================================================
IMO preamble_vals could be left at the same default as defined in pcm_iec958.c:_snd_pcm_iec958_open().
But the correct hdmi_mode parameter is necessary. Alsa configs specify it explicitly (like vc4-hdmi.conf). But the plug plugin has no such information. Adding a parameter like that to the plug plugin would cancel the effect of this change (to support hardcoded plughw devices e.g. in java, with no support for custom PCM devices).
Maybe an environment variable could be defined for the whole application which could be used for the hdmi_mode, an ugly hack though...
Thanks a lot for any hints and suggestions.
It looks like a way to go. The hdmi_mode can be set using ALSA configuration use snd_config_search (with global config - snd_config pointer) and snd_config_get_bool functions for that. The variable may be named like 'defaults.pcm.plug.iec958.hdmi_mode' or so (see alsa.conf).
That's interesting. IIUC such parameter would globally switch all plugs instances to the hdmi_mode. Would a user-based ~/.asoundrc with such variable be applied for the hard-coded plughw:XX device? Maybe it would be enough for most use cases, eliminating the need for an app-specific environment variable.
Or maybe a prioritized sequence getenv('ALSA_PCM_PLUG_IEC958_HDMI_MODE') -> snd_config_search('defaults.pcm.plug.iec958.hdmi_mode') could be used.
Thanks a lot,
Pavel.
On 29. 01. 24 18:55, Pavel Hofman wrote:
Dne 29. 01. 24 v 18:27 Jaroslav Kysela napsal(a):
On 29. 01. 24 18:06, Pavel Hofman wrote:
I was wondering if it made sense to add support for the IEC958 formats (using the iec958 plugin methods) to the plug plugin.
It may be complicated with the hdmi_mode hint though, I do not know if there is any API to recognize HDMI vs. SPDIF. Maybe a different format specifically for HDMI could have been perhaps useful but it may be too late for that.
Thank you very much for consideration.
Perhaps something trivial like this could do (header file changes not included):
=================================================================== diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c --- a/src/pcm/pcm_plug.c (revision ffed4f342678c31bf0b9edfe184be5f3de41603a) +++ b/src/pcm/pcm_plug.c (date 1706546414549) @@ -490,6 +490,14 @@ } #endif
+int snd_pcm_plug_generic_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sformat, snd_pcm_t *slave, int close_slave) { + unsigned char preamble_vals[3] = { + 0x08, 0x02, 0x04 /* Z, X, Y */ + }; + int hdmi_mode; // ???? + return snd_pcm_iec958_open(pcmp, name, sformat, slave, close_slave, NULL, preamble_vals, hdmi_mode); +}
static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_plug_params_t *clt, snd_pcm_plug_params_t *slv) { snd_pcm_plug_t *plug = pcm->private_data; @@ -567,6 +575,10 @@ f = snd_pcm_adpcm_open; break; #endif + case SND_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SND_PCM_FORMAT_IEC958_SUBFRAME_BE: + f = pcm_snd_general_iec958_open; + break; default: return -EINVAL; }
===================================================================
IMO preamble_vals could be left at the same default as defined in pcm_iec958.c:_snd_pcm_iec958_open().
But the correct hdmi_mode parameter is necessary. Alsa configs specify it explicitly (like vc4-hdmi.conf). But the plug plugin has no such information. Adding a parameter like that to the plug plugin would cancel the effect of this change (to support hardcoded plughw devices e.g. in java, with no support for custom PCM devices).
Maybe an environment variable could be defined for the whole application which could be used for the hdmi_mode, an ugly hack though...
Thanks a lot for any hints and suggestions.
It looks like a way to go. The hdmi_mode can be set using ALSA configuration use snd_config_search (with global config - snd_config pointer) and snd_config_get_bool functions for that. The variable may be named like 'defaults.pcm.plug.iec958.hdmi_mode' or so (see alsa.conf).
That's interesting. IIUC such parameter would globally switch all plugs instances to the hdmi_mode. Would a user-based ~/.asoundrc with such variable be applied for the hard-coded plughw:XX device? Maybe it would be enough for most use cases, eliminating the need for an app-specific environment variable.
Or maybe a prioritized sequence getenv('ALSA_PCM_PLUG_IEC958_HDMI_MODE') -> snd_config_search('defaults.pcm.plug.iec958.hdmi_mode') could be used.
We usually don't use getenv hacks for alsa-lib features. The global configuration path can be redirected using ALSA_CONFIG_DIR and per user (~/.asoundrc or $XDG_CONFIG_HOME/alsa/asoundrc) and per card configurations (/var/lib/alsa/card#.conf.d) are also loaded from the global alsa.conf file.
Perphaps, the iec958 plug code may include card number / card driver name to the configuration tree lookup - like 'defaults.pcm.plug.iec958.0.hdmi_mode' or 'defaults.pcm.plug.iec958.vc4-hdmi.hdmi_mode' . With this extension, this value can be set in global src/conf/cards/vc4-hdmi.conf for this hw.
Jaroslav
Dne 30. 01. 24 v 13:30 Jaroslav Kysela napsal(a):
On 29. 01. 24 18:55, Pavel Hofman wrote:
Dne 29. 01. 24 v 18:27 Jaroslav Kysela napsal(a):
On 29. 01. 24 18:06, Pavel Hofman wrote:
I was wondering if it made sense to add support for the IEC958 formats (using the iec958 plugin methods) to the plug plugin.
It may be complicated with the hdmi_mode hint though, I do not know if there is any API to recognize HDMI vs. SPDIF. Maybe a different format specifically for HDMI could have been perhaps useful but it may be too late for that.
Thank you very much for consideration.
Perhaps something trivial like this could do (header file changes not included):
=================================================================== diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c --- a/src/pcm/pcm_plug.c (revision ffed4f342678c31bf0b9edfe184be5f3de41603a) +++ b/src/pcm/pcm_plug.c (date 1706546414549) @@ -490,6 +490,14 @@ } #endif
+int snd_pcm_plug_generic_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sformat, snd_pcm_t *slave, int close_slave) { + unsigned char preamble_vals[3] = { + 0x08, 0x02, 0x04 /* Z, X, Y */ + }; + int hdmi_mode; // ???? + return snd_pcm_iec958_open(pcmp, name, sformat, slave, close_slave, NULL, preamble_vals, hdmi_mode); +}
static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_plug_params_t *clt, snd_pcm_plug_params_t *slv) { snd_pcm_plug_t *plug = pcm->private_data; @@ -567,6 +575,10 @@ f = snd_pcm_adpcm_open; break; #endif + case SND_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SND_PCM_FORMAT_IEC958_SUBFRAME_BE: + f = pcm_snd_general_iec958_open; + break; default: return -EINVAL; }
===================================================================
IMO preamble_vals could be left at the same default as defined in pcm_iec958.c:_snd_pcm_iec958_open().
But the correct hdmi_mode parameter is necessary. Alsa configs specify it explicitly (like vc4-hdmi.conf). But the plug plugin has no such information. Adding a parameter like that to the plug plugin would cancel the effect of this change (to support hardcoded plughw devices e.g. in java, with no support for custom PCM devices).
Maybe an environment variable could be defined for the whole application which could be used for the hdmi_mode, an ugly hack though...
Thanks a lot for any hints and suggestions.
It looks like a way to go. The hdmi_mode can be set using ALSA configuration use snd_config_search (with global config - snd_config pointer) and snd_config_get_bool functions for that. The variable may be named like 'defaults.pcm.plug.iec958.hdmi_mode' or so (see alsa.conf).
That's interesting. IIUC such parameter would globally switch all plugs instances to the hdmi_mode. Would a user-based ~/.asoundrc with such variable be applied for the hard-coded plughw:XX device? Maybe it would be enough for most use cases, eliminating the need for an app-specific environment variable.
Or maybe a prioritized sequence getenv('ALSA_PCM_PLUG_IEC958_HDMI_MODE') -> snd_config_search('defaults.pcm.plug.iec958.hdmi_mode') could be used.
We usually don't use getenv hacks for alsa-lib features. The global configuration path can be redirected using ALSA_CONFIG_DIR and per user (~/.asoundrc or $XDG_CONFIG_HOME/alsa/asoundrc) and per card configurations (/var/lib/alsa/card#.conf.d) are also loaded from the global alsa.conf file.
Perphaps, the iec958 plug code may include card number / card driver name to the configuration tree lookup - like 'defaults.pcm.plug.iec958.0.hdmi_mode' or 'defaults.pcm.plug.iec958.vc4-hdmi.hdmi_mode' . With this extension, this value can be set in global src/conf/cards/vc4-hdmi.conf for this hw.
Actually, looking at pcm_iec958.c and the commit which introduced the hdmi_mode param I am not sure the the hdmi_mode is of any value for the plug plugin.
IIUC the hdmi_mode value gets used only if the status is IEC958_AES0_NONAUDIO:
int single_stream = iec->hdmi_mode && (iec->status[0] & IEC958_AES0_NONAUDIO) && (channels == 8);
But the plug plugin would pass NULL as status_bits which in snd_pcm_iec958_open will be replaced with default_status_bits:
static const unsigned char default_status_bits[] = { IEC958_AES0_CON_EMPHASIS_NONE, IEC958_AES1_CON_ORIGINAL | IEC958_AES1_CON_PCM_CODER, 0, IEC958_AES3_CON_FS_NOTID, /* will be set in hwparams */ IEC958_AES4_CON_WORDLEN_NOTID /* will be set in hwparams */ };
Logically no IEC958_AES0_NONAUDIO bit is set in the default status bits.
IMO we can safely pass hdmi_mode=false to snd_pcm_iec958_open because using plug for non-audio stream would not make sense anyway.
Dne 30. 01. 24 v 20:22 Pavel Hofman napsal(a):
Dne 30. 01. 24 v 13:30 Jaroslav Kysela napsal(a):
It looks like a way to go. The hdmi_mode can be set using ALSA configuration use snd_config_search (with global config - snd_config pointer) and snd_config_get_bool functions for that. The variable may be named like 'defaults.pcm.plug.iec958.hdmi_mode' or so (see alsa.conf).
That's interesting. IIUC such parameter would globally switch all plugs instances to the hdmi_mode. Would a user-based ~/.asoundrc with such variable be applied for the hard-coded plughw:XX device? Maybe it would be enough for most use cases, eliminating the need for an app-specific environment variable.
Or maybe a prioritized sequence getenv('ALSA_PCM_PLUG_IEC958_HDMI_MODE') -> snd_config_search('defaults.pcm.plug.iec958.hdmi_mode') could be used.
We usually don't use getenv hacks for alsa-lib features. The global configuration path can be redirected using ALSA_CONFIG_DIR and per user (~/.asoundrc or $XDG_CONFIG_HOME/alsa/asoundrc) and per card configurations (/var/lib/alsa/card#.conf.d) are also loaded from the global alsa.conf file.
Perphaps, the iec958 plug code may include card number / card driver name to the configuration tree lookup - like 'defaults.pcm.plug.iec958.0.hdmi_mode' or 'defaults.pcm.plug.iec958.vc4-hdmi.hdmi_mode' . With this extension, this value can be set in global src/conf/cards/vc4-hdmi.conf for this hw.
Actually, looking at pcm_iec958.c and the commit which introduced the hdmi_mode param I am not sure the the hdmi_mode is of any value for the plug plugin.
IIUC the hdmi_mode value gets used only if the status is IEC958_AES0_NONAUDIO:
int single_stream = iec->hdmi_mode && (iec->status[0] & IEC958_AES0_NONAUDIO) && (channels == 8);
But the plug plugin would pass NULL as status_bits which in snd_pcm_iec958_open will be replaced with default_status_bits:
static const unsigned char default_status_bits[] = { IEC958_AES0_CON_EMPHASIS_NONE, IEC958_AES1_CON_ORIGINAL | IEC958_AES1_CON_PCM_CODER, 0, IEC958_AES3_CON_FS_NOTID, /* will be set in hwparams */ IEC958_AES4_CON_WORDLEN_NOTID /* will be set in hwparams */ };
Logically no IEC958_AES0_NONAUDIO bit is set in the default status bits.
IMO we can safely pass hdmi_mode=false to snd_pcm_iec958_open because using plug for non-audio stream would not make sense anyway.
A patch related to the conversion could look like this, IMO.
=================================================================== diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c --- a/src/pcm/pcm_plug.c (revision ffed4f342678c31bf0b9edfe184be5f3de41603a) +++ b/src/pcm/pcm_plug.c (date 1706857992580) @@ -490,6 +490,20 @@ } #endif
+#ifdef BUILD_PCM_PLUGIN_IEC958 +static int snd_pcm_plug_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sformat, snd_pcm_t *slave, int close_slave) +{ + return snd_pcm_iec958_open(pcmp, name, sformat, slave, close_slave, + /* using default status bits defined in the iec958 plugin*/ + NULL, + /* default preamble values as used in the iec958 plugin */ + {0x08, 0x02, 0x04 /* Z, X, Y */}, + /* hdmi_mode=0 because it is applied only for IEC958_AES0_NONAUDIO which is not in the default status bits */ + 0 + ); +} +#endif + static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_plug_params_t *clt, snd_pcm_plug_params_t *slv) { snd_pcm_plug_t *plug = pcm->private_data; @@ -565,6 +579,12 @@ #ifdef BUILD_PCM_PLUGIN_ADPCM case SND_PCM_FORMAT_IMA_ADPCM: f = snd_pcm_adpcm_open; + break; +#endif +#ifdef BUILD_PCM_PLUGIN_IEC958 + case SND_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SND_PCM_FORMAT_IEC958_SUBFRAME_BE: + f = snd_pcm_plug_iec958_open; break; #endif default:
===================================================================
Unfortunately I am afraid I do not understand fully the code in snd_pcm_plug_hw_refine_schange which calls method snd_pcm_plug_slave_format where the IEC958 formats should also be checked.
Thanks a lot for any help!
Dne 02. 02. 24 v 9:00 Pavel Hofman napsal(a):
Dne 30. 01. 24 v 20:22 Pavel Hofman napsal(a):
Dne 30. 01. 24 v 13:30 Jaroslav Kysela napsal(a):
It looks like a way to go. The hdmi_mode can be set using ALSA configuration use snd_config_search (with global config - snd_config pointer) and snd_config_get_bool functions for that. The variable may be named like 'defaults.pcm.plug.iec958.hdmi_mode' or so (see alsa.conf).
That's interesting. IIUC such parameter would globally switch all plugs instances to the hdmi_mode. Would a user-based ~/.asoundrc with such variable be applied for the hard-coded plughw:XX device? Maybe it would be enough for most use cases, eliminating the need for an app-specific environment variable.
Or maybe a prioritized sequence getenv('ALSA_PCM_PLUG_IEC958_HDMI_MODE') -> snd_config_search('defaults.pcm.plug.iec958.hdmi_mode') could be used.
We usually don't use getenv hacks for alsa-lib features. The global configuration path can be redirected using ALSA_CONFIG_DIR and per user (~/.asoundrc or $XDG_CONFIG_HOME/alsa/asoundrc) and per card configurations (/var/lib/alsa/card#.conf.d) are also loaded from the global alsa.conf file.
Perphaps, the iec958 plug code may include card number / card driver name to the configuration tree lookup - like 'defaults.pcm.plug.iec958.0.hdmi_mode' or 'defaults.pcm.plug.iec958.vc4-hdmi.hdmi_mode' . With this extension, this value can be set in global src/conf/cards/vc4-hdmi.conf for this hw.
Actually, looking at pcm_iec958.c and the commit which introduced the hdmi_mode param I am not sure the the hdmi_mode is of any value for the plug plugin.
IIUC the hdmi_mode value gets used only if the status is IEC958_AES0_NONAUDIO:
int single_stream = iec->hdmi_mode && (iec->status[0] & IEC958_AES0_NONAUDIO) && (channels == 8);
But the plug plugin would pass NULL as status_bits which in snd_pcm_iec958_open will be replaced with default_status_bits:
static const unsigned char default_status_bits[] = { IEC958_AES0_CON_EMPHASIS_NONE, IEC958_AES1_CON_ORIGINAL | IEC958_AES1_CON_PCM_CODER, 0, IEC958_AES3_CON_FS_NOTID, /* will be set in hwparams */ IEC958_AES4_CON_WORDLEN_NOTID /* will be set in hwparams */ };
Logically no IEC958_AES0_NONAUDIO bit is set in the default status bits.
IMO we can safely pass hdmi_mode=false to snd_pcm_iec958_open because using plug for non-audio stream would not make sense anyway.
A patch related to the conversion could look like this, IMO.
=================================================================== diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c --- a/src/pcm/pcm_plug.c (revision ffed4f342678c31bf0b9edfe184be5f3de41603a) +++ b/src/pcm/pcm_plug.c (date 1706857992580) @@ -490,6 +490,20 @@ } #endif
+#ifdef BUILD_PCM_PLUGIN_IEC958 +static int snd_pcm_plug_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sformat, snd_pcm_t *slave, int close_slave) +{ + return snd_pcm_iec958_open(pcmp, name, sformat, slave, close_slave, + /* using default status bits defined in the iec958 plugin*/ + NULL, + /* default preamble values as used in the iec958 plugin */ + {0x08, 0x02, 0x04 /* Z, X, Y */}, + /* hdmi_mode=0 because it is applied only for IEC958_AES0_NONAUDIO which is not in the default status bits */ + 0 + ); +} +#endif
static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_plug_params_t *clt, snd_pcm_plug_params_t *slv) { snd_pcm_plug_t *plug = pcm->private_data; @@ -565,6 +579,12 @@ #ifdef BUILD_PCM_PLUGIN_ADPCM case SND_PCM_FORMAT_IMA_ADPCM: f = snd_pcm_adpcm_open; + break; +#endif +#ifdef BUILD_PCM_PLUGIN_IEC958 + case SND_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SND_PCM_FORMAT_IEC958_SUBFRAME_BE: + f = snd_pcm_plug_iec958_open; break; #endif default:
===================================================================
Unfortunately I am afraid I do not understand fully the code in snd_pcm_plug_hw_refine_schange which calls method snd_pcm_plug_slave_format where the IEC958 formats should also be checked.
I am trying to understand the code logic in snd_pcm_plug_hw_refine_cchange and snd_pcm_plug_hw_refine_schange as there are no comments at all, to no avail.
Please what is the meaning of local variable f in this snippet of snd_pcm_plug_hw_refine_cchange https://github.com/alsa-project/alsa-lib/blob/2a736a0d2543f206fd2653aaae8a08... ?
for (format = 0; format <= SND_PCM_FORMAT_LAST; format++) { snd_pcm_format_t f; if (!snd_pcm_format_mask_test(format_mask, format)) continue; if (snd_pcm_format_mask_test(sformat_mask, format)) f = format; else { f = snd_pcm_plug_slave_format(format, sformat_mask); if (f == SND_PCM_FORMAT_UNKNOWN) continue; } snd_pcm_format_mask_set(&fmt_mask, format); }
The snd_pcm_plug_hw_refine_schange method sets the f to the fmt_mask in https://github.com/alsa-project/alsa-lib/blob/2a736a0d2543f206fd2653aaae8a08... , but not in https://github.com/alsa-project/alsa-lib/blob/2a736a0d2543f206fd2653aaae8a08...
Please what is the meaning/contract of the method snd_pcm_plug_slave_format https://github.com/alsa-project/alsa-lib/blob/2a736a0d2543f206fd2653aaae8a08... ? What is the relation between input format and output format?
Thanks a lot in advance.
On 04. 02. 24 14:59, Pavel Hofman wrote:
Dne 02. 02. 24 v 9:00 Pavel Hofman napsal(a):
Dne 30. 01. 24 v 20:22 Pavel Hofman napsal(a):
Dne 30. 01. 24 v 13:30 Jaroslav Kysela napsal(a):
It looks like a way to go. The hdmi_mode can be set using ALSA configuration use snd_config_search (with global config - snd_config pointer) and snd_config_get_bool functions for that. The variable may be named like 'defaults.pcm.plug.iec958.hdmi_mode' or so (see alsa.conf).
That's interesting. IIUC such parameter would globally switch all plugs instances to the hdmi_mode. Would a user-based ~/.asoundrc with such variable be applied for the hard-coded plughw:XX device? Maybe it would be enough for most use cases, eliminating the need for an app-specific environment variable.
Or maybe a prioritized sequence getenv('ALSA_PCM_PLUG_IEC958_HDMI_MODE') -> snd_config_search('defaults.pcm.plug.iec958.hdmi_mode') could be used.
We usually don't use getenv hacks for alsa-lib features. The global configuration path can be redirected using ALSA_CONFIG_DIR and per user (~/.asoundrc or $XDG_CONFIG_HOME/alsa/asoundrc) and per card configurations (/var/lib/alsa/card#.conf.d) are also loaded from the global alsa.conf file.
Perphaps, the iec958 plug code may include card number / card driver name to the configuration tree lookup - like 'defaults.pcm.plug.iec958.0.hdmi_mode' or 'defaults.pcm.plug.iec958.vc4-hdmi.hdmi_mode' . With this extension, this value can be set in global src/conf/cards/vc4-hdmi.conf for this hw.
Actually, looking at pcm_iec958.c and the commit which introduced the hdmi_mode param I am not sure the the hdmi_mode is of any value for the plug plugin.
IIUC the hdmi_mode value gets used only if the status is IEC958_AES0_NONAUDIO:
int single_stream = iec->hdmi_mode && (iec->status[0] & IEC958_AES0_NONAUDIO) && (channels == 8);
But the plug plugin would pass NULL as status_bits which in snd_pcm_iec958_open will be replaced with default_status_bits:
static const unsigned char default_status_bits[] = { IEC958_AES0_CON_EMPHASIS_NONE, IEC958_AES1_CON_ORIGINAL | IEC958_AES1_CON_PCM_CODER, 0, IEC958_AES3_CON_FS_NOTID, /* will be set in hwparams */ IEC958_AES4_CON_WORDLEN_NOTID /* will be set in hwparams */ };
Logically no IEC958_AES0_NONAUDIO bit is set in the default status bits.
IMO we can safely pass hdmi_mode=false to snd_pcm_iec958_open because using plug for non-audio stream would not make sense anyway.
A patch related to the conversion could look like this, IMO.
=================================================================== diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c --- a/src/pcm/pcm_plug.c (revision ffed4f342678c31bf0b9edfe184be5f3de41603a) +++ b/src/pcm/pcm_plug.c (date 1706857992580) @@ -490,6 +490,20 @@ } #endif
+#ifdef BUILD_PCM_PLUGIN_IEC958 +static int snd_pcm_plug_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sformat, snd_pcm_t *slave, int close_slave) +{ + return snd_pcm_iec958_open(pcmp, name, sformat, slave, close_slave, + /* using default status bits defined in the iec958 plugin*/ + NULL, + /* default preamble values as used in the iec958 plugin */ + {0x08, 0x02, 0x04 /* Z, X, Y */}, + /* hdmi_mode=0 because it is applied only for IEC958_AES0_NONAUDIO which is not in the default status bits */ + 0 + ); +} +#endif
static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_plug_params_t *clt, snd_pcm_plug_params_t *slv) { snd_pcm_plug_t *plug = pcm->private_data; @@ -565,6 +579,12 @@ #ifdef BUILD_PCM_PLUGIN_ADPCM case SND_PCM_FORMAT_IMA_ADPCM: f = snd_pcm_adpcm_open; + break; +#endif +#ifdef BUILD_PCM_PLUGIN_IEC958 + case SND_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SND_PCM_FORMAT_IEC958_SUBFRAME_BE: + f = snd_pcm_plug_iec958_open; break; #endif default:
===================================================================
Unfortunately I am afraid I do not understand fully the code in snd_pcm_plug_hw_refine_schange which calls method snd_pcm_plug_slave_format where the IEC958 formats should also be checked.
I am trying to understand the code logic in snd_pcm_plug_hw_refine_cchange and snd_pcm_plug_hw_refine_schange as there are no comments at all, to no avail.
Please what is the meaning of local variable f in this snippet of snd_pcm_plug_hw_refine_cchange https://github.com/alsa-project/alsa-lib/blob/2a736a0d2543f206fd2653aaae8a08... ?
for (format = 0; format <= SND_PCM_FORMAT_LAST; format++) { snd_pcm_format_t f; if (!snd_pcm_format_mask_test(format_mask, format)) continue; if (snd_pcm_format_mask_test(sformat_mask, format)) f = format; else { f = snd_pcm_plug_slave_format(format, sformat_mask); if (f == SND_PCM_FORMAT_UNKNOWN) continue; } snd_pcm_format_mask_set(&fmt_mask, format); }
The snd_pcm_plug_hw_refine_schange method sets the f to the fmt_mask in https://github.com/alsa-project/alsa-lib/blob/2a736a0d2543f206fd2653aaae8a08... , but not in https://github.com/alsa-project/alsa-lib/blob/2a736a0d2543f206fd2653aaae8a08...
Please what is the meaning/contract of the method snd_pcm_plug_slave_format https://github.com/alsa-project/alsa-lib/blob/2a736a0d2543f206fd2653aaae8a08... ? What is the relation between input format and output format?
IIUC, the existing code in snd_pcm_plug_hw_refine_cchange():
for (format = 0; format <= SND_PCM_FORMAT_LAST; format++) { snd_pcm_format_t f; if (!snd_pcm_format_mask_test(format_mask, format)) continue; if (snd_pcm_format_mask_test(sformat_mask, format)) f = format; else { f = snd_pcm_plug_slave_format(format, sformat_mask); if (f == SND_PCM_FORMAT_UNKNOWN) continue; } snd_pcm_format_mask_set(&fmt_mask, format); }
can be simplified to:
for (format = 0; format <= SND_PCM_FORMAT_LAST; format++) { if (!snd_pcm_format_mask_test(format_mask, format)) continue; if (!snd_pcm_format_mask_test(sformat_mask, format) && snd_pcm_plug_slave_format(format, sformat_mask) == SND_PCM_FORMAT_UNKNOWN) continue; snd_pcm_format_mask_set(&fmt_mask, format); }
But honestly I still do not understand what it actually does and what is the goal of snd_pcm_plug_slave_format().
Without that I cannot modify snd_pcm_plug_slave_format() correctly to incorporate support for IEC958.
On 07. 02. 24 16:34, Pavel Hofman wrote:
But honestly I still do not understand what it actually does and what is the goal of snd_pcm_plug_slave_format().
Without that I cannot modify snd_pcm_plug_slave_format() correctly to incorporate support for IEC958.
I believe you're looking to a wrong place. Here's incomplete code:
========== diff --git a/configure.ac b/configure.ac index 7a152a4a..3f238302 100644 --- a/configure.ac +++ b/configure.ac @@ -642,6 +642,9 @@ fi if test "$build_pcm_alaw" = "yes"; then AC_DEFINE([BUILD_PCM_PLUGIN_ALAW], "1", [Build PCM alaw plugin]) fi +if test "$build_pcm_iec958" = "yes"; then + AC_DEFINE([BUILD_PCM_PLUGIN_IEC958], "1", [Build PCM iec958 plugin]) +fi if test "$build_pcm_mmap_emul" = "yes"; then AC_DEFINE([BUILD_PCM_PLUGIN_MMAP_EMUL], "1", [Build PCM mmap-emul plugin]) fi diff --git a/include/pcm_plugin.h b/include/pcm_plugin.h index 2d014394..f3e8f3b5 100644 --- a/include/pcm_plugin.h +++ b/include/pcm_plugin.h @@ -133,6 +133,19 @@ int _snd_pcm_adpcm_open(snd_pcm_t **pcmp, const char *name, snd_config_t *root, snd_config_t *conf, snd_pcm_stream_t stream, int mode);
+/* + * IEC958 subframe conversion plugin + */ +int snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name, + snd_pcm_format_t sformat, snd_pcm_t *slave, + int close_slave, + const unsigned char *status_bits, + const unsigned char *preamble_vals, + int hdmi_mode); +int _snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name, + snd_config_t *root, snd_config_t *conf, + snd_pcm_stream_t stream, int mode); + /* * Route plugin for linear formats */ diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c index e5a3a189..287fb9f9 100644 --- a/src/pcm/pcm_plug.c +++ b/src/pcm/pcm_plug.c @@ -186,7 +186,8 @@ static const snd_pcm_format_t linear_preferred_formats[] = {
#if defined(BUILD_PCM_PLUGIN_MULAW) || \ defined(BUILD_PCM_PLUGIN_ALAW) || \ - defined(BUILD_PCM_PLUGIN_ADPCM) + defined(BUILD_PCM_PLUGIN_ADPCM) || \ + defined(BUILD_PCM_PLUGIN_IEC958) #define BUILD_PCM_NONLINEAR #endif
@@ -201,6 +202,10 @@ static const snd_pcm_format_t nonlinear_preferred_formats[] = { #ifdef BUILD_PCM_PLUGIN_ADPCM SND_PCM_FORMAT_IMA_ADPCM, #endif +#ifdef BUILD_PCM_PLUGIN_IEC958 + SND_PCM_FORMAT_IEC958_SUBFRAME_LE, + SND_PCM_FORMAT_IEC958_SUBFRAME_BE, +#endif }; #endif
@@ -490,6 +495,18 @@ static int snd_pcm_plug_change_channels(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm } #endif
+#ifdef BUILD_PCM_PLUGIN_IEC958 +static int iec958_open(snd_pcm_t **pcmp, const char *name, + snd_pcm_format_t sformat, snd_pcm_t *slave, + int close_slave) +{ + unsigned char preamble_vals[3] = { + 0x08, 0x02, 0x04 /* Z, X, Y */ + }; + return snd_pcm_iec958_open(pcmp, name, sformat, slave, close_slave, NULL, preamble_vals, 0); +} +#endif + static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_plug_params_t *clt, snd_pcm_plug_params_t *slv) { snd_pcm_plug_t *plug = pcm->private_data; @@ -526,6 +543,12 @@ static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_p case SND_PCM_FORMAT_IMA_ADPCM: f = snd_pcm_adpcm_open; break; +#endif +#ifdef BUILD_PCM_PLUGIN_IEC958 + case SND_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SND_PCM_FORMAT_IEC958_SUBFRAME_BE: + f = iec958_open; + break; #endif default: #ifdef BUILD_PCM_PLUGIN_LFLOAT @@ -566,6 +589,12 @@ static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_p case SND_PCM_FORMAT_IMA_ADPCM: f = snd_pcm_adpcm_open; break; +#endif +#ifdef BUILD_PCM_PLUGIN_IEC958 + case SND_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SND_PCM_FORMAT_IEC958_SUBFRAME_BE: + f = iec958_open; + break; #endif default: return -EINVAL; =========
Test:
========= ~/.asoundrc pcm.test123 { type plug slave { pcm null format IEC958_SUBFRAME_LE } } =========
$ LD_PRELOAD=~/alsa/alsa-lib/src/.libs/libasound.so aplay -v -D test123 /dev/zero Playing raw data '/dev/zero' : Unsigned 8 bit, Rate 8000 Hz, Mono Plug PCM: IEC958 subframe conversion PCM (IEC958_SUBFRAME_LE) Its setup is: stream : PLAYBACK access : RW_INTERLEAVED format : U8 subformat : STD channels : 1 rate : 8000 exact rate : 8000 (8000/1) msbits : 8 buffer_size : 4000 period_size : 1000 period_time : 125000 tstamp_mode : ENABLE tstamp_type : GETTIMEOFDAY period_step : 1 avail_min : 1000 period_event : 0 start_threshold : 4000 stop_threshold : 4000 silence_threshold: 0 silence_size : 0 boundary : 9007199254740992000 Slave: Null PCM Its setup is: stream : PLAYBACK access : MMAP_INTERLEAVED format : IEC958_SUBFRAME_LE subformat : STD channels : 1 rate : 8000 exact rate : 8000 (8000/1) msbits : 32 buffer_size : 4000 period_size : 1000 period_time : 125000 tstamp_mode : ENABLE tstamp_type : GETTIMEOFDAY period_step : 1 avail_min : 1000 period_event : 0 start_threshold : 4000 stop_threshold : 4000 silence_threshold: 0 silence_size : 0 boundary : 9007199254740992000
Jaroslav
On 08. 02. 24 15:40, Jaroslav Kysela wrote:
On 07. 02. 24 16:34, Pavel Hofman wrote:
But honestly I still do not understand what it actually does and what is the goal of snd_pcm_plug_slave_format().
Without that I cannot modify snd_pcm_plug_slave_format() correctly to incorporate support for IEC958.
I believe you're looking to a wrong place. Here's incomplete code:
========== diff --git a/configure.ac b/configure.ac index 7a152a4a..3f238302 100644 --- a/configure.ac +++ b/configure.ac @@ -642,6 +642,9 @@ fi if test "$build_pcm_alaw" = "yes"; then AC_DEFINE([BUILD_PCM_PLUGIN_ALAW], "1", [Build PCM alaw plugin]) fi +if test "$build_pcm_iec958" = "yes"; then + AC_DEFINE([BUILD_PCM_PLUGIN_IEC958], "1", [Build PCM iec958 plugin]) +fi if test "$build_pcm_mmap_emul" = "yes"; then AC_DEFINE([BUILD_PCM_PLUGIN_MMAP_EMUL], "1", [Build PCM mmap-emul plugin]) fi diff --git a/include/pcm_plugin.h b/include/pcm_plugin.h index 2d014394..f3e8f3b5 100644 --- a/include/pcm_plugin.h +++ b/include/pcm_plugin.h @@ -133,6 +133,19 @@ int _snd_pcm_adpcm_open(snd_pcm_t **pcmp, const char *name, snd_config_t *root, snd_config_t *conf, snd_pcm_stream_t stream, int mode);
+/*
- * IEC958 subframe conversion plugin
- */
+int snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name, + snd_pcm_format_t sformat, snd_pcm_t *slave, + int close_slave, + const unsigned char *status_bits, + const unsigned char *preamble_vals, + int hdmi_mode); +int _snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name, + snd_config_t *root, snd_config_t *conf, + snd_pcm_stream_t stream, int mode);
/* * Route plugin for linear formats */ diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c index e5a3a189..287fb9f9 100644 --- a/src/pcm/pcm_plug.c +++ b/src/pcm/pcm_plug.c @@ -186,7 +186,8 @@ static const snd_pcm_format_t linear_preferred_formats[] = {
#if defined(BUILD_PCM_PLUGIN_MULAW) || \ defined(BUILD_PCM_PLUGIN_ALAW) || \ - defined(BUILD_PCM_PLUGIN_ADPCM) + defined(BUILD_PCM_PLUGIN_ADPCM) || \ + defined(BUILD_PCM_PLUGIN_IEC958) #define BUILD_PCM_NONLINEAR #endif
@@ -201,6 +202,10 @@ static const snd_pcm_format_t nonlinear_preferred_formats[] = { #ifdef BUILD_PCM_PLUGIN_ADPCM SND_PCM_FORMAT_IMA_ADPCM, #endif +#ifdef BUILD_PCM_PLUGIN_IEC958 + SND_PCM_FORMAT_IEC958_SUBFRAME_LE, + SND_PCM_FORMAT_IEC958_SUBFRAME_BE, +#endif }; #endif
@@ -490,6 +495,18 @@ static int snd_pcm_plug_change_channels(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm } #endif
+#ifdef BUILD_PCM_PLUGIN_IEC958 +static int iec958_open(snd_pcm_t **pcmp, const char *name, + snd_pcm_format_t sformat, snd_pcm_t *slave, + int close_slave) +{ + unsigned char preamble_vals[3] = { + 0x08, 0x02, 0x04 /* Z, X, Y */ + }; + return snd_pcm_iec958_open(pcmp, name, sformat, slave, close_slave, NULL, preamble_vals, 0); +} +#endif
static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_plug_params_t *clt, snd_pcm_plug_params_t *slv) { snd_pcm_plug_t *plug = pcm->private_data; @@ -526,6 +543,12 @@ static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_p case SND_PCM_FORMAT_IMA_ADPCM: f = snd_pcm_adpcm_open; break; +#endif +#ifdef BUILD_PCM_PLUGIN_IEC958 + case SND_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SND_PCM_FORMAT_IEC958_SUBFRAME_BE: + f = iec958_open; + break; #endif default: #ifdef BUILD_PCM_PLUGIN_LFLOAT @@ -566,6 +589,12 @@ static int snd_pcm_plug_change_format(snd_pcm_t *pcm, snd_pcm_t **new, snd_pcm_p case SND_PCM_FORMAT_IMA_ADPCM: f = snd_pcm_adpcm_open; break; +#endif +#ifdef BUILD_PCM_PLUGIN_IEC958 + case SND_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SND_PCM_FORMAT_IEC958_SUBFRAME_BE: + f = iec958_open; + break; #endif default: return -EINVAL; =========
Jaroslav, thanks a lot! I did not know it required just extending the nonlinear_preferred_formats, no further changes to the checks.
Tested on RPi4 HDMI, working fine. Please will you submit the patch, or should I prepare it based on your code?
On 09. 02. 24 11:14, Pavel Hofman wrote:
Jaroslav, thanks a lot! I did not know it required just extending the nonlinear_preferred_formats, no further changes to the checks.
Yes, it's the simplest way to do this extension.
Tested on RPi4 HDMI, working fine. Please will you submit the patch, or should I prepare it based on your code?
Thank you for the feedback. I applied it to alsa-lib repo now:
https://github.com/alsa-project/alsa-lib/commit/d8ce72f2561f23293ad0d98d3006...
Jaroslav
participants (2)
-
Jaroslav Kysela -
Pavel Hofman