Re: [alsa-devel] [PATCH V4 2/2] drm/i915: start adding dp mst audio

From: Libin Yang <libin.yang@linux.intel.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org, intel-gfx@lists.freedesktop.org,
	daniel.vetter@intel.com, Dave Airlie <airlied@redhat.com>
Subject: Re: [alsa-devel] [PATCH V4 2/2] drm/i915: start adding dp mst audio
Date: Mon, 21 Dec 2015 16:31:03 +0800	[thread overview]
Message-ID: <5677B8C7.3090008@linux.intel.com> (raw)
In-Reply-To: <s5hy4d1jdcu.wl-tiwai@suse.de>

Sorry, I missed the emails.

On 12/11/2015 09:58 PM, Takashi Iwai wrote:
> On Fri, 11 Dec 2015 11:43:51 +0100,
> Takashi Iwai wrote:
>>
>> On Fri, 11 Dec 2015 07:07:53 +0100,
>> Libin Yang wrote:
>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
>>>>>> index 9aa83e7..5ad2e66 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_audio.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>>>>>> @@ -262,7 +262,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder)
>>>>>>    	tmp |= AUD_CONFIG_N_PROG_ENABLE;
>>>>>>    	tmp &= ~AUD_CONFIG_UPPER_N_MASK;
>>>>>>    	tmp &= ~AUD_CONFIG_LOWER_N_MASK;
>>>>>> -	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
>>>>>> +	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) ||
>>>>>> +	    intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST))
>>>>>>    		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>>
>> The same check is missing in hsw_audio_codec_enable()?
>>
>>>>>>    	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>>>>>>
>>>>>> @@ -474,7 +475,8 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>>>>>>    	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>>>>>>    	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>>>>>>    	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
>>>>>> -	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
>>>>>> +	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) ||
>>>>>> +	    intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST))
>>>>>>    		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>>
>> ... and missing for ilk_audio_codec_disable()?

Based on the discussion, I think you and Jani are right. It should be 
in hsw_audio_codec_enable() and not in ild_audio_codec_disable().

>>
>>
>>>>>>    	else
>>>>>>    		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
>>>>>> @@ -512,7 +514,8 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>>>>>>
>>>>>>    	/* ELD Conn_Type */
>>>>>>    	connector->eld[5] &= ~(3 << 2);
>>>>>> -	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT))
>>>>>> +	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) ||
>>>>>> +	    intel_pipe_has_type(crtc, INTEL_OUTPUT_DP_MST))
>>
>> IMO, it's better to have a macro to cover this two-line check instead
>> of open-coding at each place.  We'll have 5 places in the end.

OK, I will use a macro.

>
> Also, this patch still has an issue about the encoder type, namely, it
> passes intel_encoder from MST, where you can't apply
> enc_to_dig_port().  We need another help to get the digital port
> depending on the encoder type, e.g.
>
> static struct intel_digital_port *
> intel_encoder_to_dig_port(struct intel_encoder *intel_encoder)
> {
> 	struct drm_encoder *encoder = &intel_encoder->base;
>
> 	if (intel_encoder->type == INTEL_OUTPUT_DP_MST)
> 		return enc_to_mst(encoder)->primary;
> 	return enc_to_dig_port(encoder);
> }

Yes, I will fix it in the new version patch.

Best Regards,
Libin

>
>
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx