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

From: David Henningsson <david.henningsson@canonical.com>
To: Jani Nikula <jani.nikula@linux.intel.com>,
	alsa-devel@alsa-project.org, intel-gfx@lists.freedesktop.org,
	vinod.koul@intel.com, daniel.vetter@intel.com, tiwai@suse.de,
	libin.yang@intel.com
Subject: Re: [PATCH 2/4] drm/i915: Call audio pin/ELD notify function
Date: Fri, 28 Aug 2015 15:15:30 +0200	[thread overview]
Message-ID: <55E05EF2.2040708@canonical.com> (raw)
In-Reply-To: <87si735zos.fsf@intel.com>

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

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx