Bug 69675

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?

From IRC:
<Anssi> agd5f: you might've seen already, but the 23.976 Hz pixel clock  74175.824175 is rounded up to 74176 in Ville's commit, but r600_hdmi_predefined_acr[] table contains 74175 so it does not match
<Anssi> not sure why the calculated value would not work, though... probably for the same reason the recommended value is not the calculated value, but I don't understand enough to know why

Created attachment 86598 [details] [review]
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.

Something like this:

diff --git a/drivers/gpu/drm/radeon/r600_hdmi.c b/drivers/gpu/drm/radeon/r600_hdmi.c
index b0fa600..f7d2766 100644
--- a/drivers/gpu/drm/radeon/r600_hdmi.c
+++ b/drivers/gpu/drm/radeon/r600_hdmi.c
@@ -63,7 +63,7 @@ static const struct radeon_hdmi_acr r600_hdmi_predefined_acr[] = {
     {  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 */
-    {  74175, 11648, 210937, 17836, 234375, 11648, 140625 }, /*  74.25/1.001 MHz */
+    {  74180, 11648, 210937, 17836, 234375, 11648, 140625 }, /*  74.25/1.001 MHz */
     {  74250,  4096,  74250,  6272,  82500,  6144,  74250 }, /*  74.25       MHz */
     { 148351, 11648, 421875,  8918, 234375,  5824, 140625 }, /* 148.50/1.001 MHz */
     { 148500,  4096, 148500,  6272, 165000,  6144, 148500 }, /* 148.50       MHz */

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.

(In reply to comment #5)
> (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.

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.

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

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

(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... :)

(In reply to comment #2)
> From IRC:
> <Anssi> agd5f: you might've seen already, but the 23.976 Hz pixel clock 
> 74175.824175 is rounded up to 74176 in Ville's commit

FWIW I guess he did this because 74.176 appears in CEA D for 23.976, there isn't anything for 23.976 in E.

(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. :)

(In reply to comment #6)
> (In reply to comment #5)
> > (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.
> 
> 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.
> 

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

(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...

(In reply to comment #3)
> Created attachment 86598 [details] [review] [review]
> 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.

On some early R6xx the hardware generation of CTS/N values didn't worked reliable, that's why I've done it like fglrx and calculated the values manually/used a table for it.

(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.

(In reply to comment #3)
> Created attachment 86598 [details] [review] [review]
> 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.

A quick test indicates that this patch does indeed solve the issue. I've played three minutes of AC3 audio without any interruptions.

(In reply to comment #4)
> Something like this:
> 
> diff --git a/drivers/gpu/drm/radeon/r600_hdmi.c
> b/drivers/gpu/drm/radeon/r600_hdmi.c
> index b0fa600..f7d2766 100644
> --- a/drivers/gpu/drm/radeon/r600_hdmi.c
> +++ b/drivers/gpu/drm/radeon/r600_hdmi.c
> @@ -63,7 +63,7 @@ static const struct radeon_hdmi_acr
> r600_hdmi_predefined_acr[] = {
>      {  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 */
> -    {  74175, 11648, 210937, 17836, 234375, 11648, 140625 }, /* 
> 74.25/1.001 MHz */
> +    {  74180, 11648, 210937, 17836, 234375, 11648, 140625 }, /* 
> 74.25/1.001 MHz */
>      {  74250,  4096,  74250,  6272,  82500,  6144,  74250 }, /*  74.25     
> MHz */
>      { 148351, 11648, 421875,  8918, 234375,  5824, 140625 }, /*
> 148.50/1.001 MHz */
>      { 148500,  4096, 148500,  6272, 165000,  6144, 148500 }, /* 148.50     
> MHz */

This however does not improve things. But that was because the frequency was actually 74176, not 74180 (damn all that rounded output).

With that fixed, it *seems* to work. But doing the numbers, the audio clock is slightly off as those numbers assume a perfect 74.25/1.001 video clock, not the 74.176 approximation. And 33 kHz audio needs a changing CTS even with that. So it doesn't seem like a robust approach IHMO. I vote for letting the hardware do its magic.

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.

Created attachment 86739 [details] [review]
patch 1/3

I think this entire patch set should be applied.  First patch switches to 64 bit math for the cts calcuation.  Second patch patches the radeon acr tables based on the clock values generated by the drm code that generates the 1001 modes. Third patch enables hw generated cts and n values.  If there are indeed problems with using the hw generated values, we can disable the 3rd patch on the problematic asics, and the other two patches should do the right thing.

Created attachment 86740 [details] [review]
patch 2/3

Created attachment 86741 [details] [review]
patch 3/3

(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...

Created attachment 86856 [details] [review]
patch 1/3

Updated 1/3.

(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.

So what's the verdict? Can these patches be applied? It would be nice to be able to go back to a stock kernel. :)

Already queued for 3.12 fixes.  Should show up in the next RC.

*** Bug 70660 has been marked as a duplicate of this bug. ***

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

Everything works fine with 3.12-rc7. Thanks a lot!

Because of this commit, I need to reopen this bug:

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

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.

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.

> 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?

(In reply to comment #29)

> 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?

Yes.  Using the sw cts/n values resulted in better results across asics compared to the alternative fix (using the actual pll clock for the dto calculation).  Using the hw cts/n values lead to playback speed issues on some asics.

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?

To add: I tested with http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=de926800b155886c61b06146e28c0ba2e6fafc39 reverted and use "fractional fb div on DCE4+" applied

(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.

I need something to regression test.

With xbmc I can sync to the video clock and drop / dupe audio, when they are running into different directions.

Example: 

Watching 1080i50 (was better with the SW patch):
14:26:12 T:140478137739008 WARNING: CDVDMessageQueue(audio)::Get - asked for new data packet, with nothing available
14:27:08 T:140478137739008 WARNING: Previous line repeats 10 times.
14:27:08 T:140478137739008   DEBUG: CDVDPlayerAudio:: Discontinuity1 - was:74579944528.817001, should be:74579839111.000000, error:-105417.817001

1080p24 does not work reliable with the SW patch (that I told via mail).

The pll patch you linked caused severe issues for some users, therefore I unapplied it. Btw. this was also quite curious with it:

Without the pll clock patch I used radeontool to change the 0x05b4 from 1485000 to 1487500 which the pll clock should accomplish automatically - sadly it had other side effects.

Best would really be some ms timer and play some audio / video stream and measure the length.

Open for ideas here to track that down correctly, as the tests are quite a lot:
1080p24, 1080-24.0, 1080@50hz, 60hz, ac3, dts, lpcm, dts-hd.

Without highjacking this thread - what about the register dumped I sent via Mail, it seems fglrx uses a 10 khz clock (relevant registers are factor 10)

(In reply to comment #28)
> Created attachment 88450 [details] [review] [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.

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

(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. :)

Nope it is okay. You had everything I needed in your previous post (could easily copy the two functions and the struct) - will report back later.

Created attachment 88476 [details] [review]
properly calculate both N and CTS

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...

I tested on a 3.12-rc7+ kernel in the following combination:

a) sw cts/n values revert + pierre ossman patch applied
All fine - watched a whole 24p movie with dts-hd

b) kept 3.12-rc7+ as is and only applied pierre ossman patch
All fine

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.

(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.

(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.

(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.

Alex? Comments?

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 :)

Created attachment 88775 [details] [review]
patch 1/2: correct ACR table

Created attachment 88776 [details] [review]
patch 2/2: improve ACR calculation

(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 :)

(In reply to comment #49)
> (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 :)

Yeah, I was thinking of CEA. But as for why CEC; the file I have for this is called CEC_HDMI_Specification.pdf, so I fairly blindly copied that. :)

Alex, adjust at will. ;)

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.

(In reply to comment #51)
> 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.

10 ms per 6 seconds sounds very close to the 24 vs 24/1.001 difference (which is 10 ms per 10 seconds). Assuming you've gotten the correct mode configured, that would mean the audio clock in the card is misconfigured.

The rounding error on the pixel clock is several magnitudes smaller, so it doesn't sound like that.

You should be able to tell which clock is off if you measure both against the CPU clock.

> 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.

Probably not. If I've understood things correctly, then the ACR is fairly independent from the more actual audio handling. So a new bug entry is probably in order.

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.

Hi,

Freedesktop's Bugzilla instance is EOLed and open bugs are about to be migrated to http://gitlab.freedesktop.org.

To avoid migrating out of date bugs, I am now closing all the bugs that did not see any activity in the past year. If the issue is still happening, please create a new bug in the relevant project at https://gitlab.freedesktop.org/drm (use misc by default).

Sorry about the noise!