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@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
