Skip to content

Commit 029d92c

Browse files
tiwaitiwai
committed
ALSA: hda: Refactor display power management
The current HD-audio code manages the DRM audio power via too complex redirections, and this seems even still unbalanced in a corner case as Intel DRM CI has been intermittently reporting. This patch is a big surgery for addressing the complexity and the possible unbalance. Basically the patch changes the display PM in the following ways: - Both HD-audio controller and codec drivers call a single helper, snd_hdac_display_power(). (Formerly, the display power control from a codec was done indirectly via link_power bus ops.) - snd_hdac_display_power() receives the codec address index. For turning on/off from the controller, pass HDA_CODEC_IDX_CONTROLLER. - snd_hdac_display_power() doesn't manage refcounts any longer, but keeps the power status in bitmap. If any of controller or codecs is turned on, the function updates the DRM power state via get_power() or put_power(). Also this refactor allows us more cleanup: - The link_power bus ops is dropped, so there is no longer indirect management, as mentioned in the above. - hdac_device link_power_control flag is moved to hda_codec display_power_control flag, as it's only for HDA legacy. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106525 Signed-off-by: Takashi Iwai <tiwai@suse.de>
1 parent 3baffc4 commit 029d92c

File tree

11 files changed

+63
-77
lines changed

11 files changed

+63
-77
lines changed

‎include/sound/hda_codec.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,7 @@ struct hda_codec {
236236
/* misc flags */
237237
unsigned int in_freeing:1; /* being released */
238238
unsigned int registered:1; /* codec was registered */
239+
unsigned int display_power_control:1; /* needs display power */
239240
unsigned int spdif_status_reset :1; /* needs to toggle SPDIF for each
240241
* status change
241242
* (e.g. Realtek codecs)

‎include/sound/hda_component.h

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,15 @@
55
#define __SOUND_HDA_COMPONENT_H
66

77
#include <drm/drm_audio_component.h>
8+
#include <sound/hdaudio.h>
9+
10+
/* virtual idx for controller */
11+
#define HDA_CODEC_IDX_CONTROLLER HDA_MAX_CODECS
812

913
#ifdef CONFIG_SND_HDA_COMPONENT
1014
int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
11-
int snd_hdac_display_power(struct hdac_bus *bus, bool enable);
15+
int snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx,
16+
bool enable);
1217
int snd_hdac_sync_audio_rate(struct hdac_device *codec, hda_nid_t nid,
1318
int dev_id, int rate);
1419
int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid, int dev_id,
@@ -25,7 +30,8 @@ static inline int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable)
2530
{
2631
return 0;
2732
}
28-
static inline int snd_hdac_display_power(struct hdac_bus *bus, bool enable)
33+
static inline int snd_hdac_display_power(struct hdac_bus *bus,
34+
unsigned int idx, bool enable)
2935
{
3036
return 0;
3137
}

‎include/sound/hdaudio.h

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,6 @@ struct hdac_device {
7979

8080
/* misc flags */
8181
atomic_t in_pm; /* suspend/resume being performed */
82-
bool link_power_control:1;
8382

8483
/* sysfs */
8584
struct hdac_widget_tree *widgets;
@@ -237,8 +236,6 @@ struct hdac_bus_ops {
237236
/* get a response from the last command */
238237
int (*get_response)(struct hdac_bus *bus, unsigned int addr,
239238
unsigned int *res);
240-
/* control the link power */
241-
int (*link_power)(struct hdac_bus *bus, bool enable);
242239
};
243240

244241
/*
@@ -363,7 +360,8 @@ struct hdac_bus {
363360

364361
/* DRM component interface */
365362
struct drm_audio_component *audio_component;
366-
int drm_power_refcount;
363+
long display_power_status;
364+
bool display_power_active;
367365

368366
/* parameters required for enhanced capabilities */
369367
int num_streams;
@@ -404,7 +402,6 @@ int snd_hdac_bus_send_cmd(struct hdac_bus *bus, unsigned int val);
404402
int snd_hdac_bus_get_response(struct hdac_bus *bus, unsigned int addr,
405403
unsigned int *res);
406404
int snd_hdac_bus_parse_capabilities(struct hdac_bus *bus);
407-
int snd_hdac_link_power(struct hdac_device *codec, bool enable);
408405

409406
bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset);
410407
void snd_hdac_bus_stop_chip(struct hdac_bus *bus);

