On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:

to_intel_crtc() always returns a non-NULL pointer.

Eh? to_intel_crtc(NULL) should return NULL or we might have tons of breakage on our hands. Or maybe the atomic work has gotten rid of that assumption, but at least we used to depend on that heavily.

Signed-off-by: Takashi Iwai tiwai@suse.de

drivers/gpu/drm/i915/intel_audio.c | 4 ---- 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 4dccd9b003a1..0c38cc6c82ae 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -656,10 +656,6 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, intel_dig_port = enc_to_dig_port(&intel_encoder->base); if (port == intel_dig_port->port) { crtc = to_intel_crtc(intel_encoder->base.crtc);

• 	if (!crtc) {
  

• 		DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
  

• 		continue;
  

• 	}
  pipe = crtc->pipe;
  break;
  

  }

-- 2.6.3

On Fri, Dec 04, 2015 at 01:54:56PM +0100, Takashi Iwai wrote:

On Fri, 04 Dec 2015 13:16:46 +0100, Ville Syrjälä wrote:

...

On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:

...

to_intel_crtc() always returns a non-NULL pointer.

Eh? to_intel_crtc(NULL) should return NULL or we might have tons of breakage on our hands. Or maybe the atomic work has gotten rid of that assumption, but at least we used to depend on that heavily.

Well, to_intel_crtc() has been always container_of() since the very beginning of universe:

commit 79e539453b34e35f39299a899d263b0a1f1670bd Author: Jesse Barnes jbarnes@virtuousgeek.org Date: Fri Nov 7 14:24:08 2008 -0800

DRM: i915: add mode setting support

--- /dev/null +++ b/drivers/gpu/drm/i915/intel_drv.h .... +#define to_intel_crtc(x) container_of(x, struct intel_crtc, base) ....

Yes, but struct intel_crtc { struct drm_crtc base; ... }

So offsetof(struct intel_crtc, base)==0 and NULL-0 is still NULL.

I once suggested that someone should add a compile time assert to make sure this always holds for us, but no one took the bait.

Takashi

...

...

Signed-off-by: Takashi Iwai tiwai@suse.de

drivers/gpu/drm/i915/intel_audio.c | 4 ---- 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 4dccd9b003a1..0c38cc6c82ae 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -656,10 +656,6 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, intel_dig_port = enc_to_dig_port(&intel_encoder->base); if (port == intel_dig_port->port) { crtc = to_intel_crtc(intel_encoder->base.crtc);

• 	if (!crtc) {
  

• 		DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
  

• 		continue;
  

• 	}
  pipe = crtc->pipe;
  break;
  

  }

-- 2.6.3

-- Ville Syrjälä Intel OTC

On Fri, Dec 04, 2015 at 02:12:14PM +0100, Takashi Iwai wrote:

On Fri, 04 Dec 2015 14:07:03 +0100, Ville Syrjälä wrote:

...

On Fri, Dec 04, 2015 at 01:54:56PM +0100, Takashi Iwai wrote:

...

On Fri, 04 Dec 2015 13:16:46 +0100, Ville Syrjälä wrote:

...

On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:

...

to_intel_crtc() always returns a non-NULL pointer.

Eh? to_intel_crtc(NULL) should return NULL or we might have tons of breakage on our hands. Or maybe the atomic work has gotten rid of that assumption, but at least we used to depend on that heavily.

Well, to_intel_crtc() has been always container_of() since the very beginning of universe:

commit 79e539453b34e35f39299a899d263b0a1f1670bd Author: Jesse Barnes jbarnes@virtuousgeek.org Date: Fri Nov 7 14:24:08 2008 -0800

DRM: i915: add mode setting support

--- /dev/null +++ b/drivers/gpu/drm/i915/intel_drv.h .... +#define to_intel_crtc(x) container_of(x, struct intel_crtc, base) ....

Yes, but struct intel_crtc { struct drm_crtc base; ... }

So offsetof(struct intel_crtc, base)==0 and NULL-0 is still NULL.

I once suggested that someone should add a compile time assert to make sure this always holds for us, but no one took the bait.

Argh, only from the usage of container_of(), no one expects this implicit assumption :-< Indeed intel code has lots of these silent rules, e.g. calling kfree() for the base class object.

Daniel, could you please take my patch back?

Oops. Patch reverted, sorry for the mess. -Daniel

thanks,

Takashi

...

...

Takashi

...

...

Signed-off-by: Takashi Iwai tiwai@suse.de

drivers/gpu/drm/i915/intel_audio.c | 4 ---- 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 4dccd9b003a1..0c38cc6c82ae 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -656,10 +656,6 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, intel_dig_port = enc_to_dig_port(&intel_encoder->base); if (port == intel_dig_port->port) { crtc = to_intel_crtc(intel_encoder->base.crtc);

• 	if (!crtc) {
  

• 		DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
  

• 		continue;
  

• 	}
  pipe = crtc->pipe;
  break;
  

  }

