https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #1 from Andy Furniss adf.lists@gmail.com --- IIRC it was the XBMC people that wanted the ntsc variants in the first place as their player defaults to sync to video exactly and would need to resample sound at 24Hz because of this (blu-ray are 24/1.001).

Assuming you can reproduce the issue just using mplayer playing a CD - then I can't, maybe your receiver is just more fussy than my TV.

Does it claim CEA compliance?

Of course your GPU is likely different to mine (HD4890) so I can't test like for like properly.

The old mode is the first one listed by xrandr - can't you just avoid the ntsc ones rather than needing them to be removed?

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #3 from Alex Deucher agd5f@yahoo.com --- Created attachment 86598 --> https://bugs.freedesktop.org/attachment.cgi?id=86598&action=edit use hw generated cts and n values rather than the sw programmed values

Does this patch help? If not, it may be worth adding some slop to the r600_hdmi_predefined_acr[] table to handle rounding differences so the appropriate defined values get selected.

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #5 from Anssi Hannula anssi@mageia.org --- To expand a bit on my earlier comment that Alex pasted:

HDMI sends ACR packets in the stream that will allow the sink to recover the audio clock. Two values are sent:

N: Divisor used on the audio reference clock (128 * samplerate). CTS: Amount of TMDS clock cycles per one N-divided audio reference clock cycle.

AFAICS normally the source hardware is expected to measure the CTS in real-time by counting the cycles. If the audio/video clocks are perfectly in sync and N is selected optimally (if possible), CTS will of course stay constant. In other cases it is expected to alternate between several values.

The HDMI specification has some recommended N values (and corresponding expected CTS values) for common TMDS clocks. The recommended values result in a constant CTS if the clocks are synchronous (constant CTS is "recommended whenever possible"). The recommendation table does include TMDS clock 74.25/1.001, which is the one used for 1080p23.976. However, the provided N will result in constant CTS only if the clock is exactly 74.25/1.001 without rounding (and the modeline is of course rounded, unless something in the driver or hw does something to try to restore the fractional rate - I have no idea if that is the case).

The radeon driver contains the recommended table and hardwires N/CTS values. If the TMDS clock is not found in the table, recommended N is selected and the CTS is simply directly calculated and set as constant.

The table in question contains N/CTS 11648/140625 for 48kHz and 74.175 MHz (74.175 is down-rounded 74.25/1.001). However, if the TMDS freq is actually exactly 74.175 Mhz and not 74.25/1.001 (74175.824) (i.e. nothing corrects 74.175 => 74.25/1.001), the N/CTS values are already wrong (74175000*11648/(48000*128) = 140623.4375 != 140625.

The driver calculated N/CTS (for clocks not in the table) are correct only if the TMDS clock is exactly the value used (relative to audio clock). I have a feeling that might not be the case (evidenced by e.g. this bug), though I didn't look at the radeon driver internals that closely.

To me it would seem like using hardware measurement for CTS value would be better than to deal with all this. According to header comments radeon hw seems to support that, but for some reason that is not used and driver-wired values are used instead. Alex' attached patch should switch the ACR to hw measured CTS. If that actually works, hopefully it fixes the audio synchronization issues...

(In reply to comment #4)

Something like this:

Not really, the selection doesn't work like that - clock has to match exactly to the table value, it is not an upper bound.

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #7 from Anssi Hannula anssi@mageia.org --- (In reply to comment #6)

I just meant to patch the table so that you end up using the pre-defined values for CTS and N rather than calculating them from the formula.

{ xxx, 11648, 210937, 17836, 234375, 11648, 140625 }, /* 74.25/1.001 MHz */

Replace xxx with whatever clock value the drm edid code gives you for 74.25/1.001 MHz.

Ah, OK.

The's presumably a reason the predefined values are there rather just using the formula for everything.

Well my guess is that the table could be there just because the HDMI spec has that table. Note how everything in the table except the three /1.001 rates (that have differing N) just have the same N/CTS value that the manual calculation would result in anyway.

The reason the table is in the spec is because using the "default" N for the (exact, relative to audio) /1.001 rates results in a non-constant CTS - which is not preferred though should still work.

The actual clock generated by the PLL will not always be exact either. It's limited by reference clock and the divider ranges.

Hence my preference for HW counted CTS if at all possible.

If HW CTS will not work and it is confirmed that the CTS value is indeed the issue here, it might get a bit tricky to figure out the proper values in all cases...

Of course it could be this is not CTS/N related at all and something else in HDMI audio programming is wrong for these "unusual" pixel clocks.

Better not get too much ahead of ourselves until we know if the HW measured CTS patch makes any difference, though... :)

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #9 from Pierre Ossman pierre-bugzilla@ossman.eu --- (In reply to comment #1)

IIRC it was the XBMC people that wanted the ntsc variants in the first place as their player defaults to sync to video exactly and would need to resample sound at 24Hz because of this (blu-ray are 24/1.001).

Sure? The source material is 24 Hz, and wasn't the whole point of 24p to get away from NTSC conversions? TV shows might be a different matter though...

Assuming you can reproduce the issue just using mplayer playing a CD - then I can't, maybe your receiver is just more fussy than my TV.

Does it claim CEA compliance?

I would assume so. But it's not really something blingy enough to brag about on the box. There are specs here:

http://eu.harmankardon.com/harman-kardon-product-detail-eu/avr_265.html

Of course your GPU is likely different to mine (HD4890) so I can't test like for like properly.

The old mode is the first one listed by xrandr - can't you just avoid the ntsc ones rather than needing them to be removed?

If I can, I don't know how. It is xbmc that's my use case, and it tends to pick that mode. Besides, we shouldn't have modes listed that don't work properly. :)

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #11 from Pierre Ossman pierre-bugzilla@ossman.eu --- (In reply to comment #10)

