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

[Takashi Iwai <tiwai@suse.de>]

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@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.
> 
> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel