[Intel-gfx] [PATCH 1/2] drm/i915/audio: extend get_saved_enc() to support more scenarios

[Yang, Libin]

>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
>Sent: Friday, November 25, 2016 10:46 PM
>To: Yang, Libin <libin.yang at intel.com>; intel-gfx at lists.freedesktop.org;
>ville.syrjala at linux.intel.com; Vetter, Daniel <daniel.vetter at intel.com>;
>Pandiyan, Dhinakaran <dhinakaran.pandiyan at intel.com>; Kp, Jeeja
><jeeja.kp at intel.com>; tiwai at suse.de
>Cc: Yang, Libin <libin.yang at intel.com>; Libin Yang <libin.yang at linux.intel.com>
>Subject: Re: [PATCH 1/2] drm/i915/audio: extend get_saved_enc() to support
>more scenarios
>
>On Tue, 15 Nov 2016, libin.yang at intel.com wrote:
>> From: Libin Yang <libin.yang at linux.intel.com>
>>
>> When bootup, audio driver may not know it is MST or not. The audio
>> driver will poll all the port & pipe combinations in either MST or
>> Non-MST mode. get_saved_enc() should handle this situation.
>>
>> Signed-off-by: Libin Yang <libin.yang at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_audio.c | 32
>> ++++++++++++++++++++++++++++----
>>  1 file changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> b/drivers/gpu/drm/i915/intel_audio.c
>> index 49f1053..c8a1345 100644
>> --- a/drivers/gpu/drm/i915/intel_audio.c
>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>> @@ -737,25 +737,49 @@ static int
>i915_audio_component_get_cdclk_freq(struct device *kdev)
>>  	return dev_priv->cdclk_freq;
>>  }
>>
>> +/*
>> + * get the intel_encoder according to the parameter port and pipe
>> + * intel_encoder is saved by the index of pipe
>> + * MST & (pipe >= 0): return the av_enc_map[pipe],
>> + *   when port is matched
>> + * MST & (pipe < 0): this is invalid
>> + * Non-MST & (pipe >= 0): only pipe = 0 (the first device entry)
>> + *   will get the right intel_encoder with port matched
>> + * Non-MST & (pipe < 0): get the right intel_encoder with port
>> +matched
>
>This essentially describes the code in English. I can read the code. I need to
>know more about the higher level of what this does and *why*.

In initialization, audio driver will call functions get_eld() and etc. But
at that time, audio driver may not know whether it is DP MST or not.
In the original function get_saved_enc(), if it is DP MST, it will set the pipe
to the correct value, otherwise, it will be set -1.

Although audio driver can get the knowledge whether it is in DP MST 
mode or not by reading the codec register. But it will drop performance
each time it call the get_eld and other functions. As gfx driver can easily
know whether it is in DP MST mode or not. I would like to extend the
get_saved_enc() function to handle the situation that audio driver still
send the device id info even it is in DP mode. In this situation, 
get_saved_enc() will return the correct intel_encoder instead of panic.

>
>I also look at the call sites of get_saved_enc and wonder why they have
>different checks for the return values. Also patch 2/2 only modifies one of
>them, not both. Why?

The 2 check should be similar? In sync_audio_rate(), it calls get_saved_enc()
and checked base.crtc, I think it is because it will use base.crtc later.

And it will check the intel_encoder->type because this function only need
handle HDMI/DP/DP MST situation to setup  the audio config.

Regards,
Libin

>
>So maybe I'm just too clueless about DP (MST) audio in general, and I
>wouldn't even get offended if you told me that. But I don't think the commit
>message and the comments in this patch properly help me either.
>
>BR,
>Jani.
>
>
>> + */
>>  static struct intel_encoder *get_saved_enc(struct drm_i915_private
>*dev_priv,
>>  					       int port, int pipe)
>>  {
>> +	struct intel_encoder *encoder;
>>
>>  	if (WARN_ON(pipe >= I915_MAX_PIPES))
>>  		return NULL;
>>
>>  	/* MST */
>> -	if (pipe >= 0)
>> -		return dev_priv->av_enc_map[pipe];
>> +	if (pipe >= 0) {
>> +		encoder = dev_priv->av_enc_map[pipe];
>> +		/*
>> +		 * when bootup, audio driver may not know it is
>> +		 * MST or not. So it will poll all the port & pipe
>> +		 * combinations
>> +		 */
>> +		if (encoder != NULL && encoder->port == port &&
>> +		    encoder->type == INTEL_OUTPUT_DP_MST)
>> +			return encoder;
>> +	}
>>
>>  	/* Non-MST */
>> -	for_each_pipe(dev_priv, pipe) {
>> -		struct intel_encoder *encoder;
>> +	if (pipe > 0)
>> +		return NULL;
>>
>> +	for_each_pipe(dev_priv, pipe) {
>>  		encoder = dev_priv->av_enc_map[pipe];
>>  		if (encoder == NULL)
>>  			continue;
>>
>> +		if (encoder->type == INTEL_OUTPUT_DP_MST)
>> +			continue;
>> +
>>  		if (port == encoder->port)
>>  			return encoder;
>>  	}
>
>--
>Jani Nikula, Intel Open Source Technology Center