diff mbox

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

Message ID 1439974138-3296-3-git-send-email-david.henningsson@canonical.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Henningsson Aug. 19, 2015, 8:48 a.m. UTC 
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(-)

Comments

Jani Nikula Aug. 28, 2015, 1:07 p.m. UTC | #1 
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
>
David Henningsson Aug. 28, 2015, 1:15 p.m. UTC | #2 
On 2015-08-28 15:07, Jani Nikula wrote:
> 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?

Thanks for the review(s). I'm happy to add a 
drm_modeset_lock_all/drm_modeset_unlock_all - I'm not that familiar with 
the graphics side, really, so, is there another option...?

>
> 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
>>
>
diff mbox

Patch

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);
 }
 
 /**
@@ -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 = {