So what should I actually try for the second patch? What you wrote? Or the value from the mode line? :)

Which were the same, so don't mind me...

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #13 from Andy Furniss adf.lists@gmail.com --- (In reply to comment #9)

(In reply to comment #1)

...

IIRC it was the XBMC people that wanted the ntsc variants in the first place as their player defaults to sync to video exactly and would need to resample sound at 24Hz because of this (blu-ray are 24/1.001).

Sure? The source material is 24 Hz, and wasn't the whole point of 24p to get away from NTSC conversions? TV shows might be a different matter though...

Well I guess my blu-ray collection of 2 may not be very representitive, but they are both films rather than TV, were bought in the UK with PAL DVDs in the same case and both ffmpeg and mediainfo show them as being 23.976.

...

Does it claim CEA compliance?

I would assume so. But it's not really something blingy enough to brag about on the box. There are specs here:

Oh, OK it was just a thought posted before all the other detail about the mode appeared.

...

The old mode is the first one listed by xrandr - can't you just avoid the ntsc ones rather than needing them to be removed?

If I can, I don't know how. It is xbmc that's my use case, and it tends to pick that mode. Besides, we shouldn't have modes listed that don't work properly. :)

Hopefully it can be fixed.

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #17 from Alex Deucher agd5f@yahoo.com --- Created attachment 86740 --> https://bugs.freedesktop.org/attachment.cgi?id=86740&action=edit patch 2/3

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #19 from Anssi Hannula anssi@mageia.org --- (In reply to comment #16)

• u64 n, d;
• if (*CTS == 0) {

• n = (u64)clock * N;
  

• d = 128ULL * (u64)freq;
  

• *CTS = div64_u64(n, d);
  

• *CTS *= 1000ULL;
  

• }

Hmm, I believe we should multiply before division, otherwise the result will not be right... like this: + if (*CTS == 0) { + n = (u64)clock * N * 1000ULL; + d = 128ULL * (u64)freq; + *CTS = div64_u64(n, d); + }

Also, maximum value for d is actually 128*48000, so no need for u64 for it, and then one can use div_64u(u64,u32) for division. Similarly, CTS/N values are just 20-bit so no need to u64 them (it might even be confusing), u64 for just the "n" variable should be enough.

Subject: [PATCH 3/3] drm/radeon: use hw generated CTS/N values for audio

Just checking: What N value does the Hw use in that mode? The ones written in by the driver, some hardcoded N or does it select one on its own? Though it doesn't really matter much (since any reasonable N should work as long as CTS is correct), except that if it uses the driver-set value we better not remove the write to the N register :)

(In reply to comment #15)

Somewhat related, the calculation in r600_hdmi_calc_cts() is not very good as it loses a tonne of precision (roughly ten bits' worth). Given the range of the inputs, you might need to do the calculations in a 64-bit space.

I'm a bit interested if the calculated values "seem" to work if you only fix the precisions there (Alex' patch1 plus my first tweak above), without adding the correct values in the table?

If the actual freq is closer to 74176 instead of 74175.8, the calculated values should actually be better than the table value (as you noticed, the table values expect exact /1.001 rate), and it could be the calculation precision issue (*1000 in the wrong place) then caused the audio issues as the CTS was more wildly wrong...

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #21 from Alex Deucher agd5f@yahoo.com --- (In reply to comment #19)

...

Subject: [PATCH 3/3] drm/radeon: use hw generated CTS/N values for audio

Just checking: What N value does the Hw use in that mode? The ones written in by the driver, some hardcoded N or does it select one on its own? Though it doesn't really matter much (since any reasonable N should work as long as CTS is correct), except that if it uses the driver-set value we better not remove the write to the N register :)

It's my understanding that the hw generates the CTS and N values directly, but I would need to double check with the hw teams.

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #23 from Alex Deucher agd5f@yahoo.com --- Already queued for 3.12 fixes. Should show up in the next RC.

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #25 from Tom Yan tom.ty89@gmail.com --- I am just testing the fixes with Linux 3.12-rc6, which is built with this: https://aur.archlinux.org/packages/linux-mainline/

But only worse things happened. It seems that I can't have HDMI sound at all now. When I play a video, it just play really quick silently like what was mentioned in this: https://bugs.freedesktop.org/show_bug.cgi?id=30151

https://bugs.freedesktop.org/show_bug.cgi?id=69675

Pierre Ossman pierre-bugzilla@ossman.eu changed:

What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |REOPENED Resolution|FIXED |---

--- Comment #27 from Pierre Ossman pierre-bugzilla@ossman.eu --- Because of this commit, I need to reopen this bug:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=de...

The 24/1.001 mode does not work with the computed values. Audio glitches every 9 minutes in this state.

Most likely because rounding means that there is no single correct fixed value. The HW needs to toggle the values dynamically constantly. This is even mentioned in the HDMI spec.

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #29 from Anssi Hannula anssi@mageia.org ---

Use the driver calculated CTS and N values rather than having hardware generate them. This allows us to use the modeline pixel clock rather than the actual pll clock when setting up the dto for audio. Fixes problems with audio playback rate on certain asics if the pll clock does not match the pixel clock exactly.

Hm, so actual problems were confirmed fixed with this? That seems rather strange, since that would mean the hardware counted the clock cycles wrong for the CTS value... Maybe the HW doesn't do it per-spec, instead calculating CTS in some other way?

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #31 from Peter Frühberger fritsch@xbmc.org --- As send via E-Mail, I told the same with 24.0 and 24p having these issues. The "sw cts/n values" patch made audio drops with 50 / 60hz much better - but I also have to admit:

Reverting the "sw cts/n values" patch makes 24p working flawless again.

If it is not too late for 3.12 perhaps reverting the "sw cts/n values" patch would be an option?

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #33 from Alex Deucher agd5f@yahoo.com --- (In reply to comment #31)

As send via E-Mail, I told the same with 24.0 and 24p having these issues. The "sw cts/n values" patch made audio drops with 50 / 60hz much better - but I also have to admit:

Reverting the "sw cts/n values" patch makes 24p working flawless again.

If it is not too late for 3.12 perhaps reverting the "sw cts/n values" patch would be an option?

Are you sure it was flawless? You had said you needed this patch: http://lists.freedesktop.org/archives/dri-devel/2013-October/047585.html to fix the audio playback in addition to using hw cts/n. That patch broke playback for a number of other users. So it doesn't seem to be completely fixed in either case.

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #35 from Pierre Ossman pierre-bugzilla@ossman.eu --- (In reply to comment #28)

Created attachment 88450 [details] [review] use fractional fb div on DCE4+

Does this patch help? This should allow the pll value to more closely match the actual requested clock.

Nope. Still glitches around 9 minutes.

The calculations in the table are off though, so I'm not surprised. With corrected values it works fine. I also changed the algorithm to be able to calculate these values properly:

static const struct radeon_hdmi_acr r600_hdmi_predefined_acr[] = { /* 32kHz 44.1kHz 48kHz */ /* Clock N CTS N CTS N CTS */ { 25175, 4096, 25175, 28224, 125875, 6144, 25175 }, /* 25,20/1.001 MHz */ { 25200, 4096, 25200, 6272, 28000, 6144, 25200 }, /* 25.20 MHz */ { 27000, 4096, 27000, 6272, 30000, 6144, 27000 }, /* 27.00 MHz */ { 27027, 4096, 27027, 6272, 30030, 6144, 27027 }, /* 27.00*1.001 MHz */ { 54000, 4096, 54000, 6272, 60000, 6144, 54000 }, /* 54.00 MHz */ { 54054, 4096, 54054, 6272, 60060, 6144, 54054 }, /* 54.00*1.001 MHz */ { 74176, 4096, 74176, 5733, 75335, 6144, 74176 }, /* 74.25/1.001 MHz */ { 74250, 4096, 74250, 6272, 82500, 6144, 74250 }, /* 74.25 MHz */ { 148352, 4096, 148352, 5733, 150670, 6144, 148352 }, /* 148.50/1.001 MHz */ { 148500, 4096, 148500, 6272, 165000, 6144, 148500 }, /* 148.50 MHz */ };

/* * calculate CTS and N values if they are not found in the table */ static void r600_hdmi_calc_cts(uint32_t clock, int *CTS, int *N, int freq) { int n, cts; unsigned long div, mul;

/* Safe, but overly large values */ n = 128 * freq; cts = clock * 1000;

/* Smallest valid fraction */ div = gcd(n, cts);

n /= div; cts /= div;

/* * The optimal N is 128*freq/1000. Calculate the closest larger * value that doesn't truncate any bits. */ mul = ((128*freq/1000) + (n-1))/n;

n *= mul; cts *= mul;

*N = n; *CTS = cts;

DRM_DEBUG("Calculated ACR timing N=%d CTS=%d for frequency %d\n", *N, *CTS, freq); }

struct radeon_hdmi_acr r600_hdmi_acr(uint32_t clock) { struct radeon_hdmi_acr res; u8 i;

/* Precalculated values for common clocks */ for (i = 0; i < ARRAY_SIZE(r600_hdmi_predefined_acr); i++) { if (r600_hdmi_predefined_acr[i].clock == clock) { return r600_hdmi_predefined_acr[i]; } }

/* And odd clocks get manually calculated */ r600_hdmi_calc_cts(clock, &res.cts_32khz, &res.n_32khz, 32000); r600_hdmi_calc_cts(clock, &res.cts_44_1khz, &res.n_44_1khz, 44100); r600_hdmi_calc_cts(clock, &res.cts_48khz, &res.n_48khz, 48000);

return res; }

Two notes here:

1. The 25175 clock at 44.1 kHz is out of spec. There are no correct values to make it in spec. So either change the clock, rely on hw calculated values, or hope that sinks tolerate the large N.

2. None the N values for 44100 listed in the HDMI spec are optimal. Not sure why, so I haven't changed them except where needed.

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #37 from Pierre Ossman pierre-bugzilla@ossman.eu --- (In reply to comment #36)

Could you post a patch, then I can try on my hardware.

I was hoping to be lazy, but I'll sort it out then. :)

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #39 from Pierre Ossman pierre-bugzilla@ossman.eu --- Created attachment 88476 --> https://bugs.freedesktop.org/attachment.cgi?id=88476&action=edit properly calculate both N and CTS

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #40 from Anssi Hannula anssi@mageia.org --- The changes look sound to me.

I had already forgotten that the table values were off for the rounded clocks (even though I had mentioned it in comment #5).

Also, I see now that as the DTO is also set according to the target clock (instead of pll clock), this compensates for possible difference between target/pll clocks when calculating CTS. So the CTS/N values need to be selected as if the clock was exactly the rounded clock (e.g. 74176), instead of using the spec values (that are correct only for exact 74250/1.001). This should also mean that the SW values are all-OK except in the cases where an integer CTS is not possible as per Pierre's note 1.

1. The 25175 clock at 44.1 kHz is out of spec. There are no correct values to

make it in spec. So either change the clock, rely on hw calculated values, or hope that sinks tolerate the large N.

4th alternative is to round CTS and leave the glitches in on those modes. I might even slightly prefer that than produce out-of-spec N, but that is just my preference and I don't really have any real-world data of course...

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #42 from Pierre Ossman pierre-bugzilla@ossman.eu --- (In reply to comment #41)

I think with a) the "table" was used and with b) the values were calculated. So both seem fine. I am a bit afraid with the "out of spec" values - but it did not harm here and the kernel print did not happen.

Actually, I believe with a) the sw values (table, calculated or otherwise) are ignored and the hw does its own thing. With b) you will most likely use the table since all common modes will be in that table. So the calculation was most likely never tested.

As for the out of spec one, that would be 640x480 at 60/1.001 Hz.

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #44 from Anssi Hannula anssi@mageia.org --- (In reply to comment #43)

(In reply to comment #40)

...

...

1. The 25175 clock at 44.1 kHz is out of spec. There are no correct values to

make it in spec. So either change the clock, rely on hw calculated values, or hope that sinks tolerate the large N.

4th alternative is to round CTS and leave the glitches in on those modes. I might even slightly prefer that than produce out-of-spec N, but that is just my preference and I don't really have any real-world data of course...

If we truncate the clock to 25274 instead instead of rounding it up, we can get sensible N/CTS. Is that something that's possible to do? I don't know how the code that generates the clock looks like.

I don't think it makes sense to massage the video clock to get integer CTS. After all, the clock can come from other sources as well (user modeline, EDID...).

However, there's a similar 6th alternative: Massage the target clock frequency that is used by r600_hdmi_acr() and r600_audio_set_dto() (and evergreen_audio_set_dto()) so that an integer CTS is achieved. This will cause the CTS to be correct and in-spec, but the audio speed will be on the order of ~0.004% slower/faster than video (remember that video+audio may not be this accurate otherwise either, as pll clock may not match target clock exactly).

Not sure without testing which is worse, rounted (wrong) CTS or very slightly slower/faster audio compared to video.

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #46 from Alex Deucher agd5f@yahoo.com --- Patch looks good to me. I'll probably wait until drm-next is merged with 3.12 so that it will apply cleanly. If you want to attach a git patch, that would be great :)

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #48 from Pierre Ossman pierre-bugzilla@ossman.eu --- Created attachment 88776 --> https://bugs.freedesktop.org/attachment.cgi?id=88776&action=edit patch 2/2: improve ACR calculation

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #49 from Anssi Hannula anssi@mageia.org --- (In reply to comment #47) (In reply to comment #48)

CEC HDMI spec

Just a nitpick, but CEC is Consumer Electronics Control, an unrelated subset of the HDMI spec. You may be thinking of CEA (Consumer Electronics Association), but that is not directly associated with HDMI AFAICS. HDMI is developed by HDMI Forum and licensed by HDMI Licensing, LLC.

Anyway, changes (still) look fine to me :)

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #51 from Peter Frühberger fritsch@xbmc.org --- Sorry, to warm up this thread. I have also seen that 24p playback is now "working" again. If one measure the audio vs the video clock. One is approx 10ms behind per 6 seconds, which is compensated by "Duplicating package" or "resampling" if your player supports this.

On 50hz I am 10ms too far ahead per 6 seconds. This cause for example LiveTV to run out of packages.

I compared every register with fglrx I could find and there is no real difference between those two.

Log example with xbmc syncing to video clock (50hz): 15:15:58 T:139930428761856 WARNING: CDVDMessageQueue(audio)::Get - asked for new data packet, with nothing available 15:16:04 T:139930428761856 WARNING: Previous line repeats 6 times. 15:16:04 T:139930428761856 DEBUG: CDVDPlayerAudio:: Discontinuity2 - was:53826205859.132309, should be:53826216146.139511, error:10287.007199 15:16:04 T:139930428761856 WARNING: CDVDMessageQueue(audio)::Get - asked for new data packet, with nothing available 15:16:10 T:139930428761856 WARNING: Previous line repeats 10 times. 15:16:10 T:139930428761856 DEBUG: CDVDPlayerAudio:: Discontinuity2 - was:53832387675.043510, should be:53832398035.209015, error:10360.165502 15:16:12 T:139930428761856 WARNING: CDVDMessageQueue(audio)::Get - asked for new data packet, with nothing available 15:16:17 T:139930428761856 WARNING: Previous line repeats 7 times. 15:16:17 T:139930428761856 DEBUG: CDVDPlayerAudio:: Discontinuity2 - was:53838568755.002014, should be:53838579138.448776, error:10383.446764 15:16:17 T:139930428761856 WARNING: CDVDMessageQueue(audio)::Get - asked for new data packet, with nothing available 15:16:23 T:139930428761856 WARNING: Previous line repeats 16 times. 15:16:23 T:139930428761856 DEBUG: CDVDPlayerAudio:: Discontinuity2 - was:53844735030.241776, should be:53844745675.325325, error:10645.083549 15:16:23 T:139930428761856 WARNING: CDVDMessageQueue(audio)::Get - asked for new data packet, with nothing available

Log with 24p: 12:00:37 T:140730861328128 DEBUG: CDVDPlayerAudio::Discontinuity2 - was:246782652.089114, should be:246769963.622166, error:-12688.466948 12:00:45 T:140730861328128 DEBUG: CDVDPlayerAudio::Discontinuity2 - was:254909959.765166, should be:254897679.533340, error:-12280.231826 12:00:53 T:140730861328128 DEBUG: CDVDPlayerAudio::Discontinuity2 - was:263037680.565340, should be:263025287.637961,

I tried to play with register 0x05b4 and 0x05b8 but could not find a ratio so that the drops would stop. Not sure if this is related to the original problem though.

Btw. you won't notice this problem, if you play local content and for example resample audio.

https://bugs.freedesktop.org/show_bug.cgi?id=69675

--- Comment #53 from Rainer Hochecker fernetmenta@online.de --- We did measure system clock against refresh rate. When running at 50hz or 60hz, refresh rate seems to be fast whereas when running at 24 is is behind. Means that not only 24hz is affected but all refresh rates.