Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

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.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel