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

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

[libin.yang]

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
+ */
 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;
 	}
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915/audio: Extend audio sync rate support for DP MST

[libin.yang]

From: Libin Yang <libin.yang@intel.com>

This patch extends the support of audio sample rate
sync up to DP MST.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 drivers/gpu/drm/i915/intel_audio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index c8a1345..88ed869 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -807,7 +807,8 @@ static int i915_audio_component_sync_audio_rate(struct device *kdev, int port,
 	intel_encoder = get_saved_enc(dev_priv, port, pipe);
 	if (!intel_encoder || !intel_encoder->base.crtc ||
 	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
-	     intel_encoder->type != INTEL_OUTPUT_DP)) {
+	     intel_encoder->type != INTEL_OUTPUT_DP &&
+		 intel_encoder->type != INTEL_OUTPUT_DP_MST)) {
 		DRM_DEBUG_KMS("Not valid for port %c\n", port_name(port));
 		err = -ENODEV;
 		goto unlock;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/audio: extend get_saved_enc() to support more scenarios

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/audio: extend get_saved_enc() to support more scenarios
URL   : https://patchwork.freedesktop.org/series/15321/
State : success

== Summary ==

Series 15321v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/15321/revisions/1/mbox/


fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

2f21978cfd8984c79e4cbd77ce63d9f73fe226ef drm-intel-nightly: 2016y-11m-14d-21h-23m-10s UTC integration manifest
de72750 drm/i915/audio: Extend audio sync rate support for DP MST
06279ec drm/i915/audio: extend get_saved_enc() to support more scenarios

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2995/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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

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?

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Yang, Libin]

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

>
>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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for DP MST

[Jani Nikula]

On Tue, 15 Nov 2016, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
>
> This patch extends the support of audio sample rate
> sync up to DP MST.
>
> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index c8a1345..88ed869 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -807,7 +807,8 @@ static int i915_audio_component_sync_audio_rate(struct device *kdev, int port,
>  	intel_encoder = get_saved_enc(dev_priv, port, pipe);
>  	if (!intel_encoder || !intel_encoder->base.crtc ||
>  	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
> -	     intel_encoder->type != INTEL_OUTPUT_DP)) {
> +	     intel_encoder->type != INTEL_OUTPUT_DP &&
> +		 intel_encoder->type != INTEL_OUTPUT_DP_MST)) {

I think the better option is to make absolutely sure we never store
other kinds of encoders in dev_priv->av_enc_map[pipe] to begin with. I
think that's true already, but please add

	if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI &&
		    intel_encoder->type != INTEL_OUTPUT_DP &&
		    intel_encoder->type != INTEL_OUTPUT_DP_MST))
        	return;

near the beginning of intel_audio_codec_enable(), and remove the type
checks here. This reduces the confusion about different kinds of checks
after calling get_saved_enc().

BR,
Jani.


>  		DRM_DEBUG_KMS("Not valid for port %c\n", port_name(port));
>  		err = -ENODEV;
>  		goto unlock;

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

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

[Jani Nikula]

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

Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for DP MST

[Ville Syrjälä]

On Tue, Nov 29, 2016 at 06:33:50PM +0200, Jani Nikula wrote:
> On Tue, 15 Nov 2016, libin.yang@intel.com wrote:
> > From: Libin Yang <libin.yang@intel.com>
> >
> > This patch extends the support of audio sample rate
> > sync up to DP MST.
> >
> > Signed-off-by: Libin Yang <libin.yang@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index c8a1345..88ed869 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -807,7 +807,8 @@ static int i915_audio_component_sync_audio_rate(struct device *kdev, int port,
> >  	intel_encoder = get_saved_enc(dev_priv, port, pipe);
> >  	if (!intel_encoder || !intel_encoder->base.crtc ||
> >  	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
> > -	     intel_encoder->type != INTEL_OUTPUT_DP)) {
> > +	     intel_encoder->type != INTEL_OUTPUT_DP &&
> > +		 intel_encoder->type != INTEL_OUTPUT_DP_MST)) {
> 
> I think the better option is to make absolutely sure we never store
> other kinds of encoders in dev_priv->av_enc_map[pipe] to begin with. I
> think that's true already, but please add
> 
> 	if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI &&
> 		    intel_encoder->type != INTEL_OUTPUT_DP &&
> 		    intel_encoder->type != INTEL_OUTPUT_DP_MST))
>         	return;
> 
> near the beginning of intel_audio_codec_enable(), and remove the type
> checks here. This reduces the confusion about different kinds of checks
> after calling get_saved_enc().

This while encoder->type thing is pretty broken actually. Currently
we're semi-randomly flipping DDI encoders between UNKNOWN,HDMI and DP.
I want to eliminate that so that their type will always be just "DDI".
For iguring out what kind of signal we are driving out you should be
looking at crtc_state->output_types.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for DP MST

[Yang, Libin]

>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>Sent: Wednesday, November 30, 2016 12:34 AM
>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>
>Subject: Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for
>DP MST
>
>On Tue, 15 Nov 2016, libin.yang@intel.com wrote:
>> From: Libin Yang <libin.yang@intel.com>
>>
>> This patch extends the support of audio sample rate sync up to DP MST.
>>
>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_audio.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> b/drivers/gpu/drm/i915/intel_audio.c
>> index c8a1345..88ed869 100644
>> --- a/drivers/gpu/drm/i915/intel_audio.c
>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>> @@ -807,7 +807,8 @@ static int
>i915_audio_component_sync_audio_rate(struct device *kdev, int port,
>>  	intel_encoder = get_saved_enc(dev_priv, port, pipe);
>>  	if (!intel_encoder || !intel_encoder->base.crtc ||
>>  	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
>> -	     intel_encoder->type != INTEL_OUTPUT_DP)) {
>> +	     intel_encoder->type != INTEL_OUTPUT_DP &&
>> +		 intel_encoder->type != INTEL_OUTPUT_DP_MST)) {
>
>I think the better option is to make absolutely sure we never store other kinds
>of encoders in dev_priv->av_enc_map[pipe] to begin with. I think that's true
>already, but please add
>
>	if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI &&
>		    intel_encoder->type != INTEL_OUTPUT_DP &&
>		    intel_encoder->type != INTEL_OUTPUT_DP_MST))
>        	return;
>
>near the beginning of intel_audio_codec_enable(), and remove the type
>checks here. This reduces the confusion about different kinds of checks after
>calling get_saved_enc().

We can't put the check at the beginning of the function.
We still need the check in the same place as we need get intel_encoder at first.

Regards,
Libin

>
>BR,
>Jani.
>
>
>>  		DRM_DEBUG_KMS("Not valid for port %c\n",
>port_name(port));
>>  		err = -ENODEV;
>>  		goto unlock;
>
>--
>Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for DP MST

[Yang, Libin]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Wednesday, November 30, 2016 12:50 AM
>To: Jani Nikula <jani.nikula@linux.intel.com>
>Cc: Yang, Libin <libin.yang@intel.com>; intel-gfx@lists.freedesktop.org; Vetter,
>Daniel <daniel.vetter@intel.com>; Pandiyan, Dhinakaran
><dhinakaran.pandiyan@intel.com>; Kp, Jeeja <jeeja.kp@intel.com>;
>tiwai@suse.de
>Subject: Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for
>DP MST
>
>On Tue, Nov 29, 2016 at 06:33:50PM +0200, Jani Nikula wrote:
>> On Tue, 15 Nov 2016, libin.yang@intel.com wrote:
>> > From: Libin Yang <libin.yang@intel.com>
>> >
>> > This patch extends the support of audio sample rate sync up to DP
>> > MST.
>> >
>> > Signed-off-by: Libin Yang <libin.yang@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_audio.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> > b/drivers/gpu/drm/i915/intel_audio.c
>> > index c8a1345..88ed869 100644
>> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> > @@ -807,7 +807,8 @@ static int
>i915_audio_component_sync_audio_rate(struct device *kdev, int port,
>> >  	intel_encoder = get_saved_enc(dev_priv, port, pipe);
>> >  	if (!intel_encoder || !intel_encoder->base.crtc ||
>> >  	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
>> > -	     intel_encoder->type != INTEL_OUTPUT_DP)) {
>> > +	     intel_encoder->type != INTEL_OUTPUT_DP &&
>> > +		 intel_encoder->type != INTEL_OUTPUT_DP_MST)) {
>>
>> I think the better option is to make absolutely sure we never store
>> other kinds of encoders in dev_priv->av_enc_map[pipe] to begin with. I
>> think that's true already, but please add
>>
>> 	if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI &&
>> 		    intel_encoder->type != INTEL_OUTPUT_DP &&
>> 		    intel_encoder->type != INTEL_OUTPUT_DP_MST))
>>         	return;
>>
>> near the beginning of intel_audio_codec_enable(), and remove the type
>> checks here. This reduces the confusion about different kinds of
>> checks after calling get_saved_enc().
>
>This while encoder->type thing is pretty broken actually. Currently we're semi-
>randomly flipping DDI encoders between UNKNOWN,HDMI and DP.
>I want to eliminate that so that their type will always be just "DDI".
>For iguring out what kind of signal we are driving out you should be looking at
>crtc_state->output_types.

