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

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Takashi Iwai <tiwai@suse.de>, Libin Yang <libin.yang@linux.intel.com>
Cc: alsa-devel@alsa-project.org, intel-gfx@lists.freedesktop.org,
	Dave Airlie <airlied@redhat.com>,
	daniel.vetter@intel.com
Subject: Re: [PATCH V4 2/2] drm/i915: start adding dp mst audio
Date: Mon, 14 Dec 2015 11:33:44 +0200	[thread overview]
Message-ID: <87io41v0g7.fsf@intel.com> (raw)
In-Reply-To: <s5h6105l0yg.wl-tiwai@suse.de>

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.

BR,
Jani.

>
>
> thanks,
>
> Takashi

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx