[PATCH] drm/radeon: fix AVI infoframe generation

[Rafał Miłecki]

2013/6/10 Alex Deucher <alexdeucher at gmail.com>:
> On Sat, Jun 8, 2013 at 7:46 AM, Rafał Miłecki <zajec5 at gmail.com> wrote:
>> 2013/6/7  <alexdeucher at gmail.com>:
>>> From: Alex Deucher <alexander.deucher at 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!

-- 
Rafał