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.
(snip)
> > struct i915_audio_component_audio_ops {
> > @@ -55,6 +58,9 @@ struct i915_audio_component_audio_ops {
> > * pin sense and/or ELD information has changed.
> > * @audio_ptr: HDA driver object
> > * @port: Which port has changed (PORTA / PORTB / PORTC etc)
> > + *
> > + * Note that you can't call i915_audio_component_ops.get_eld directly
> > + * from the notifier callback as it may lead to deadlocks.
>
> With av_mutex we don't even need that note here ;-)
So here is the problem. av_mutex itself doesn't suffice for
drm_select_eld(), and taking the modeset lock leads to a deadlock when
invoked from eld_notify.
Maybe one alternative is to pass the audio state and ELD bytes already
in eld_notify itself. Then it doesn't have to call get_eld from
there. But we still need the explicit fetch in some cases (at the
first probe and at resume), so get_eld op is still required. Then it
needs to take locks by itself.
thanks,
Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
