From: Alex Deucher alexander.deucher@amd.com
- 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 alexander.deucher@amd.com Cc: Rafał Miłecki zajec5@gmail.com --- drivers/gpu/drm/radeon/evergreen_hdmi.c | 11 ++--------- drivers/gpu/drm/radeon/r600_hdmi.c | 11 ++--------- 2 files changed, 4 insertions(+), 18 deletions(-)
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)); }
/*
2013/6/7 alexdeucher@gmail.com:
From: Alex Deucher alexander.deucher@amd.com
- 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
On Sat, Jun 8, 2013 at 7:46 AM, Rafał Miłecki zajec5@gmail.com wrote:
2013/6/7 alexdeucher@gmail.com:
From: Alex Deucher alexander.deucher@amd.com
- 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ł
On Sat, Jun 8, 2013 at 7:46 AM, Rafał Miłecki zajec5@gmail.com wrote:
2013/6/7 alexdeucher@gmail.com:
From: Alex Deucher alexander.deucher@amd.com
- 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ł
2013/6/10 Alex Deucher alexdeucher@gmail.com:
On Sat, Jun 8, 2013 at 7:46 AM, Rafał Miłecki zajec5@gmail.com wrote:
2013/6/7 alexdeucher@gmail.com:
From: Alex Deucher alexander.deucher@amd.com
- 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!
dri-devel@lists.freedesktop.org
-
Alex Deucher -
alexdeucher＠gmail.com
-
Rafał Miłecki