[1/2] drm/i915/dp: there is no audio on port A on non-DDI platforms
diff mbox

Message ID 1430749249-15565-1-git-send-email-jani.nikula@intel.com
State New
Headers show

Commit Message

Jani Nikula May 4, 2015, 2:20 p.m. UTC
The eDP port A register on PCH split platforms has a slightly different
register layout from the other ports, with bit 6 being either alternate
scrambler reset or reserved, depending on the generation. Our
misinterpretation of the bit as audio has lead to warnings.

Fix this by not enabling audio on port A on non-DDI platforms. The check
could be made more specific, but this corresponds to the audio codec
enabling code which emits the warning for port A.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89958
Reported-and-tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Sivakumar Thulasimani May 5, 2015, 5:31 a.m. UTC | #1
On 5/4/2015 7:50 PM, Jani Nikula wrote:
> The eDP port A register on PCH split platforms has a slightly different
> register layout from the other ports, with bit 6 being either alternate
> scrambler reset or reserved, depending on the generation. Our
> misinterpretation of the bit as audio has lead to warnings.
>
> Fix this by not enabling audio on port A on non-DDI platforms. The check
> could be made more specific, but this corresponds to the audio codec
> enabling code which emits the warning for port A.
shouldn't we disable audio for eDP on all platforms , including DDI ones ?
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89958
> Reported-and-tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 937ba31d8dde..fa71fb4d6542 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1355,7 +1355,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>   
>   	pipe_config->has_dp_encoder = true;
>   	pipe_config->has_drrs = false;
> -	pipe_config->has_audio = intel_dp->has_audio;
> +	if (HAS_DDI(dev) || port != PORT_A)
> +		pipe_config->has_audio = intel_dp->has_audio;
>   
this still enables audio for PORT_A on for HSW/BDW/etc .  the following 
is better IMO

+	if (!is_edp(encoder))
+		pipe_config->has_audio = intel_dp->has_audio;
Jani Nikula May 5, 2015, 9:09 a.m. UTC | #2
On Tue, 05 May 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> On 5/4/2015 7:50 PM, Jani Nikula wrote:
>> The eDP port A register on PCH split platforms has a slightly different
>> register layout from the other ports, with bit 6 being either alternate
>> scrambler reset or reserved, depending on the generation. Our
>> misinterpretation of the bit as audio has lead to warnings.
>>
>> Fix this by not enabling audio on port A on non-DDI platforms. The check
>> could be made more specific, but this corresponds to the audio codec
>> enabling code which emits the warning for port A.
> shouldn't we disable audio for eDP on all platforms , including DDI ones ?
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89958
>> Reported-and-tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 937ba31d8dde..fa71fb4d6542 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1355,7 +1355,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>   
>>   	pipe_config->has_dp_encoder = true;
>>   	pipe_config->has_drrs = false;
>> -	pipe_config->has_audio = intel_dp->has_audio;
>> +	if (HAS_DDI(dev) || port != PORT_A)
>> +		pipe_config->has_audio = intel_dp->has_audio;
>>   
> this still enables audio for PORT_A on for HSW/BDW/etc .  the following 
> is better IMO
>
> +	if (!is_edp(encoder))
> +		pipe_config->has_audio = intel_dp->has_audio;

I wanted the check to match what's in intel_audio.c for these
platforms. And those only need the pipe.

Perhaps it's a theoretical question, but is it impossible for edp to
have audio?

Jani.




>
>
>
> -- 
> regards,
> Sivakumar
>
Sivakumar Thulasimani May 5, 2015, 12:18 p.m. UTC | #3
two points
1) The eDP spec says Audio is optional so it is allowed to have audio, 
but i am yet to come across any eDP panel that supports Audio.
2) Also, there is no support for audio in DDI A port as well :)

So please change the check to

if (!is_edp(encoder))

On 5/5/2015 2:39 PM, Jani Nikula wrote:
> On Tue, 05 May 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> On 5/4/2015 7:50 PM, Jani Nikula wrote:
>>> The eDP port A register on PCH split platforms has a slightly different
>>> register layout from the other ports, with bit 6 being either alternate
>>> scrambler reset or reserved, depending on the generation. Our
>>> misinterpretation of the bit as audio has lead to warnings.
>>>
>>> Fix this by not enabling audio on port A on non-DDI platforms. The check
>>> could be made more specific, but this corresponds to the audio codec
>>> enabling code which emits the warning for port A.
>> shouldn't we disable audio for eDP on all platforms , including DDI ones ?
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89958
>>> Reported-and-tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_dp.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 937ba31d8dde..fa71fb4d6542 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -1355,7 +1355,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>    
>>>    	pipe_config->has_dp_encoder = true;
>>>    	pipe_config->has_drrs = false;
>>> -	pipe_config->has_audio = intel_dp->has_audio;
>>> +	if (HAS_DDI(dev) || port != PORT_A)
>>> +		pipe_config->has_audio = intel_dp->has_audio;
>>>    
>> this still enables audio for PORT_A on for HSW/BDW/etc .  the following
>> is better IMO
>>
>> +	if (!is_edp(encoder))
>> +		pipe_config->has_audio = intel_dp->has_audio;
> I wanted the check to match what's in intel_audio.c for these
> platforms. And those only need the pipe.
>
> Perhaps it's a theoretical question, but is it impossible for edp to
> have audio?
>
> Jani.
>
>
>
>
>>
>>
>> -- 
>> regards,
>> Sivakumar
>>

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 937ba31d8dde..fa71fb4d6542 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1355,7 +1355,8 @@  intel_dp_compute_config(struct intel_encoder *encoder,
 
 	pipe_config->has_dp_encoder = true;
 	pipe_config->has_drrs = false;
-	pipe_config->has_audio = intel_dp->has_audio;
+	if (HAS_DDI(dev) || port != PORT_A)
+		pipe_config->has_audio = intel_dp->has_audio;
 
 	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
 		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
@@ -2228,7 +2229,7 @@  static void intel_dp_get_config(struct intel_encoder *encoder,
 	int dotclock;
 
 	tmp = I915_READ(intel_dp->output_reg);
-	if (tmp & DP_AUDIO_OUTPUT_ENABLE)
+	if (port != PORT_A && tmp & DP_AUDIO_OUTPUT_ENABLE)
 		pipe_config->has_audio = true;
 
 	if ((port == PORT_A) || !HAS_PCH_CPT(dev)) {