-- 2.6.3

-- Ville Syrjälä Intel OTC

-- Ville Syrjälä Intel OTC

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:

Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current audio status and ELD of the given HDMI/DP port. It returns the size of expected ELD bytes if it's valid, zero if no valid ELD is found, or a negative error code. The current state of audio on/off is stored in the given pointer, too.

Note that the returned size isn't limited to the given max bytes. If the size is greater than the max bytes, it means that only a part of ELD has been copied back.

A big warning about the usage of this callback is: you must not call it from eld_notify. The eld_notify itself is called in the modeset lock, and it leads to a deadlock since get_eld takes the modeset lock, too. You need to call get_eld in a work, for example, in such a case. We'll see the actual implementation in the later patch in sound/pci/hda/patch_hdmi.c.

For achieving this implementation, a new field audio_enabled is added to struct intel_digital_port. This is set/reset at each audio enable/disable call in intel_audio.c. It's protected with the modeset lock as well.

Signed-off-by: Takashi Iwai tiwai@suse.de

v1->v2:

• Use modeset lock for get_eld lock, drop av mutex
• Return the expected size from get_eld, not the copied size

drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_component.h | 6 ++++++ 3 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 0c38cc6c82ae..1965a61769ea 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -521,6 +521,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)

connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;

• intel_dig_port->audio_enabled = true; if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, adjusted_mode);

@@ -545,6 +546,7 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder) struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); enum port port = intel_dig_port->port;

• intel_dig_port->audio_enabled = false; if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(intel_encoder);

@@ -702,6 +704,43 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, return 0; }

+static int i915_audio_component_get_eld(struct device *dev, int port,

• 			bool *enabled,
  

• 			unsigned char *buf, int max_bytes)
  

+{

• struct drm_i915_private *dev_priv = dev_to_i915(dev);
• struct drm_device *drm_dev = dev_priv->dev;
• struct intel_encoder *intel_encoder;
• struct intel_digital_port *intel_dig_port;
• struct drm_connector *connector;
• unsigned char *eld;
• int ret = -EINVAL;
• drm_modeset_lock_all(drm_dev);

This is super expensive and shouldn't ever be used in new code. So either just the connection_mutex or resurrect the av_mutex and just cache what you need under that. Tbh I prefer the separate lock + cache for such specific things since it completely avoids spreading and entangling locking contexts. We use the same design to get modeset information into the PSR tracking, FBC tracking and other code which sits between KMS and other subsystems.

• for_each_intel_encoder(drm_dev, intel_encoder) {

• if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&
  

•     intel_encoder->type != INTEL_OUTPUT_HDMI)
  

• 	continue;
  

• intel_dig_port = enc_to_dig_port(&intel_encoder->base);
  

• if (port == intel_dig_port->port) {
  

• 	ret = 0;
  

• 	*enabled = intel_dig_port->audio_enabled;
  

• 	if (!*enabled)
  

• 		break;
  

• 	connector = drm_select_eld(&intel_encoder->base);
  

• 	if (!connector)
  

• 		break;
  

• 	eld = connector->eld;
  

• 	ret = drm_eld_size(eld);
  

• 	memcpy(buf, eld, min(max_bytes, ret));
  

• 	break;
  

• }
  

• }
• drm_modeset_unlock_all(drm_dev);
• return ret;

+}

static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, @@ -709,6 +748,7 @@ static const struct i915_audio_component_ops i915_audio_component_ops = { .codec_wake_override = i915_audio_component_codec_wake_override, .get_cdclk_freq = i915_audio_component_get_cdclk_freq, .sync_audio_rate = i915_audio_component_sync_audio_rate,

• .get_eld = i915_audio_component_get_eld,

};

static int i915_audio_component_bind(struct device *i915_dev, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0598932ce623..4afc7560be04 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -798,6 +798,7 @@ struct intel_digital_port { u32 saved_port_bits; struct intel_dp dp; struct intel_hdmi hdmi;

• bool audio_enabled; enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool); bool release_cl2_override;

}; diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..013779db7d24 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@

• @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

• • @get_eld: fill the audio state and ELD bytes for the given port
  */

struct i915_audio_component_ops { struct module *owner; @@ -46,6 +47,8 @@ struct i915_audio_component_ops { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); int (*sync_audio_rate)(struct device *, int port, int rate);

• int (*get_eld)(struct device *, int port, bool *enabled,

•        unsigned char *buf, int max_bytes);
  

};

struct i915_audio_component_audio_ops { @@ -55,6 +58,9 @@ struct i915_audio_component_audio_ops { * pin sense and/or ELD information has changed. * @audio_ptr: HDA driver object * @port: Which port has changed (PORTA / PORTB / PORTC etc)

• *
  

• * Note that you can't call i915_audio_component_ops.get_eld directly
  

• * from the notifier callback as it may lead to deadlocks.
  

With av_mutex we don't even need that note here ;-) -Daniel

*/

void (*pin_eld_notify)(void *audio_ptr, int port); }; -- 2.6.3

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

On Fri, Dec 04, 2015 at 11:49:46AM +0100, Takashi Iwai wrote:

On Fri, 04 Dec 2015 11:21:02 +0100, Daniel Vetter wrote:

...

On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:

...

Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current audio status and ELD of the given HDMI/DP port. It returns the size of expected ELD bytes if it's valid, zero if no valid ELD is found, or a negative error code. The current state of audio on/off is stored in the given pointer, too.

Note that the returned size isn't limited to the given max bytes. If the size is greater than the max bytes, it means that only a part of ELD has been copied back.

A big warning about the usage of this callback is: you must not call it from eld_notify. The eld_notify itself is called in the modeset lock, and it leads to a deadlock since get_eld takes the modeset lock, too. You need to call get_eld in a work, for example, in such a case. We'll see the actual implementation in the later patch in sound/pci/hda/patch_hdmi.c.

For achieving this implementation, a new field audio_enabled is added to struct intel_digital_port. This is set/reset at each audio enable/disable call in intel_audio.c. It's protected with the modeset lock as well.

Signed-off-by: Takashi Iwai tiwai@suse.de

v1->v2:

• Use modeset lock for get_eld lock, drop av mutex
• Return the expected size from get_eld, not the copied size

drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_component.h | 6 ++++++ 3 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 0c38cc6c82ae..1965a61769ea 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -521,6 +521,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)

connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;

• intel_dig_port->audio_enabled = true; if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, adjusted_mode);

@@ -545,6 +546,7 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder) struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); enum port port = intel_dig_port->port;

• intel_dig_port->audio_enabled = false; if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(intel_encoder);

@@ -702,6 +704,43 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, return 0; }

+static int i915_audio_component_get_eld(struct device *dev, int port,

• 			bool *enabled,
  

• 			unsigned char *buf, int max_bytes)
  

+{

• struct drm_i915_private *dev_priv = dev_to_i915(dev);
• struct drm_device *drm_dev = dev_priv->dev;
• struct intel_encoder *intel_encoder;
• struct intel_digital_port *intel_dig_port;
• struct drm_connector *connector;
• unsigned char *eld;
• int ret = -EINVAL;
• drm_modeset_lock_all(drm_dev);

This is super expensive and shouldn't ever be used in new code. So either just the connection_mutex or resurrect the av_mutex and just cache what you need under that.

OK, I need to make it harder, then.

...

Tbh I prefer the separate lock + cache for such specific things since it completely avoids spreading and entangling locking contexts. We use the same design to get modeset information into the PSR tracking, FBC tracking and other code which sits between KMS and other subsystems.

I didn't want to be involved with the modeset lock, but it has to be. This function calls drm_select_eld() and it requires both mode_config.mutex and connection_mutex.

drm_select_eld() would seem pointless to me if we cached the required information somewhere. But we'd still need to actually get the eld, and that means either caching it again somewhere, or perhaps it would be better to move the drm_edid_to_eld() call to happen at modeset audio enable time protected by the av_mutex? That way connector->eld contents would remain constant as long as we have a mode set.

On Fri, Dec 04, 2015 at 11:49:46AM +0100, Takashi Iwai wrote:

On Fri, 04 Dec 2015 11:21:02 +0100, Daniel Vetter wrote:

...

On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:

...

Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current audio status and ELD of the given HDMI/DP port. It returns the size of expected ELD bytes if it's valid, zero if no valid ELD is found, or a negative error code. The current state of audio on/off is stored in the given pointer, too.

Note that the returned size isn't limited to the given max bytes. If the size is greater than the max bytes, it means that only a part of ELD has been copied back.

A big warning about the usage of this callback is: you must not call it from eld_notify. The eld_notify itself is called in the modeset lock, and it leads to a deadlock since get_eld takes the modeset lock, too. You need to call get_eld in a work, for example, in such a case. We'll see the actual implementation in the later patch in sound/pci/hda/patch_hdmi.c.

For achieving this implementation, a new field audio_enabled is added to struct intel_digital_port. This is set/reset at each audio enable/disable call in intel_audio.c. It's protected with the modeset lock as well.

Signed-off-by: Takashi Iwai tiwai@suse.de

v1->v2:

• Use modeset lock for get_eld lock, drop av mutex
• Return the expected size from get_eld, not the copied size

drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_component.h | 6 ++++++ 3 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 0c38cc6c82ae..1965a61769ea 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -521,6 +521,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)

connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;

• intel_dig_port->audio_enabled = true; if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, adjusted_mode);

@@ -545,6 +546,7 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder) struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); enum port port = intel_dig_port->port;

• intel_dig_port->audio_enabled = false; if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(intel_encoder);

@@ -702,6 +704,43 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, return 0; }

+static int i915_audio_component_get_eld(struct device *dev, int port,

• 			bool *enabled,
  

• 			unsigned char *buf, int max_bytes)
  

+{

• struct drm_i915_private *dev_priv = dev_to_i915(dev);
• struct drm_device *drm_dev = dev_priv->dev;
• struct intel_encoder *intel_encoder;
• struct intel_digital_port *intel_dig_port;
• struct drm_connector *connector;
• unsigned char *eld;
• int ret = -EINVAL;
• drm_modeset_lock_all(drm_dev);

This is super expensive and shouldn't ever be used in new code. So either just the connection_mutex or resurrect the av_mutex and just cache what you need under that.

OK, I need to make it harder, then.

...

Tbh I prefer the separate lock + cache for such specific things since it completely avoids spreading and entangling locking contexts. We use the same design to get modeset information into the PSR tracking, FBC tracking and other code which sits between KMS and other subsystems.

I didn't want to be involved with the modeset lock, but it has to be. This function calls drm_select_eld() and it requires both mode_config.mutex and connection_mutex.

(snip)

...

...

struct i915_audio_component_audio_ops { @@ -55,6 +58,9 @@ struct i915_audio_component_audio_ops { * pin sense and/or ELD information has changed. * @audio_ptr: HDA driver object * @port: Which port has changed (PORTA / PORTB / PORTC etc)

• *
  

• * Note that you can't call i915_audio_component_ops.get_eld directly
  

• * from the notifier callback as it may lead to deadlocks.
  

With av_mutex we don't even need that note here ;-)

So here is the problem. av_mutex itself doesn't suffice for drm_select_eld(), and taking the modeset lock leads to a deadlock when invoked from eld_notify.

Yeah, select_eld is broken currently in atomic land, and we need to fix that. It's by far not the only one that's iffy.

Maybe one alternative is to pass the audio state and ELD bytes already in eld_notify itself. Then it doesn't have to call get_eld from there. But we still need the explicit fetch in some cases (at the first probe and at resume), so get_eld op is still required. Then it needs to take locks by itself.

Well my idea was that in the enable/disable hooks (where we should hold relevant modeset locks already, except for that icky unsolved thing I need to take care of anyway), and store a copy (protected by av_lock). Then get_eld would only look at that copy. That kind of "cache relevant data, protected with new leaf lock" trick is what I meant we should use here, and it's the usual approach to avoid acquiring modeset locks from random other subsystems (since that ends in deadlocks sooner or later). So no calling drm_get_eld from the new get_eld hook at all.

There's still the problem that currently calling drm_get_eld is broken with atomic modesets even in i915 audio enable/disable functions. But that's a preexisting problem with atomic, and one I know we need to fix still before we can enable atomic for real (all legacy paths get away since there we take more locks).

Cheers, Daniel

On Fri, Dec 04, 2015 at 04:15:24PM +0100, Takashi Iwai wrote:

On Fri, 04 Dec 2015 16:00:46 +0100, Daniel Vetter wrote:

...

On Fri, Dec 04, 2015 at 11:49:46AM +0100, Takashi Iwai wrote:

...

On Fri, 04 Dec 2015 11:21:02 +0100, Daniel Vetter wrote:

...

On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote:

...

Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current audio status and ELD of the given HDMI/DP port. It returns the size of expected ELD bytes if it's valid, zero if no valid ELD is found, or a negative error code. The current state of audio on/off is stored in the given pointer, too.

Note that the returned size isn't limited to the given max bytes. If the size is greater than the max bytes, it means that only a part of ELD has been copied back.

A big warning about the usage of this callback is: you must not call it from eld_notify. The eld_notify itself is called in the modeset lock, and it leads to a deadlock since get_eld takes the modeset lock, too. You need to call get_eld in a work, for example, in such a case. We'll see the actual implementation in the later patch in sound/pci/hda/patch_hdmi.c.

For achieving this implementation, a new field audio_enabled is added to struct intel_digital_port. This is set/reset at each audio enable/disable call in intel_audio.c. It's protected with the modeset lock as well.

Signed-off-by: Takashi Iwai tiwai@suse.de

v1->v2:

• Use modeset lock for get_eld lock, drop av mutex
• Return the expected size from get_eld, not the copied size

drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_component.h | 6 ++++++ 3 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 0c38cc6c82ae..1965a61769ea 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -521,6 +521,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)

connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;

• intel_dig_port->audio_enabled = true; if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, adjusted_mode);

@@ -545,6 +546,7 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder) struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); enum port port = intel_dig_port->port;

