[alsa-devel] HDMI codec, way forward?

Commit Message

Takashi Iwai Oct. 21, 2015, 4:46 p.m. UTC

On Wed, 21 Oct 2015 17:36:01 +0200,
Daniel Vetter wrote:
> 
> On Wed, Oct 21, 2015 at 04:37:07PM +0200, Takashi Iwai wrote:
> > On Wed, 21 Oct 2015 16:10:31 +0200,
> > Takashi Iwai wrote:
> > > 
> > > On Wed, 21 Oct 2015 16:03:07 +0200,
> > > Russell King - ARM Linux wrote:
> > > > 
> > > > On Wed, Oct 21, 2015 at 03:41:44PM +0200, Takashi Iwai wrote:
> > > > > On Wed, 21 Oct 2015 11:27:44 +0200,
> > > > > Russell King - ARM Linux wrote:
> > > > > > 
> > > > > > On Tue, Oct 20, 2015 at 07:31:48PM +0530, Vinod Koul wrote:
> > > > > > > On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote:
> > > > > > > > > > Currently i915/audio component works as you described.  The audio is
> > > > > > > > > > optional and HDMI graphics works without audio, while HDMI HD-audio
> > > > > > > > > > mandates i915 graphics.
> > > > > > > > > 
> > > > > > > > > Right, but we also add additional interface on top of this to allow
> > > > > > > > > things like ensuring display is on when audio wants to run and now
> > > > > > > > > notification for events.
> > > > > > > > > 
> > > > > > > > > I don't see a reason why this can be used as single interface to bind as
> > > > > > > > > well as talk to display from various components, unless I missed obvious
> > > > > > > > > which prevent us from doing this in non i915 cases...
> > > > > > > > 
> > > > > > > > Maybe I can comment more specifically if I saw the code.  Right now all
> > > > > > > > I'm aware of is this idea without any code, and I don't like it.
> > > > > > > 
> > > > > > > Ok, i will be post my patches tomorrow. FWIW uses interface in
> > > > > > > sound/hda/hdac_i915.c for display power up/down
> > > > > > 
> > > > > > This:
> > > > > > 
> > > > > >         component_match_add(dev, &match, hdac_component_master_match, bus);
> > > > > >         ret = component_master_add_with_match(dev, &hdac_component_master_ops,
> > > > > >                                               match);
> > > > > >         if (ret < 0)
> > > > > >                 goto out_err;
> > > > > > 
> > > > > >         /*
> > > > > >          * Atm, we don't support deferring the component binding, so make sure
> > > > > >          * i915 is loaded and that the binding successfully completes.
> > > > > >          */
> > > > > >         request_module("i915");
> > > > > > 
> > > > > >         if (!acomp->ops) {
> > > > > >                 ret = -ENODEV;
> > > > > >                 goto out_master_del;
> > > > > >         }
> > > > > > 
> > > > > > is really not nice.
> > > > > 
> > > > > Yeah, admittedly it's a really ugly workaround.  It's coded in that
> > > > > way just to make our lives easier: it works well in most cases for
> > > > > i915 / hd-audio.
> > > > > 
> > > > > Actually, it's easy to modify the hda code in the right way, i.e. to
> > > > > defer via component bind ops.  However, one drawback is that it's not
> > > > > trivial to handle the fallback case; namely, HD-audio might not need
> > > > > i915 binding, depending on the chip model and the configuration.
> > > > > 
> > > > > Braswell and Skylake have a (virtually) single audio controller as
> > > > > default, and this manages both the analog and HDMI/DP codecs.  The
> > > > > i915 interaction is required only for the latter codecs, and thus for
> > > > > the former, it's optional.  If we defer binding, the instantiation
> > > > > won't happen unless it's bound with i915.  So, if i915 isn't
> > > > > initialized (e.g. by booting with nomodeset option), the whole audio
> > > > > will be gone, too.  OTOH, Haswell and Braswell have a dedicated HDA
> > > > > controller for HDMI/DP, and they must be disabled (or fail) unless
> > > > > bound with graphics.
> > > > > 
> > > > > It's a corner case, so we might better ignore this.  But it'd be
> > > > > certainly a "regression" -- at least, I used to test this in that way
> > > > > sometimes in the past.  So it remains in the current way as is.
> > > > > 
> > > > > It's one of the reasons I mentioned it being a stop gap.  But, I think
> > > > > the concept, passing ops via component, is actually working, and
> > > > > should be applicable to other drm/audio drivers.  That's the point.
> > > > 
> > > > It's only the point if you can code it up properly, which from what I
> > > > read in that file, it isn't.
> > > 
> > > An idea can fly without coding, too :)
> > > 
> > > > Build the i915 DRM drivers as a module, load up the system, and then
> > > > try removing the i915 DRM module and see what happens to the audio part.
> > > > For starters, you have no protection what so ever against acomp->ops or
> > > > acomp->dev becoming NULL - it's hellishly racy.
> > > 
> > > Yes, very likely.
> > > 
> > > > Secondly, you reject the initialisation if acomp->ops isn't set, but you
> > > > allow acomp->ops to later become unset by the i915 DRM module being
> > > > removed.  If you can cope with acomp->ops being unset at a random point
> > > > during the audio driver's use, why can't you cope with it being set at
> > > > some random point later?
> > > 
> > > Setting/unsetting on the fly would be picky because the code does
> > > refcounting.  Maybe an easier option is to inc/dec module usage
> > > count appropriately in use.
> > 
> > ... and actually we pin at master bind:
> > 
> > static int hdac_component_master_bind(struct device *dev)
> > {
> > .....
> > 	/*
> > 	 * Atm, we don't support dynamic unbinding initiated by the child
> > 	 * component, so pin its containing module until we unbind.
> > 	 */
> > 	if (!try_module_get(acomp->ops->owner)) {
> > 
> > so unloading i915 module won't happen (but other method for unbinding
> > still possible).
> 
> Pinning the module only prevents the code from disappearing, not the
> driver itself from disappearing. You can still just manually unbind
> through sysfs.

Yes, that's what I mentioned in parenthesis in the above :)
We have always a brute force kill method.

> The real fix is really what Russell described.

Right... I guess the fix is fairly trivial, though.  All accesses to
audio components are done via only helper functions, so we'd just need
some lock there to protect from the unbind.

Below is a test patch I cooked quickly.  This is the third patch
applied after other two more patches: a cleanup patch and a patch for
deferred probe of HD-audio with component.


Takashi

--- 8< ---
From: Takashi Iwai <[e-mail]>
Subject: [PATCH 3/3] ALSA: hda - Add mutex protection at unbinding slave

For a safer unbinding, each access to audio component ops is protected
via a new mutex, hdac_bus.audio_component_lock.

Signed-off-by: Takashi Iwai <[e-mail]>
---
 include/sound/hdaudio.h |  1 +
 sound/hda/hdac_i915.c   | 63 +++++++++++++++++++++++++++++++++++--------------
 2 files changed, 46 insertions(+), 18 deletions(-)

Patch

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index e2b712c90d3f..b88443c33aee 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -303,6 +303,7 @@  struct hdac_bus {
 
 	/* i915 component interface */
 	struct i915_audio_component *audio_component;
+	struct mutex audio_component_lock;
 	int i915_power_refcount;
 };
 
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index f6fc16cfd02c..625e74ad2d12 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -26,17 +26,31 @@  struct hdac_gfx_component {
 	const struct component_master_ops *ops;
 };
 
+static struct i915_audio_component *bus_get_acomp(struct hdac_bus *bus)
+{
+	mutex_lock(&bus->audio_component_lock);
+	return bus->audio_component;
+}
+
+static void bus_put_acomp(struct hdac_bus *bus)
+{
+	mutex_lock(&bus->audio_component_lock);
+}
+
 int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable)
 {
-	struct i915_audio_component *acomp = bus->audio_component;
+	struct i915_audio_component *acomp = bus_get_acomp(bus);
+	int ret = 0;
 
-	if (!acomp || !acomp->ops)
-		return -ENODEV;
+	if (!acomp || !acomp->ops) {
+		ret = -ENODEV;
+		goto out;
+	}
 
 	if (!acomp->ops->codec_wake_override) {
 		dev_warn(bus->dev,
 			"Invalid codec wake callback\n");
-		return 0;
+		goto out;
 	}
 
 	dev_dbg(bus->dev, "%s codec wakeup\n",
@@ -44,16 +58,21 @@  int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable)
 
 	acomp->ops->codec_wake_override(acomp->dev, enable);
 
-	return 0;
+ out:
+	bus_put_acomp(bus);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_set_codec_wakeup);
 
 int snd_hdac_display_power(struct hdac_bus *bus, bool enable)
 {
-	struct i915_audio_component *acomp = bus->audio_component;
+	struct i915_audio_component *acomp = bus_get_acomp(bus);
+	int ret = 0;
 
-	if (!acomp || !acomp->ops)
-		return -ENODEV;
+	if (!acomp || !acomp->ops) {
+		ret = -ENODEV;
+		goto out;
+	}
 
 	dev_dbg(bus->dev, "display power %s\n",
 		enable ? "enable" : "disable");
@@ -70,18 +89,22 @@  int snd_hdac_display_power(struct hdac_bus *bus, bool enable)
 			acomp->ops->put_power(acomp->dev);
 	}
 
-	return 0;
+ out:
+	bus_put_acomp(bus);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_display_power);
 
 int snd_hdac_get_display_clk(struct hdac_bus *bus)
 {
-	struct i915_audio_component *acomp = bus->audio_component;
+	struct i915_audio_component *acomp = bus_get_acomp(bus);
+	int ret;
 
 	if (!acomp || !acomp->ops)
-		return -ENODEV;
-
-	return acomp->ops->get_cdclk_freq(acomp->dev);
+		ret = -ENODEV;
+	ret = acomp->ops->get_cdclk_freq(acomp->dev);
+	bus_put_acomp(bus);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_get_display_clk);
 
@@ -89,14 +112,14 @@  static int hdac_component_master_bind(struct device *dev)
 {
 	struct hdac_device *codec = dev_to_hdac_dev(dev);
 	struct hdac_bus *bus = codec->bus;
-	struct i915_audio_component *acomp = bus->audio_component;
+	struct i915_audio_component *acomp = bus_get_acomp(bus);
 	struct hdac_gfx_component *hda_comp =
 		container_of(acomp, struct hdac_gfx_component, acomp);
 	int ret;
 
 	ret = component_bind_all(dev, acomp);
 	if (ret < 0)
-		return ret;
+		goto out_unlock;
 
 	if (WARN_ON(!(acomp->dev && acomp->ops && acomp->ops->get_power &&
 		      acomp->ops->put_power && acomp->ops->get_cdclk_freq))) {
@@ -125,7 +148,8 @@  static int hdac_component_master_bind(struct device *dev)
 
 out_unbind:
 	component_unbind_all(dev, acomp);
-
+ out_unlock:
+	bus_put_acomp(bus);
 	return ret;
 }
 
@@ -133,7 +157,7 @@  static void hdac_component_master_unbind(struct device *dev)
 {
 	struct hdac_device *codec = dev_to_hdac_dev(dev);
 	struct hdac_bus *bus = codec->bus;
-	struct i915_audio_component *acomp = bus->audio_component;
+	struct i915_audio_component *acomp = bus_get_acomp(bus);
 	struct hdac_gfx_component *hda_comp =
 		container_of(acomp, struct hdac_gfx_component, acomp);
 
@@ -142,6 +166,7 @@  static void hdac_component_master_unbind(struct device *dev)
 	module_put(acomp->ops->owner);
 	component_unbind_all(dev, acomp);
 	WARN_ON(acomp->ops || acomp->dev);
+	bus_put_acomp(bus);
 }
 
 static const struct component_master_ops hdac_component_master_ops = {
@@ -159,12 +184,13 @@  int snd_hdac_i915_register_notifier(struct hdac_device *codec,
 				    const struct i915_audio_component_audio_ops *aops)
 {
 	struct hdac_bus *bus = codec->bus;
-	struct i915_audio_component *acomp = bus->audio_component;
+	struct i915_audio_component *acomp = bus_get_acomp(bus);
 
 	if (WARN_ON(!acomp))
 		return -ENODEV;
 
 	acomp->audio_ops = aops;
+	bus_put_acomp(bus);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_i915_register_notifier);
@@ -179,6 +205,7 @@  int snd_hdac_i915_init(struct hdac_bus *bus,
 	hda_comp = kzalloc(sizeof(*hda_comp), GFP_KERNEL);
 	if (!hda_comp)
 		return -ENOMEM;
+	mutex_init(&bus->audio_component_lock);
 	bus->audio_component = &hda_comp->acomp;
 	hda_comp->ops = codec_ops;