[RFC PATCH] ALSA: hda: call ext hda codec link up/down if available

[RFC PATCH] ALSA: hda: call ext hda codec link up/down if available

[Kai Vehmanen]

The extended HDA bus (hdac_ext) provides interfaces for more
fine-grained control of individual links than what plain HDA provides
for. Links can be powered off when they are not used and if all
links are released, controller can shut down the command DMA.

These interfaces are currently not used by common HDA codec drivers.
When a HDA codec is runtime suspended, it calls snd_hdac_codec_link_down(),
but there is no link to the HDA extended bus, and links are shut down
only when all codecs are suspended.

To support more fine-grained control of link power, this patch
implements new helper functions for codec drivers to turn codec links
up and down. The HDA common suspend/resume code is modified to use
the new functions. This allows to fully reuse the driver code both
for plain and extended HDA controllers.

For compatibility, code reverts to plain HDA implementation if HDA
extended bus is disabled in the kernel build, or if the controller is
a plain HDA one.

Co-developed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 include/sound/hdaudio_ext.h         | 15 +++++++++++++++
 sound/hda/ext/hdac_ext_controller.c | 28 ++++++++++++++++++++++++++++
 sound/pci/hda/hda_codec.c           |  5 +++--
 3 files changed, 46 insertions(+), 2 deletions(-)

NOTES:
 - RFC status due to new usage of hdaudio_ext.h
 - Not fully tested! Validation still ongoing...

diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
index 7abf74c1c474..151922fa3c1e 100644
--- a/include/sound/hdaudio_ext.h
+++ b/include/sound/hdaudio_ext.h
@@ -131,6 +131,21 @@ void snd_hdac_ext_link_clear_stream_id(struct hdac_ext_link *link,
 int snd_hdac_ext_bus_link_get(struct hdac_bus *bus, struct hdac_ext_link *link);
 int snd_hdac_ext_bus_link_put(struct hdac_bus *bus, struct hdac_ext_link *link);
 
+#if IS_ENABLED(CONFIG_SND_HDA_EXT_CORE)
+void snd_hdac_ext_codec_link_up(struct hdac_device *codec);
+void snd_hdac_ext_codec_link_down(struct hdac_device *codec);
+#else
+static inline void snd_hdac_ext_codec_link_up(struct hdac_device *codec)
+{
+	snd_hdac_codec_link_up(codec);
+}
+
+static inline void snd_hdac_ext_codec_link_down(struct hdac_device *codec)
+{
+	snd_hdac_codec_link_down(codec);
+}
+#endif
+
 /* update register macro */
 #define snd_hdac_updatel(addr, reg, mask, val)		\
 	writel(((readl(addr + reg) & ~(mask)) | (val)), \
diff --git a/sound/hda/ext/hdac_ext_controller.c b/sound/hda/ext/hdac_ext_controller.c
index b0c0ef824d7d..fd03b5d3a10e 100644
--- a/sound/hda/ext/hdac_ext_controller.c
+++ b/sound/hda/ext/hdac_ext_controller.c
@@ -332,3 +332,31 @@ int snd_hdac_ext_bus_link_put(struct hdac_bus *bus,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_link_put);
+
+void snd_hdac_ext_codec_link_up(struct hdac_device *codec)
+{
+	snd_hdac_codec_link_up(codec);
+
+	if (codec->type == HDA_DEV_ASOC) {
+		const char *devname = dev_name(&codec->dev);
+		struct hdac_ext_link *hlink =
+			snd_hdac_ext_bus_get_link(codec->bus, devname);
+		if (hlink)
+			snd_hdac_ext_bus_link_get(codec->bus, hlink);
+	}
+}
+EXPORT_SYMBOL_GPL(snd_hdac_ext_codec_link_up);
+
+void snd_hdac_ext_codec_link_down(struct hdac_device *codec)
+{
+	snd_hdac_codec_link_down(codec);
+
+	if (codec->type == HDA_DEV_ASOC) {
+		const char *devname = dev_name(&codec->dev);
+		struct hdac_ext_link *hlink =
+			snd_hdac_ext_bus_get_link(codec->bus, devname);
+		if (hlink)
+			snd_hdac_ext_bus_link_put(codec->bus, hlink);
+	}
+}
+EXPORT_SYMBOL_GPL(snd_hdac_ext_codec_link_down);
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 9b755062d841..6113f787b494 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -14,6 +14,7 @@
 #include <linux/pm_runtime.h>
 #include <sound/core.h>
 #include <sound/hda_codec.h>
+#include <sound/hdaudio_ext.h>
 #include <sound/asoundef.h>
 #include <sound/tlv.h>
 #include <sound/initval.h>
@@ -2948,7 +2949,7 @@ static int hda_codec_runtime_suspend(struct device *dev)
 	if (codec->link_down_at_suspend ||
 	    (codec_has_clkstop(codec) && codec_has_epss(codec) &&
 	     (state & AC_PWRST_CLK_STOP_OK)))
-		snd_hdac_codec_link_down(&codec->core);
+		snd_hdac_ext_codec_link_down(&codec->core);
 	codec_display_power(codec, false);
 	return 0;
 }
@@ -2962,7 +2963,7 @@ static int hda_codec_runtime_resume(struct device *dev)
 		return 0;
 
 	codec_display_power(codec, true);
-	snd_hdac_codec_link_up(&codec->core);
+	snd_hdac_ext_codec_link_up(&codec->core);
 	hda_call_codec_resume(codec);
 	pm_runtime_mark_last_busy(dev);
 	return 0;

base-commit: 307cfa506ef4e6d7fe9430e513db2353925fb440
-- 
2.29.2

Re: [RFC PATCH] ALSA: hda: call ext hda codec link up/down if available

[Kai Vehmanen]

Hi,

On Thu, 4 Feb 2021, Kai Vehmanen wrote:
> To support more fine-grained control of link power, this patch
> implements new helper functions for codec drivers to turn codec links
> up and down. The HDA common suspend/resume code is modified to use
> the new functions. This allows to fully reuse the driver code both
> for plain and extended HDA controllers.

Takashi and others, this patch is a follow-up to the earlier thread about 
hdac link management:
https://mailman.alsa-project.org/pipermail/alsa-devel/2021-January/179715.html

We have absolutely no calls from sound/pci/hda/ to hdaudio_ext.h before
this patch, so I'm wondering if this is the right approach to take. 

It does seem to be the natural path forward and mimics what was done in 
e.g. hdac_hdmi. But we want to keep reusing driver code for all HDA 
implementations, so adding hdac-ext support to codec drivers seem like the 
best path.

Br, Kai

Re: [RFC PATCH] ALSA: hda: call ext hda codec link up/down if available

[Takashi Iwai]

On Thu, 04 Feb 2021 09:07:09 +0100,
Kai Vehmanen wrote:
> 
> Hi,
> 
> On Thu, 4 Feb 2021, Kai Vehmanen wrote:
> > To support more fine-grained control of link power, this patch
> > implements new helper functions for codec drivers to turn codec links
> > up and down. The HDA common suspend/resume code is modified to use
> > the new functions. This allows to fully reuse the driver code both
> > for plain and extended HDA controllers.
> 
> Takashi and others, this patch is a follow-up to the earlier thread about 
> hdac link management:
> https://mailman.alsa-project.org/pipermail/alsa-devel/2021-January/179715.html
> 
> We have absolutely no calls from sound/pci/hda/ to hdaudio_ext.h before
> this patch, so I'm wondering if this is the right approach to take. 

An obvious drawback by this patch is that it'll make the hda-ext code
always loaded whenever it's enabled, no matter whether it's really
used or not.

Maybe adding a new callback (link) in hdac_bus_ops can help?

> It does seem to be the natural path forward and mimics what was done in 
> e.g. hdac_hdmi. But we want to keep reusing driver code for all HDA 
> implementations, so adding hdac-ext support to codec drivers seem like the 
> best path.

Yes, I find we're heading to the right direction.


thanks,

Takashi

Re: [RFC PATCH] ALSA: hda: call ext hda codec link up/down if available

[Kai Vehmanen]

Hey,

On Thu, 4 Feb 2021, Takashi Iwai wrote:

> On Thu, 04 Feb 2021 09:07:09 +0100, Kai Vehmanen wrote:
> > Takashi and others, this patch is a follow-up to the earlier thread about 
> > hdac link management:
> > https://mailman.alsa-project.org/pipermail/alsa-devel/2021-January/179715.html
> > 
> > We have absolutely no calls from sound/pci/hda/ to hdaudio_ext.h before
> > this patch, so I'm wondering if this is the right approach to take. 
> 
> An obvious drawback by this patch is that it'll make the hda-ext code
> always loaded whenever it's enabled, no matter whether it's really
> used or not.
> 
> Maybe adding a new callback (link) in hdac_bus_ops can help?

hmm, that's true. Let me try adding a separate callback and resend for 
review. That will look cleaner in hda_codec.c.

>> It does seem to be the natural path forward and mimics what was done in 
>> e.g. hdac_hdmi. But we want to keep reusing driver code for all HDA 
>> implementations, so adding hdac-ext support to codec drivers seem like the 
>> best path.
> 
> Yes, I find we're heading to the right direction.

Ack, thanks for the quick review.

br, Kai

Re: [RFC PATCH] ALSA: hda: call ext hda codec link up/down if available

[Takashi Iwai]

On Thu, 04 Feb 2021 11:42:20 +0100,
Kai Vehmanen wrote:
> 
> Hey,
> 
> On Thu, 4 Feb 2021, Takashi Iwai wrote:
> 
> > On Thu, 04 Feb 2021 09:07:09 +0100, Kai Vehmanen wrote:
> > > Takashi and others, this patch is a follow-up to the earlier thread about 
> > > hdac link management:
> > > https://mailman.alsa-project.org/pipermail/alsa-devel/2021-January/179715.html
> > > 
> > > We have absolutely no calls from sound/pci/hda/ to hdaudio_ext.h before
> > > this patch, so I'm wondering if this is the right approach to take. 
> > 
> > An obvious drawback by this patch is that it'll make the hda-ext code
> > always loaded whenever it's enabled, no matter whether it's really
> > used or not.
> > 
> > Maybe adding a new callback (link) in hdac_bus_ops can help?
> 
> hmm, that's true. Let me try adding a separate callback and resend for 
> review. That will look cleaner in hda_codec.c.

Thanks.

BTW, one more thing: the code link up/down isn't always paired.  It's
a bit map and only holding states.  For example, in hda_codec.c, the
link power down may happen conditionally marked in
hda_codec_runtime_suspend() while the resume always marks the link
power up.  So you'd need to check the bitmap change to perform the
refcount up/down in the hda-ext side.


Takashi