[alsa-devel] [Intel-gfx] [PATCH v2 2/9] drm/i915: Add get_eld audio component

[Takashi Iwai]

On Fri, 04 Dec 2015 16:53:02 +0100,
Daniel Vetter wrote:
> 
> On Fri, Dec 04, 2015 at 04:15:24PM +0100, Takashi Iwai wrote:
> > On Fri, 04 Dec 2015 16:00:46 +0100,
> > Daniel Vetter 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 at 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.
> > > 
> > > Yeah, select_eld is broken currently in atomic land, and we need to fix
> > > that. It's by far not the only one that's iffy.
> > > 
> > > > 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.
> > > 
> > > Well my idea was that in the enable/disable hooks (where we should hold
> > > relevant modeset locks already, except for that icky unsolved thing I need
> > > to take care of anyway), and store a copy (protected by av_lock). Then
> > > get_eld would only look at that copy. That kind of "cache relevant data,
> > > protected with new leaf lock" trick is what I meant we should use here,
> > > and it's the usual approach to avoid acquiring modeset locks from random
> > > other subsystems (since that ends in deadlocks sooner or later). So no
> > > calling drm_get_eld from the new get_eld hook at all.
> > > 
> > > There's still the problem that currently calling drm_get_eld is broken
> > > with atomic modesets even in i915 audio enable/disable functions. But
> > > that's a preexisting problem with atomic, and one I know we need to fix
> > > still before we can enable atomic for real (all legacy paths get away
> > > since there we take more locks).
> > 
> > While I'm coding it, I wonder whether we really need to cache/copy the
> > whole ELD content again.  Wouldn't tracking the drm_connector point
> > work?  If the connector might be removed / updated, then it involves
> > with the modeset updates, and calling audio_codec_disable() in
> > anyway.
> 
> So for context with atomic the locking completely splits the display
> configuration from output detection. Display config is protected by a pile
> of wait/wound mutexes (abstracted a bit with drm_modeset_lock.c), output
> probing is protected by dev->mode_config.mutex.
> 
> Now in the compute config phase we need to inspect probe state to figure
> out what we should do (stuff like enabling audio), but after that the
> configuration should stay static until userspace asks for an update.
> Otherwise it will just end up in confusion.
 
OK.

> My idea to fix this all is to track all this stuff (so including has_audio
> and the eld and whatever else we need) in the atomic state structures. And
> set up a bunch of _get functions (for use in compute config hooks) and
> _set functions (for updating probed state) with internal locking. We
> really need to do this copying, otherwise we'll always run int fun stuff
> with the configuration changing underneath us, or just leaking of locks
> from one side to the other, rendering the fine-grained locking a bit
> pointless.
> 
> So in the end there'll be 2 copies: probe -> modeset code, and the one you
> added here which copies modeset code -> audio code. It looks a bit silly,
> but imo it's the simplest solution and should be easy to add locking
> asserts to _get and _set functions to make sure they're always called in
> the right context.

Yeah, syncing all these is a really hard job.  Keeping self-contained
is a safer design.


> > FWIW, the below is the draft version I finished rewriting now
> > (just compile-tested).
> 
> One question below.
(snip)
> > +	mutex_lock(&dev_priv->av_mutex);
> > +	for_each_intel_encoder(drm_dev, intel_encoder) {
> 
> I guess the loop will dissipate with your cleanup patch to have a audio
> prot -> intel_dig_port mapping?

Right.  It'll be simplified by the next patch.
I'm going to submit the v3 patchset.

Thanks!


Takashi