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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx