Re: [PATCH 2/4] drm/i915: Call audio pin/ELD notify function

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 19 Aug 2015, David Henningsson <david.henningsson@canonical.com> wrote:
> When the audio codec is enabled or disabled, notify the audio driver.
> This will enable the audio driver to get the notification at all times
> (even when audio is in different powersave states).
>
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_audio.c | 23 ++++++++++++++++++++---
>  2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fd1de45..1fc327d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1809,6 +1809,7 @@ struct drm_i915_private {
>  	struct drm_property *force_audio_property;
>  
>  	/* hda/i915 audio component */
> +	struct i915_audio_component *audio_component;
>  	bool audio_component_registered;
>  
>  	uint32_t hw_context_size;
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 3da9b84..969835d 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -399,6 +399,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>  	struct drm_connector *connector;
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_audio_component *acomp = dev_priv->audio_component;
> +	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
> +	enum port port = intel_dig_port->port;
>  
>  	connector = drm_select_eld(encoder, mode);
>  	if (!connector)
> @@ -419,6 +422,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>  
>  	if (dev_priv->display.audio_codec_enable)
>  		dev_priv->display.audio_codec_enable(connector, intel_encoder, mode);
> +
> +	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
> +		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port, 0);
>  }
>  
>  /**
> @@ -428,13 +434,20 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>   * The disable sequences must be performed before disabling the transcoder or
>   * port.
>   */
> -void intel_audio_codec_disable(struct intel_encoder *encoder)
> +void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
>  {
> -	struct drm_device *dev = encoder->base.dev;
> +	struct drm_encoder *encoder = &intel_encoder->base;
> +	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_audio_component *acomp = dev_priv->audio_component;
> +	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
> +	enum port port = intel_dig_port->port;
>  
>  	if (dev_priv->display.audio_codec_disable)
> -		dev_priv->display.audio_codec_disable(encoder);
> +		dev_priv->display.audio_codec_disable(intel_encoder);
> +
> +	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
> +		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port, 0);

AFAICT this has a race with i915_audio_component_unbind, i.e. modeset
while hda is being unloaded might crash.

What do others think, is drm_modeset_lock_all in combonent bind/unbind
overkill?

With that resolved one way or another (I'm not dismissing "we don't need
to care") this is

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  }
>  
>  /**
> @@ -525,12 +538,14 @@ static int i915_audio_component_bind(struct device *i915_dev,
>  				     struct device *hda_dev, void *data)
>  {
>  	struct i915_audio_component *acomp = data;
> +	struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
>  
>  	if (WARN_ON(acomp->ops || acomp->dev))
>  		return -EEXIST;
>  
>  	acomp->ops = &i915_audio_component_ops;
>  	acomp->dev = i915_dev;
> +	dev_priv->audio_component = acomp;
>  
>  	return 0;
>  }
> @@ -539,9 +554,11 @@ static void i915_audio_component_unbind(struct device *i915_dev,
>  					struct device *hda_dev, void *data)
>  {
>  	struct i915_audio_component *acomp = data;
> +	struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
>  
>  	acomp->ops = NULL;
>  	acomp->dev = NULL;
> +	dev_priv->audio_component = NULL;
>  }
>  
>  static const struct component_ops i915_audio_component_bind_ops = {
> -- 
> 1.9.1
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx