Re: [PATCH v2 2/9] drm/i915: Add get_eld audio component

[Takashi Iwai <tiwai@suse.de>]

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