Re: [alsa-devel] [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders
few questions/remarks BR, Arnaud
+static void hdmi_codec_abort(struct device *dev) +{
- struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
- dev_dbg(dev, "%s()\n", __func__);
- mutex_lock(&hcp->current_stream_lock);
- if (hcp->current_stream && hcp->current_stream->runtime &&
snd_pcm_running(hcp->current_stream)) {dev_info(dev, "HDMI audio playback aborted\n");snd_pcm_stream_lock_irq(hcp->current_stream);snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);snd_pcm_stream_unlock_irq(hcp->current_stream);- }
- mutex_unlock(&hcp->current_stream_lock);
+}
Does driver should stop the stream in case of unplug? This could generate unexpected behavior at application level. Perhaps should be better if this part is managed in DRM driver. if HDMI master, I2S bus should be maintained ON to consume the audio stream until application action.
+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- struct hdmi_codec_params hp = {
.iec = {.status = { 0 },.subcode = { 0 },.pad = 0,.dig_subframe = { 0 },}- };
- int ret;
- dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
params_width(params), params_rate(params),params_channels(params));- ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status,
sizeof(hp.iec.status));
Tell me if i am wrong, but in case of SPDIF format, IEC status is managed by cpu_dai not by the codec. To manage IEC61937 a control should be needed to update IEC status bits...
- if (ret < 0) {
dev_err(dai->dev, "Creating IEC958 channel status failed %d\n",ret);return ret;- }
- ret = hdmi_codec_new_stream(substream, dai);
- if (ret)
return ret;- hdmi_audio_infoframe_init(&hp.cea);
- hp.cea.channels = params_channels(params);
- hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
- hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
- hp.cea.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
- hp.sample_width = params_width(params);
- hp.sample_rate = params_rate(params);
- hp.channels = params_channels(params);
- return hcp->hcd.ops->hw_params(dai->dev->parent,
&hcp->daifmt[dai->id],
&hp);+}
On Mon, Sep 28, 2015 at 11:01:34AM +0200, Arnaud Pouliquen wrote:
few questions/remarks BR, Arnaud
+static void hdmi_codec_abort(struct device *dev) +{
- struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
- dev_dbg(dev, "%s()\n", __func__);
- mutex_lock(&hcp->current_stream_lock);
- if (hcp->current_stream && hcp->current_stream->runtime &&
snd_pcm_running(hcp->current_stream)) {dev_info(dev, "HDMI audio playback aborted\n");snd_pcm_stream_lock_irq(hcp->current_stream);snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);snd_pcm_stream_unlock_irq(hcp->current_stream);- }
- mutex_unlock(&hcp->current_stream_lock);
+}
Does driver should stop the stream in case of unplug? This could generate unexpected behavior at application level. Perhaps should be better if this part is managed in DRM driver. if HDMI master, I2S bus should be maintained ON to consume the audio stream until application action.
If it does, that's really horrid.
Firstly, do you expect your video playback to stop just because you've unplugged the TV? Maybe you've got a dumb HDMI switch, and you've switched from one input to another to momentarily check something on another input - should the video playback suddenly end up with its audio path being dumped on the floor because of that?
Also, as I've said before, there's hardware out there where the SPDIF audio output is routed to more than just the HDMI interface, and the capabilities of HDMI really don't apply in that situation - you may have an AV receiver connected to the SPDIF output which is able to accept much more than the basic audio that most TVs accept. Having stuff restricted to the union of the abilities is far too restrictive. Stopping the audio output because the TV went away in this case is also plain idiotic.
SPDIF is something that can be routed to multiple devices simultaneously, and there's no capability or connection reporting involved in it. Imposing the status of one "SPDIF listener" on the entire audio system is unreasonable.
+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- struct hdmi_codec_params hp = {
.iec = {.status = { 0 },.subcode = { 0 },.pad = 0,.dig_subframe = { 0 },}- };
- int ret;
- dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
params_width(params), params_rate(params),params_channels(params));- ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status,
sizeof(hp.iec.status));Tell me if i am wrong, but in case of SPDIF format, IEC status is managed by cpu_dai not by the codec.
Correct. I2S needs the IEC958 status programmed into the HDMI interface though, because HDMI is basically SPDIF.
On Mon, Sep 28, 2015 at 12:26:28PM +0100, Russell King - ARM Linux wrote:
On Mon, Sep 28, 2015 at 11:01:34AM +0200, Arnaud Pouliquen wrote:
few questions/remarks BR, Arnaud
+static void hdmi_codec_abort(struct device *dev) +{
- struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
- dev_dbg(dev, "%s()\n", __func__);
- mutex_lock(&hcp->current_stream_lock);
- if (hcp->current_stream && hcp->current_stream->runtime &&
snd_pcm_running(hcp->current_stream)) {dev_info(dev, "HDMI audio playback aborted\n");snd_pcm_stream_lock_irq(hcp->current_stream);snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);snd_pcm_stream_unlock_irq(hcp->current_stream);- }
- mutex_unlock(&hcp->current_stream_lock);
+}
Does driver should stop the stream in case of unplug? This could generate unexpected behavior at application level. Perhaps should be better if this part is managed in DRM driver. if HDMI master, I2S bus should be maintained ON to consume the audio stream until application action.
If it does, that's really horrid.
Atm the rule for display outputs is that nothing gets yanked until userspace approves, since otherwise compositors get stuck (or fall over with an unexpected -EINVAL from the kernel). The exception is DP MST because the current implementation is a complete hack for DP MST sink lifetimes and that's why we need to synchronously nuke them (which means shutting down everything). I'm surprised not a hole lot more people complain about this ... -Daniel
On 09/28/15 12:01, Arnaud Pouliquen wrote:
few questions/remarks BR, Arnaud
+static void hdmi_codec_abort(struct device *dev) +{
- struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
- dev_dbg(dev, "%s()\n", __func__);
- mutex_lock(&hcp->current_stream_lock);
- if (hcp->current_stream && hcp->current_stream->runtime &&
snd_pcm_running(hcp->current_stream)) {dev_info(dev, "HDMI audio playback aborted\n");snd_pcm_stream_lock_irq(hcp->current_stream);snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);snd_pcm_stream_unlock_irq(hcp->current_stream);- }
- mutex_unlock(&hcp->current_stream_lock);
+}
Does driver should stop the stream in case of unplug? This could generate unexpected behavior at application level. Perhaps should be better if this part is managed in DRM driver. if HDMI master, I2S bus should be maintained ON to consume the audio stream until application action.
The whole point of this abort callback is to provide the means for the video side to stop the audio playback in a clean way.
The abort callback is given to the video side in startup() callback (ASoC side calls video side). If the video side sees that the playback can not go on it can call the abort callback and the audio playback will abort in standard ALSA way and a properly written applications should not get confused.
Nothing is forcing the video side to call the callback ever, if so decided. But if for instance power management stops the i2s clocks when connector is unplugged, then the audio can not go on any more and it should be aborted (actually it would abort in a moment when ALSA realizes that the DMA is not running).
+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- struct hdmi_codec_params hp = {
.iec = {.status = { 0 },.subcode = { 0 },.pad = 0,.dig_subframe = { 0 },}- };
- int ret;
- dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
params_width(params), params_rate(params),params_channels(params));- ret = snd_pcm_create_iec958_consumer_hw_params(params,
hp.iec.status,
sizeof(hp.iec.status));Tell me if i am wrong, but in case of SPDIF format, IEC status is managed by cpu_dai not by the codec. To manage IEC61937 a control should be needed to update IEC status bits...
AFAIK yes. However, the video side implementation is free to ignore status bits in the struct hdmi_codec_params and it should normally do so if the format is SPDIF. Of course I could save couple CPU cycles in the codec code if would not even produce the bits when the format is SPDIF.
Best regards, Jyri
- if (ret < 0) {
dev_err(dai->dev, "Creating IEC958 channel status failed %d\n",ret);return ret;- }
- ret = hdmi_codec_new_stream(substream, dai);
- if (ret)
return ret;- hdmi_audio_infoframe_init(&hp.cea);
- hp.cea.channels = params_channels(params);
- hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
- hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
- hp.cea.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
- hp.sample_width = params_width(params);
- hp.sample_rate = params_rate(params);
- hp.channels = params_channels(params);
- return hcp->hcd.ops->hw_params(dai->dev->parent,
&hcp->daifmt[dai->id],
&hp);+}
participants (4)
-
Arnaud Pouliquen -
Daniel Vetter -
Jyri Sarha -
Russell King - ARM Linux