Re: [Intel-gfx] [PATCH 2/7] drm/i915: Add get_eld audio component
On 2015-11-30 16:29, Takashi Iwai wrote: On Mon, 30 Nov 2015 16:24:41 +0100, Ville Syrjälä wrote: On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote: Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code. Why do we need this? Isn't it something the eld notify hook should pass from i915 to the audio driver? We need this at least in two situations: - when the audio driver is probed - when the audio driver is resumed At least with the locking you have for this, the audio driver can not call this from the eld notify hook since it would deadlock. OK, then we may change the eld_notify to pass the values in the arguments, and also add the new op with a lock to be called from other places from other places. Maybe we have to make eld_notify not do anything except to call schedule_work then? And that work in turn will ask for updated eld from the i915 driver, and handle the result. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On 2015-11-27 14:38, Takashi Iwai wrote:
On Fri, 27 Nov 2015 03:55:28 +0100,
Zhang, Xiong Y wrote:
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index bdb6f226d006..0cd7bb30b045 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2352,6 +2352,10 @@ static void intel_pin_eld_notify(void *audio_ptr, int
port)
struct hda_codec *codec = audio_ptr;
int pin_nid = port + 0x04;
+ /* skip notification during suspend */
+ if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
+ return;
+
check_presence_and_report(codec, pin_nid);
}
[Zhang, Xiong Y] yes, this patch could remove the error message.
Alright, then it's indeed a failure in the sound driver side.
Below is the official patch I'm going to merge.
Just a quick question; have you checked that this does not bring back
the original bug the entire patch set was supposed to fix?
thanks,
Takashi
-- 8< --
From: Takashi Iwai <ti...@suse.de>
Subject: [PATCH] ALSA: hda - Skip ELD notification during system suspend
The recent addition of ELD notifier for Intel HDMI/DP codec may lead
the bad codec connection found as kernel messages like below:
Suspending console(s) (use no_console_suspend to debug)
hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: Codec=2
Pin=6 Presence_Detect=1 ELD_Valid=1
snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08
snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08
snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 128
snd_hda_intel :00:1f.3: azx_get_response timeout, switching to polling
mode: last cmd=0x206f2f00
snd_hda_intel :00:1f.3: No response from codec, disabling MSI: last
cmd=0x206f2f00
snd_hda_intel :00:1f.3: azx_get_response timeout, switching to single_cmd
mode: last cmd=0x206f2f00
azx_single_wait_for_response: 42 callbacks suppressed
This seems appearing when the sound driver went to suspend before i915
driver. Then i915 driver disables HDMI/DP audio bit and calls the
registered notifier, and the HDA codec tries to handle it as a
hot(un)plug. But since the driver is already in the suspended state,
it fails miserably.
As this is a sort of spurious wakeup, it can be ignored safely, as
long as it's delivered during the system suspend. OTOH, if a
notification comes during the runtime suspend, the situation is
different: we need to wake up. But during the system suspend, such a
notification can't be the reason for a wakeup.
This patch addresses it by a simple check of the current sound card
status. The skipped notification doesn't matter because the HDA
driver will check the plugged status forcibly at the resume in
return.
Then, why the card status, not a runtime PM status or else? The HDA
controller driver is supposed to set the card status to D3 at the
system suspend but not at the runtime suspend. So we can see it as a
flag that is set only for the system suspend. Admittedly, it's a bit
ugly, but it should work well for now.
Reported-and-tested-by: "Zhang, Xiong Y" <xiong.y.zh...@intel.com>
Fixes: 25adc137c546 ('ALSA: hda - Wake the codec up on pin/ELD notify events')
Cc: <sta...@vger.kernel.org> # v4.3+
Signed-off-by: Takashi Iwai <ti...@suse.de>
---
sound/pci/hda/patch_hdmi.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index bdb6f226d006..4b6fb668c91c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2352,6 +2352,12 @@ static void intel_pin_eld_notify(void *audio_ptr, int
port)
struct hda_codec *codec = audio_ptr;
int pin_nid = port + 0x04;
+ /* skip notification during system suspend (but not in runtime PM);
+* the state will be updated at resume
+*/
+ if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
+ return;
+
check_presence_and_report(codec, pin_nid);
}
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On 2015-11-26 10:24, Takashi Iwai wrote: On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote: On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote: BTW, I have a patchset to avoid the audio h/w wakeup by a new component ops to get ELD and connection states. It was posted to alsa-devel shortly ago just as a reference, but this should work well in such a case, too. The test patches are found in test/hdmi-jack branch of my sound git tree. Did you try this branch (merge onto intel-test-nightly)? [Zhang, Xiong Y] Yes, this branch could fix my issue. OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes. Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap. Takashi --- diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) struct hda_codec *codec = audio_ptr; int pin_nid = port + 0x04; - check_presence_and_report(codec, pin_nid); + if (!atomic_read(>core.in_pm) && + !pm_runtime_suspended(hda_codec_dev(codec))) + check_presence_and_report(codec, pin_nid); } static int patch_generic_hdmi(struct hda_codec *codec) [Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend(). OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path. We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...? -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On 2015-11-26 16:23, Takashi Iwai wrote: On Thu, 26 Nov 2015 16:08:06 +0100, David Henningsson wrote: On 2015-11-26 10:24, Takashi Iwai wrote: On Thu, 26 Nov 2015 10:16:17 +0100, Zhang, Xiong Y wrote: On Thu, 26 Nov 2015 08:57:30 +0100, Zhang, Xiong Y wrote: BTW, I have a patchset to avoid the audio h/w wakeup by a new component ops to get ELD and connection states. It was posted to alsa-devel shortly ago just as a reference, but this should work well in such a case, too. The test patches are found in test/hdmi-jack branch of my sound git tree. Did you try this branch (merge onto intel-test-nightly)? [Zhang, Xiong Y] Yes, this branch could fix my issue. OK, good to hear. But this isn't for 4.4 or backport, as it's more radical changes. Could you check the patch below instead? This suppresses the notifier handling during suspend. It's bad, but the hotplug should be still handled via normal unsol event, so it should keep working, good enough as a stop gap. Takashi --- diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..f7c70e2ae65c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) struct hda_codec *codec = audio_ptr; int pin_nid = port + 0x04; - check_presence_and_report(codec, pin_nid); + if (!atomic_read(>core.in_pm) && + !pm_runtime_suspended(hda_codec_dev(codec))) + check_presence_and_report(codec, pin_nid); } static int patch_generic_hdmi(struct hda_codec *codec) [Zhang, Xiong Y] this patch couldn't remove the error message. So audio controller isn't in Runtime D3 after azx_suspend(). OK, then the problem isn't about the HD-audio driver but rather i915 driver. As already mentioned, i915 driver shouldn't issue eld_notify callback in the suspend code path. We don't have a complete drm log so I'm only speculating here; but the HDA log seems to indicate the power well is off when we try to talk to the HDA controller. That means either the i915 shut it down while we were holding a lock on it, or HDA tried to lock it and that didn't make it through; in which case the HDA side should handle an error from get_power more gracefully...? My understanding is that it's triggered *during* i915 suspend. Then the path calls the HDA callback, and HDA controller tries to get power and proceeds as it has no idea that it's being shut off. Well; that can also be stopped by letting the get_power call return a failure code in case i915 is trying to suspend itself. That seems more robust to me, as it could potentially avoid other S3 races too...? Unfortunately, neither get_power callback or the corresponding HDA code has a capability to check that state. So, changing / fixing this there would be nice but very hard. Surely the i915 driver has some "am_i_suspending" variable it can check in the get_power callback? So I believe it's easier to avoid calling the eld_notify callback from i915 side during its suspend code. Takashi -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3] drm/i915: Improve kernel-doc for i915_audio_component struct
Signed-off-by: David Henningsson <david.hennings...@canonical.com>
---
Now rebased against drm-intel git master.
include/drm/i915_component.h | 69
1 file changed, 51 insertions(+), 18 deletions(-)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index 30d89e0..fab1385 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -31,47 +31,80 @@
#define MAX_PORTS 5
/**
- * struct i915_audio_component_ops - callbacks defined in gfx driver
- * @owner: the module owner
- * @get_power: get the POWER_DOMAIN_AUDIO power well
- * @put_power: put the POWER_DOMAIN_AUDIO power well
- * @codec_wake_override: Enable/Disable generating the codec wake signal
- * @get_cdclk_freq: get the Core Display Clock in KHz
- * @sync_audio_rate: set n/cts based on the sample rate
+ * struct i915_audio_component_ops - Ops implemented by i915 driver, called by
hda driver
*/
struct i915_audio_component_ops {
+ /**
+* @owner: i915 module
+*/
struct module *owner;
+ /**
+* @get_power: get the POWER_DOMAIN_AUDIO power well
+*
+* Request the power well to be turned on.
+*/
void (*get_power)(struct device *);
+ /**
+* @put_power: put the POWER_DOMAIN_AUDIO power well
+*
+* Allow the power well to be turned off.
+*/
void (*put_power)(struct device *);
+ /**
+* @codec_wake_override: Enable/disable codec wake signal
+*/
void (*codec_wake_override)(struct device *, bool enable);
+ /**
+* @get_cdclk_freq: Get the Core Display Clock in kHz
+*/
int (*get_cdclk_freq)(struct device *);
+ /**
+* @sync_audio_rate: set n/cts based on the sample rate
+*
+* Called from audio driver. After audio driver sets the
+* sample rate, it will call this function to set n/cts
+*/
int (*sync_audio_rate)(struct device *, int port, int rate);
};
+/**
+ * struct i915_audio_component_audio_ops - Ops implemented by hda driver,
called by i915 driver
+ */
struct i915_audio_component_audio_ops {
+ /**
+* @audio_ptr: Pointer to be used in call to pin_eld_notify
+*/
void *audio_ptr;
/**
-* Call from i915 driver, notifying the HDA driver that
-* pin sense and/or ELD information has changed.
-* @audio_ptr: HDA driver object
-* @port: Which port has changed (PORTA / PORTB / PORTC etc)
+* @pin_eld_notify: Notify the HDA driver that pin sense and/or ELD
information has changed
+*
+* Called when the i915 driver has set up audio pipeline or has just
+* begun to tear it down. This allows the HDA driver to update its
+* status accordingly (even when the HDA controller is in power save
+* mode).
*/
void (*pin_eld_notify)(void *audio_ptr, int port);
};
/**
- * struct i915_audio_component - used for audio video interaction
- * @dev: the device from gfx driver
- * @aud_sample_rate: the array of audio sample rate per port
- * @ops: callback for audio driver calling
- * @audio_ops: Call from i915 driver
+ * struct i915_audio_component - Used for direct communication between i915
and hda drivers
*/
struct i915_audio_component {
+ /**
+* @dev: i915 device, used as parameter for ops
+*/
struct device *dev;
+ /**
+* @aud_sample_rate: the array of audio sample rate per port
+*/
int aud_sample_rate[MAX_PORTS];
-
+ /**
+* @ops: Ops implemented by i915 driver, called by hda driver
+*/
const struct i915_audio_component_ops *ops;
-
+ /**
+* @audio_ops: Ops implemented by hda driver, called by i915 driver
+*/
const struct i915_audio_component_audio_ops *audio_ops;
};
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Improve kernel-doc for i915_audio_component struct
On 2015-10-12 10:07, David Henningsson wrote:
To make kernel-doc happy, the i915_audio_component_audio_ops struct
cannot be nested.
Signed-off-by: David Henningsson <david.hennings...@canonical.com>
---
Changes since v1:
* Added a notice about when pin_eld_notify is called
* Uses new inline struct member style
Verified with "make htmldocs", looks fine to me.
Also, applies to Takashi's tree (master branch), maybe we should take it
through Takashi's tree to avoid conflicts, especially as there might be
more stuff coming that way (dp mst support from Mengdong, and perhaps
info retrieval from me).
Documentation/DocBook/drm.tmpl | 1 +
include/drm/i915_component.h | 92 ++
2 files changed, 67 insertions(+), 26 deletions(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 9ddf8c6..f16e4e8 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -4051,6 +4051,7 @@ int num_ioctls;
High Definition Audio
!Pdrivers/gpu/drm/i915/intel_audio.c High Definition Audio over HDMI and
Display Port
!Idrivers/gpu/drm/i915/intel_audio.c
+!Iinclude/drm/i915_component.h
Panel Self Refresh PSR (PSR/SRD)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index 89dc7d6..76c10c8 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -30,38 +30,78 @@
*/
#define MAX_PORTS 5
+/**
+ * struct i915_audio_component_ops - Ops implemented by i915 driver, called by
hda driver
+ */
+struct i915_audio_component_ops {
+ /**
+* @owner: i915 module
+*/
+ struct module *owner;
+ /**
+* @get_power: Request that power well is to be turned on
+*/
+ void (*get_power)(struct device *);
+ /**
+* @put_power: Allow the power well to be turned off
+*/
+ void (*put_power)(struct device *);
+ /**
+* @codec_wake_override: Force the audio codec to stay awake
+*/
+ void (*codec_wake_override)(struct device *, bool enable);
+ /**
+* @get_cdclk_freq: Query the i915 driver about the current cdclk
frequency
+*/
+ int (*get_cdclk_freq)(struct device *);
+ /**
+* @sync_audio_rate: set n/cts based on the sample rate
+*
+* Called from audio driver. After audio driver sets the
+* sample rate, it will call this function to set n/cts
+*/
+ int (*sync_audio_rate)(struct device *, int port, int rate);
+};
+
+/**
+ * struct i915_audio_component_audio_ops - Ops implemented by hda driver,
called by i915 driver
+ */
+struct i915_audio_component_audio_ops {
+ /**
+* @audio_ptr: Pointer to be used in call to pin_eld_notify
+*/
+ void *audio_ptr;
+ /**
+* @pin_eld_notify: Notify the HDA driver that pin sense and/or ELD
information has changed
+*
+* Called when the i915 driver has set up audio pipeline or has just
+* begun to tear it down. This allows the HDA driver to update its
+* status accordingly (even when the HDA controller is in power save
+* mode).
+*/
+ void (*pin_eld_notify)(void *audio_ptr, int port);
+};
+
+/**
+ * struct i915_audio_component - Used for direct communication between i915
and hda drivers
+ */
struct i915_audio_component {
+ /**
+* @dev: i915 device, used as parameter for ops
+*/
struct device *dev;
/**
* @aud_sample_rate: the array of audio sample rate per port
*/
int aud_sample_rate[MAX_PORTS];
-
- const struct i915_audio_component_ops {
- struct module *owner;
- void (*get_power)(struct device *);
- void (*put_power)(struct device *);
- void (*codec_wake_override)(struct device *, bool enable);
- int (*get_cdclk_freq)(struct device *);
- /**
-* @sync_audio_rate: set n/cts based on the sample rate
-*
-* Called from audio driver. After audio driver sets the
-* sample rate, it will call this function to set n/cts
-*/
- int (*sync_audio_rate)(struct device *, int port, int rate);
- } *ops;
-
- const struct i915_audio_component_audio_ops {
- void *audio_ptr;
- /**
-* Call from i915 driver, notifying the HDA driver that
-* pin sense and/or ELD information has changed.
-* @audio_ptr: HDA driver object
-* @port: Which port has changed (PORTA / PORTB /
PORTC etc)
-*/
- void (*pin_eld_notify)(void *audio_ptr, int port);
- } *audio_ops;
+ /**
+* @ops: Ops implemented by i915 driver, called by hda driver
+*/
+
[Intel-gfx] [PATCH v2] drm/i915: Improve kernel-doc for i915_audio_component struct
To make kernel-doc happy, the i915_audio_component_audio_ops struct
cannot be nested.
Signed-off-by: David Henningsson <david.hennings...@canonical.com>
---
Documentation/DocBook/drm.tmpl | 1 +
include/drm/i915_component.h | 92 ++
2 files changed, 67 insertions(+), 26 deletions(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 9ddf8c6..f16e4e8 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -4051,6 +4051,7 @@ int num_ioctls;
High Definition Audio
!Pdrivers/gpu/drm/i915/intel_audio.c High Definition Audio over HDMI and
Display Port
!Idrivers/gpu/drm/i915/intel_audio.c
+!Iinclude/drm/i915_component.h
Panel Self Refresh PSR (PSR/SRD)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index 89dc7d6..76c10c8 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -30,38 +30,78 @@
*/
#define MAX_PORTS 5
+/**
+ * struct i915_audio_component_ops - Ops implemented by i915 driver, called by
hda driver
+ */
+struct i915_audio_component_ops {
+ /**
+* @owner: i915 module
+*/
+ struct module *owner;
+ /**
+* @get_power: Request that power well is to be turned on
+*/
+ void (*get_power)(struct device *);
+ /**
+* @put_power: Allow the power well to be turned off
+*/
+ void (*put_power)(struct device *);
+ /**
+* @codec_wake_override: Force the audio codec to stay awake
+*/
+ void (*codec_wake_override)(struct device *, bool enable);
+ /**
+* @get_cdclk_freq: Query the i915 driver about the current cdclk
frequency
+*/
+ int (*get_cdclk_freq)(struct device *);
+ /**
+* @sync_audio_rate: set n/cts based on the sample rate
+*
+* Called from audio driver. After audio driver sets the
+* sample rate, it will call this function to set n/cts
+*/
+ int (*sync_audio_rate)(struct device *, int port, int rate);
+};
+
+/**
+ * struct i915_audio_component_audio_ops - Ops implemented by hda driver,
called by i915 driver
+ */
+struct i915_audio_component_audio_ops {
+ /**
+* @audio_ptr: Pointer to be used in call to pin_eld_notify
+*/
+ void *audio_ptr;
+ /**
+* @pin_eld_notify: Notify the HDA driver that pin sense and/or ELD
information has changed
+*
+* Called when the i915 driver has set up audio pipeline or has just
+* begun to tear it down. This allows the HDA driver to update its
+* status accordingly (even when the HDA controller is in power save
+* mode).
+*/
+ void (*pin_eld_notify)(void *audio_ptr, int port);
+};
+
+/**
+ * struct i915_audio_component - Used for direct communication between i915
and hda drivers
+ */
struct i915_audio_component {
+ /**
+* @dev: i915 device, used as parameter for ops
+*/
struct device *dev;
/**
* @aud_sample_rate: the array of audio sample rate per port
*/
int aud_sample_rate[MAX_PORTS];
-
- const struct i915_audio_component_ops {
- struct module *owner;
- void (*get_power)(struct device *);
- void (*put_power)(struct device *);
- void (*codec_wake_override)(struct device *, bool enable);
- int (*get_cdclk_freq)(struct device *);
- /**
-* @sync_audio_rate: set n/cts based on the sample rate
-*
-* Called from audio driver. After audio driver sets the
-* sample rate, it will call this function to set n/cts
-*/
- int (*sync_audio_rate)(struct device *, int port, int rate);
- } *ops;
-
- const struct i915_audio_component_audio_ops {
- void *audio_ptr;
- /**
-* Call from i915 driver, notifying the HDA driver that
-* pin sense and/or ELD information has changed.
-* @audio_ptr: HDA driver object
-* @port: Which port has changed (PORTA / PORTB /
PORTC etc)
-*/
- void (*pin_eld_notify)(void *audio_ptr, int port);
- } *audio_ops;
+ /**
+* @ops: Ops implemented by i915 driver, called by hda driver
+*/
+ const struct i915_audio_component_ops *ops;
+ /**
+* @audio_ops: Ops implemented by hda driver, called by i915 driver
+*/
+ const struct i915_audio_component_audio_ops *audio_ops;
};
#endif /* _I915_COMPONENT_H_ */
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Improve kernel-doc for i915_audio_component struct
On 2015-09-04 10:03, Daniel Vetter wrote: Also please use the new inline style for struct members. I tried that, but I couldn't get it to work. This was with Takashi's for-next tree, do I need to apply some docbook special patches on top of that to get the new functionality? -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4 v5] i915 to call hda driver on HDMI plug/unplug
On 2015-08-28 19:02, David Henningsson wrote: Hopefully last version? :-) * Added commit text about duplicate events (patch 4/4) * Added locks in bind/unbind on i915 side (patch 2/4) * Fixed docbook comments in i915 struct (patch 1/4) * Removed port_mst_streaming parameter (patch 1/4) * Added Reviewed-by - 1 & 2 by Jani, 3 & 4 by Takashi. Hopefully this was correct, otherwise feel free to adjust yourself before committing. Hi Takashi, Thanks for finally merging it. Just want to make you aware that it seems like you merged v4 instead of v5, so the changes mentioned above did not make it in. Anything either you or I should do about that? -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4 v5] i915 to call hda driver on HDMI plug/unplug
On 2015-09-03 10:31, Takashi Iwai wrote: On Thu, 03 Sep 2015 09:52:00 +0200, David Henningsson wrote: On 2015-08-28 19:02, David Henningsson wrote: Hopefully last version? :-) * Added commit text about duplicate events (patch 4/4) * Added locks in bind/unbind on i915 side (patch 2/4) * Fixed docbook comments in i915 struct (patch 1/4) * Removed port_mst_streaming parameter (patch 1/4) * Added Reviewed-by - 1 & 2 by Jani, 3 & 4 by Takashi. Hopefully this was correct, otherwise feel free to adjust yourself before committing. Hi Takashi, Thanks for finally merging it. Just want to make you aware that it seems like you merged v4 instead of v5, so the changes mentioned above did not make it in. Anything either you or I should do about that? Argh, that's bad. Could you post incremental patches to correct to v5? Posted now. To avoid spamming it was to you and the alsa-devel ML only. http://mailman.alsa-project.org/pipermail/alsa-devel/2015-September/097216.html At the next time please put revision number in the patch itself, not only in cover letter. You can do it with --subject-prefix option of git-format-patch. Ok, will try to remember :-) -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Improve kernel-doc for i915_audio_component struct
To make kernel-doc happy, the i915_audio_component_audio_ops struct
cannot be nested.
Signed-off-by: David Henningsson <david.hennings...@canonical.com>
---
Note that I didn't do the same un-nesting for i915_audio_component_ops.
This is to make it easier to merge the pending sync_audio_rate patch set.
It applies on top of my just sent patches so should probably be taken
through Takashi's tree.
Documentation/DocBook/drm.tmpl | 1 +
include/drm/i915_component.h | 33 +++--
2 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 2fb9a54..7554679 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -4048,6 +4048,7 @@ int num_ioctls;
High Definition Audio
!Pdrivers/gpu/drm/i915/intel_audio.c High Definition Audio over HDMI and
Display Port
!Idrivers/gpu/drm/i915/intel_audio.c
+!Iinclude/drm/i915_component.h
Panel Self Refresh PSR (PSR/SRD)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index b2d56dd..89d6362 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -24,6 +24,28 @@
#ifndef _I915_COMPONENT_H_
#define _I915_COMPONENT_H_
+/**
+ * struct i915_audio_component_audio_ops - i915 directly calling hda driver
+ *
+ * @audio_ptr: Pointer to pass when calling pin_eld_notify.
+ * @pin_eld_notify: Called from i915 driver, notifying the HDA driver that
+ * pin sense and/or ELD information has changed.
+ *
+ * These functions are implemented by hda driver and called by the i915
+ * driver.
+ */
+struct i915_audio_component_audio_ops {
+ void *audio_ptr;
+ void (*pin_eld_notify)(void *audio_ptr, int port);
+};
+
+/**
+ * struct i915_audio_component - Used for direct communication between i915
and hda drivers
+ *
+ * @dev: i915 device, used as parameter for ops
+ * @ops: Ops implemented by i915 driver, called by hda driver
+ * @audio_ops: Ops implemented by hda driver, called by i915 driver
+ */
struct i915_audio_component {
struct device *dev;
@@ -35,16 +57,7 @@ struct i915_audio_component {
int (*get_cdclk_freq)(struct device *);
} *ops;
- const struct i915_audio_component_audio_ops {
- void *audio_ptr;
- /**
-* Call from i915 driver, notifying the HDA driver that
-* pin sense and/or ELD information has changed.
-* @audio_ptr: HDA driver object
-* @port: Which port has changed (PORTA / PORTB /
PORTC etc)
-*/
- void (*pin_eld_notify)(void *audio_ptr, int port);
- } *audio_ops;
+ const struct i915_audio_component_audio_ops *audio_ops;
};
#endif /* _I915_COMPONENT_H_ */
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm/i915: Call audio pin/ELD notify function
On 2015-08-28 15:07, Jani Nikula wrote:
On Wed, 19 Aug 2015, David Henningsson david.hennings...@canonical.com wrote:
When the audio codec is enabled or disabled, notify the audio driver.
This will enable the audio driver to get the notification at all times
(even when audio is in different powersave states).
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
drivers/gpu/drm/i915/i915_drv.h| 1 +
drivers/gpu/drm/i915/intel_audio.c | 23 ---
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fd1de45..1fc327d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1809,6 +1809,7 @@ struct drm_i915_private {
struct drm_property *force_audio_property;
/* hda/i915 audio component */
+ struct i915_audio_component *audio_component;
bool audio_component_registered;
uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 3da9b84..969835d 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -399,6 +399,9 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
struct drm_connector *connector;
struct drm_device *dev = encoder-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_audio_component *acomp = dev_priv-audio_component;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+ enum port port = intel_dig_port-port;
connector = drm_select_eld(encoder, mode);
if (!connector)
@@ -419,6 +422,9 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
if (dev_priv-display.audio_codec_enable)
dev_priv-display.audio_codec_enable(connector, intel_encoder,
mode);
+
+ if (acomp acomp-audio_ops acomp-audio_ops-pin_eld_notify)
+ acomp-audio_ops-pin_eld_notify(acomp-audio_ops-audio_ptr,
(int) port, 0);
}
/**
@@ -428,13 +434,20 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
* The disable sequences must be performed before disabling the transcoder or
* port.
*/
-void intel_audio_codec_disable(struct intel_encoder *encoder)
+void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
{
- struct drm_device *dev = encoder-base.dev;
+ struct drm_encoder *encoder = intel_encoder-base;
+ struct drm_device *dev = encoder-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_audio_component *acomp = dev_priv-audio_component;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+ enum port port = intel_dig_port-port;
if (dev_priv-display.audio_codec_disable)
- dev_priv-display.audio_codec_disable(encoder);
+ dev_priv-display.audio_codec_disable(intel_encoder);
+
+ if (acomp acomp-audio_ops acomp-audio_ops-pin_eld_notify)
+ acomp-audio_ops-pin_eld_notify(acomp-audio_ops-audio_ptr,
(int) port, 0);
AFAICT this has a race with i915_audio_component_unbind, i.e. modeset
while hda is being unloaded might crash.
What do others think, is drm_modeset_lock_all in combonent bind/unbind
overkill?
Thanks for the review(s). I'm happy to add a
drm_modeset_lock_all/drm_modeset_unlock_all - I'm not that familiar with
the graphics side, really, so, is there another option...?
With that resolved one way or another (I'm not dismissing we don't need
to care) this is
Reviewed-by: Jani Nikula jani.nik...@intel.com
}
/**
@@ -525,12 +538,14 @@ static int i915_audio_component_bind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
if (WARN_ON(acomp-ops || acomp-dev))
return -EEXIST;
acomp-ops = i915_audio_component_ops;
acomp-dev = i915_dev;
+ dev_priv-audio_component = acomp;
return 0;
}
@@ -539,9 +554,11 @@ static void i915_audio_component_unbind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
acomp-ops = NULL;
acomp-dev = NULL;
+ dev_priv-audio_component = NULL;
}
static const struct component_ops i915_audio_component_bind_ops = {
--
1.9.1
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] ALSA: hda - allow codecs to access the i915 pin/ELD callback
This lets the interested codec be notified when an i915 pin/ELD
event happens.
Reviewed-by: Takashi Iwai ti...@suse.de
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
include/sound/hda_i915.h | 7 +++
sound/hda/hdac_i915.c| 10 ++
2 files changed, 17 insertions(+)
diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h
index adb5ba5..a0a1d67 100644
--- a/include/sound/hda_i915.h
+++ b/include/sound/hda_i915.h
@@ -4,12 +4,15 @@
#ifndef __SOUND_HDA_I915_H
#define __SOUND_HDA_I915_H
+#include drm/i915_component.h
+
#ifdef CONFIG_SND_HDA_I915
int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
int snd_hdac_display_power(struct hdac_bus *bus, bool enable);
int snd_hdac_get_display_clk(struct hdac_bus *bus);
int snd_hdac_i915_init(struct hdac_bus *bus);
int snd_hdac_i915_exit(struct hdac_bus *bus);
+int snd_hdac_i915_register_notifier(const struct
i915_audio_component_audio_ops *);
#else
static int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable)
{
@@ -31,6 +34,10 @@ static inline int snd_hdac_i915_exit(struct hdac_bus *bus)
{
return 0;
}
+static inline int snd_hdac_i915_register_notifier(const struct
i915_audio_component_audio_ops *)
+{
+ return -ENODEV;
+}
#endif
#endif /* __SOUND_HDA_I915_H */
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 5676b84..55c3df4 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -134,6 +134,16 @@ static int hdac_component_master_match(struct device *dev,
void *data)
return !strcmp(dev-driver-name, i915);
}
+int snd_hdac_i915_register_notifier(const struct
i915_audio_component_audio_ops *aops)
+{
+ if (WARN_ON(!hdac_acomp))
+ return -ENODEV;
+
+ hdac_acomp-audio_ops = aops;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_i915_register_notifier);
+
int snd_hdac_i915_init(struct hdac_bus *bus)
{
struct component_match *match = NULL;
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915: Call audio pin/ELD notify function
When the audio codec is enabled or disabled, notify the audio driver.
This will enable the audio driver to get the notification at all times
(even when audio is in different powersave states).
Reviewed-by: Jani Nikula jani.nik...@intel.com
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
drivers/gpu/drm/i915/i915_drv.h| 1 +
drivers/gpu/drm/i915/intel_audio.c | 27 ---
2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fd1de45..1fc327d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1809,6 +1809,7 @@ struct drm_i915_private {
struct drm_property *force_audio_property;
/* hda/i915 audio component */
+ struct i915_audio_component *audio_component;
bool audio_component_registered;
uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 3da9b84..678a34f 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -399,6 +399,9 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
struct drm_connector *connector;
struct drm_device *dev = encoder-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_audio_component *acomp = dev_priv-audio_component;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+ enum port port = intel_dig_port-port;
connector = drm_select_eld(encoder, mode);
if (!connector)
@@ -419,6 +422,9 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
if (dev_priv-display.audio_codec_enable)
dev_priv-display.audio_codec_enable(connector, intel_encoder,
mode);
+
+ if (acomp acomp-audio_ops acomp-audio_ops-pin_eld_notify)
+ acomp-audio_ops-pin_eld_notify(acomp-audio_ops-audio_ptr,
(int) port);
}
/**
@@ -428,13 +434,20 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
* The disable sequences must be performed before disabling the transcoder or
* port.
*/
-void intel_audio_codec_disable(struct intel_encoder *encoder)
+void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
{
- struct drm_device *dev = encoder-base.dev;
+ struct drm_encoder *encoder = intel_encoder-base;
+ struct drm_device *dev = encoder-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_audio_component *acomp = dev_priv-audio_component;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+ enum port port = intel_dig_port-port;
if (dev_priv-display.audio_codec_disable)
- dev_priv-display.audio_codec_disable(encoder);
+ dev_priv-display.audio_codec_disable(intel_encoder);
+
+ if (acomp acomp-audio_ops acomp-audio_ops-pin_eld_notify)
+ acomp-audio_ops-pin_eld_notify(acomp-audio_ops-audio_ptr,
(int) port);
}
/**
@@ -525,12 +538,16 @@ static int i915_audio_component_bind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
if (WARN_ON(acomp-ops || acomp-dev))
return -EEXIST;
+ drm_modeset_lock_all(dev_priv-dev);
acomp-ops = i915_audio_component_ops;
acomp-dev = i915_dev;
+ dev_priv-audio_component = acomp;
+ drm_modeset_unlock_all(dev_priv-dev);
return 0;
}
@@ -539,9 +556,13 @@ static void i915_audio_component_unbind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
+ drm_modeset_lock_all(dev_priv-dev);
acomp-ops = NULL;
acomp-dev = NULL;
+ dev_priv-audio_component = NULL;
+ drm_modeset_unlock_all(dev_priv-dev);
}
static const struct component_ops i915_audio_component_bind_ops = {
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/4 v5] i915 to call hda driver on HDMI plug/unplug
Hopefully last version? :-) * Added commit text about duplicate events (patch 4/4) * Added locks in bind/unbind on i915 side (patch 2/4) * Fixed docbook comments in i915 struct (patch 1/4) * Removed port_mst_streaming parameter (patch 1/4) * Added Reviewed-by - 1 2 by Jani, 3 4 by Takashi. Hopefully this was correct, otherwise feel free to adjust yourself before committing. Both Jani and Takashi seem okay with this going into 4.3. Nobody has said which tree you prefer to take it through, so how about Takashi merging it? David Henningsson (4): drm/i915: Add audio pin sense / ELD callback drm/i915: Call audio pin/ELD notify function ALSA: hda - allow codecs to access the i915 pin/ELD callback ALSA: hda - Wake the codec up on pin/ELD notify events drivers/gpu/drm/i915/i915_drv.h| 1 + drivers/gpu/drm/i915/intel_audio.c | 27 --- include/drm/i915_component.h | 16 include/sound/hda_i915.h | 7 +++ sound/hda/hdac_i915.c | 10 ++ sound/pci/hda/patch_hdmi.c | 22 +- 6 files changed, 79 insertions(+), 4 deletions(-) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
Whenever there is an event from the i915 driver, wake the codec
and recheck plug/unplug + ELD status.
This fixes the issue with lost unsol events in power save mode,
the codec and controller can now sleep in D3 and still know when
the HDMI monitor has been connected.
Right now, this might mean we get two callbacks from the same event,
one from the unsol event and one from the i915 driver, but this is
not harmful and can be optimised away in a later patch.
Reviewed-by: Takashi Iwai ti...@suse.de
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
sound/pci/hda/patch_hdmi.c | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index a97db5f..acbfbe0 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -37,6 +37,8 @@
#include sound/jack.h
#include sound/asoundef.h
#include sound/tlv.h
+#include sound/hdaudio.h
+#include sound/hda_i915.h
#include hda_codec.h
#include hda_local.h
#include hda_jack.h
@@ -144,6 +146,9 @@ struct hdmi_spec {
*/
struct hda_multi_out multiout;
struct hda_pcm_stream pcm_playback;
+
+ /* i915/powerwell (Haswell+/Valleyview+) specific */
+ struct i915_audio_component_audio_ops i915_audio_ops;
};
@@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
struct hdmi_spec *spec = codec-spec;
int pin_idx;
+ if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+ snd_hdac_i915_register_notifier(NULL);
+
for (pin_idx = 0; pin_idx spec-num_pins; pin_idx++) {
struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
@@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec
*codec, hda_nid_t fg,
snd_hda_codec_set_power_to_all(codec, fg, power_state);
}
+static void intel_pin_eld_notify(void *audio_ptr, int port)
+{
+ struct hda_codec *codec = audio_ptr;
+ int pin_nid = port + 0x04;
+
+ check_presence_and_report(codec, pin_nid);
+}
+
static int patch_generic_hdmi(struct hda_codec *codec)
{
struct hdmi_spec *spec;
@@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec)
if (is_valleyview_plus(codec) || is_skylake(codec))
codec-core.link_power_control = 1;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+ if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
codec-depop_delay = 0;
+ spec-i915_audio_ops.audio_ptr = codec;
+ spec-i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
+ snd_hdac_i915_register_notifier(spec-i915_audio_ops);
+ }
if (hdmi_parse_codec(codec) 0) {
codec-spec = NULL;
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: Add audio pin sense / ELD callback
This callback will be called by the i915 driver to notify the hda
driver that its HDMI information needs to be refreshed, i e,
that audio output is now available (or unavailable) - usually as a
result of a monitor being plugged in (or unplugged).
Reviewed-by: Jani Nikula jani.nik...@intel.com
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
include/drm/i915_component.h | 16
1 file changed, 16 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..c0f4d45 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -34,6 +34,22 @@ struct i915_audio_component {
void (*codec_wake_override)(struct device *, bool enable);
int (*get_cdclk_freq)(struct device *);
} *ops;
+
+ const struct i915_audio_component_audio_ops {
+ /**
+* @audio_ptr:
+*
+* Pointer to pass when calling pin_eld_notify.
+*/
+ void *audio_ptr;
+ /**
+* @pin_eld_notify:
+*
+* Called from i915 driver, notifying the HDA driver that
+* pin sense and/or ELD information has changed.
+*/
+ void (*pin_eld_notify)(void *audio_ptr, int port);
+ } *audio_ops;
};
#endif /* _I915_COMPONENT_H_ */
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [alsa-devel] [PATCH 1/4] drm/i915: Add audio pin sense / ELD callback
On 2015-08-26 10:33, Daniel Vetter wrote:
On Wed, Aug 19, 2015 at 10:48:55AM +0200, David Henningsson wrote:
This callback will be called by the i915 driver to notify the hda
driver that its HDMI information needs to be refreshed, i e,
that audio output is now available (or unavailable) - usually as a
result of a monitor being plugged in (or unplugged).
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
include/drm/i915_component.h | 12
1 file changed, 12 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..ab5bde37 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -34,6 +34,18 @@ struct i915_audio_component {
void (*codec_wake_override)(struct device *, bool enable);
int (*get_cdclk_freq)(struct device *);
} *ops;
+
+ const struct i915_audio_component_audio_ops {
+ void *audio_ptr;
+ /**
+* Call from i915 driver, notifying the HDA driver that
+* pin sense and/or ELD information has changed.
+* @audio_ptr: HDA driver object
+* @port: Which port has changed (PORTA / PORTB /
PORTC etc)
+* @port_mst_index: Index within that port, for DisplayPort
multistreaming
+*/
+ void (*pin_eld_notify)(void *audio_ptr, int port, int
port_mst_index);
+ } *audio_ops;
This won't work as proper kerneldoc, but you get away with it since it's
not pulled into the drm.tmpl. See my comments for the new set_audio_rate
callback.
Sorry, my google failed me, so I can't find your comments for the
set_audio_rate callback.
Apart from the kerneldoc issue, are you okay with acking the patch, at
least the first two i915 ones, and agree with Takashi which tree this
should go through?
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On 2015-08-20 11:28, Takashi Iwai wrote:
On Wed, 19 Aug 2015 10:48:58 +0200,
David Henningsson wrote:
Whenever there is an event from the i915 driver, wake the codec
and recheck plug/unplug + ELD status.
This fixes the issue with lost unsol events in power save mode,
the codec and controller can now sleep in D3 and still know when
the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.hennings...@canonical.com
This addition looks fine, but then we'll get double notification for
the normal hotplug/unplug, one via component ops and another via unsol
event?
Right, in case the unsol event actually works...
I would argue that the normal case would be that the controller and
codec is in D3 which means that the unsol event never gets through - due
to hw limitations - which is what triggered this patch set in the first
place.
But yes, in some case we might get double notification, but this should
not cause any trouble in practice. The unsol event could be turned off,
but would it be okay to save that for a later patch set (so I don't miss
the upcoming merge window)?
thanks,
Takashi
---
sound/pci/hda/patch_hdmi.c | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index a97db5f..932292c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -37,6 +37,8 @@
#include sound/jack.h
#include sound/asoundef.h
#include sound/tlv.h
+#include sound/hdaudio.h
+#include sound/hda_i915.h
#include hda_codec.h
#include hda_local.h
#include hda_jack.h
@@ -144,6 +146,9 @@ struct hdmi_spec {
*/
struct hda_multi_out multiout;
struct hda_pcm_stream pcm_playback;
+
+ /* i915/powerwell (Haswell+/Valleyview+) specific */
+ struct i915_audio_component_audio_ops i915_audio_ops;
};
@@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
struct hdmi_spec *spec = codec-spec;
int pin_idx;
+ if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+ snd_hdac_i915_register_notifier(NULL);
+
for (pin_idx = 0; pin_idx spec-num_pins; pin_idx++) {
struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
@@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec
*codec, hda_nid_t fg,
snd_hda_codec_set_power_to_all(codec, fg, power_state);
}
+static void intel_pin_eld_notify(void *audio_ptr, int port, int port_mst_index)
+{
+ struct hda_codec *codec = audio_ptr;
+ int pin_nid = port + 0x04;
+
+ check_presence_and_report(codec, pin_nid);
+}
+
static int patch_generic_hdmi(struct hda_codec *codec)
{
struct hdmi_spec *spec;
@@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec)
if (is_valleyview_plus(codec) || is_skylake(codec))
codec-core.link_power_control = 1;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+ if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
codec-depop_delay = 0;
+ spec-i915_audio_ops.audio_ptr = codec;
+ spec-i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
+ snd_hdac_i915_register_notifier(spec-i915_audio_ops);
+ }
if (hdmi_parse_codec(codec) 0) {
codec-spec = NULL;
--
1.9.1
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
Whenever there is an event from the i915 driver, wake the codec
and recheck plug/unplug + ELD status.
This fixes the issue with lost unsol events in power save mode,
the codec and controller can now sleep in D3 and still know when
the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
sound/pci/hda/patch_hdmi.c | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index a97db5f..932292c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -37,6 +37,8 @@
#include sound/jack.h
#include sound/asoundef.h
#include sound/tlv.h
+#include sound/hdaudio.h
+#include sound/hda_i915.h
#include hda_codec.h
#include hda_local.h
#include hda_jack.h
@@ -144,6 +146,9 @@ struct hdmi_spec {
*/
struct hda_multi_out multiout;
struct hda_pcm_stream pcm_playback;
+
+ /* i915/powerwell (Haswell+/Valleyview+) specific */
+ struct i915_audio_component_audio_ops i915_audio_ops;
};
@@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
struct hdmi_spec *spec = codec-spec;
int pin_idx;
+ if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+ snd_hdac_i915_register_notifier(NULL);
+
for (pin_idx = 0; pin_idx spec-num_pins; pin_idx++) {
struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
@@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec
*codec, hda_nid_t fg,
snd_hda_codec_set_power_to_all(codec, fg, power_state);
}
+static void intel_pin_eld_notify(void *audio_ptr, int port, int port_mst_index)
+{
+ struct hda_codec *codec = audio_ptr;
+ int pin_nid = port + 0x04;
+
+ check_presence_and_report(codec, pin_nid);
+}
+
static int patch_generic_hdmi(struct hda_codec *codec)
{
struct hdmi_spec *spec;
@@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec)
if (is_valleyview_plus(codec) || is_skylake(codec))
codec-core.link_power_control = 1;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+ if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
codec-depop_delay = 0;
+ spec-i915_audio_ops.audio_ptr = codec;
+ spec-i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
+ snd_hdac_i915_register_notifier(spec-i915_audio_ops);
+ }
if (hdmi_parse_codec(codec) 0) {
codec-spec = NULL;
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] ALSA: hda - allow codecs to access the i915 pin/ELD callback
This lets the interested codec be notified when an i915 pin/ELD
event happens.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
include/sound/hda_i915.h | 7 +++
sound/hda/hdac_i915.c| 10 ++
2 files changed, 17 insertions(+)
diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h
index adb5ba5..a0a1d67 100644
--- a/include/sound/hda_i915.h
+++ b/include/sound/hda_i915.h
@@ -4,12 +4,15 @@
#ifndef __SOUND_HDA_I915_H
#define __SOUND_HDA_I915_H
+#include drm/i915_component.h
+
#ifdef CONFIG_SND_HDA_I915
int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
int snd_hdac_display_power(struct hdac_bus *bus, bool enable);
int snd_hdac_get_display_clk(struct hdac_bus *bus);
int snd_hdac_i915_init(struct hdac_bus *bus);
int snd_hdac_i915_exit(struct hdac_bus *bus);
+int snd_hdac_i915_register_notifier(const struct
i915_audio_component_audio_ops *);
#else
static int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable)
{
@@ -31,6 +34,10 @@ static inline int snd_hdac_i915_exit(struct hdac_bus *bus)
{
return 0;
}
+static inline int snd_hdac_i915_register_notifier(const struct
i915_audio_component_audio_ops *)
+{
+ return -ENODEV;
+}
#endif
#endif /* __SOUND_HDA_I915_H */
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 5676b84..55c3df4 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -134,6 +134,16 @@ static int hdac_component_master_match(struct device *dev,
void *data)
return !strcmp(dev-driver-name, i915);
}
+int snd_hdac_i915_register_notifier(const struct
i915_audio_component_audio_ops *aops)
+{
+ if (WARN_ON(!hdac_acomp))
+ return -ENODEV;
+
+ hdac_acomp-audio_ops = aops;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_i915_register_notifier);
+
int snd_hdac_i915_init(struct hdac_bus *bus)
{
struct component_match *match = NULL;
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915: Call audio pin/ELD notify function
When the audio codec is enabled or disabled, notify the audio driver.
This will enable the audio driver to get the notification at all times
(even when audio is in different powersave states).
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
drivers/gpu/drm/i915/i915_drv.h| 1 +
drivers/gpu/drm/i915/intel_audio.c | 23 ---
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fd1de45..1fc327d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1809,6 +1809,7 @@ struct drm_i915_private {
struct drm_property *force_audio_property;
/* hda/i915 audio component */
+ struct i915_audio_component *audio_component;
bool audio_component_registered;
uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 3da9b84..969835d 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -399,6 +399,9 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
struct drm_connector *connector;
struct drm_device *dev = encoder-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_audio_component *acomp = dev_priv-audio_component;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+ enum port port = intel_dig_port-port;
connector = drm_select_eld(encoder, mode);
if (!connector)
@@ -419,6 +422,9 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
if (dev_priv-display.audio_codec_enable)
dev_priv-display.audio_codec_enable(connector, intel_encoder,
mode);
+
+ if (acomp acomp-audio_ops acomp-audio_ops-pin_eld_notify)
+ acomp-audio_ops-pin_eld_notify(acomp-audio_ops-audio_ptr,
(int) port, 0);
}
/**
@@ -428,13 +434,20 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
* The disable sequences must be performed before disabling the transcoder or
* port.
*/
-void intel_audio_codec_disable(struct intel_encoder *encoder)
+void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
{
- struct drm_device *dev = encoder-base.dev;
+ struct drm_encoder *encoder = intel_encoder-base;
+ struct drm_device *dev = encoder-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_audio_component *acomp = dev_priv-audio_component;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+ enum port port = intel_dig_port-port;
if (dev_priv-display.audio_codec_disable)
- dev_priv-display.audio_codec_disable(encoder);
+ dev_priv-display.audio_codec_disable(intel_encoder);
+
+ if (acomp acomp-audio_ops acomp-audio_ops-pin_eld_notify)
+ acomp-audio_ops-pin_eld_notify(acomp-audio_ops-audio_ptr,
(int) port, 0);
}
/**
@@ -525,12 +538,14 @@ static int i915_audio_component_bind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
if (WARN_ON(acomp-ops || acomp-dev))
return -EEXIST;
acomp-ops = i915_audio_component_ops;
acomp-dev = i915_dev;
+ dev_priv-audio_component = acomp;
return 0;
}
@@ -539,9 +554,11 @@ static void i915_audio_component_unbind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
acomp-ops = NULL;
acomp-dev = NULL;
+ dev_priv-audio_component = NULL;
}
static const struct component_ops i915_audio_component_bind_ops = {
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v4 0/4] i915 to call hda driver on HDMI plug/unplug
It's been a while since the last patch set iteration, due to me being on vacation. But here's a new set, and I still hope that it can make it into the next merge window. Changes since v3 (with the person suggesting that change within parantheses): * Valleyview now has three pins like all the others (Libin Yang) * Renamed a few references from hotplug to pin_eld to reduce confusion with hotplug code on the i915 side (Jani Nikula) * Rewrote the dispatching from hda core to codec on the hda side (Takashi Iwai) This iteration has been tested and working on one Skylake machine. David Henningsson (4): drm/i915: Add audio pin sense / ELD callback drm/i915: Call audio pin/ELD notify function ALSA: hda - allow codecs to access the i915 pin/ELD callback ALSA: hda - Wake the codec up on pin/ELD notify events drivers/gpu/drm/i915/i915_drv.h| 1 + drivers/gpu/drm/i915/intel_audio.c | 23 --- include/drm/i915_component.h | 12 include/sound/hda_i915.h | 7 +++ sound/hda/hdac_i915.c | 10 ++ sound/pci/hda/patch_hdmi.c | 22 +- 6 files changed, 71 insertions(+), 4 deletions(-) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: Add audio pin sense / ELD callback
This callback will be called by the i915 driver to notify the hda
driver that its HDMI information needs to be refreshed, i e,
that audio output is now available (or unavailable) - usually as a
result of a monitor being plugged in (or unplugged).
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
include/drm/i915_component.h | 12
1 file changed, 12 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..ab5bde37 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -34,6 +34,18 @@ struct i915_audio_component {
void (*codec_wake_override)(struct device *, bool enable);
int (*get_cdclk_freq)(struct device *);
} *ops;
+
+ const struct i915_audio_component_audio_ops {
+ void *audio_ptr;
+ /**
+* Call from i915 driver, notifying the HDA driver that
+* pin sense and/or ELD information has changed.
+* @audio_ptr: HDA driver object
+* @port: Which port has changed (PORTA / PORTB /
PORTC etc)
+* @port_mst_index: Index within that port, for DisplayPort
multistreaming
+*/
+ void (*pin_eld_notify)(void *audio_ptr, int port, int
port_mst_index);
+ } *audio_ops;
};
#endif /* _I915_COMPONENT_H_ */
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Add audio hotplug info struct
On 2015-07-23 08:17, David Henningsson wrote:
I'm about to go on vacation so it would be good to get some closure
here. If you both prefer this setup, how about I remove struct
i915_audio_hotplug_info for now? We will then have:
const struct i915_audio_component_audio_ops {
void (*hotplug_notify)(struct hdac_bus *);
}
Sorry, it would look like this:
const struct i915_audio_component_audio_ops {
void (*hotplug_notify)(struct hdac_bus *, int port, int
port_mst_index);
}
...to indicate what port needs updating.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Add audio hotplug info struct
On 2015-07-22 22:31, Takashi Iwai wrote:
On Wed, 22 Jul 2015 19:52:23 +0200,
David Henningsson wrote:
On 2015-07-22 16:13, Vinod Koul wrote:
On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
On Wed, 22 Jul 2015 10:50:03 +0200,
David Henningsson wrote:
struct i915_audio_component {
struct device *dev;
+ struct hdac_bus *hdac_bus;
If we want to be more generic, using a struct device would be better,
e.g.
struct device *audio_dev;
Does this work? If we want to have the hdac_bus.dev ptr instead of a
hdac_bus ptr, there does not seem to be an obvious way to go from the
audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an
arbitrary dev pointer).
Hrm, right, currently it's not straightforward. Scratch the idea,
then.
That depends on the device we register this with. Actually this makes more
sense to me :)
If we register with struct device *audio_dev, which in this case would be
the codec device we create while probing the bus. This way you are linking i915
ops to the codec device. Ofcourse hdac_device has bus pointer but you can
invoke device callback without even searching for the device :)
It would require some extra setup, so I skipped it (at least for now).
I e, in order to detect codecs, we need to call hdac_i915 functions to
turn the power well on. And in order to connect the codec to the
i915_audio_component, we need to have detected a codec.
Which, now that I think of it, actually gives an interesting potential
race condition, in case the following happens:
1) Monitor is plugged in at boot time
2) i915 initializes
3) hda starts initializing and sets up the audio component
4) i915 finishes initialization and as part of that, calls the hotplug
notify to let hda know that the monitor is plugged in. However, at this
point, hda has not finished initialization yet, so there are no codecs
that listen for this information.
Anyhow, this is not a problem really yet, but it might be if we ever
decide to not write the ELD to the hardware.
For avoid such a problem, maybe we need two ops, one for notification
and one for getting the assigned data. At the initialization time,
the audio driver queries the assigned status and data. When a hotplug
happens, it's just notified. That is, it simply replaces the current
unsol event and the ELD data read via two ops.
I'm about to go on vacation so it would be good to get some closure
here. If you both prefer this setup, how about I remove struct
i915_audio_hotplug_info for now? We will then have:
const struct i915_audio_component_audio_ops {
void (*hotplug_notify)(struct hdac_bus *);
}
...which will make the hda driver read from the hardware. Asking the
i915 driver for ELD, connector and hotplug status can then be done in a
later patch.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915: Call audio hotplug notify function
On HDMI hotplug events, notify the audio driver. This will enable
the audio driver to get the notification at all times (even when
audio is in different powersave states).
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
drivers/gpu/drm/i915/i915_drv.h|1 +
drivers/gpu/drm/i915/intel_audio.c | 23 ---
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 542fac6..696624c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1808,6 +1808,7 @@ struct drm_i915_private {
struct drm_property *force_audio_property;
/* hda/i915 audio component */
+ struct i915_audio_component *audio_component;
bool audio_component_registered;
uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 3da9b84..3cc8849 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -399,6 +399,9 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
struct drm_connector *connector;
struct drm_device *dev = encoder-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_audio_component *acomp = dev_priv-audio_component;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+ enum port port = intel_dig_port-port;
connector = drm_select_eld(encoder, mode);
if (!connector)
@@ -419,6 +422,9 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
if (dev_priv-display.audio_codec_enable)
dev_priv-display.audio_codec_enable(connector, intel_encoder,
mode);
+
+ if (acomp acomp-audio_ops acomp-audio_ops-hotplug_notify)
+ acomp-audio_ops-hotplug_notify(acomp-audio_ptr, (int) port,
0);
}
/**
@@ -428,13 +434,20 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
* The disable sequences must be performed before disabling the transcoder or
* port.
*/
-void intel_audio_codec_disable(struct intel_encoder *encoder)
+void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
{
- struct drm_device *dev = encoder-base.dev;
+ struct drm_encoder *encoder = intel_encoder-base;
+ struct drm_device *dev = encoder-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_audio_component *acomp = dev_priv-audio_component;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+ enum port port = intel_dig_port-port;
if (dev_priv-display.audio_codec_disable)
- dev_priv-display.audio_codec_disable(encoder);
+ dev_priv-display.audio_codec_disable(intel_encoder);
+
+ if (acomp acomp-audio_ops acomp-audio_ops-hotplug_notify)
+ acomp-audio_ops-hotplug_notify(acomp-audio_ptr, (int) port,
0);
}
/**
@@ -525,12 +538,14 @@ static int i915_audio_component_bind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
if (WARN_ON(acomp-ops || acomp-dev))
return -EEXIST;
acomp-ops = i915_audio_component_ops;
acomp-dev = i915_dev;
+ dev_priv-audio_component = acomp;
return 0;
}
@@ -539,9 +554,11 @@ static void i915_audio_component_unbind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
acomp-ops = NULL;
acomp-dev = NULL;
+ dev_priv-audio_component = NULL;
}
static const struct component_ops i915_audio_component_bind_ops = {
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] ALSA: hda - Dispatch incoming HDMI hotplug i915 callback
This lets interested codec(s) be notified of HDMI hotplug
events sent from the i915 driver.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
include/sound/hdaudio.h |4
sound/hda/hdac_i915.c | 23 +++
2 files changed, 27 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 4caf1fd..8142d03 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -79,6 +79,10 @@ struct hdac_device {
int (*exec_verb)(struct hdac_device *dev, unsigned int cmd,
unsigned int flags, unsigned int *res);
+ /* Used for hotplug notification from i915 driver */
+ void (*i915_hotplug_notify)(struct hdac_device *, int port,
+ int port_mst_index);
+
/* widgets */
unsigned int num_nodes;
hda_nid_t start_nid, end_nid;
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 5676b84..17295c2 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -118,6 +118,8 @@ static void hdac_component_master_unbind(struct device *dev)
{
struct i915_audio_component *acomp = hdac_acomp;
+ acomp-audio_ops = NULL;
+ acomp-audio_ptr = NULL;
module_put(acomp-ops-owner);
component_unbind_all(dev, acomp);
WARN_ON(acomp-ops || acomp-dev);
@@ -128,6 +130,24 @@ static const struct component_master_ops
hdac_component_master_ops = {
.unbind = hdac_component_master_unbind,
};
+static void i915_audio_component_hotplug_notify(void *audio_ptr,
+ int port, int port_mst_index)
+{
+ struct hdac_device *d;
+ struct hdac_bus *bus = audio_ptr;
+
+ dev_dbg(bus-dev, i915 hotplug event (port = %d:%d),
+ port, port_mst_index);
+
+ list_for_each_entry(d, bus-codec_list, list)
+ if (d-i915_hotplug_notify)
+ d-i915_hotplug_notify(d, port, port_mst_index);
+}
+
+static const struct i915_audio_component_audio_ops
i915_audio_component_audio_ops = {
+ .hotplug_notify = i915_audio_component_hotplug_notify,
+};
+
static int hdac_component_master_match(struct device *dev, void *data)
{
/* i915 is the only supported component */
@@ -163,6 +183,9 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
ret = -ENODEV;
goto out_master_del;
}
+ acomp-audio_ops = i915_audio_component_audio_ops;
+ acomp-audio_ptr = bus;
+
dev_dbg(dev, bound to i915 component master\n);
return 0;
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on hotplug notify events
Whenever there is an event from the i915 driver, wake the codec
and recheck plug/unplug + ELD status.
This fixes the issue with lost unsol events in power save mode,
the codec and controller can now sleep in D3 and still know when
the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
sound/pci/hda/patch_hdmi.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 2f24338..6cc1524 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2316,6 +2316,15 @@ static void haswell_set_power_state(struct hda_codec
*codec, hda_nid_t fg,
snd_hda_codec_set_power_to_all(codec, fg, power_state);
}
+static void intel_hotplug_notify(struct hdac_device *dev, int port,
+int port_mst_index)
+{
+ struct hda_codec *codec = container_of(dev, struct hda_codec, core);
+ int pin_nid = is_valleyview(codec) ? 0x03 : port + 0x04;
+
+ check_presence_and_report(codec, pin_nid);
+}
+
static int patch_generic_hdmi(struct hda_codec *codec)
{
struct hdmi_spec *spec;
@@ -2342,8 +2351,10 @@ static int patch_generic_hdmi(struct hda_codec *codec)
if (is_valleyview_plus(codec) || is_skylake(codec))
codec-core.link_power_control = 1;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+ if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
codec-depop_delay = 0;
+ codec-core.i915_hotplug_notify = intel_hotplug_notify;
+ }
if (hdmi_parse_codec(codec) 0) {
codec-spec = NULL;
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 0/4] i915 to call hda driver on HDMI plug/unplug
Changes since v2 is that the patch set has diminished to not transfer connector/eld information, since it might be better that such information to be transferred by a separate call in the ordinary direction. That could be added in a later patch set. The audio_ptr is now a void* (instead of a hdac_bus*) so that the audio side can easily refactor the ptr to point to the codec instead of the bus if we would find that beneficial. David Henningsson (4): drm/i915: Add audio hotplug info callback drm/i915: Call audio hotplug notify function ALSA: hda - Dispatch incoming HDMI hotplug i915 callback ALSA: hda - Wake the codec up on hotplug notify events drivers/gpu/drm/i915/i915_drv.h|1 + drivers/gpu/drm/i915/intel_audio.c | 23 --- include/drm/i915_component.h |5 + include/sound/hdaudio.h|4 sound/hda/hdac_i915.c | 23 +++ sound/pci/hda/patch_hdmi.c | 13 - 6 files changed, 65 insertions(+), 4 deletions(-) -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: Add audio hotplug info callback
This callback will be called by the i915 driver to notify the hda
driver that HDMI has been hotplugged.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
include/drm/i915_component.h |5 +
1 file changed, 5 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..d053008 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -26,6 +26,7 @@
struct i915_audio_component {
struct device *dev;
+ void *audio_ptr;
const struct i915_audio_component_ops {
struct module *owner;
@@ -34,6 +35,10 @@ struct i915_audio_component {
void (*codec_wake_override)(struct device *, bool enable);
int (*get_cdclk_freq)(struct device *);
} *ops;
+
+ const struct i915_audio_component_audio_ops {
+ void (*hotplug_notify)(void *audio_ptr, int port, int
port_mst_index);
+ } *audio_ops;
};
#endif /* _I915_COMPONENT_H_ */
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 0/4] i915 to call hda driver on HDMI plug/unplug
Changes since previous version: * Renames and refactorings according to Daniel's and Takashi's suggestions. * Debug message slightly improved. * Braswell now has all three pins too, Baytrail still has only one pin. It would still be good with some confirmation from Intel that this is correct. David Henningsson (4): drm/i915: Add audio hotplug info struct drm/i915: Call audio hotplug notify function ALSA: hda - Dispatch incoming HDMI hotplug i915 callback ALSA: hda - Wake the codec up on hotplug notify events drivers/gpu/drm/i915/i915_drv.h|1 + drivers/gpu/drm/i915/intel_audio.c | 41 include/drm/i915_component.h | 18 include/sound/hdaudio.h|4 sound/hda/hdac_i915.c | 26 +++ sound/pci/hda/patch_hdmi.c | 13 +++- 6 files changed, 102 insertions(+), 1 deletion(-) -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915: Call audio hotplug notify function
On HDMI hotplug events, notify the audio driver. This will enable
the audio driver to get the information at all times (even when
audio is in different powersave states), and also without reading
it from the hardware.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
drivers/gpu/drm/i915/i915_drv.h|1 +
drivers/gpu/drm/i915/intel_audio.c | 41
2 files changed, 42 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 542fac6..696624c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1808,6 +1808,7 @@ struct drm_i915_private {
struct drm_property *force_audio_property;
/* hda/i915 audio component */
+ struct i915_audio_component *audio_component;
bool audio_component_registered;
uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 3da9b84..2700521 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -384,6 +384,39 @@ static void ilk_audio_codec_enable(struct drm_connector
*connector,
I915_WRITE(aud_config, tmp);
}
+static void audio_hotplug_notify(struct intel_encoder *intel_encoder,
+struct drm_connector *connector)
+{
+ struct i915_audio_hotplug_info audio_info;
+ struct drm_encoder *encoder = intel_encoder-base;
+ struct drm_device *dev = encoder-dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_audio_component *acomp = dev_priv-audio_component;
+ struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+ enum port port = intel_dig_port-port;
+
+ if (!acomp || !acomp-audio_ops || !acomp-audio_ops-hotplug_notify)
+ return;
+
+ memset((void *) audio_info, 0, sizeof(audio_info));
+
+ if (connector) {
+ audio_info.connector_type = connector-connector_type;
+ audio_info.connector_type_id = connector-connector_type_id;
+ audio_info.plugged_in = true;
+ audio_info.eld = connector-eld;
+ audio_info.eld_size = drm_eld_size(audio_info.eld);
+ } else {
+ audio_info.connector_type = -1;
+ audio_info.connector_type_id = -1;
+ }
+
+ audio_info.port = (int) port;
+ /* DP Mst is unsupported for now */
+
+ acomp-audio_ops-hotplug_notify(acomp-hdac_bus, audio_info);
+}
+
/**
* intel_audio_codec_enable - Enable the audio codec for HD audio
* @intel_encoder: encoder on which to enable audio
@@ -419,6 +452,8 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
if (dev_priv-display.audio_codec_enable)
dev_priv-display.audio_codec_enable(connector, intel_encoder,
mode);
+
+ audio_hotplug_notify(intel_encoder, connector);
}
/**
@@ -435,6 +470,8 @@ void intel_audio_codec_disable(struct intel_encoder
*encoder)
if (dev_priv-display.audio_codec_disable)
dev_priv-display.audio_codec_disable(encoder);
+
+ audio_hotplug_notify(encoder, NULL);
}
/**
@@ -525,12 +562,14 @@ static int i915_audio_component_bind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
if (WARN_ON(acomp-ops || acomp-dev))
return -EEXIST;
acomp-ops = i915_audio_component_ops;
acomp-dev = i915_dev;
+ dev_priv-audio_component = acomp;
return 0;
}
@@ -539,9 +578,11 @@ static void i915_audio_component_unbind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
acomp-ops = NULL;
acomp-dev = NULL;
+ dev_priv-audio_component = NULL;
}
static const struct component_ops i915_audio_component_bind_ops = {
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: Add audio hotplug info struct
This struct will be used to transfer information from the i915
driver to the hda driver on HDMI hotplug events.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
include/drm/i915_component.h | 18 ++
1 file changed, 18 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..d4c5648 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -24,8 +24,22 @@
#ifndef _I915_COMPONENT_H_
#define _I915_COMPONENT_H_
+struct hdac_bus;
+
+struct i915_audio_hotplug_info {
+ int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
+ int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type,
meant for userspace */
+ int port; /* Used for mapping to affected nid */
+ int port_multi_stream_device; /* For DP multi-streaming */
+
+ bool plugged_in;
+ const unsigned char *eld;
+ int eld_size;
+};
+
struct i915_audio_component {
struct device *dev;
+ struct hdac_bus *hdac_bus;
const struct i915_audio_component_ops {
struct module *owner;
@@ -34,6 +48,10 @@ struct i915_audio_component {
void (*codec_wake_override)(struct device *, bool enable);
int (*get_cdclk_freq)(struct device *);
} *ops;
+
+ const struct i915_audio_component_audio_ops {
+ void (*hotplug_notify)(struct hdac_bus *, const struct
i915_audio_hotplug_info *);
+ } *audio_ops;
};
#endif /* _I915_COMPONENT_H_ */
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] ALSA: hda - Dispatch incoming HDMI hotplug i915 callback
This lets interested codec(s) be notified of HDMI hotplug
events sent from the i915 driver.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
include/sound/hdaudio.h |4
sound/hda/hdac_i915.c | 26 ++
2 files changed, 30 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 4caf1fd..18a29ba 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -79,6 +79,10 @@ struct hdac_device {
int (*exec_verb)(struct hdac_device *dev, unsigned int cmd,
unsigned int flags, unsigned int *res);
+ /* Used for hotplug notification from i915 driver */
+ void (*i915_hotplug_notify)(struct hdac_device *,
+ const struct i915_audio_hotplug_info *);
+
/* widgets */
unsigned int num_nodes;
hda_nid_t start_nid, end_nid;
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 5676b84..7a54b76 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -118,6 +118,8 @@ static void hdac_component_master_unbind(struct device *dev)
{
struct i915_audio_component *acomp = hdac_acomp;
+ acomp-audio_ops = NULL;
+ acomp-hdac_bus = NULL;
module_put(acomp-ops-owner);
component_unbind_all(dev, acomp);
WARN_ON(acomp-ops || acomp-dev);
@@ -128,6 +130,27 @@ static const struct component_master_ops
hdac_component_master_ops = {
.unbind = hdac_component_master_unbind,
};
+static void i915_audio_component_hotplug_notify(struct hdac_bus *bus,
+ const struct i915_audio_hotplug_info *info)
+{
+ struct hdac_device *d;
+
+ if (info-plugged_in)
+ dev_dbg(bus-dev, i915 plugged in event (port = %d, connector
= %d,%d),
+ info-port, info-connector_type,
info-connector_type_id);
+ else
+ dev_dbg(bus-dev, i915 unplug event (port = %d), info-port);
+
+ list_for_each_entry(d, bus-codec_list, list) {
+ if (d-i915_hotplug_notify)
+ d-i915_hotplug_notify(d, info);
+ }
+}
+
+static const struct i915_audio_component_audio_ops
i915_audio_component_audio_ops = {
+ .hotplug_notify = i915_audio_component_hotplug_notify,
+};
+
static int hdac_component_master_match(struct device *dev, void *data)
{
/* i915 is the only supported component */
@@ -163,6 +186,9 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
ret = -ENODEV;
goto out_master_del;
}
+ acomp-audio_ops = i915_audio_component_audio_ops;
+ acomp-hdac_bus = bus;
+
dev_dbg(dev, bound to i915 component master\n);
return 0;
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on hotplug notify events
As a first cautious step, we're not going to trust the information
coming directly from the i915 driver, we're just going to use
the fact that there was an event to wakeup the codec and ask for
its status.
This fixes the issue with lost unsol events in power save mode,
the codec and controller can now sleep in D3 and still know when
the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
sound/pci/hda/patch_hdmi.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 2f24338..870b6d2 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2316,6 +2316,15 @@ static void haswell_set_power_state(struct hda_codec
*codec, hda_nid_t fg,
snd_hda_codec_set_power_to_all(codec, fg, power_state);
}
+static void intel_hotplug_notify(struct hdac_device *dev,
+const struct i915_audio_hotplug_info *info)
+{
+ struct hda_codec *codec = container_of(dev, struct hda_codec, core);
+ int pin_nid = is_valleyview(codec) ? 0x03 : info-port + 0x04;
+
+ check_presence_and_report(codec, pin_nid);
+}
+
static int patch_generic_hdmi(struct hda_codec *codec)
{
struct hdmi_spec *spec;
@@ -2342,8 +2351,10 @@ static int patch_generic_hdmi(struct hda_codec *codec)
if (is_valleyview_plus(codec) || is_skylake(codec))
codec-core.link_power_control = 1;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+ if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
codec-depop_delay = 0;
+ codec-core.i915_hotplug_notify = intel_hotplug_notify;
+ }
if (hdmi_parse_codec(codec) 0) {
codec-spec = NULL;
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] i915 to call hda driver on HDMI plug/unplug
On 2015-07-21 19:37, R, Durgadoss wrote: Hi David, -Original Message- From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of David Henningsson Sent: Tuesday, July 21, 2015 1:27 PM To: alsa-de...@alsa-project.org; intel-gfx@lists.freedesktop.org; ti...@suse.de; Vetter, Daniel; jani.nik...@linux.intel.com Cc: Koul, Vinod; David Henningsson Subject: [Intel-gfx] [PATCH 0/4] i915 to call hda driver on HDMI plug/unplug This patch set aims to resolve three problems: * The first - and most serious one - is that the audio driver is not woken up properly when in power save modes, especially not when the HDA controller is in D3. By having the i915 driver call directly into the hda driver, the HDA driver is always notified that an HDMI hotplug event has happened. * Second, there is currently no way for userspace to match an HDMI audio output with an HDMI video output. We fix this by sending connector_type and connector_type_id in the HDMI hotplug callback. * Third, writing ELD info to the hardware just so the HDA driver can read it from the hardware seems a bit inefficient. We could just pass that information in the callback, too. The patch in its current form fixes the first of these problems and provides most of the infrastructure for the second and third problem. The patch set is based on 4.2rc2 + my recent codec wakeup patch. So far, it has been tested (and working) on one Skylake machine. I believe you tested these patches with hda driver after few cycles of D3. By any chance, did you also try this once after i915 driver's D3 cycle also ? In this case, can the check_presence_and_report() function get the pin presence and ELD valid bits read out properly..? When running this code with drm.debug=0xe, I can see a lot of calls to set different power wells on and off, to the extent that I don't keep track of them. So I assume that means that the i915 driver goes into D3 as well during my tests. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Add audio hotplug info struct
On 2015-07-22 16:13, Vinod Koul wrote:
On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
On Wed, 22 Jul 2015 10:50:03 +0200,
David Henningsson wrote:
struct i915_audio_component {
struct device *dev;
+ struct hdac_bus *hdac_bus;
If we want to be more generic, using a struct device would be better,
e.g.
struct device *audio_dev;
Does this work? If we want to have the hdac_bus.dev ptr instead of a
hdac_bus ptr, there does not seem to be an obvious way to go from the
audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an
arbitrary dev pointer).
Hrm, right, currently it's not straightforward. Scratch the idea,
then.
That depends on the device we register this with. Actually this makes more
sense to me :)
If we register with struct device *audio_dev, which in this case would be
the codec device we create while probing the bus. This way you are linking i915
ops to the codec device. Ofcourse hdac_device has bus pointer but you can
invoke device callback without even searching for the device :)
It would require some extra setup, so I skipped it (at least for now).
I e, in order to detect codecs, we need to call hdac_i915 functions to
turn the power well on. And in order to connect the codec to the
i915_audio_component, we need to have detected a codec.
Which, now that I think of it, actually gives an interesting potential
race condition, in case the following happens:
1) Monitor is plugged in at boot time
2) i915 initializes
3) hda starts initializing and sets up the audio component
4) i915 finishes initialization and as part of that, calls the hotplug
notify to let hda know that the monitor is plugged in. However, at this
point, hda has not finished initialization yet, so there are no codecs
that listen for this information.
Anyhow, this is not a problem really yet, but it might be if we ever
decide to not write the ELD to the hardware.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Add audio hotplug info struct
On 2015-07-22 10:22, Takashi Iwai wrote:
On Tue, 21 Jul 2015 09:57:24 +0200,
David Henningsson wrote:
This struct will be used to transfer information from the i915
driver to the hda driver on HDMI hotplug events.
Signed-off-by: David Henningsson david.hennings...@canonical.com
Looks good to me, just a few nitpicking:
---
include/drm/i915_component.h | 19 +++
1 file changed, 19 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..4fc0db3 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -24,8 +24,22 @@
#ifndef _I915_COMPONENT_H_
#define _I915_COMPONENT_H_
+struct hdac_bus;
+
+struct i915_audio_hotplug_info {
+ int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
+ int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type,
meant for userspace */
+ int port; /* Used for mapping to affected nid */
+ int port_multi_stream_device; /* For DP multi-streaming */
+
+ bool plugged_in;
+ uint8_t *eld;
Use u8 or just unsigned char as it's a in-kernel API.
Also, safer to add const, since this is read-only for audio side.
Ok.
+ int eld_size;
+};
+
struct i915_audio_component {
struct device *dev;
+ struct hdac_bus *hdac_bus;
If we want to be more generic, using a struct device would be better,
e.g.
struct device *audio_dev;
Does this work? If we want to have the hdac_bus.dev ptr instead of a
hdac_bus ptr, there does not seem to be an obvious way to go from the
audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an
arbitrary dev pointer).
const struct i915_audio_component_ops {
struct module *owner;
@@ -34,6 +48,11 @@ struct i915_audio_component {
void (*codec_wake_override)(struct device *, bool enable);
int (*get_cdclk_freq)(struct device *);
} *ops;
+
+ const struct i915_audio_component_cb_ops {
+ struct module *owner;
Do we need the owner field at all?
It was merely for symmetry. I'll remove it for v2.
+ void (*hotplug_notify)(struct hdac_bus *, const struct
i915_audio_hotplug_info *);
+ } *cb_ops;
cb_ops doesn't sound intuitive. Any better name?
I was thinking of it as callback ops, i e, calls that go in the
reverse direction compared to the already existing ops.
But if we call the device audio_dev as you suggested above, then maybe
audio_ops would be nice and symmetric?
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] ALSA: hda - Dispatch incoming HDMI hotplug i915 callback
This lets interested codec(s) be notified of HDMI hotplug
events sent from the i915 driver.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
include/sound/hdaudio.h |4
sound/hda/hdac_i915.c | 24
2 files changed, 28 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 4caf1fd..ce34182 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -79,6 +79,10 @@ struct hdac_device {
int (*exec_verb)(struct hdac_device *dev, unsigned int cmd,
unsigned int flags, unsigned int *res);
+ /* Used for hotplug notification from i915 driver */
+ void (*hotplug_notify)(struct hdac_device *,
+ const struct i915_audio_hotplug_info *);
+
/* widgets */
unsigned int num_nodes;
hda_nid_t start_nid, end_nid;
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 5676b84..134ef9c 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -118,6 +118,8 @@ static void hdac_component_master_unbind(struct device *dev)
{
struct i915_audio_component *acomp = hdac_acomp;
+ acomp-cb_ops = NULL;
+ acomp-hdac_bus = NULL;
module_put(acomp-ops-owner);
component_unbind_all(dev, acomp);
WARN_ON(acomp-ops || acomp-dev);
@@ -128,6 +130,25 @@ static const struct component_master_ops
hdac_component_master_ops = {
.unbind = hdac_component_master_unbind,
};
+static void i915_audio_component_hotplug_notify(struct hdac_bus *bus,
+ const struct i915_audio_hotplug_info *info)
+{
+ struct hdac_device *d;
+
+ dev_dbg(bus-dev, Received HDMI hotplug callback (connector = %d-%d,
plugged in = %d),
+ info-connector_type, info-connector_type_id, (int)
info-plugged_in);
+
+ list_for_each_entry(d, bus-codec_list, list) {
+ if (d-hotplug_notify)
+ d-hotplug_notify(d, info);
+ }
+}
+
+static const struct i915_audio_component_cb_ops i915_audio_component_cb_ops = {
+ .owner = THIS_MODULE,
+ .hotplug_notify = i915_audio_component_hotplug_notify,
+};
+
static int hdac_component_master_match(struct device *dev, void *data)
{
/* i915 is the only supported component */
@@ -163,6 +184,9 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
ret = -ENODEV;
goto out_master_del;
}
+ acomp-cb_ops = i915_audio_component_cb_ops;
+ acomp-hdac_bus = bus;
+
dev_dbg(dev, bound to i915 component master\n);
return 0;
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/4] i915 to call hda driver on HDMI plug/unplug
This patch set aims to resolve three problems: * The first - and most serious one - is that the audio driver is not woken up properly when in power save modes, especially not when the HDA controller is in D3. By having the i915 driver call directly into the hda driver, the HDA driver is always notified that an HDMI hotplug event has happened. * Second, there is currently no way for userspace to match an HDMI audio output with an HDMI video output. We fix this by sending connector_type and connector_type_id in the HDMI hotplug callback. * Third, writing ELD info to the hardware just so the HDA driver can read it from the hardware seems a bit inefficient. We could just pass that information in the callback, too. The patch in its current form fixes the first of these problems and provides most of the infrastructure for the second and third problem. The patch set is based on 4.2rc2 + my recent codec wakeup patch. So far, it has been tested (and working) on one Skylake machine. David Henningsson (4): drm/i915: Add audio hotplug info struct drm/i915: Call audio hotplug notify function ALSA: hda - Dispatch incoming HDMI hotplug i915 callback ALSA: hda - Wake the codec up on hotplug notify events drivers/gpu/drm/i915/i915_drv.h|1 + drivers/gpu/drm/i915/intel_audio.c | 46 include/drm/i915_component.h | 19 +++ include/sound/hdaudio.h|4 sound/hda/hdac_i915.c | 24 +++ sound/pci/hda/patch_hdmi.c | 22 - 6 files changed, 115 insertions(+), 1 deletion(-) -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915: Call audio hotplug notify function
On HDMI hotplug events, notify the audio driver. This will enable
the audio driver to get the information at all times (even when
audio is in different powersave states), and also without reading
it from the hardware.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
drivers/gpu/drm/i915/i915_drv.h|1 +
drivers/gpu/drm/i915/intel_audio.c | 46
2 files changed, 47 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 542fac6..696624c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1808,6 +1808,7 @@ struct drm_i915_private {
struct drm_property *force_audio_property;
/* hda/i915 audio component */
+ struct i915_audio_component *audio_component;
bool audio_component_registered;
uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 3da9b84..eb29e1f 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -384,6 +384,44 @@ static void ilk_audio_codec_enable(struct drm_connector
*connector,
I915_WRITE(aud_config, tmp);
}
+static void audio_hotplug_notify(struct drm_i915_private *dev_priv,
+struct drm_connector *connector,
+struct intel_encoder *encoder,
+bool plugged_in)
+{
+ struct i915_audio_component *acomp = dev_priv-audio_component;
+ struct i915_audio_hotplug_info audio_info;
+ struct intel_digital_port *intel_dig_port =
+ enc_to_dig_port(encoder-base);
+ enum port port = intel_dig_port-port;
+
+ if (!acomp || !acomp-cb_ops || !acomp-cb_ops-hotplug_notify)
+ return;
+
+ if (connector) {
+ audio_info.connector_type = connector-connector_type;
+ audio_info.connector_type_id = connector-connector_type_id;
+ } else {
+ audio_info.connector_type = -1;
+ audio_info.connector_type_id = -1;
+ }
+
+ audio_info.port = (int) port;
+ /* DP Mst is unsupported for now */
+ audio_info.port_multi_stream_device = 0;
+
+ audio_info.plugged_in = plugged_in;
+ if (plugged_in connector) {
+ audio_info.eld = connector-eld;
+ audio_info.eld_size = drm_eld_size(audio_info.eld);
+ } else {
+ audio_info.eld = NULL;
+ audio_info.eld_size = 0;
+ }
+
+ acomp-cb_ops-hotplug_notify(acomp-hdac_bus, audio_info);
+}
+
/**
* intel_audio_codec_enable - Enable the audio codec for HD audio
* @intel_encoder: encoder on which to enable audio
@@ -419,6 +457,8 @@ void intel_audio_codec_enable(struct intel_encoder
*intel_encoder)
if (dev_priv-display.audio_codec_enable)
dev_priv-display.audio_codec_enable(connector, intel_encoder,
mode);
+
+ audio_hotplug_notify(dev_priv, connector, intel_encoder, true);
}
/**
@@ -435,6 +475,8 @@ void intel_audio_codec_disable(struct intel_encoder
*encoder)
if (dev_priv-display.audio_codec_disable)
dev_priv-display.audio_codec_disable(encoder);
+
+ audio_hotplug_notify(dev_priv, NULL, encoder, false);
}
/**
@@ -525,12 +567,14 @@ static int i915_audio_component_bind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
if (WARN_ON(acomp-ops || acomp-dev))
return -EEXIST;
acomp-ops = i915_audio_component_ops;
acomp-dev = i915_dev;
+ dev_priv-audio_component = acomp;
return 0;
}
@@ -539,9 +583,11 @@ static void i915_audio_component_unbind(struct device
*i915_dev,
struct device *hda_dev, void *data)
{
struct i915_audio_component *acomp = data;
+ struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
acomp-ops = NULL;
acomp-dev = NULL;
+ dev_priv-audio_component = NULL;
}
static const struct component_ops i915_audio_component_bind_ops = {
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on hotplug notify events
As a first cautious step, we're not going to trust the information
coming directly from the i915 driver, we're just going to use
the fact that there was an event to wakeup the codec and ask for
its status.
This fixes the issue with lost unsol events in power save mode,
the codec and controller can now sleep in D3 and still know when
the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
sound/pci/hda/patch_hdmi.c | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 2f24338..230d83d 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2316,6 +2316,24 @@ static void haswell_set_power_state(struct hda_codec
*codec, hda_nid_t fg,
snd_hda_codec_set_power_to_all(codec, fg, power_state);
}
+static void intel_haswell_hotplug_notify(struct hdac_device *dev,
+const struct i915_audio_hotplug_info
*info)
+{
+ struct hda_codec *codec = container_of(dev, struct hda_codec, core);
+ int pin_nid = info-port + 0x04;
+
+ check_presence_and_report(codec, pin_nid);
+}
+
+static void intel_valleyview_hotplug_notify(struct hdac_device *dev,
+ const struct
i915_audio_hotplug_info *info)
+{
+ struct hda_codec *codec = container_of(dev, struct hda_codec, core);
+ int pin_nid = 0x03; /* TODO: Ask Intel engineers to verify this */
+
+ check_presence_and_report(codec, pin_nid);
+}
+
static int patch_generic_hdmi(struct hda_codec *codec)
{
struct hdmi_spec *spec;
@@ -2331,7 +2349,9 @@ static int patch_generic_hdmi(struct hda_codec *codec)
if (is_haswell_plus(codec)) {
intel_haswell_enable_all_pins(codec, true);
intel_haswell_fixup_enable_dp12(codec);
- }
+ codec-core.hotplug_notify = intel_haswell_hotplug_notify;
+ } else if (is_valleyview_plus(codec))
+ codec-core.hotplug_notify = intel_valleyview_hotplug_notify;
/* For Valleyview/Cherryview, only the display codec is in the display
* power well and can use link_power ops to request/release the power.
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: Add audio hotplug info struct
This struct will be used to transfer information from the i915
driver to the hda driver on HDMI hotplug events.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
include/drm/i915_component.h | 19 +++
1 file changed, 19 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..4fc0db3 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -24,8 +24,22 @@
#ifndef _I915_COMPONENT_H_
#define _I915_COMPONENT_H_
+struct hdac_bus;
+
+struct i915_audio_hotplug_info {
+ int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
+ int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type,
meant for userspace */
+ int port; /* Used for mapping to affected nid */
+ int port_multi_stream_device; /* For DP multi-streaming */
+
+ bool plugged_in;
+ uint8_t *eld;
+ int eld_size;
+};
+
struct i915_audio_component {
struct device *dev;
+ struct hdac_bus *hdac_bus;
const struct i915_audio_component_ops {
struct module *owner;
@@ -34,6 +48,11 @@ struct i915_audio_component {
void (*codec_wake_override)(struct device *, bool enable);
int (*get_cdclk_freq)(struct device *);
} *ops;
+
+ const struct i915_audio_component_cb_ops {
+ struct module *owner;
+ void (*hotplug_notify)(struct hdac_bus *, const struct
i915_audio_hotplug_info *);
+ } *cb_ops;
};
#endif /* _I915_COMPONENT_H_ */
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm/i915: Call audio hotplug notify function
On 2015-07-21 11:14, Daniel Vetter wrote:
On Tue, Jul 21, 2015 at 09:57:25AM +0200, David Henningsson wrote:
On HDMI hotplug events, notify the audio driver. This will enable
the audio driver to get the information at all times (even when
audio is in different powersave states), and also without reading
it from the hardware.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
drivers/gpu/drm/i915/i915_drv.h|1 +
drivers/gpu/drm/i915/intel_audio.c | 46
2 files changed, 47 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 542fac6..696624c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1808,6 +1808,7 @@ struct drm_i915_private {
struct drm_property *force_audio_property;
/* hda/i915 audio component */
+ struct i915_audio_component *audio_component;
bool audio_component_registered;
uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 3da9b84..eb29e1f 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -384,6 +384,44 @@ static void ilk_audio_codec_enable(struct drm_connector
*connector,
I915_WRITE(aud_config, tmp);
}
+static void audio_hotplug_notify(struct drm_i915_private *dev_priv,
+struct drm_connector *connector,
+struct intel_encoder *encoder,
+bool plugged_in)
plugged_in is redundant, just check for NULL. Also just derive dev_priv
from intel_encoder imo. And finally we tend to put the object that a
function operates on first. Since this sends a notify out for the given
encoder (well dig port really) I'd put that first.
Sure, will fix these and send out a v2 (after Takashi has reviewed, too).
I also think that it would make sense to switch all the audio
enable/disable functions from intel_encoder to intel_dig_port. A lot need
to do upcasts anyway, and audio really only works on dig ports (which is
for hdmi/dp only).
That's up to you - seems a bit out of scope for this patch set though, I
assume.
Btw, there are two open questions though that you (or someone else)
might be able to share some insight on:
1) The mapping between ports and nid is fixed in patch 4/4, it would
be good with an Ack from an Intel engineer on this mapping.
2) Whether to use connector_type / connector_type_id combo or
connector_name as identification - it seems like X has a different
naming compared to the kernel (HDMI2 in X vs HDMI-A-2 in kernel),
not sure what Wayland/Mir/others do here...
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [alsa-devel] [PATCH 0/4 V7] Power-well API implementation for Haswell
On 07/24/2013 01:33 PM, Wang, Xingchao wrote: Yes, I agree. I'm debugging this issue on Ubuntu, not sure it happens on other distribution too. If it's related to Ubuntu, maybe need check Ubuntu power policy. Does anyone know the Ubuntu power-policy on laptop? i.e. when charger connected, will Ubuntu make decision to disable power-save feature for audio subsystem? I'm not a power management expert, but I got a pointer from my team mate to pm-utils: http://cgit.freedesktop.org/pm-utils/tree/pm/power.d/intel-audio-powersave If I understand correctly, The scripts in power.d are executed when battery / AC-power is changed. Takashi, does SUSE also use pm-utils? -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [alsa-devel] [PATCH 0/4 V7] Power-well API implementation for Haswell
have to admit I was expecting the audio driver would only require the power well when it is actually needed, and release it as soon as possible. It would behave so, if all setups are for power-saving. But, in your case, the runtime PM control attribute shows on; it implies that the runtime PM is effectively disabled, thus disabling power well is also impossible (because it would require turning off the audio controller, too). So, if the machine only has an eDP (which has no audio function in itself, right?) and never HDMI, DP output because there are no such physical ports, the audio controller has no function. Maybe we can, before doing anything else, ask the video driver first if this is the case, and if so, never create the sound card at all, and just leave things the way the video driver wants? -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device
On 06/14/2013 05:20 PM, Wang Xingchao wrote: when user open HDMI device 3/7/8, if it has no physical device connected, return error. This patch will cause regressions in two big use cases: 1) Older chipsets (at least from non-Intel vendors) might not support correct ELD reporting. Thus this will cause HDMI audio to stop working there. 2) In PulseAudio's current design, PulseAudio probes the device at startup and caches the result. Unfortunately, there is no reprobing at plug/unplug, so if the monitor is hotplugged, it will not work unless PulseAudio is restarted afterwards. The bug is from Haswell platform, All pins choose converter 0 by default in hardware level, maybe only pin 1 had valid monitor connected. if user play audio on pin 0/2, pin 1 can get audio data too. Signed-off-by: Wang Xingchao xingchao.w...@linux.intel.com --- sound/pci/hda/patch_hdmi.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8db5eb6..d766f40 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1139,6 +1139,9 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, per_pin = get_pin(spec, pin_idx); eld = per_pin-sink_eld; + if (!eld-monitor_present || !eld-eld_valid) + return -EIO; + if (codec-vendor_id == 0x80862807) hsw_verify_cvt_D0(spec, codec); -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device
On 06/17/2013 01:54 PM, Wang, Xingchao wrote: -Original Message- From: David Henningsson [mailto:david.hennings...@canonical.com] Sent: Monday, June 17, 2013 4:24 PM To: Wang Xingchao Cc: ti...@suse.de; daniel.vet...@ffwll.ch; alsa-de...@alsa-project.org; intel-gfx@lists.freedesktop.org; Wang, Xingchao Subject: Re: [PATCH 2/4] ALSA: hda - Return error when open empty hdmi device On 06/14/2013 05:20 PM, Wang Xingchao wrote: when user open HDMI device 3/7/8, if it has no physical device connected, return error. This patch will cause regressions in two big use cases: 1) Older chipsets (at least from non-Intel vendors) might not support correct ELD reporting. Thus this will cause HDMI audio to stop working there. Thanks for pointing this out. It's a good case I missed. :) I thought ELD info was monitor specific, and reported on common platforms. We have a generation of Nvidia cards that does not support presence detect (and no ELD) for the audio codec at all. I'm not sure if we also have cards where presence detect works, but there never comes any ELD. Or if this can potentially vary depending on the connected monitor. But I think this should also be handled in the best way. But sometimes user get confused when opening a hdmi device and started to play audio,but hear nothing, even there's no error message. Is there any way for old chipset which has no correct ELD reporting but do have audio functionality? App should check before really play audio on the device. PulseAudio (with support from some of the GUIs) support looking at the Presence Detect, and hiding the device if nothing is currently connected. I agree in principle with that you should be given some warning if you try to play back to unconnected things, but for me it's not a high priority to improve. E g, the situation is the same for analog audio - imagine a small desktop with only a headphone output: you can still play audio to the headphone output even when there's no headphone connected in the jack. 2) In PulseAudio's current design, PulseAudio probes the device at startup and caches the result. Unfortunately, there is no reprobing at plug/unplug, so if the monitor is hotplugged, it will not work unless PulseAudio is restarted afterwards. For haswell ult board only, DDI port D is not supported, this results in screen flicker when playing on third pin. But for other haswell boards, the DDI port D is normal. So at first glance my idea is to disable opening device without physical device connecting. Maybe it's possible to fix on the gfx side? It sounds like a graphic driver bug if the screen flickers due to anything happening on the audio side. Thanks --xingchao The bug is from Haswell platform, All pins choose converter 0 by default in hardware level, maybe only pin 1 had valid monitor connected. if user play audio on pin 0/2, pin 1 can get audio data too. Signed-off-by: Wang Xingchao xingchao.w...@linux.intel.com --- sound/pci/hda/patch_hdmi.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8db5eb6..d766f40 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1139,6 +1139,9 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, per_pin = get_pin(spec, pin_idx); eld = per_pin-sink_eld; + if (!eld-monitor_present || !eld-eld_valid) + return -EIO; + if (codec-vendor_id == 0x80862807) hsw_verify_cvt_D0(spec, codec); -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Haswell: Ensuring HDA codec pins refer to physical outputs
On 05/27/2013 09:30 AM, Wang xingchao wrote: On Thu, May 16, 2013 at 09:00:06AM +0200, David Henningsson wrote: Hi, I want to take this problem up again, because it's important we get this right. The HDA driver assumes that a codec pin widget node always refers to the same physical output. With Haswell, it seems like this is not guaranteed to be true. I would like to see this fixed on the graphics side. If not, I don't know how to work around it on the audio side. in gfx side, eld valid bit was set according to *pipe*, not physical outputs. For haswell, the codec pin output connected to transcoder(pipe) directly. I cannot confirm whether the three codec Pins are fixed connected to three transcoders(pipes) atm, i assume it is as i cannot find any programing manual in bspec(please correct me if i'm wrong here). And between transcoder and external DDI ports, there's cross-point mux used for selection: pipe/transcoder can select the output DDI ports. i.e. the physical HDMI cable is connected to DDI port B. if current using pipe is PIPE A, the eld valid bit for PIPE A is set. But we cannot guarantee only the first hdmi device is available for audio output. when play audio through first codec pin, it means output audio data to Transcoder A(Pipe A), that depends on whether PIPE A select DDI port B. If output data to second codec PIN, it outputs data to PIPE B, if pipe B select DDI B too, you can hear sound, even the second pin doesnt have valid eld info. thanks --xingchao The problems that occur on the audio side are: 1) Some BIOSes set default pin config. E g, if the machine has a single HDMI out, it can set two of the codec pins to not connected and let the third remain jack. As a result, the HDA driver will ignore the two codec pins and only enable the third. As such, HDMI audio will not work correctly, unless it's the third codec pin that is connected to the physical output. 2) Saving and restoring mutes, volumes etc is done on a per-pin basis. E g, imagine that a user has a dual monitor setup and always wants audio output from the left side monitor, and keep the right side monitor silent. If it is not reliable which codec pin refers to which physical output, one day suddenly the sound might come out on the right side monitor instead. Thanks for your answer, but it doesn't really resolve the problems as outlined above. If it is utterly impossible to make sure that the audio codec pin always represent the same physical output, we might need even more communication between the audio and video driver. For solving problem 1), the audio driver needs to inform the video driver what outputs are disabled by BIOS default pin config, so it can avoid assigning those pipes to anything with audio output. (Or we can just ignore BIOS default pin config for Haswell in the audio driver, but I'm not sure if this leads to problems somewhere else.) For solving problem 2), we should perhaps call into the video driver somehow to enumerate the physical outputs and what codec pin is currently assigned to what output. Also, when can these PIPE - DDI connections change? Can we get a notification? Otherwise it would be very confusing for the user if (s)he's currently playing an audio stream and suddenly the audio comes out of something else than when (s)he started the stream. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Haswell: Ensuring HDA codec pins refer to physical outputs
Hi, I want to take this problem up again, because it's important we get this right. The HDA driver assumes that a codec pin widget node always refers to the same physical output. With Haswell, it seems like this is not guaranteed to be true. I would like to see this fixed on the graphics side. If not, I don't know how to work around it on the audio side. The problems that occur on the audio side are: 1) Some BIOSes set default pin config. E g, if the machine has a single HDMI out, it can set two of the codec pins to not connected and let the third remain jack. As a result, the HDA driver will ignore the two codec pins and only enable the third. As such, HDMI audio will not work correctly, unless it's the third codec pin that is connected to the physical output. 2) Saving and restoring mutes, volumes etc is done on a per-pin basis. E g, imagine that a user has a dual monitor setup and always wants audio output from the left side monitor, and keep the right side monitor silent. If it is not reliable which codec pin refers to which physical output, one day suddenly the sound might come out on the right side monitor instead. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
;
+}
+EXPORT_SYMBOL_GPL(audio_link_to_i915_driver);
/* Power well has impact on Haswell controller and codecs */
void hda_display_power(bool enable)
{
- snd_printk(KERN_INFO HDA display power %s \n, enable ? Enable :
Disable);
-
- if (!get_power || !put_power)
- return;
+ if (!get_power || !put_power) {
+ if (hsw_i915_load) {
+ get_power = i915_request_power_well;
+ put_power = i915_release_power_well;
+ } else
+ return;
+ }
+ snd_printk(KERN_DEBUG HDA display power %s \n,
+ enable ? Enable : Disable);
if (enable)
get_power(hda);
else
put_power(hda);
}
-EXPORT_SYMBOL(hda_display_power);
+EXPORT_SYMBOL_GPL(hda_display_power);
-static int __init hda_i915_init(void)
+/* In case i915 module loaded first, the APIs are there.
+ * Otherwise wait until i915 module notify us. */
+int hda_i915_init(void)
{
- struct module *i915;
- mutex_lock(module_mutex);
- i915 = find_module(name);
- mutex_unlock(module_mutex);
+ struct module *i915;
- if (!i915)
- request_module_nowait(name);
+ mutex_lock(module_mutex);
+ i915 = find_module(module_name);
+ mutex_unlock(module_mutex);
- if (!try_module_get(i915))
- return -EFAULT;
+ if (!i915)
+ request_module_nowait(module_name);
get_power = symbol_get(i915_request_power_well);
+ if (!get_power)
+ goto out;
+
put_power = symbol_get(i915_release_power_well);
+ if (!put_power)
+ goto out;
- module_put(i915);
+ snd_printk(KERN_DEBUG HDA driver get symbol successfully from i915
module\n);
+out:
return 0;
}
+EXPORT_SYMBOL_GPL(hda_i915_init);
-#if 0
-static void __exit hda_i915_exit(void)
+int hda_i915_exit(void)
{
- drm_pci_exit(driver, i915_pci_driver);
+ symbol_put(i915_request_power_well);
+ symbol_put(i915_release_power_well);
}
+EXPORT_SYMBOL_GPL(hda_i915_exit);
-module_init(hda_i915_init);
-module_exit(hda_i915_exit);
-#endif
-module_init(hda_i915_init);
-MODULE_DESCRIPTION(HDA power well);
+MODULE_DESCRIPTION(HDA power well For Haswell);
MODULE_LICENSE(GPL);
diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h
index a7e5324..b0516ab 100644
--- a/sound/pci/hda/hda_i915.h
+++ b/sound/pci/hda/hda_i915.h
@@ -3,8 +3,13 @@
#ifdef CONFIG_SND_HDA_I915
void hda_display_power(bool enable);
+int hda_i915_init(void);
+int hda_i915_exit(void);
#else
void hda_display_power(bool enable) {}
+int hda_i915_init(void) {}
+int hda_i915_exit(void) {}
#endif
+void audio_link_to_i915_driver(void);
#endif
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 8bb6075..431027d 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -3684,8 +3684,10 @@ static int azx_probe(struct pci_dev *pci,
return -ENOENT;
}
- if (IS_HSW(pci))
+ if (IS_HSW(pci)) {
+ hda_i915_init();
hda_display_power(true);
+ }
err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, card);
if (err 0) {
@@ -3822,8 +3824,10 @@ static void azx_remove(struct pci_dev *pci)
if (card)
snd_card_free(card);
pci_set_drvdata(pci, NULL);
- if (IS_HSW(pci))
+ if (IS_HSW(pci)) {
hda_display_power(false);
+ hda_i915_exit();
+ }
}
/* PCI IDs */
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
On 05/13/2013 01:55 PM, Wang, Xingchao wrote:
Hi David,
-Original Message-
From: alsa-devel-boun...@alsa-project.org
[mailto:alsa-devel-boun...@alsa-project.org] On Behalf Of David Henningsson
Sent: Monday, May 13, 2013 4:29 PM
To: Wang Xingchao
Cc: alsa-de...@alsa-project.org; dan...@ffwll.ch; ti...@suse.de; Lin,
Mengdong; intel-gfx@lists.freedesktop.org; Li, Jocelyn; Barnes, Jesse;
Girdwood, Liam R; Zanoni, Paulo R
Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API
existense before calling
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API
doesnot exist in such case. This patch intended to avoid loading
dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines
removed), so it's a little hard to follow. But I'll try to write some overall
comments on how I have envisioned things...
1. I don't think there's any need to create an additional kernel module, we can
just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if
DRM_I915 is defined.
2. We don't need an IS_HSW macro on the audio side. Instead declare a new
AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
3. Somewhere in the beginning of the probing in hda_intel.c, we'll need
something like:
if (driver_caps AZX_DCAPS_NEED_I915_POWER)
hda_i915_init(chip);
4. The hda_i915_init does not need to be exported (they're now both in the
same module). hda_i915.h could have something like:
#ifdef DRM_I915
void hda_i915_init(chip);
#else
#define hda_i915_init(chip) do {} while(0) #endif
Or perhaps even better
static inline void hda_i915_init(azx *chip) {}
Thanks your suggestions. Will change them in next version.
5. You're saying this patch is about avoid loading dependency between
snd-hda-intel and i915 module. That does not make sense to me, since the
powerwell API is in the i915 module, snd-hda-intel must load it when it wants to
enable power on haswell, right? Thus there is a loading dependency. If the i915
module is not loaded at that point, we must wait for it to load, so we can have
proper power, instead of continuing probing without the power well?
If i915 module not loaded, snd-had-intel will load it in current code.
The question is the tolerant delay of waiting for i915 loading.
Could you explain in more detail, what you mean with tolerant delay
and what will happen if you exceed that delay?
Continuing probing would immediately fail.
Isn't that what's happening with your current patch set, if
snd-hda-intel is loaded first?
In azx_probe, hda_i915_init won't get the symbols, because
request_module is nowait. Then hda_display_power(true) won't enable
power, because there are no symbols.
Probing audio controller will then continue without power well, which is
bad?
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [alsa-devel] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
On 05/03/2013 10:28 AM, Wang, Xingchao wrote: Hi David, Thank you very much for your draft patch. I have some more work on a new patchset, some ideas are from your patch. Thanks. Here's a brief introduction of attached patchset: 1. a new bus type in /sound/had_bus.c, used to bind the single module and codec device It looks like ac97_bus.c I don't understand why this is needed. It does not look like it's used from the gfx side either, or anything like that? 2. add a new device node in struct hda_codec, it's used to register for new bus type. 3. a new single module hdmi_i915, which compiled in only when DRM_I915 and CODEC_HDMI enabled. It stores the private API for gfx part. There's no support to probe haswell hdmi codec only yet. Power well will be used only for haswell display audio. 4. power well API implementation in gfx side. Please feel free to add your idea and I will help test your patch too. Ok. So the patch I wrote would (if it works) be combined with your patch 3, which implements the gfx side. The gfx side is not my area of expertise. The proposed way in my patch would be more elegant since it does not introduce any i915 related code in hda_codec* files. Still, Takashi is the boss here so he has the final say :-) Thanks --xingchao -Original Message- From: David Henningsson [mailto:david.hennings...@canonical.com] Sent: Thursday, May 02, 2013 10:49 PM To: Liam Girdwood Cc: Barnes, Jesse; alsa-de...@alsa-project.org; Zanoni, Paulo R; Takashi Iwai; Daniel Vetter; intel-gfx@lists.freedesktop.org; Wysocki, Rafael J; Wang Xingchao; Wang, Xingchao; Li, Jocelyn; Hindman, Gavin; Daniel Vetter; Lin, Mengdong; ville.syrj...@linux.intel.com Subject: Re: [alsa-devel] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team On 04/30/2013 04:41 PM, Liam Girdwood wrote: On Tue, 2013-04-30 at 12:29 +0200, David Henningsson wrote: On 04/29/2013 05:02 PM, Jesse Barnes wrote: On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Sat, Apr 27, 2013 at 09:20:39AM +, Wang, Xingchao wrote: Let me throw a basic proposal on Audio driver side, please give your comments freely. it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. #2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. #3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. #4: audio release power well when module unload Audio release power well at remove callback to let i915 know. I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for runtime PM. We _must_ be able to switch off the power well whenever possible. Xingchao, I'm not an audio developer so I'm probably way off. But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing. If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better? For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions). --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code codec-patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3); + if (i915_shared_power_well) + i915_put_power_well(codec-i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(codec-power_work); @@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo return; spin_unlock(codec-power_lock); + if (i915_shared_power_well) + i915_get_power_well(codec-i915_data); Is it wise that a _get function actually has side effects? Perhaps _push and _pop or something else would be better semantics. I think the intention here is to model on the PM runtime subsystem where we can get/put the reference count on a PM resource. I'd expect
Re: [Intel-gfx] [alsa-devel] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
On 04/30/2013 04:41 PM, Liam Girdwood wrote: On Tue, 2013-04-30 at 12:29 +0200, David Henningsson wrote: On 04/29/2013 05:02 PM, Jesse Barnes wrote: On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Sat, Apr 27, 2013 at 09:20:39AM +, Wang, Xingchao wrote: Let me throw a basic proposal on Audio driver side, please give your comments freely. it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. #2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. #3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. #4: audio release power well when module unload Audio release power well at remove callback to let i915 know. I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for runtime PM. We _must_ be able to switch off the power well whenever possible. Xingchao, I'm not an audio developer so I'm probably way off. But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing. If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better? For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions). --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code codec-patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3); + if (i915_shared_power_well) + i915_put_power_well(codec-i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(codec-power_work); @@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo return; spin_unlock(codec-power_lock); + if (i915_shared_power_well) + i915_get_power_well(codec-i915_data); Is it wise that a _get function actually has side effects? Perhaps _push and _pop or something else would be better semantics. I think the intention here is to model on the PM runtime subsystem where we can get/put the reference count on a PM resource. I'd expect with this API that i915_get_power_well() will increment the count and prevent the shared PM resource from being powered OFF. Ok, I stand corrected. + cancel_delayed_work_sync(codec-power_work); spin_lock(codec-power_lock); With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present... Takashi, any other ideas? The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend. I think this sounds about right. The question is how to avoid a dependency on the i915 driver when it's not necessary, such as when the HDMI codec is AMD or Nvidia. The most obvious way to me seems to be to create a new snd-hda-codec-hdmi-haswell module (that depends on both i915 and snd-hda-codec-hdmi), and then let that call into i915 and codec-hdmi drivers as necessary, e g using the set_power_state callback for the i915 stuff. But maybe there's something smarter to do here, as I'm not experienced in mending kernel pieces together :-) Interesting idea, we could have something similar to the AC97 ad-hoc device support where we could load other HW specific AC97 modules (like touchscreen controllers) without breaking the generic nature of the base driver. e.g. the WM9712 AC97 touchscreen driver could connect to the generic AC97 audio driver (get it's device struct ac97 *) and perform AC97 register IO, ac97 API calls etc. Nobody objected, so I wrote a very draft patch, which is attached to this email. It probably does not even compile, but should show how I envision things could be mended
Re: [Intel-gfx] [alsa-devel] [PATCH] drm/i915: Add private api for power well usage -- alignment between graphic team and audio team
On 04/29/2013 05:02 PM, Jesse Barnes wrote: On Sat, 27 Apr 2013 13:35:29 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Sat, Apr 27, 2013 at 09:20:39AM +, Wang, Xingchao wrote: Let me throw a basic proposal on Audio driver side, please give your comments freely. it contains the power well control usage points: #1: audio request power well at boot up. I915 may shut down power well after bootup initialization, as there's no monitor connected outside or only eDP on pipe A. #2: audio request power on resume After exit from D3 mode, audio driver quest power on. This may happen at normal resume or runtime resume. #3: audio release power well control at suspend Audio driver will let i915 know it doensot need power well anymore as it's going to suspend. This may happened at normal suspend or runtime suspend point. #4: audio release power well when module unload Audio release power well at remove callback to let i915 know. I miss the power well grab/dropping at runtime from the audio side. If the audio driver forces the power well to be on the entire time it's loaded, that's not good, since the power well stuff is very much for runtime PM. We _must_ be able to switch off the power well whenever possible. Xingchao, I'm not an audio developer so I'm probably way off. But what we really need is a very small and targeted set of calls into the i915 driver from say the HDMI driver in HDA. It looks like the prepare/cleanup pair in the pcm_ops structure might be the right place to put things? If that's too fine grained, you could do it at open/close time I guess, but the danger there is that some app will keep the device open even while it's not playing. If that won't work, maybe calling i915 from hda_power_work in the higher level code would be better? For detecting whether to call i915 at all, you can filter on the PCI IDs (just look for an Intel graphics device and if present, try to get the i915 symbols for the power functions). --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3860,6 +3860,8 @@ static unsigned int hda_call_codec_suspend(struct hda_code codec-patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3); + if (i915_shared_power_well) + i915_put_power_well(codec-i915_data); /* Cancel delayed work if we aren't currently running from it. */ if (!in_wq) cancel_delayed_work_sync(codec-power_work); @@ -4807,6 +4809,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bo return; spin_unlock(codec-power_lock); + if (i915_shared_power_well) + i915_get_power_well(codec-i915_data); Is it wise that a _get function actually has side effects? Perhaps _push and _pop or something else would be better semantics. + cancel_delayed_work_sync(codec-power_work); spin_lock(codec-power_lock); With some code at init time to get the i915 symbols you need to call and whether or not the shared power well is present... Takashi, any other ideas? The high level goal here should be for the audio driver to call into i915 with get/put power well around the sequences where it needs the power to be up (reading/writing registers, playing audio), but not across the whole time the driver is loaded, just like you already do with the powersave work functions, e.g. hda_call_codec_suspend. I think this sounds about right. The question is how to avoid a dependency on the i915 driver when it's not necessary, such as when the HDMI codec is AMD or Nvidia. The most obvious way to me seems to be to create a new snd-hda-codec-hdmi-haswell module (that depends on both i915 and snd-hda-codec-hdmi), and then let that call into i915 and codec-hdmi drivers as necessary, e g using the set_power_state callback for the i915 stuff. But maybe there's something smarter to do here, as I'm not experienced in mending kernel pieces together :-) -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
