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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 29 Nov 2016, "Yang, Libin" <libin.yang@intel.com> wrote:
>>-----Original Message-----
>>From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>>Sent: Friday, November 25, 2016 10:46 PM
>>To: Yang, Libin <libin.yang@intel.com>; intel-gfx@lists.freedesktop.org;
>>ville.syrjala@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>;
>>Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>; Kp, Jeeja
>><jeeja.kp@intel.com>; tiwai@suse.de
>>Cc: Yang, Libin <libin.yang@intel.com>; Libin Yang <libin.yang@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@intel.com wrote:
>>> From: Libin Yang <libin.yang@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@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.

Please describe that in the commit message and/or comments in the patch.

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

I think addressing my review comment on patch 2 will help this.

BR,
Jani.

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

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