‎sound/hda/hdac_component.c

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -54,38 +54,45 @@ EXPORT_SYMBOL_GPL(snd_hdac_set_codec_wakeup);
5454
/**
5555
* snd_hdac_display_power - Power up / down the power refcount
5656
* @bus: HDA core bus
57+
* @idx: HDA codec address, pass HDA_CODEC_IDX_CONTROLLER for controller
5758
* @enable: power up or down
5859
*
59-
* This function is supposed to be used only by a HD-audio controller
60-
* driver that needs the interaction with graphics driver.
60+
* This function is used by either HD-audio controller or codec driver that
61+
* needs the interaction with graphics driver.
6162
*
62-
* This function manages a refcount and calls the get_power() and
63+
* This function updates the power status, and calls the get_power() and
6364
* put_power() ops accordingly, toggling the codec wakeup, too.
6465
*
6566
* Returns zero for success or a negative error code.
6667
*/
67-
int snd_hdac_display_power(struct hdac_bus *bus, bool enable)
68+
int snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable)
6869
{
6970
struct drm_audio_component *acomp = bus->audio_component;
7071

71-
if (!acomp || !acomp->ops)
72-
return -ENODEV;
73-
7472
dev_dbg(bus->dev, "display power %s\n",
7573
enable ? "enable" : "disable");
74+
if (enable)
75+
set_bit(idx, &bus->display_power_status);
76+
else
77+
clear_bit(idx, &bus->display_power_status);
7678

77-
if (enable) {
78-
if (!bus->drm_power_refcount++) {
79+
if (!acomp || !acomp->ops)
80+
return 0;
81+
82+
if (bus->display_power_status) {
83+
if (!bus->display_power_active) {
7984
if (acomp->ops->get_power)
8085
acomp->ops->get_power(acomp->dev);
8186
snd_hdac_set_codec_wakeup(bus, true);
8287
snd_hdac_set_codec_wakeup(bus, false);
88+
bus->display_power_active = true;
8389
}
8490
} else {
85-
WARN_ON(!bus->drm_power_refcount);
86-
if (!--bus->drm_power_refcount)
91+
if (bus->display_power_active) {
8792
if (acomp->ops->put_power)
8893
acomp->ops->put_power(acomp->dev);
94+
bus->display_power_active = false;
95+
}
8996
}
9097

9198
return 0;
@@ -321,10 +328,12 @@ int snd_hdac_acomp_exit(struct hdac_bus *bus)
321328
if (!acomp)
322329
return 0;
323330

324-
WARN_ON(bus->drm_power_refcount);
325-
if (bus->drm_power_refcount > 0 && acomp->ops)
331+
if (WARN_ON(bus->display_power_active) && acomp->ops)
326332
acomp->ops->put_power(acomp->dev);
327333

334+
bus->display_power_active = false;
335+
bus->display_power_status = 0;
336+
328337
component_master_del(dev, &hdac_component_master_ops);
329338

330339
bus->audio_component = NULL;

‎sound/hda/hdac_device.c

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -622,23 +622,6 @@ int snd_hdac_power_down_pm(struct hdac_device *codec)
622622
EXPORT_SYMBOL_GPL(snd_hdac_power_down_pm);
623623
#endif
624624

625-
/**
626-
* snd_hdac_link_power - Enable/disable the link power for a codec
627-
* @codec: the codec object
628-
* @bool: enable or disable the link power
629-
*/
630-
int snd_hdac_link_power(struct hdac_device *codec, bool enable)
631-
{
632-
if (!codec->link_power_control)
633-
return 0;
634-
635-
if (codec->bus->ops->link_power)
636-
return codec->bus->ops->link_power(codec->bus, enable);
637-
else
638-
return -EINVAL;
639-
}
640-
EXPORT_SYMBOL_GPL(snd_hdac_link_power);
641-
642625
/* codec vendor labels */
643626
struct hda_vendor_id {
644627
unsigned int id;

‎sound/pci/hda/hda_codec.c

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
#include "hda_beep.h"
3737
#include "hda_jack.h"
3838
#include <sound/hda_hwdep.h>
39+
#include <sound/hda_component.h>
3940

4041
#define codec_in_pm(codec) snd_hdac_is_in_pm(&codec->core)
4142
#define hda_codec_is_power_on(codec) snd_hdac_is_power_on(&codec->core)
@@ -799,14 +800,21 @@ void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec)
799800
static unsigned int hda_set_power_state(struct hda_codec *codec,
800801
unsigned int power_state);
801802

803+
/* enable/disable display power per codec */
804+
static void codec_display_power(struct hda_codec *codec, bool enable)
805+
{
806+
if (codec->display_power_control)
807+
snd_hdac_display_power(&codec->bus->core, codec->addr, enable);
808+
}
809+
802810
/* also called from hda_bind.c */
803811
void snd_hda_codec_register(struct hda_codec *codec)
804812
{
805813
if (codec->registered)
806814
return;
807815
if (device_is_registered(hda_codec_dev(codec))) {
808816
snd_hda_register_beep_device(codec);
809-
snd_hdac_link_power(&codec->core, true);
817+
codec_display_power(codec, true);
810818
pm_runtime_enable(hda_codec_dev(codec));
811819
/* it was powered up in snd_hda_codec_new(), now all done */
812820
snd_hda_power_down(codec);
@@ -834,7 +842,7 @@ static int snd_hda_codec_dev_free(struct snd_device *device)
834842

835843
codec->in_freeing = 1;
836844
snd_hdac_device_unregister(&codec->core);
837-
snd_hdac_link_power(&codec->core, false);
845+
codec_display_power(codec, false);
838846
put_device(hda_codec_dev(codec));
839847
return 0;
840848
}
@@ -2926,15 +2934,15 @@ static int hda_codec_runtime_suspend(struct device *dev)
29262934
(codec_has_clkstop(codec) && codec_has_epss(codec) &&
29272935
(state & AC_PWRST_CLK_STOP_OK)))
29282936
snd_hdac_codec_link_down(&codec->core);
2929-
snd_hdac_link_power(&codec->core, false);
2937+
codec_display_power(codec, false);
29302938
return 0;
29312939
}
29322940

29332941
static int hda_codec_runtime_resume(struct device *dev)
29342942
{
29352943
struct hda_codec *codec = dev_to_hda_codec(dev);
29362944

2937-
snd_hdac_link_power(&codec->core, true);
2945+
codec_display_power(codec, true);
29382946
snd_hdac_codec_link_up(&codec->core);
29392947
hda_call_codec_resume(codec);
29402948
pm_runtime_mark_last_busy(dev);

‎sound/pci/hda/hda_controller.c

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -989,20 +989,9 @@ static int azx_get_response(struct hdac_bus *bus, unsigned int addr,
989989
return azx_rirb_get_response(bus, addr, res);
990990
}
991991

992-
static int azx_link_power(struct hdac_bus *bus, bool enable)
993-
{
994-
struct azx *chip = bus_to_azx(bus);
995-
996-
if (chip->ops->link_power)
997-
return chip->ops->link_power(chip, enable);
998-
else
999-
return -EINVAL;
1000-
}
1001-
1002992
static const struct hdac_bus_ops bus_core_ops = {
1003993
.command = azx_send_cmd,
1004994
.get_response = azx_get_response,
1005-
.link_power = azx_link_power,
1006995
};
1007996

1008997
#ifdef CONFIG_SND_HDA_DSP_LOADER

‎sound/pci/hda/hda_intel.c

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -667,13 +667,8 @@ static int azx_position_check(struct azx *chip, struct azx_dev *azx_dev)
667667
return 0;
668668
}
669669

670-
/* Enable/disable i915 display power for the link */
671-
static int azx_intel_link_power(struct azx *chip, bool enable)
672-
{
673-
struct hdac_bus *bus = azx_bus(chip);
674-
675-
return snd_hdac_display_power(bus, enable);
676-
}
670+
#define display_power(chip, enable) \
671+
snd_hdac_display_power(azx_bus(chip), HDA_CODEC_IDX_CONTROLLER, enable)
677672

678673
/*
679674
* Check whether the current DMA position is acceptable for updating
@@ -957,7 +952,7 @@ static void __azx_runtime_suspend(struct azx *chip)
957952
azx_clear_irq_pending(chip);
958953
if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL) &&
959954
hda->need_i915_power)
960-
snd_hdac_display_power(azx_bus(chip), false);
955+
display_power(chip, false);
961956
}
962957

963958
static void __azx_runtime_resume(struct azx *chip)
@@ -968,7 +963,7 @@ static void __azx_runtime_resume(struct azx *chip)
968963
int status;
969964

970965
if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
971-
snd_hdac_display_power(bus, true);
966+
display_power(chip, true);
972967
if (hda->need_i915_power)
973968
snd_hdac_i915_set_bclk(bus);
974969
}
@@ -989,7 +984,7 @@ static void __azx_runtime_resume(struct azx *chip)
989984
/* power down again for link-controlled chips */
990985
if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL) &&
991986
!hda->need_i915_power)
992-
snd_hdac_display_power(bus, false);
987+
display_power(chip, false);
993988
}
994989

995990
#ifdef CONFIG_PM_SLEEP
@@ -1355,7 +1350,7 @@ static int azx_free(struct azx *chip)
13551350

13561351
if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
13571352
if (hda->need_i915_power)
1358-
snd_hdac_display_power(bus, false);
1353+
display_power(chip, false);
13591354
}
13601355
if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT)
13611356
snd_hdac_i915_exit(bus);
@@ -2056,7 +2051,6 @@ static const struct hda_controller_ops pci_hda_ops = {
20562051
.disable_msi_reset_irq = disable_msi_reset_irq,
20572052
.pcm_mmap_prepare = pcm_mmap_prepare,
20582053
.position_check = azx_position_check,
2059-
.link_power = azx_intel_link_power,
20602054
};
20612055

20622056
static int azx_probe(struct pci_dev *pci,
@@ -2239,7 +2233,7 @@ static int azx_probe_continue(struct azx *chip)
22392233
if (CONTROLLER_IN_GPU(pci))
22402234
hda->need_i915_power = 1;
22412235

2242-
err = snd_hdac_display_power(bus, true);
2236+
err = display_power(chip, true);
22432237
if (err < 0) {
22442238
dev_err(chip->card->dev,
22452239
"Cannot turn on display power on i915\n");
@@ -2295,7 +2289,7 @@ static int azx_probe_continue(struct azx *chip)
22952289
out_free:
22962290
if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
22972291
&& !hda->need_i915_power)
2298-
snd_hdac_display_power(bus, false);
2292+
display_power(chip, false);
22992293

23002294
i915_power_fail:
23012295
if (err < 0)

‎sound/pci/hda/patch_hdmi.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2620,7 +2620,7 @@ static int intel_hsw_common_init(struct hda_codec *codec, hda_nid_t vendor_nid)
26202620
* can cover the codec power request, and so need not set this flag.
26212621
*/
26222622
if (!is_haswell(codec) && !is_broadwell(codec))
2623-
codec->core.link_power_control = 1;
2623+
codec->display_power_control = 1;
26242624

26252625
codec->patch_ops.set_power_state = haswell_set_power_state;
26262626
codec->depop_delay = 0;
@@ -2656,7 +2656,7 @@ static int patch_i915_byt_hdmi(struct hda_codec *codec)
26562656
/* For Valleyview/Cherryview, only the display codec is in the display
26572657
* power well and can use link_power ops to request/release the power.
26582658
*/
2659-
codec->core.link_power_control = 1;
2659+
codec->display_power_control = 1;
26602660

26612661
codec->depop_delay = 0;
26622662
codec->auto_runtime_pm = 1;

‎sound/soc/codecs/hdac_hdmi.c

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2031,14 +2031,13 @@ static int hdac_hdmi_dev_probe(struct hdac_device *hdev)
20312031
* Turned off in the runtime_suspend during the first explicit
20322032
* pm_runtime_suspend call.
20332033
*/
2034-
ret = snd_hdac_display_power(hdev->bus, true);
2034+
ret = snd_hdac_display_power(hdev->bus, hdev->addr, true);
20352035
if (ret < 0) {
20362036
dev_err(&hdev->dev,
20372037
"Cannot turn on display power on i915 err: %d\n",
20382038
ret);
20392039
return ret;
20402040
}
2041-
20422041
ret = hdac_hdmi_parse_and_map_nid(hdev, &hdmi_dais, &num_dais);
20432042
if (ret < 0) {
20442043
dev_err(&hdev->dev,
@@ -2196,7 +2195,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
21962195

21972196
snd_hdac_ext_bus_link_put(bus, hlink);
21982197

2199-
err = snd_hdac_display_power(bus, false);
2198+
err = snd_hdac_display_power(bus, hdev->addr, false);
22002199
if (err < 0)
22012200
dev_err(dev, "Cannot turn off display power on i915\n");
22022201

@@ -2224,7 +2223,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
22242223

22252224
snd_hdac_ext_bus_link_get(bus, hlink);
22262225

2227-
err = snd_hdac_display_power(bus, true);
2226+
err = snd_hdac_display_power(bus, hdev->addr, true);
22282227
if (err < 0) {
22292228
dev_err(dev, "Cannot turn on display power on i915\n");
22302229
return err;

0 commit comments

Comments
 (0)