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

[Takashi Iwai <tiwai@suse.de>]

On Mon, 30 Nov 2015 15:11:16 +0100,
Daniel Vetter 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.
> > 
> > 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;
> > +
> 
> Locking is a bit in-flux atm with atomic, but needs
> dev_priv->dev->mode_config->mutex here to protect against the eld
> changing.

Noted, I'll add it in the next respin.

> > +	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)
> 
> MST? Not that I have a clue how that should work ;-)

Well, I supposed that MST entry doesn't actually serve for the digital
port, but I'm not entirely sure.  In anyway, the whole MST audio
support shall be added / handled by other Intel people soon later, so
let's keep this as is -- which is the same condition as the current
i915 code.

> > +			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));
> 
> How can the caller figure out what the eld size is if you use min here? At
> least in drm we just return the size we want if it's too small.

A good point.  The function should return the size "to be written
all", indeed.

> > +			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;
> 
> Needs a comment that this is protected by dev_priv->av_mutex.

OK.

> >  	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
> 
> In 4.4 kerneldoc supports extended in-line comments for struct members
> like this:
> 
> >   */
> >  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);
> 
> 	/**
> 	 * @get_eld:
> 	 *
> 	 * lots of blabla, possibly in multiple paragraphs.
> 	 */
> 
> Please use that layout and put a copy of the more detailed description you
> put into the commit message of how ->get_eld exactly works.
> 
> I did ask for this to get done as part of some of the previous, and it was
> partially done but not exactly how kerneldoc wants it :( But I think we
> need to start somewhere ...

Yeah, it looks like that I missed the back-merge of 4.4-rc although I
thought I did.  Will rebase and rewrite that.


thanks,

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