[v2,2/9] drm/i915: Add get_eld audio component

Submitted by Takashi Iwai on Dec. 1, 2015, 4:09 p.m.

Details

Message ID 1448986198-3488-3-git-send-email-tiwai@suse.de
State New
Headers show
Series "Add get_eld audio component for i915/HD-audio" ( rev: 2 ) in Intel GFX

Commit Message

Takashi Iwai Dec. 1, 2015, 4:09 p.m.
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(+)

Patch hide | download patch | download mbox

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);
+	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.
 	 */
 	void (*pin_eld_notify)(void *audio_ptr, int port);
 };

Comments

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, 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.

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.


thanks,

Takashi
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, 04 Dec 2015 13:10:01 +0100,
Ville Syrjälä 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.
> 
> 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.

Yeah, this is another idea that came to my mind during lunch, too, and
already started coding it ;)


thanks,

Takashi
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, 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.

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


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(+)
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, 04 Dec 2015 16:53:02 +0100,
Daniel Vetter wrote:
> 
> 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.
 
OK.

> 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.

Yeah, syncing all these is a really hard job.  Keeping self-contained
is a safer design.


> > FWIW, the below is the draft version I finished rewriting now
> > (just compile-tested).
> 
> One question below.
(snip)
> > +	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?

Right.  It'll be simplified by the next patch.
I'm going to submit the v3 patchset.

Thanks!


Takashi
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?


Takashi
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: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.


Takashi
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, 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.
-Daniel
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