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