Re: [PATCH 2/7] drm/i915: Add get_eld audio component

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 30, 2015 at 05:24:41PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> > Implement a new i915_audio_component_ops, get_eld().  It's called by
> > the audio driver to fetch the current ELD of the given HDMI/DP port.
> > It returns the size of ELD bytes if it's valid, or zero if the audio
> > is disabled or unplugged, or a negative error code.
> 
> Why do we need this? Isn't it something the eld notify hook should
> pass from i915 to the audio driver?
> 
> At least with the locking you have for this, the audio driver can not
> call this from the eld notify hook since it would deadlock.

Hmm. Actually the locking isn't perhaps quite like that atm. But I guess
the mode_config.mutex will make it so.

Apart from that it seesm to me that you should pull the av_mutex
lock/unlock from the .audio_code_eanble/disable hooks into
intel_audio_codec_enable/disable, so that it protects the audio_enabled
flag as well. Not sure if the eld_notify should be called while holding
that lock or not. If we need to avoid calling it from the eld_notify
anywya due to other locks then maybe it can be under av_mutex as well.

> 
> > 
> > For achieving this, 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.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> >  include/drm/i915_component.h       |  3 +++
> >  3 files changed, 44 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 0c38cc6c82ae..6b318a8d5dc9 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;
> > +
> > +	mutex_lock(&dev_priv->av_mutex);
> > +	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 = min(max_bytes, drm_eld_size(eld));
> > +			memcpy(buf, eld, 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,
> > @@ -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..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
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx