drm/radeon: fix AVI infoframe generation

Commit Message

Alex Deucher June 7, 2013, 2:44 p.m. UTC

From: Alex Deucher <[e-mail]>

- remove adding 2 to checksum, this breaks certain monitors
- properly emit the AVI infoframe version, not emitting
the version breaks some monitors.

This should fix blank screen when HDMI audio is enabled on
certain monitors.

Signed-off-by: Alex Deucher <[e-mail]>
Cc: Rafa? Mi?ecki <[e-mail]>
---
 drivers/gpu/drm/radeon/evergreen_hdmi.c |   11 ++---------
 drivers/gpu/drm/radeon/r600_hdmi.c      |   11 ++---------
 2 files changed, 4 insertions(+), 18 deletions(-)

Comments

Rafał Miłecki June 8, 2013, 11:46 a.m. UTC | #1

2013/6/7  <[e-mail]>:
> From: Alex Deucher <[e-mail]>
>
> - remove adding 2 to checksum, this breaks certain monitors
> - properly emit the AVI infoframe version, not emitting
> the version breaks some monitors.
>
> This should fix blank screen when HDMI audio is enabled on
> certain monitors.

Err, nack. I believe this it actually going to *break* some monitors
compatibility.

For some unknown reason AMD hardware uses 0x81 and 0x01 for type and
version of infoframe. See this comment (written/published by you):
/* The following packets and infoframes are required for HDMI:
 * Packets:
 * 0x00 - Null packet
 * 0x01 - Audio clock regen
 * 0x02 - Audio sample
 * 0x03 - General Control
 * Infoframes:
 * 0x81 0x01 - AVI
 * 0x83 0x03 - audio
 */

As you can see, AMD hardware uses 0x81 and 0x01. I've no idea why,
according to the HDMI standard it should be 0x82 and 0x02. All other
vendors seems to also use 0x82 and 0x02. In function
hdmi_avi_infoframe_init type it set to 0x82 and 0x02.

This type and version it's written anywhere, but they are used to
calculate checksum. Checksum it what we store is frame[0x0]. We
calculate checksum as 0x100 - sum (see hdmi_infoframe_checksum).

Now... with common drm code we use 0x82 and 0x02 instead of 0x81 and
0x01. It means our "sum" is too big by 0x02. It means we have to use
0x100 - sum + 0x02 as checksum for AMD hardware.

This whole hack was introduced in 92db7f6c860b8190571a9dc1fcbc16d003422fe8:
drm/radeon/kms: workaround invalid AVI infoframe checksum issue
and I'm pretty sure it was verified to fix somebody's HDMI mode. It
also seems to be compatible with AMD specs (at least the part of it we
can see in the quoted comment).

For proof of fglrx doing the same, please see my e-mail where I
provided dumps from registers when using fglrx:
http://lists.freedesktop.org/archives/dri-devel/2011-December/017717.html
(it was tested on 5 different cards!).

When working on that patch I was talking & debugging with few people
on IRC. That mean I don't have logs anymore, but I'm sure I was
testing exactly that single value and it was immediately fixing
selected displays. I mean testing by writing something like:

avivotool regset 0x10884 0x00a85e4f
vs.
avivotool regset 0x10884 0x00a85e4d

Alex Deucher June 8, 2013, 4:04 p.m. UTC | #2

On Sat, Jun 8, 2013 at 7:46 AM, Rafa? Mi?ecki <[e-mail]> wrote:
> 2013/6/7  <[e-mail]>:
>> From: Alex Deucher <[e-mail]>
>>
>> - remove adding 2 to checksum, this breaks certain monitors
>> - properly emit the AVI infoframe version, not emitting
>> the version breaks some monitors.
>>
>> This should fix blank screen when HDMI audio is enabled on
>> certain monitors.
>
> Err, nack. I believe this it actually going to *break* some monitors
> compatibility.

Yeah, it seems to break as many as it fixes :(

>
> For some unknown reason AMD hardware uses 0x81 and 0x01 for type and
> version of infoframe. See this comment (written/published by you):
> /* The following packets and infoframes are required for HDMI:
>  * Packets:
>  * 0x00 - Null packet
>  * 0x01 - Audio clock regen
>  * 0x02 - Audio sample
>  * 0x03 - General Control
>  * Infoframes:
>  * 0x81 0x01 - AVI
>  * 0x83 0x03 - audio
>  */
>
> As you can see, AMD hardware uses 0x81 and 0x01. I've no idea why,
> according to the HDMI standard it should be 0x82 and 0x02. All other
> vendors seems to also use 0x82 and 0x02. In function
> hdmi_avi_infoframe_init type it set to 0x82 and 0x02.


The internal afmt validation code I've seen uses 0x82 and 0x2 (for
DCE4 and newer asics, I'm not sure what we do for older versions), so
I think it may depend on the monitor and the GPU hardware.  We may
need to add some logic to determine what version to send based on the
monitor's caps.

>
> This type and version it's written anywhere, but they are used to
> calculate checksum. Checksum it what we store is frame[0x0]. We
> calculate checksum as 0x100 - sum (see hdmi_infoframe_checksum).
>
> Now... with common drm code we use 0x82 and 0x02 instead of 0x81 and
> 0x01. It means our "sum" is too big by 0x02. It means we have to use
> 0x100 - sum + 0x02 as checksum for AMD hardware.
>

It would probably be cleaner to just adjust the header and version
before calling hdmi_avi_infoframe_pack() so we don't have to add the
hack.

Alex

> This whole hack was introduced in 92db7f6c860b8190571a9dc1fcbc16d003422fe8:
> drm/radeon/kms: workaround invalid AVI infoframe checksum issue
> and I'm pretty sure it was verified to fix somebody's HDMI mode. It
> also seems to be compatible with AMD specs (at least the part of it we
> can see in the quoted comment).
>
> For proof of fglrx doing the same, please see my e-mail where I
> provided dumps from registers when using fglrx:
> http://lists.freedesktop.org/archives/dri-devel/2011-December/017717.html
> (it was tested on 5 different cards!).
>
> When working on that patch I was talking & debugging with few people
> on IRC. That mean I don't have logs anymore, but I'm sure I was
> testing exactly that single value and it was immediately fixing
> selected displays. I mean testing by writing something like:
>
> avivotool regset 0x10884 0x00a85e4f
> vs.
> avivotool regset 0x10884 0x00a85e4d
>
> --
> Rafa?

Alex Deucher June 10, 2013, 3:13 p.m. UTC | #3

On Sat, Jun 8, 2013 at 7:46 AM, Rafa? Mi?ecki <[e-mail]> wrote:
> 2013/6/7  <[e-mail]>:
>> From: Alex Deucher <[e-mail]>
>>
>> - remove adding 2 to checksum, this breaks certain monitors
>> - properly emit the AVI infoframe version, not emitting
>> the version breaks some monitors.
>>
>> This should fix blank screen when HDMI audio is enabled on
>> certain monitors.
>
> Err, nack. I believe this it actually going to *break* some monitors
> compatibility.
>
> For some unknown reason AMD hardware uses 0x81 and 0x01 for type and
> version of infoframe. See this comment (written/published by you):
> /* The following packets and infoframes are required for HDMI:
>  * Packets:
>  * 0x00 - Null packet
>  * 0x01 - Audio clock regen
>  * 0x02 - Audio sample
>  * 0x03 - General Control
>  * Infoframes:
>  * 0x81 0x01 - AVI
>  * 0x83 0x03 - audio
>  */
>
> As you can see, AMD hardware uses 0x81 and 0x01. I've no idea why,
> according to the HDMI standard it should be 0x82 and 0x02. All other
> vendors seems to also use 0x82 and 0x02. In function
> hdmi_avi_infoframe_init type it set to 0x82 and 0x02.
>
> This type and version it's written anywhere, but they are used to
> calculate checksum. Checksum it what we store is frame[0x0]. We
> calculate checksum as 0x100 - sum (see hdmi_infoframe_checksum).
>
> Now... with common drm code we use 0x82 and 0x02 instead of 0x81 and
> 0x01. It means our "sum" is too big by 0x02. It means we have to use
> 0x100 - sum + 0x02 as checksum for AMD hardware.
>
> This whole hack was introduced in 92db7f6c860b8190571a9dc1fcbc16d003422fe8:
> drm/radeon/kms: workaround invalid AVI infoframe checksum issue
> and I'm pretty sure it was verified to fix somebody's HDMI mode. It
> also seems to be compatible with AMD specs (at least the part of it we
> can see in the quoted comment).
>
> For proof of fglrx doing the same, please see my e-mail where I
> provided dumps from registers when using fglrx:
> http://lists.freedesktop.org/archives/dri-devel/2011-December/017717.html
> (it was tested on 5 different cards!).

Actually, I think the patch is correct.  I think you were adding the
version twice:  From your examples:
[Rafa? Mi?ecki][RV620] fglrx:
0x7454: 00 A8 5E 79	R600_HDMI_VIDEOINFOFRAME_0
0x7458: 00 28 00 10	R600_HDMI_VIDEOINFOFRAME_1
0x745C: 00 48 00 28	R600_HDMI_VIDEOINFOFRAME_2
0x7460: 02 00 00 48	R600_HDMI_VIDEOINFOFRAME_3
===================
(0x82 + 0x2 + 0xD) + 0x1F8 = 0x289
-0x289 = 0x77

The payload sum is not 0x1f8, it's 0x1f6.
00 + A8 + 5E + 00 +
00 + 28 + 00 + 10 +
00 + 48 + 00 + 28 +
00 + 48 =
0x1f6

Bits 25:24 of HDMI_VIDEOINFOFRAME_3 are the packet version, not part
of the payload.  So the total would be:
(0x82 + 0x2 + 0xD) + 0x1f6 = 0x287
-0x287 = 0x79

I think the issue is that we are not setting the version in bits 25:24
of HDMI_VIDEOINFOFRAME_3 so we were sending a version 0 AVI packet.

Alex

>
> When working on that patch I was talking & debugging with few people
> on IRC. That mean I don't have logs anymore, but I'm sure I was
> testing exactly that single value and it was immediately fixing
> selected displays. I mean testing by writing something like:
>
> avivotool regset 0x10884 0x00a85e4f
> vs.
> avivotool regset 0x10884 0x00a85e4d
>
> --
> Rafa?

Rafał Miłecki June 28, 2013, 5:10 a.m. UTC | #4

2013/6/10 Alex Deucher <[e-mail]>:
> On Sat, Jun 8, 2013 at 7:46 AM, Rafa? Mi?ecki <[e-mail]> wrote:
>> 2013/6/7  <[e-mail]>:
>>> From: Alex Deucher <[e-mail]>
>>>
>>> - remove adding 2 to checksum, this breaks certain monitors
>>> - properly emit the AVI infoframe version, not emitting
>>> the version breaks some monitors.
>>>
>>> This should fix blank screen when HDMI audio is enabled on
>>> certain monitors.
>>
>> Err, nack. I believe this it actually going to *break* some monitors
>> compatibility.
>>
>> For some unknown reason AMD hardware uses 0x81 and 0x01 for type and
>> version of infoframe. See this comment (written/published by you):
>> /* The following packets and infoframes are required for HDMI:
>>  * Packets:
>>  * 0x00 - Null packet
>>  * 0x01 - Audio clock regen
>>  * 0x02 - Audio sample
>>  * 0x03 - General Control
>>  * Infoframes:
>>  * 0x81 0x01 - AVI
>>  * 0x83 0x03 - audio
>>  */
>>
>> As you can see, AMD hardware uses 0x81 and 0x01. I've no idea why,
>> according to the HDMI standard it should be 0x82 and 0x02. All other
>> vendors seems to also use 0x82 and 0x02. In function
>> hdmi_avi_infoframe_init type it set to 0x82 and 0x02.
>>
>> This type and version it's written anywhere, but they are used to
>> calculate checksum. Checksum it what we store is frame[0x0]. We
>> calculate checksum as 0x100 - sum (see hdmi_infoframe_checksum).
>>
>> Now... with common drm code we use 0x82 and 0x02 instead of 0x81 and
>> 0x01. It means our "sum" is too big by 0x02. It means we have to use
>> 0x100 - sum + 0x02 as checksum for AMD hardware.
>>
>> This whole hack was introduced in 92db7f6c860b8190571a9dc1fcbc16d003422fe8:
>> drm/radeon/kms: workaround invalid AVI infoframe checksum issue
>> and I'm pretty sure it was verified to fix somebody's HDMI mode. It
>> also seems to be compatible with AMD specs (at least the part of it we
>> can see in the quoted comment).
>>
>> For proof of fglrx doing the same, please see my e-mail where I
>> provided dumps from registers when using fglrx:
>> http://lists.freedesktop.org/archives/dri-devel/2011-December/017717.html
>> (it was tested on 5 different cards!).
>
> Actually, I think the patch is correct.  I think you were adding the
> version twice:  From your examples:
> [Rafa? Mi?ecki][RV620] fglrx:
> 0x7454: 00 A8 5E 79     R600_HDMI_VIDEOINFOFRAME_0
> 0x7458: 00 28 00 10     R600_HDMI_VIDEOINFOFRAME_1
> 0x745C: 00 48 00 28     R600_HDMI_VIDEOINFOFRAME_2
> 0x7460: 02 00 00 48     R600_HDMI_VIDEOINFOFRAME_3
> ===================
> (0x82 + 0x2 + 0xD) + 0x1F8 = 0x289
> -0x289 = 0x77
>
> The payload sum is not 0x1f8, it's 0x1f6.
> 00 + A8 + 5E + 00 +
> 00 + 28 + 00 + 10 +
> 00 + 48 + 00 + 28 +
> 00 + 48 =
> 0x1f6
>
> Bits 25:24 of HDMI_VIDEOINFOFRAME_3 are the packet version, not part
> of the payload.  So the total would be:
> (0x82 + 0x2 + 0xD) + 0x1f6 = 0x287
> -0x287 = 0x79
>
> I think the issue is that we are not setting the version in bits 25:24
> of HDMI_VIDEOINFOFRAME_3 so we were sending a version 0 AVI packet.

Sorry for late reply.
Yeah, it makes sense now. I didn't think 0xFF000000 isn't part of
infoframe. Your initial patch was missing that "header[1] << 24" part,
so I'm glad you investigated on that.
Thanks a lot!

Patch

diff --git a/drivers/gpu/drm/radeon/evergreen_hdmi.c b/drivers/gpu/drm/radeon/evergreen_hdmi.c
index ed7c8a7..b9c6f76 100644
--- a/drivers/gpu/drm/radeon/evergreen_hdmi.c
+++ b/drivers/gpu/drm/radeon/evergreen_hdmi.c
@@ -128,14 +128,7 @@  static void evergreen_hdmi_update_avi_infoframe(struct drm_encoder *encoder,
 	struct radeon_encoder_atom_dig *dig = radeon_encoder->enc_priv;
 	uint32_t offset = dig->afmt->offset;
 	uint8_t *frame = buffer + 3;
-
-	/* Our header values (type, version, length) should be alright, Intel
-	 * is using the same. Checksum function also seems to be OK, it works
-	 * fine for audio infoframe. However calculated value is always lower
-	 * by 2 in comparison to fglrx. It breaks displaying anything in case
-	 * of TVs that strictly check the checksum. Hack it manually here to
-	 * workaround this issue. */
-	frame[0x0] += 2;
+	uint8_t *header = buffer;
 
 	WREG32(AFMT_AVI_INFO0 + offset,
 		frame[0x0] | (frame[0x1] << 8) | (frame[0x2] << 16) | (frame[0x3] << 24));
@@ -144,7 +137,7 @@  static void evergreen_hdmi_update_avi_infoframe(struct drm_encoder *encoder,
 	WREG32(AFMT_AVI_INFO2 + offset,
 		frame[0x8] | (frame[0x9] << 8) | (frame[0xA] << 16) | (frame[0xB] << 24));
 	WREG32(AFMT_AVI_INFO3 + offset,
-		frame[0xC] | (frame[0xD] << 8));
+		frame[0xC] | (frame[0xD] << 8) | (header[1] << 24));
 }
 
 static void evergreen_audio_set_dto(struct drm_encoder *encoder, u32 clock)
diff --git a/drivers/gpu/drm/radeon/r600_hdmi.c b/drivers/gpu/drm/radeon/r600_hdmi.c
index 456750a..e73b2a7 100644
--- a/drivers/gpu/drm/radeon/r600_hdmi.c
+++ b/drivers/gpu/drm/radeon/r600_hdmi.c
@@ -133,14 +133,7 @@  static void r600_hdmi_update_avi_infoframe(struct drm_encoder *encoder,
 	struct radeon_encoder_atom_dig *dig = radeon_encoder->enc_priv;
 	uint32_t offset = dig->afmt->offset;
 	uint8_t *frame = buffer + 3;
-
-	/* Our header values (type, version, length) should be alright, Intel
-	 * is using the same. Checksum function also seems to be OK, it works
-	 * fine for audio infoframe. However calculated value is always lower
-	 * by 2 in comparison to fglrx. It breaks displaying anything in case
-	 * of TVs that strictly check the checksum. Hack it manually here to
-	 * workaround this issue. */
-	frame[0x0] += 2;
+	uint8_t *header = buffer;
 
 	WREG32(HDMI0_AVI_INFO0 + offset,
 		frame[0x0] | (frame[0x1] << 8) | (frame[0x2] << 16) | (frame[0x3] << 24));
@@ -149,7 +142,7 @@  static void r600_hdmi_update_avi_infoframe(struct drm_encoder *encoder,
 	WREG32(HDMI0_AVI_INFO2 + offset,
 		frame[0x8] | (frame[0x9] << 8) | (frame[0xA] << 16) | (frame[0xB] << 24));
 	WREG32(HDMI0_AVI_INFO3 + offset,
-		frame[0xC] | (frame[0xD] << 8));
+		frame[0xC] | (frame[0xD] << 8) | (header[1] << 24));
 }
 
 /*