• intel_dig_port->audio_enabled = false; if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(intel_encoder);

@@ -702,6 +704,43 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, return 0; }

+static int i915_audio_component_get_eld(struct device *dev, int port,

• 			bool *enabled,
  

• 			unsigned char *buf, int max_bytes)
  

+{

• struct drm_i915_private *dev_priv = dev_to_i915(dev);
• struct drm_device *drm_dev = dev_priv->dev;
• struct intel_encoder *intel_encoder;
• struct intel_digital_port *intel_dig_port;
• struct drm_connector *connector;
• unsigned char *eld;
• int ret = -EINVAL;
• drm_modeset_lock_all(drm_dev);

This is super expensive and shouldn't ever be used in new code. So either just the connection_mutex or resurrect the av_mutex and just cache what you need under that.

OK, I need to make it harder, then.

...

Tbh I prefer the separate lock + cache for such specific things since it completely avoids spreading and entangling locking contexts. We use the same design to get modeset information into the PSR tracking, FBC tracking and other code which sits between KMS and other subsystems.

I didn't want to be involved with the modeset lock, but it has to be. This function calls drm_select_eld() and it requires both mode_config.mutex and connection_mutex.

(snip)

...

...

struct i915_audio_component_audio_ops { @@ -55,6 +58,9 @@ struct i915_audio_component_audio_ops { * pin sense and/or ELD information has changed. * @audio_ptr: HDA driver object * @port: Which port has changed (PORTA / PORTB / PORTC etc)

• *
  

• * Note that you can't call i915_audio_component_ops.get_eld directly
  

• * from the notifier callback as it may lead to deadlocks.
  

With av_mutex we don't even need that note here ;-)

So here is the problem. av_mutex itself doesn't suffice for drm_select_eld(), and taking the modeset lock leads to a deadlock when invoked from eld_notify.

Yeah, select_eld is broken currently in atomic land, and we need to fix that. It's by far not the only one that's iffy.

...

Maybe one alternative is to pass the audio state and ELD bytes already in eld_notify itself. Then it doesn't have to call get_eld from there. But we still need the explicit fetch in some cases (at the first probe and at resume), so get_eld op is still required. Then it needs to take locks by itself.

Well my idea was that in the enable/disable hooks (where we should hold relevant modeset locks already, except for that icky unsolved thing I need to take care of anyway), and store a copy (protected by av_lock). Then get_eld would only look at that copy. That kind of "cache relevant data, protected with new leaf lock" trick is what I meant we should use here, and it's the usual approach to avoid acquiring modeset locks from random other subsystems (since that ends in deadlocks sooner or later). So no calling drm_get_eld from the new get_eld hook at all.

There's still the problem that currently calling drm_get_eld is broken with atomic modesets even in i915 audio enable/disable functions. But that's a preexisting problem with atomic, and one I know we need to fix still before we can enable atomic for real (all legacy paths get away since there we take more locks).

While I'm coding it, I wonder whether we really need to cache/copy the whole ELD content again. Wouldn't tracking the drm_connector point work? If the connector might be removed / updated, then it involves with the modeset updates, and calling audio_codec_disable() in anyway.

So for context with atomic the locking completely splits the display configuration from output detection. Display config is protected by a pile of wait/wound mutexes (abstracted a bit with drm_modeset_lock.c), output probing is protected by dev->mode_config.mutex.

Now in the compute config phase we need to inspect probe state to figure out what we should do (stuff like enabling audio), but after that the configuration should stay static until userspace asks for an update. Otherwise it will just end up in confusion.

My idea to fix this all is to track all this stuff (so including has_audio and the eld and whatever else we need) in the atomic state structures. And set up a bunch of _get functions (for use in compute config hooks) and _set functions (for updating probed state) with internal locking. We really need to do this copying, otherwise we'll always run int fun stuff with the configuration changing underneath us, or just leaking of locks from one side to the other, rendering the fine-grained locking a bit pointless.

So in the end there'll be 2 copies: probe -> modeset code, and the one you added here which copies modeset code -> audio code. It looks a bit silly, but imo it's the simplest solution and should be easy to add locking asserts to _get and _set functions to make sure they're always called in the right context.

FWIW, the below is the draft version I finished rewriting now (just compile-tested).

One question below.

thanks,

Takashi

-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] drm/i915: Add get_eld audio component

Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current audio status and ELD of the given HDMI/DP port. It returns the size of expected ELD bytes if it's valid, zero if no valid ELD is found, or a negative error code. The current state of audio on/off is stored in the given pointer, too.

Note that the returned size isn't limited to the given max bytes. If the size is greater than the max bytes, it means that only a part of ELD has been copied back.

For achieving this implementation, a new field audio_connector is added to struct intel_digital_port. It points to the connector assigned to the given digital port. It's set/reset at each audio enable/disable call in intel_audio.c, and protected with av_mutex.

Signed-off-by: Takashi Iwai tiwai@suse.de

drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ include/drm/i915_component.h | 3 +++ 3 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 4dccd9b003a1..7f45e73f58f5 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -525,6 +525,10 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder) dev_priv->display.audio_codec_enable(connector, intel_encoder, adjusted_mode);

• mutex_lock(&dev_priv->av_mutex);
• intel_dig_port->audio_connector = connector;
• mutex_unlock(&dev_priv->av_mutex);
• if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify) acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);

} @@ -548,6 +552,10 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder) if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(intel_encoder);

• mutex_lock(&dev_priv->av_mutex);
• intel_dig_port->audio_connector = NULL;
• mutex_unlock(&dev_priv->av_mutex);
• if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify) acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);

} @@ -706,6 +714,39 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, return 0; }

+static int i915_audio_component_get_eld(struct device *dev, int port,

• 			bool *enabled,
  

• 			unsigned char *buf, int max_bytes)
  

+{

• struct drm_i915_private *dev_priv = dev_to_i915(dev);
• struct drm_device *drm_dev = dev_priv->dev;
• struct intel_encoder *intel_encoder;
• struct intel_digital_port *intel_dig_port;
• const u8 *eld;
• int ret = -EINVAL;
• mutex_lock(&dev_priv->av_mutex);
• for_each_intel_encoder(drm_dev, intel_encoder) {

I guess the loop will dissipate with your cleanup patch to have a audio prot -> intel_dig_port mapping?

lgtm otherwise. -Daniel

• if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&
  

•     intel_encoder->type != INTEL_OUTPUT_HDMI)
  

• 	continue;
  

• intel_dig_port = enc_to_dig_port(&intel_encoder->base);
  

• if (port == intel_dig_port->port) {
  

• 	ret = 0;
  

• 	*enabled = intel_dig_port->audio_connector != NULL;
  

• 	if (!*enabled)
  

• 		break;
  

• 	eld = intel_dig_port->audio_connector->eld;
  

• 	ret = drm_eld_size(eld);
  

• 	memcpy(buf, eld, min(max_bytes, ret));
  

• 	break;
  

• }
  

• }
• mutex_unlock(&dev_priv->av_mutex);
• return ret;

+}

static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, @@ -713,6 +754,7 @@ static const struct i915_audio_component_ops i915_audio_component_ops = { .codec_wake_override = i915_audio_component_codec_wake_override, .get_cdclk_freq = i915_audio_component_get_cdclk_freq, .sync_audio_rate = i915_audio_component_sync_audio_rate,

• .get_eld = i915_audio_component_get_eld,

};

static int i915_audio_component_bind(struct device *i915_dev, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0598932ce623..607922e4937c 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -800,6 +800,8 @@ struct intel_digital_port { struct intel_hdmi hdmi; enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool); bool release_cl2_override;

• /* for communication with audio component; protected by av_mutex */
• const struct drm_connector *audio_connector;

};

struct intel_dp_mst_encoder { diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@

• @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

• • @get_eld: fill the audio state and ELD bytes for the given port
  */

struct i915_audio_component_ops { struct module *owner; @@ -46,6 +47,8 @@ struct i915_audio_component_ops { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); int (*sync_audio_rate)(struct device *, int port, int rate);

• int (*get_eld)(struct device *, int port, bool *enabled,

•        unsigned char *buf, int max_bytes);
  

};

struct i915_audio_component_audio_ops {

2.6.3

On Fri, Dec 04, 2015 at 04:15:24PM +0100, Takashi Iwai wrote:

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@

• @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

• • @get_eld: fill the audio state and ELD bytes for the given port

One more: You seem to still be on an old baseline, with switched to the new in-line comment layout. That allows you to spec the callback semantics in much more detail since it allows real paragraphs. -Daniel

*/ struct i915_audio_component_ops { struct module *owner; @@ -46,6 +47,8 @@ struct i915_audio_component_ops { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); int (*sync_audio_rate)(struct device *, int port, int rate);

• int (*get_eld)(struct device *, int port, bool *enabled,

•        unsigned char *buf, int max_bytes);
  

};

struct i915_audio_component_audio_ops {

2.6.3

On Fri, Dec 04, 2015 at 05:03:55PM +0100, Takashi Iwai wrote:

On Fri, 04 Dec 2015 16:54:32 +0100, Daniel Vetter wrote:

...

On Fri, Dec 04, 2015 at 04:15:24PM +0100, Takashi Iwai wrote:

...

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@

• @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

• • @get_eld: fill the audio state and ELD bytes for the given port

One more: You seem to still be on an old baseline, with switched to the new in-line comment layout. That allows you to spec the callback semantics in much more detail since it allows real paragraphs.

Yes, I've been waiting for your (or Dave's) answer to my previous question: which branch can I use as a solid base?

Ooops sorry. drm-next is now open and has everything you need.

git://people.freedesktop.org/~airlied/linux drm-next

Cheers, Daniel

On Fri, 04 Dec 2015 17:20:15 +0100, Takashi Iwai wrote:

On Fri, 04 Dec 2015 17:15:59 +0100, Daniel Vetter wrote:

...

On Fri, Dec 04, 2015 at 05:03:55PM +0100, Takashi Iwai wrote:

...

On Fri, 04 Dec 2015 16:54:32 +0100, Daniel Vetter wrote:

...

On Fri, Dec 04, 2015 at 04:15:24PM +0100, Takashi Iwai wrote:

...

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@

• @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

• • @get_eld: fill the audio state and ELD bytes for the given port

One more: You seem to still be on an old baseline, with switched to the new in-line comment layout. That allows you to spec the callback semantics in much more detail since it allows real paragraphs.

Yes, I've been waiting for your (or Dave's) answer to my previous question: which branch can I use as a solid base?

Ooops sorry. drm-next is now open and has everything you need.

git://people.freedesktop.org/~airlied/linux drm-next

Thanks, I'll rebase on it.

Hmm, this branch gives a compile warning:

drivers/gpu/drm/i915/intel_display.c:5217:0: warning: "for_each_power_domain" redefined #define for_each_power_domain(domain, mask) \ ^ In file included from drivers/gpu/drm/i915/intel_drv.h:32:0, from drivers/gpu/drm/i915/intel_display.c:36: drivers/gpu/drm/i915/i915_drv.h:312:0: note: this is the location of the previous definition #define for_each_power_domain(domain, mask) \ ^ LD [M] drivers/gpu/drm/i915/i915.o

Takashi

On Fri, 04 Dec 2015 17:49:43 +0100, Daniel Vetter wrote:

On Fri, Dec 04, 2015 at 05:27:12PM +0100, Takashi Iwai wrote:

...

On Fri, 04 Dec 2015 17:20:15 +0100, Takashi Iwai wrote:

...

On Fri, 04 Dec 2015 17:15:59 +0100, Daniel Vetter wrote:

...

On Fri, Dec 04, 2015 at 05:03:55PM +0100, Takashi Iwai wrote:

...

On Fri, 04 Dec 2015 16:54:32 +0100, Daniel Vetter wrote:

...

On Fri, Dec 04, 2015 at 04:15:24PM +0100, Takashi Iwai wrote: > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h > index 30d89e0da2c6..058d39e8d57f 100644 > --- a/include/drm/i915_component.h > +++ b/include/drm/i915_component.h > @@ -38,6 +38,7 @@ > * @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 > + * @get_eld: fill the audio state and ELD bytes for the given port

One more: You seem to still be on an old baseline, with switched to the new in-line comment layout. That allows you to spec the callback semantics in much more detail since it allows real paragraphs.

Yes, I've been waiting for your (or Dave's) answer to my previous question: which branch can I use as a solid base?

Ooops sorry. drm-next is now open and has everything you need.

git://people.freedesktop.org/~airlied/linux drm-next

Thanks, I'll rebase on it.

Hmm, this branch gives a compile warning:

drivers/gpu/drm/i915/intel_display.c:5217:0: warning: "for_each_power_domain" redefined #define for_each_power_domain(domain, mask) \ ^ In file included from drivers/gpu/drm/i915/intel_drv.h:32:0, from drivers/gpu/drm/i915/intel_display.c:36: drivers/gpu/drm/i915/i915_drv.h:312:0: note: this is the location of the previous definition #define for_each_power_domain(domain, mask) \ ^ LD [M] drivers/gpu/drm/i915/i915.o

Hilarious merge fail on my side, the patch to fix it up is queued in drm-intel.git. I'll send a pull request for that to Dave end of next week or so.

Alright. Meanwhile I rebased the patchset, and it's enough for review, at least. I can rebase again at the final merge time to the fixed branch.

thanks,

Takashi

On Fri, Dec 04, 2015 at 03:40:03PM +0100, Takashi Iwai wrote:

On Tue, 01 Dec 2015 17:09:53 +0100, Takashi Iwai wrote:

...

This patch adds a reverse mapping from a digital port number to intel_encoder object containing the corresponding intel_digital_port. It simplifies the query of the encoder a lot.

While this is good for a code reduction, I guess it's better to leave away for now, as there will be more changes there for MST support. It may put yet another loop, and the mapping implemented here might not be the best way. So I'm going to drop this from the next patchset.

If anyone still thinks this is worth to include, let me know. I'll re-add this.