Do you mean the code should be like:
@@ -807,10 +807,7 @@ static int i915_audio_component_sync_audio_rate(struct device *kdev, int port,
 
 	/* 1. get the pipe */
 	intel_encoder = get_saved_enc(dev_priv, port, pipe);
-	if (!intel_encoder || !intel_encoder->base.crtc ||
-	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
-	     intel_encoder->type != INTEL_OUTPUT_DP &&
-		 intel_encoder->type != INTEL_OUTPUT_DP_MST)) {
+	if (!intel_encoder || !intel_encoder->base.crtc) {
 		DRM_DEBUG_KMS("Not valid for port %c\n", port_name(port));
 		err = -ENODEV;
 		goto unlock;
@@ -819,7 +816,12 @@ static int i915_audio_component_sync_audio_rate(struct device *kdev, int port,
 	/* pipe passed from the audio driver will be -1 for Non-MST case */
 	crtc = to_intel_crtc(intel_encoder->base.crtc);
 	crtc_state = crtc->config;
+	if (WARN_ON((crtc_state->output_types &
+		((1 << INTEL_OUTPUT_HDMI) |
+		 (1 << INTEL_OUTPUT_DP) |
+		 (1 << INTEL_OUTPUT_DP_MST))) == 0))
+		return 0;
+
 	pipe = crtc->pipe;

I did the test, and it seems intel_encoder->type can show the correct type 
on my platform. It is similar with crtc_state->output_types.

Regards,
Libin

>
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for DP MST

[Jani Nikula]

On Wed, 30 Nov 2016, "Yang, Libin" <libin.yang@intel.com> wrote:
>>-----Original Message-----
>>From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>>Sent: Wednesday, November 30, 2016 12:34 AM
>>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>
>>Subject: Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for
>>DP MST
>>
>>On Tue, 15 Nov 2016, libin.yang@intel.com wrote:
>>> From: Libin Yang <libin.yang@intel.com>
>>>
>>> This patch extends the support of audio sample rate sync up to DP MST.
>>>
>>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_audio.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c
>>> b/drivers/gpu/drm/i915/intel_audio.c
>>> index c8a1345..88ed869 100644
>>> --- a/drivers/gpu/drm/i915/intel_audio.c
>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>>> @@ -807,7 +807,8 @@ static int
>>i915_audio_component_sync_audio_rate(struct device *kdev, int port,
>>>  	intel_encoder = get_saved_enc(dev_priv, port, pipe);
>>>  	if (!intel_encoder || !intel_encoder->base.crtc ||
>>>  	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
>>> -	     intel_encoder->type != INTEL_OUTPUT_DP)) {
>>> +	     intel_encoder->type != INTEL_OUTPUT_DP &&
>>> +		 intel_encoder->type != INTEL_OUTPUT_DP_MST)) {
>>
>>I think the better option is to make absolutely sure we never store other kinds
>>of encoders in dev_priv->av_enc_map[pipe] to begin with. I think that's true
>>already, but please add
>>
>>	if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI &&
>>		    intel_encoder->type != INTEL_OUTPUT_DP &&
>>		    intel_encoder->type != INTEL_OUTPUT_DP_MST))
>>        	return;
>>
>>near the beginning of intel_audio_codec_enable(), and remove the type
>>checks here. This reduces the confusion about different kinds of checks after
>>calling get_saved_enc().
>
> We can't put the check at the beginning of the function.
> We still need the check in the same place as we need get intel_encoder at first.

We do not need to duplicate the checks here if we ensure only good stuff
is ever put into av_enc_map.

>
> Regards,
> Libin
>
>>
>>BR,
>>Jani.
>>
>>
>>>  		DRM_DEBUG_KMS("Not valid for port %c\n",
>>port_name(port));
>>>  		err = -ENODEV;
>>>  		goto unlock;
>>
>>--
>>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

Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for DP MST

[Yang, Libin]

>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>Sent: Wednesday, November 30, 2016 11:52 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
>Subject: RE: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for
>DP MST
>
>On Wed, 30 Nov 2016, "Yang, Libin" <libin.yang@intel.com> wrote:
>>>-----Original Message-----
>>>From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>>>Sent: Wednesday, November 30, 2016 12:34 AM
>>>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>
>>>Subject: Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate
>>>support for DP MST
>>>
>>>On Tue, 15 Nov 2016, libin.yang@intel.com wrote:
>>>> From: Libin Yang <libin.yang@intel.com>
>>>>
>>>> This patch extends the support of audio sample rate sync up to DP MST.
>>>>
>>>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_audio.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c
>>>> b/drivers/gpu/drm/i915/intel_audio.c
>>>> index c8a1345..88ed869 100644
>>>> --- a/drivers/gpu/drm/i915/intel_audio.c
>>>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>>>> @@ -807,7 +807,8 @@ static int
>>>i915_audio_component_sync_audio_rate(struct device *kdev, int port,
>>>>  	intel_encoder = get_saved_enc(dev_priv, port, pipe);
>>>>  	if (!intel_encoder || !intel_encoder->base.crtc ||
>>>>  	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
>>>> -	     intel_encoder->type != INTEL_OUTPUT_DP)) {
>>>> +	     intel_encoder->type != INTEL_OUTPUT_DP &&
>>>> +		 intel_encoder->type != INTEL_OUTPUT_DP_MST)) {
>>>
>>>I think the better option is to make absolutely sure we never store
>>>other kinds of encoders in dev_priv->av_enc_map[pipe] to begin with. I
>>>think that's true already, but please add
>>>
>>>	if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI &&
>>>		    intel_encoder->type != INTEL_OUTPUT_DP &&
>>>		    intel_encoder->type != INTEL_OUTPUT_DP_MST))
>>>        	return;
>>>
>>>near the beginning of intel_audio_codec_enable(), and remove the type
>>>checks here. This reduces the confusion about different kinds of
>>>checks after calling get_saved_enc().
>>
>> We can't put the check at the beginning of the function.
>> We still need the check in the same place as we need get intel_encoder at
>first.
>
>We do not need to duplicate the checks here if we ensure only good stuff is
>ever put into av_enc_map.

Yes, I agree. I will remove the type judgement.

Regards,
Libin

>
>>
>> Regards,
>> Libin
>>
>>>
>>>BR,
>>>Jani.
>>>
>>>
>>>>  		DRM_DEBUG_KMS("Not valid for port %c\n",
>>>port_name(port));
>>>>  		err = -ENODEV;
>>>>  		goto unlock;
>>>
>>>--
>>>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

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

[libin.yang]

From: Libin Yang <libin.yang@linux.intel.com>

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 requires to
set the pipe to the correct value, otherwise, pipe to be -1.

Although audio driver can get the knowledge whether it is in DP MST mode
or not by reading the codec register. It will drop performance each time
before it calls the get_eld and other similar functions. As gfx driver can
easily know whether it is in DP MST mode or not. Let's extend the
get_saved_enc() function to handle the situation that audio driver
still sends the device id info even it is in DP SST mode and return
the correct intel_encoder instead of panic.

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
+ */
 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;
 	}
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx