| Message ID | 1448890671-2983-3-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Nov 30, 2015 at 03:11:16PM +0100, Daniel Vetter wrote: > On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote: > > 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 ... Strike that, I looked at the wrong tree ;-) linux-next should have all the patches using the new extended style. -Daniel
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
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. > > 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
On Mon, 30 Nov 2015 16:24:41 +0100, 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? We need this at least in two situations: - when the audio driver is probed - when the audio driver is resumed > 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. OK, then we may change the eld_notify to pass the values in the arguments, and also add the new op with a lock to be called from other places from other places. Takashi
On 2015-11-30 16:29, Takashi Iwai wrote: > On Mon, 30 Nov 2015 16:24:41 +0100, > 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? > > We need this at least in two situations: > - when the audio driver is probed > - when the audio driver is resumed > >> 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. > > OK, then we may change the eld_notify to pass the values in the > arguments, and also add the new op with a lock to be called from other > places from other places. Maybe we have to make eld_notify not do anything except to call schedule_work then? And that work in turn will ask for updated eld from the i915 driver, and handle the result.
On Mon, 30 Nov 2015 16:34:22 +0100, David Henningsson wrote: > > > > On 2015-11-30 16:29, Takashi Iwai wrote: > > On Mon, 30 Nov 2015 16:24:41 +0100, > > 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? > > > > We need this at least in two situations: > > - when the audio driver is probed > > - when the audio driver is resumed > > > >> 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. > > > > OK, then we may change the eld_notify to pass the values in the > > arguments, and also add the new op with a lock to be called from other > > places from other places. > > Maybe we have to make eld_notify not do anything except to call > schedule_work then? And that work in turn will ask for updated eld from > the i915 driver, and handle the result. Yes, it would work, too -- though, this would need a bit more code reorganization. Takashi
On Mon, 30 Nov 2015 15:17:05 +0100, Daniel Vetter wrote: > > On Mon, Nov 30, 2015 at 03:11:16PM +0100, Daniel Vetter wrote: > > On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote: > > > 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 ... > > Strike that, I looked at the wrong tree ;-) linux-next should have all the > patches using the new extended style. OK, so this is a post-4.4 change. I can rebase on it. Could you point a steady branch suitable for it? thanks, Takashi
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
On Mon, Nov 30, 2015 at 04:55:50PM +0100, Takashi Iwai wrote: > On Mon, 30 Nov 2015 15:17:05 +0100, > Daniel Vetter wrote: > > > > On Mon, Nov 30, 2015 at 03:11:16PM +0100, Daniel Vetter wrote: > > > On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote: > > > > 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 ... > > > > Strike that, I looked at the wrong tree ;-) linux-next should have all the > > patches using the new extended style. > > OK, so this is a post-4.4 change. I can rebase on it. Could you > point a steady branch suitable for it? Dave's drm-next would be it, if Dave opens it up and pulls in the 2 pull requests I've sent out earlier. Dave? -Daniel
On Mon, Nov 30, 2015 at 06:09:33PM +0200, Ville Syrjälä wrote: > 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. If we need this we could create a substruct in dev_priv with copies of everything, which would only be protected by av_mutex. That's the usual approach we use when faced with this kind of locking inversion: Copy/update relevant data in the modeset ->enable/disable hooks, then just use these local locks to access those copies. Usually that's enough to untangle things, with no need to resort to workers. -Daniel
On Mon, Nov 30, 2015 at 05:34:45PM +0100, Daniel Vetter wrote: > On Mon, Nov 30, 2015 at 06:09:33PM +0200, Ville Syrjälä wrote: > > 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. > > If we need this we could create a substruct in dev_priv with copies of > everything, which would only be protected by av_mutex. That's the usual > approach we use when faced with this kind of locking inversion: > Copy/update relevant data in the modeset ->enable/disable hooks, then just > use these local locks to access those copies. Usually that's enough to > untangle things, with no need to resort to workers. Yeah, IIRC I suggested just that originally.
On Mon, 30 Nov 2015 17:09:33 +0100, Ville Syrjälä wrote: > > 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. Currently I'm thinking of: - not allow to call get_eld directly from eld_notify; I found that the existing eld_repoll work in the HDA can be reused easily, so let's follow Daniel's advice. - drm_select_eld() seems requring mode_config.mutex and connection_mutex modeset lock in anyway; so let get_eld taking both. Is it OK to call drm_modeset_lock_all() for simplicity? - Get rid of av_mutex from get_eld instead; get_eld doesn't conflict with other ops In that way, audio_enabled flag is protected in both places via modeset lock, I suppose. thanks, Takashi
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 {
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(+)