Yeah I think this looks good. Fixing up conflicts with MST shouldn't be that much trouble really. -Daniel

thanks,

Takashi

...

Signed-off-by: Takashi Iwai tiwai@suse.de

drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_audio.c | 22 ++-------------------- drivers/gpu/drm/i915/intel_ddi.c | 1 + drivers/gpu/drm/i915/intel_dp.c | 1 + drivers/gpu/drm/i915/intel_hdmi.c | 2 ++ 5 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 95bb27de774f..3483d8125eac 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1955,6 +1955,8 @@ struct drm_i915_private { /* perform PHY state sanity checks? */ bool chv_phy_assert[2];

• struct intel_encoder *dig_port_map[I915_MAX_PORTS];
• /*
  • NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
  • will be rejected. Instead look for a better place.

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 19958bdb9bd0..67ee99f00ddd 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -630,28 +630,10 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }

-static struct intel_encoder *audio_port_to_encoder(struct drm_device *drm_dev,

• 				   int port)
  

-{

• struct intel_encoder *intel_encoder;
• struct intel_digital_port *intel_dig_port;
• for_each_intel_encoder(drm_dev, intel_encoder) {

• if (intel_encoder->type != INTEL_OUTPUT_HDMI &&
  

•     intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT)
  

• 	continue;
  

• intel_dig_port = enc_to_dig_port(&intel_encoder->base);
  

• if (port == intel_dig_port->port)
  

• 	return intel_encoder;
  

• }
• return NULL;

-}

static int i915_audio_component_sync_audio_rate(struct device *dev, int port, int rate) { struct drm_i915_private *dev_priv = dev_to_i915(dev);

• struct drm_device *drm_dev = dev_priv->dev; struct intel_encoder *intel_encoder; struct intel_crtc *crtc; struct drm_display_mode *mode;

@@ -668,7 +650,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,

mutex_lock(&dev_priv->av_mutex); /* 1. get the pipe */

• intel_encoder = audio_port_to_encoder(drm_dev, port);

• intel_encoder = dev_priv->dig_port_map[port]; if (!intel_encoder || intel_encoder->type != INTEL_OUTPUT_HDMI) { DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port)); mutex_unlock(&dev_priv->av_mutex);

@@ -725,7 +707,7 @@ static int i915_audio_component_get_eld(struct device *dev, int port, int ret = -EINVAL;

drm_modeset_lock_all(drm_dev);

• intel_encoder = audio_port_to_encoder(drm_dev, port);

• intel_encoder = dev_priv->dig_port_map[port]; if (intel_encoder) { ret = 0; intel_dig_port = enc_to_dig_port(&intel_encoder->base);

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index a6752a61d99f..6770110a4075 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3285,6 +3285,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port) intel_encoder->get_config = intel_ddi_get_config;

intel_dig_port->port = port;

• dev_priv->dig_port_map[port] = intel_encoder; intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) & (DDI_BUF_PORT_REVERSAL | DDI_A_4_LANES);

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 09bdd94ca3ba..1c02c6466f30 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -6201,6 +6201,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) }

intel_dig_port->port = port;

• dev_priv->dig_port_map[port] = intel_encoder; intel_dig_port->dp.output_reg = output_reg;

  intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 9eafa191cee2..1d9f522a6679 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -2133,6 +2133,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,

void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port) {

• struct drm_i915_private *dev_priv = dev->dev_private; struct intel_digital_port *intel_dig_port; struct intel_encoder *intel_encoder; struct intel_connector *intel_connector;

@@ -2201,6 +2202,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port) intel_encoder->cloneable |= 1 << INTEL_OUTPUT_HDMI;

intel_dig_port->port = port;

• dev_priv->dig_port_map[port] = intel_encoder; intel_dig_port->hdmi.hdmi_reg = hdmi_reg; intel_dig_port->dp.output_reg = 0;

-- 2.6.3

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

On Tue, 01 Dec 2015 17:09:57 +0100, Takashi Iwai wrote:

The ELD notification can be received asynchronously from the graphics side, and this may happen just at the moment the sound driver is processing the suspend or the resume, and it would confuse the whole procedure. Since the ELD and connection states are updated in anyway at the end of the resume, we can skip it when received during PM process.

Signed-off-by: Takashi Iwai tiwai@suse.de

I applied this particular patch now to for-next branch, as it's basically irrelevant with others.

The rest still needs review.

thanks,

Takashi

---

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 4dc21ecf7230..75815acb77db 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2450,6 +2450,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) */ if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0) return;

• /* ditto during suspend/resume process itself */

• if (atomic_read(&(codec)->core.in_pm))

• return;
  
  

  /* execute ELD update in a work for avoiding a deadlock */ per_pin = get_pin(spec, pin_idx);

-- 2.6.3