On Mon, 14 Dec 2015 11:34:53 +0100, Jani Nikula wrote:
On Mon, 14 Dec 2015, Takashi Iwai tiwai@suse.de wrote:
On Mon, 14 Dec 2015 10:33:44 +0100, Jani Nikula wrote:
On Fri, 11 Dec 2015, Takashi Iwai tiwai@suse.de 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()?
>> 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.
I don't think that's necessary. Only the hsw_* functions need it, the other platforms (using the other functions) don't support DP MST.
Then the patch changes the functions. And still three places have this check.
Probably the mistake in the patch was to add the check to ilk_audio_codec_enable when it should have been added to hsw_audio_codec_enable.
Yeah, I wanted to write "the patch changes the wrong functions", too.
Takashi