[Intel-gfx] [PATCH 2/7] drm/i915: Add get_eld audio component

[Daniel Vetter]

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

> +	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 ;-)

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

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

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

Cheers, Daniel


> +	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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch