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

From: David Henningsson <david.henningsson@canonical.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: alsa-devel@alsa-project.org, tiwai@suse.de,
	intel-gfx@lists.freedesktop.org, vinod.koul@intel.com,
	daniel.vetter@intel.com
Subject: Re: [PATCH 2/4] drm/i915: Call audio hotplug notify function
Date: Tue, 21 Jul 2015 16:26:17 +0200	[thread overview]
Message-ID: <55AE5689.1080901@canonical.com> (raw)
In-Reply-To: <20150721091457.GE16722@phenom.ffwll.local>

On 2015-07-21 11:14, Daniel Vetter wrote:
> On Tue, Jul 21, 2015 at 09:57:25AM +0200, David Henningsson wrote:
>> On HDMI hotplug events, notify the audio driver. This will enable
>> the audio driver to get the information at all times (even when
>> audio is in different powersave states), and also without reading
>> it from the hardware.
>>
>> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h    |    1 +
>>   drivers/gpu/drm/i915/intel_audio.c |   46 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 542fac6..696624c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1808,6 +1808,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..eb29e1f 100644
>> --- a/drivers/gpu/drm/i915/intel_audio.c
>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>> @@ -384,6 +384,44 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>>   	I915_WRITE(aud_config, tmp);
>>   }
>>
>> +static void audio_hotplug_notify(struct drm_i915_private *dev_priv,
>> +				 struct drm_connector *connector,
>> +				 struct intel_encoder *encoder,
>> +				 bool plugged_in)
>
> plugged_in is redundant, just check for NULL. Also just derive dev_priv
> from intel_encoder imo. And finally we tend to put the object that a
> function operates on first. Since this sends a notify out for the given
> encoder (well dig port really) I'd put that first.

Sure, will fix these and send out a v2 (after Takashi has reviewed, too).

> I also think that it would make sense to switch all the audio
> enable/disable functions from intel_encoder to intel_dig_port. A lot need
> to do upcasts anyway, and audio really only works on dig ports (which is
> for hdmi/dp only).

That's up to you - seems a bit out of scope for this patch set though, I 
assume.

Btw, there are two open questions though that you (or someone else) 
might be able to share some insight on:

  1) The mapping between ports and nid is fixed in patch 4/4, it would 
be good with an Ack from an Intel engineer on this mapping.

  2) Whether to use connector_type / connector_type_id combo or 
connector_name as identification - it seems like X has a different 
naming compared to the kernel ("HDMI2" in X vs "HDMI-A-2" in kernel), 
not sure what Wayland/Mir/others do here...

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