On Fri, 04 Dec 2015 13:10:01 +0100, Ville Syrjälä wrote: > > On Fri, Dec 04, 2015 at 11:49:46AM +0100, Takashi Iwai wrote: > > On Fri, 04 Dec 2015 11:21:02 +0100, > > Daniel Vetter wrote: > > > > > > On Tue, Dec 01, 2015 at 05:09:51PM +0100, Takashi Iwai wrote: > > > > Implement a new i915_audio_component_ops, get_eld(). It's called by > > > > the audio driver to fetch the current audio status and ELD of the > > > > given HDMI/DP port. It returns the size of expected ELD bytes if it's > > > > valid, zero if no valid ELD is found, or a negative error code. The > > > > current state of audio on/off is stored in the given pointer, too. > > > > > > > > Note that the returned size isn't limited to the given max bytes. If > > > > the size is greater than the max bytes, it means that only a part of > > > > ELD has been copied back. > > > > > > > > A big warning about the usage of this callback is: you must not call > > > > it from eld_notify. The eld_notify itself is called in the modeset > > > > lock, and it leads to a deadlock since get_eld takes the modeset lock, > > > > too. You need to call get_eld in a work, for example, in such a case. > > > > We'll see the actual implementation in the later patch in > > > > sound/pci/hda/patch_hdmi.c. > > > > > > > > For achieving this implementation, 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. It's protected with the modeset > > > > lock as well. > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > > > > --- > > > > v1->v2: > > > > * Use modeset lock for get_eld lock, drop av mutex > > > > * Return the expected size from get_eld, not the copied size > > > > > > > > drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++ > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > include/drm/i915_component.h | 6 ++++++ > > > > 3 files changed, 47 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > > > > index 0c38cc6c82ae..1965a61769ea 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; > > > > + > > > > + drm_modeset_lock_all(drm_dev); > > > > > > This is super expensive and shouldn't ever be used in new code. So either > > > just the connection_mutex or resurrect the av_mutex and just cache what > > > you need under that. > > > > OK, I need to make it harder, then. > > > > > Tbh I prefer the separate lock + cache for such > > > specific things since it completely avoids spreading and entangling > > > locking contexts. We use the same design to get modeset information into > > > the PSR tracking, FBC tracking and other code which sits between KMS and > > > other subsystems. > > > > I didn't want to be involved with the modeset lock, but it has to be. > > This function calls drm_select_eld() and it requires both > > mode_config.mutex and connection_mutex. > > drm_select_eld() would seem pointless to me if we cached the > required information somewhere. But we'd still need to actually > get the eld, and that means either caching it again somewhere, > or perhaps it would be better to move the drm_edid_to_eld() call to > happen at modeset audio enable time protected by the av_mutex? > That way connector->eld contents would remain constant as long > as we have a mode set. Yeah, this is another idea that came to my mind during lunch, too, and already started coding it ;) thanks, Takashi _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx