| Message ID | E1ZO6hc-0003NW-55@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Russell, On Sat, Aug 8, 2015 at 9:10 AM, Russell King <rmk+kernel@arm.linux.org.uk> wrote: > Adjust the pixel clock values in the N calculation to match the more > accurate clock values we're given by the DRM subsystem, which are the > kHz pixel rate, with any fractional kHz rounded down in the case of > the non-240, non-480 line modes, or rounded up for the others. So, > > 25.20 / 1.001 => 25175 > 27.00 * 1.001 => 27027 > 74.25 / 1.001 => 74176 > 148.50 / 1.001 => 148352 > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > drivers/gpu/drm/bridge/dw_hdmi.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) For what it's worth, I backported this change into my local 3.14-based tree and it doesn't cause any problems, though it looks like the code isn't being run in my case... I can confirm that the rates you list match the rates I actually see requested by DRM, but in my current tree I've actually got something that allows a little bit of "slop" in HDMI rates because my system can't actually always make exactly the modes requested, but it appears that getting "close enough" works, especially if your clock jitter is low enough (because the sink needs to have a little bit of wiggle room for jitter anyway). For instance, when 25.175 is requested we actually end up making 25.170732. In my tree this adjustment happens in mode_fixup by changing the adj_mode. In one particular case, some debug prints show: 640x480, mode=25200000, adj=25171000, actual=25170732 freq=48000, pixel_clk=25171000, n=6144 I'm not enough of an HDMI expert to say whether it's better to be using n=6144 or n=6864 in this case, but audio does play with either on the TV I tested. In any case, I'd say that your change at least makes things better than they were, so I'd be in favor of taking it. If someone later decides that we should add a little margin to these numbers, then a patch to add that could go atop yours. Reviewed-by: Douglas Anderson <dianders@chromium.org>
Hi, On Fri, Sep 4, 2015 at 11:21 AM, Doug Anderson <dianders@chromium.org> wrote: > Russell, > > On Sat, Aug 8, 2015 at 9:10 AM, Russell King > <rmk+kernel@arm.linux.org.uk> wrote: >> Adjust the pixel clock values in the N calculation to match the more >> accurate clock values we're given by the DRM subsystem, which are the >> kHz pixel rate, with any fractional kHz rounded down in the case of >> the non-240, non-480 line modes, or rounded up for the others. So, >> >> 25.20 / 1.001 => 25175 >> 27.00 * 1.001 => 27027 >> 74.25 / 1.001 => 74176 >> 148.50 / 1.001 => 148352 >> >> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> >> --- >> drivers/gpu/drm/bridge/dw_hdmi.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) > > For what it's worth, I backported this change into my local 3.14-based > tree and it doesn't cause any problems, though it looks like the code > isn't being run in my case... > > I can confirm that the rates you list match the rates I actually see > requested by DRM, but in my current tree I've actually got something > that allows a little bit of "slop" in HDMI rates because my system > can't actually always make exactly the modes requested, but it appears > that getting "close enough" works, especially if your clock jitter is > low enough (because the sink needs to have a little bit of wiggle room > for jitter anyway). For instance, when 25.175 is requested we > actually end up making 25.170732. > > In my tree this adjustment happens in mode_fixup by changing the > adj_mode. In one particular case, some debug prints show: > 640x480, mode=25200000, adj=25171000, actual=25170732 > freq=48000, pixel_clk=25171000, n=6144 > > I'm not enough of an HDMI expert to say whether it's better to be > using n=6144 or n=6864 in this case, but audio does play with either > on the TV I tested. > > In any case, I'd say that your change at least makes things better > than they were, so I'd be in favor of taking it. If someone later > decides that we should add a little margin to these numbers, then a > patch to add that could go atop yours. Oh! I just figured this out! :) Basically the spec is saying that you want both N and CTS to be integral. ...as you say you really want: CTS = (TMDS * N) / (128 * audio_freq) ...CTS has no other restrictions (other than being integral) and you're allowed a bit of slop for N (you aim for 128 * audio_freq but can go up or down a bit). ...and the TMDS frequency has no such restrictions for being integral in their calculations. Apparently it's more important to optimize for the CTS formula working out then it is for getting close to "128 * audio freq". ...and that's the reason for these special case N values... So to put some numbers: We're perfect when we have exactly 25.2: 25200 * 4096 / (128 * 32) => 25200, so CTS for 25.2 MHz is 25200. Perfect ...but when we have 25.2 / 1.001 we get a non-integral CTS: (25200 / 1.001) * 4096 / (128 * 32) => 25174.82517482518 ...we can get an integral CTS and still remain in range if: (25200 / 1.001) * 4576 / (128 * 32) => 28125 In the case of Linux, I'm afraid we just don't have this type of accuracy in our APIs. The spec is talking about making 25.17482517482518 MHz. As I said, in my case I'm actually making 25170732. In your case you're probably making the value that Linux asked you to make, AKA 25.175000 MHz. Unsurprisingly, if you do the calculations with 25.175 MHz (or any integral kHz value) you don't have to do any special optimization to stay integral: 25175 * 4096 / (128 * 32) => 25175 So unless you have some way to know that the underlying clock is actually (25.2 / 1.001) MHz and not just 25.175 MHz then your patch looks wrong. As a first step I'd suggest just removing all the special cases and add a comment. From real world testing it doesn't seem terribly critical to be slightly off on CTS. ...and in any case for any clock rates except the small handful in the HDMI spec we'll be slightly off on CTS anyway... As a second step you could actually use the rate from "clk_get_rate()" to see what clock rate was actually made. You'll at least get Hz here. If you've somehow structured your machine to give you 25174825 Hz when DRM asked for 25175000 Hz (or if you redo the calculations and ignore what DRM told you), then that would give you this slightly more optimal rate. As a third step you could somehow add the more detailed Hz information to DRM (sounds like a big task, but I'm nowhere near a DRM expert). As a fourth step you could try to write the code in a generic way to figure out the best N / CTS to minimize error in the formula while still staying within the required ranges. If you did that, it probably would belong in some generic helper and not in dw_hdmi... ...anyway, I'm not suggestion that you do everything above since I think just removing the special cases is probably good enough. ...but if you wanted everything to be perfect it seems like the way to go. So I guess remove my Reviewed-by for this patch? -Doug
On Fri, Sep 04, 2015 at 12:48:02PM -0700, Doug Anderson wrote: > Hi, > > On Fri, Sep 4, 2015 at 11:21 AM, Doug Anderson <dianders@chromium.org> wrote: > > Russell, > > > > On Sat, Aug 8, 2015 at 9:10 AM, Russell King > > <rmk+kernel@arm.linux.org.uk> wrote: > >> Adjust the pixel clock values in the N calculation to match the more > >> accurate clock values we're given by the DRM subsystem, which are the > >> kHz pixel rate, with any fractional kHz rounded down in the case of > >> the non-240, non-480 line modes, or rounded up for the others. So, > >> > >> 25.20 / 1.001 => 25175 > >> 27.00 * 1.001 => 27027 > >> 74.25 / 1.001 => 74176 > >> 148.50 / 1.001 => 148352 > >> > >> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > >> --- > >> drivers/gpu/drm/bridge/dw_hdmi.c | 20 ++++++++++---------- > >> 1 file changed, 10 insertions(+), 10 deletions(-) > > > > For what it's worth, I backported this change into my local 3.14-based > > tree and it doesn't cause any problems, though it looks like the code > > isn't being run in my case... > > > > I can confirm that the rates you list match the rates I actually see > > requested by DRM, but in my current tree I've actually got something > > that allows a little bit of "slop" in HDMI rates because my system > > can't actually always make exactly the modes requested, but it appears > > that getting "close enough" works, especially if your clock jitter is > > low enough (because the sink needs to have a little bit of wiggle room > > for jitter anyway). For instance, when 25.175 is requested we > > actually end up making 25.170732. > > > > In my tree this adjustment happens in mode_fixup by changing the > > adj_mode. In one particular case, some debug prints show: > > 640x480, mode=25200000, adj=25171000, actual=25170732 > > freq=48000, pixel_clk=25171000, n=6144 > > > > I'm not enough of an HDMI expert to say whether it's better to be > > using n=6144 or n=6864 in this case, but audio does play with either > > on the TV I tested. > > > > In any case, I'd say that your change at least makes things better > > than they were, so I'd be in favor of taking it. If someone later > > decides that we should add a little margin to these numbers, then a > > patch to add that could go atop yours. > > Oh! I just figured this out! :) > > Basically the spec is saying that you want both N and CTS to be > integral. ...as you say you really want: > CTS = (TMDS * N) / (128 * audio_freq) In the case of software-programmed CTS and N values, they have to be integral because there's no such thing as fractional division here. The CTS and N values get sent across the HDMI link to the sink, and they use those in a PLL like arrangement to derive the audio clock. More "inteligent" hardware automatically measures the CTS number and continually updates the sink, which allows the sink to remain in sync with the audio at non-coherent rates. > ...CTS has no other restrictions (other than being integral) and > you're allowed a bit of slop for N (you aim for 128 * audio_freq but > can go up or down a bit). No. Both CTS and N have to be accurate to generate the correct sample rate from the TDMS clock. > Apparently it's more important to optimize for the CTS formula working > out then it is for getting close to "128 * audio freq". ...and that's > the reason for these special case N values... The "128 * audio freq" is just a recommendation. Going through the HDMI spec's recommended values for various clock rates and sample rates reveals that quite a number of them are far from this "recommendation". So I wouldn't read too much into the "128 * audio freq" thing. > So to put some numbers: > > We're perfect when we have exactly 25.2: > 25200 * 4096 / (128 * 32) > => 25200, so CTS for 25.2 MHz is 25200. Perfect > > ...but when we have 25.2 / 1.001 we get a non-integral CTS: > (25200 / 1.001) * 4096 / (128 * 32) > => 25174.82517482518 > > ...we can get an integral CTS and still remain in range if: > (25200 / 1.001) * 4576 / (128 * 32) > => 28125 Correct. These are the values given in the HDMI specification for each of your clock rates you mention above. You can even use 4096 for N _provided_ the source measures and sends the CTS value (that's basically what happens in the case of "non-coherent" clocks.) > In the case of Linux, I'm afraid we just don't have this type of > accuracy in our APIs. We don't have that kind of precision in the DRM API, but we do have the precision in the clock API. > The spec is talking about making 25.17482517482518 MHz. +/- 0.5%, according to CEA-861-B. > As I said, in my case I'm actually making 25170732. ... which is within 0.02%, so is within spec. > In your case you're probably making the value that Linux > asked you to make, AKA 25.175000 MHz. ... which is the spec value. > Unsurprisingly, if you do the > calculations with 25.175 MHz (or any integral kHz value) you don't > have to do any special optimization to stay integral: > > 25175 * 4096 / (128 * 32) > => 25175 > > > So unless you have some way to know that the underlying clock is > actually (25.2 / 1.001) MHz and not just 25.175 MHz then your patch > looks wrong. I don't believe you can make that statement. If you wish to take the lack of precision up with the authors of the CEA-861 and HDMI specifications, since they "approximate" to the values I have in this patch, and are what userspace passes in the mode structures to kernel space. > As a first step I'd suggest just removing all the special cases and > add a comment. From real world testing it doesn't seem terribly > critical to be slightly off on CTS. ...and in any case for any clock > rates except the small handful in the HDMI spec we'll be slightly off > on CTS anyway... They're not "special cases" made up to fit something - they're from the tables in the HDMI specification. [everything else cut I'm getting tired...] At the end of the day, when it comes to video playback, what matters more is that your video and audio rates are related. If the stream audio is 48kHz and your video is expected to be 60fps, then the decoder is going to want to see audio being consumed at 48kHz and video at 60fps. If your actual video output is slightly slow due to a crap hardware implementation, then having the audio clock slow by the same proportion means that the video decoder doesn't have to stretch or squeeze the audio to try and make things fit, or worse, skip frames. That assumes that the audio and video clocks are coherent. On iMX6 hardware using this, the audio is clocked at the rate defined by the TDMS clock and the CTS/N values. Other hardware, where the audio clock is derived differently (and therefore, noncoherently), won't be using the CTS value software supplies, because that's meaningless - it's got to measure the audio clock rate, and pass that over to the sink using CTS - so called auto-CTS mode. That allows the sink to track the audio clock rate irrespective of the actual TDMS clock rate.
Russell, On Fri, Sep 4, 2015 at 2:24 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: >> Basically the spec is saying that you want both N and CTS to be >> integral. ...as you say you really want: >> CTS = (TMDS * N) / (128 * audio_freq) > > In the case of software-programmed CTS and N values, they have to be > integral because there's no such thing as fractional division here. > The CTS and N values get sent across the HDMI link to the sink, and > they use those in a PLL like arrangement to derive the audio clock. > > More "inteligent" hardware automatically measures the CTS number and > continually updates the sink, which allows the sink to remain in > sync with the audio at non-coherent rates. > >> ...CTS has no other restrictions (other than being integral) and >> you're allowed a bit of slop for N (you aim for 128 * audio_freq but >> can go up or down a bit). > > No. Both CTS and N have to be accurate to generate the correct > sample rate from the TDMS clock. I guess by "other" I meant no restrictions other than that, which is listed above that "CTS = (TMDS * N) / (128 * audio_freq)". Anyway, sounds like we're on the same page here... >> Apparently it's more important to optimize for the CTS formula working >> out then it is for getting close to "128 * audio freq". ...and that's >> the reason for these special case N values... > > The "128 * audio freq" is just a recommendation. Going through the HDMI > spec's recommended values for various clock rates and sample rates > reveals that quite a number of them are far from this "recommendation". > > So I wouldn't read too much into the "128 * audio freq" thing. Again, same page. >> So to put some numbers: >> >> We're perfect when we have exactly 25.2: >> 25200 * 4096 / (128 * 32) >> => 25200, so CTS for 25.2 MHz is 25200. Perfect >> >> ...but when we have 25.2 / 1.001 we get a non-integral CTS: >> (25200 / 1.001) * 4096 / (128 * 32) >> => 25174.82517482518 >> >> ...we can get an integral CTS and still remain in range if: >> (25200 / 1.001) * 4576 / (128 * 32) >> => 28125 > > Correct. These are the values given in the HDMI specification for each > of your clock rates you mention above. > > You can even use 4096 for N _provided_ the source measures and sends > the CTS value (that's basically what happens in the case of > "non-coherent" clocks.) > >> In the case of Linux, I'm afraid we just don't have this type of >> accuracy in our APIs. > > We don't have that kind of precision in the DRM API, but we do have the > precision in the clock API. Yup. On the same page. See my suggestions of using the common clock framework. >> The spec is talking about making 25.17482517482518 MHz. > > +/- 0.5%, according to CEA-861-B. > >> As I said, in my case I'm actually making 25170732. > > ... which is within 0.02%, so is within spec. Yup, that's why we're doing it. Note that total jitter has to be under +/- 0.5% right? ...so if you've got error here you've got to make sure your clock is extra clean I think. >> In your case you're probably making the value that Linux >> asked you to make, AKA 25.175000 MHz. > > ... which is the spec value. This is where we're not on the same page. Where in the spec does it say 25.17500 MHz? I see in the spec: 25.2 / 1.001 ...and this is a crucial difference here. Please double-check my math, but: (25175000 * 4576) / (128 * 32000.) => 28125.1953125 (25174825 * 4576) / (128 * 32000.) => 28125.0 This calculation is what led to my belief that the goal here is to make an integral CTS. If you have 25.175 MHZ clock and N of 4576 you _will not_ have an integral CTS. If you instead have 25.174825 MHz clock and N of 4576 you _will_ have an integral CTS. Said another way: 1. The reason 25174825 Hz has a different N is to make an integral CTS. 2. If you are indeed making 25175000 then there is no need for a different N to make an integral CTS 3. If you use 4576 for N but you're making 25175000 Hz, you end up in a _worse_ position than if you use the standard 4096 for N. >> Unsurprisingly, if you do the >> calculations with 25.175 MHz (or any integral kHz value) you don't >> have to do any special optimization to stay integral: >> >> 25175 * 4096 / (128 * 32) >> => 25175 >> >> >> So unless you have some way to know that the underlying clock is >> actually (25.2 / 1.001) MHz and not just 25.175 MHz then your patch >> looks wrong. > > I don't believe you can make that statement. If you wish to take the > lack of precision up with the authors of the CEA-861 and HDMI > specifications, since they "approximate" to the values I have in this > patch, and are what userspace passes in the mode structures to kernel > space. I will repeat my mantra: I'm a visitor here and decidedly not an expert. However, from my reading of the HDMI spec shows that the spec itself is fine. They are just assuming that you're providing a 25.174825 MHz clock and giving you optimized values for said clock. If the current driver says that it's providing 25.175000 MHz then you shouldn't assume that it's actually making 25.174825 MHz >> As a first step I'd suggest just removing all the special cases and >> add a comment. From real world testing it doesn't seem terribly >> critical to be slightly off on CTS. ...and in any case for any clock >> rates except the small handful in the HDMI spec we'll be slightly off >> on CTS anyway... > > They're not "special cases" made up to fit something - they're from the > tables in the HDMI specification. They are definitely "special cases". There is a general rule in the code you posted (aim for 128 * freq) and these are cases for certain clocks that are an exception to the general rule. AKA they are special cases. I'm not arguing that there's not a valid reason for these special cases. I'm simply arguing that the special cases are likely for a different situation than the one we're in. The HDMI spec itself (loosely interpreted) pretty much says: if there's any doubt, just use the equations--don't use the tables. > That assumes that the audio and video clocks are coherent. On iMX6 > hardware using this, the audio is clocked at the rate defined by the > TDMS clock and the CTS/N values. I'll admit I haven't looked at the audio section of dw_hdmi much, but I'd imagine that for all users of this controller / PHY the audio and video clocks are coherent. I think in the perfect world we'd be able to generate exactly 25174825.174825177 Hz and we'd use all the rates from the HDMI spec. and we'd get spot on 32 kHz audio. ...but I'm simply saying that we're not in that perfect world yet. Also note that there are many many rates not in the HDMI spec that could benefit from similar optimization of trying to adjust N to make an integral CTS. --- As a side note: I realized one part of the HDMI spec that isn't trying to make an integral value but still uses a different value for N: 297 MHz. From the DesignWare spec I have it appears that 594 MHz is similar. For those cases it looks like we have: if (pixel_clk == 297000000) { switch (freq) { case 32000: return (128 * freq) / 1333; case 44100: case 48000: case 88200: case 96000: case 176400: return (128 * freq) / 1200; } } else if (pixel_clk == 594000000) { switch (freq) { case 32000: return (128 * freq) / 1333; case 44100: case 88200: case 176400: return (128 * freq) / 600; } }
On Fri, Sep 04, 2015 at 04:50:03PM -0700, Doug Anderson wrote: > Russell, > > On Fri, Sep 4, 2015 at 2:24 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > >> In your case you're probably making the value that Linux > >> asked you to make, AKA 25.175000 MHz. > > > > ... which is the spec value. > > This is where we're not on the same page. Where in the spec does it > say 25.17500 MHz? I see in the spec: > 25.2 / 1.001 Section 4 of CEA-861-B, which defines the video clock rates and their accuracy of 0.5%. > ...and this is a crucial difference here. Please double-check my math, but: > > (25175000 * 4576) / (128 * 32000.) > => 28125.1953125 > > (25174825 * 4576) / (128 * 32000.) > => 28125.0 > > This calculation is what led to my belief that the goal here is to > make an integral CTS. If you have 25.175 MHZ clock and N of 4576 you > _will not_ have an integral CTS. If you instead have 25.174825 MHz > clock and N of 4576 you _will_ have an integral CTS. Right, but 25.175 is close enough to 25.174825. Do this calculation: 25175000 * 4576 / 28125 / 128 That'll give you the resulting audio sample rate, which is 32000.222Hz. That's an error of... 0.00069%, which is probably around the typical error of your average crystal oscillator. Really not worth bothering with. > Said another way: > > 1. The reason 25174825 Hz has a different N is to make an integral CTS. > > 2. If you are indeed making 25175000 then there is no need for a > different N to make an integral CTS > > 3. If you use 4576 for N but you're making 25175000 Hz, you end up in > a _worse_ position than if you use the standard 4096 for N. Total rubbish. Sorry, but it is. Follow the code. Pixel clock is 25175000. For 32kHz, N will be 4576. 25175000 * 4576 = 1.152008e11. Divide that by the audio clock rate (128 * 32000) gives 28125.19531. Since we're using integer division, that gets rounded down to 28125. DRM uses a clock rate of "25175" to represent 25.2/1.001 modes. So, if your hardware sets a video clock rate of 25.2MHz/1.001, then you end up with a sample rate of exactly 32kHz. If you set exactly 25.175MHz, you end up with an approximate 32kHz sample rate - one which is 0.00069% in error, which is (excluse the language) fuck all different from exactly 32kHz. Are you _really_ going to continue arguing over a 0.00069% error? If you are, I'm not going to listen anymore - it's soo damned small that it's not worth bothering with. At all. The only time that you'd need to worry about it is if you wanted a super-accurate system, and for that you'd need an atomic clock to source your system clocks to reduce aging effects, temperature induced drift, etc, maybe locking the atomic clock to a national frequency standard like the Anthorn MSF 60kHz transmitter signal broadcast by the UK National Physics Laboratory. > >> As a first step I'd suggest just removing all the special cases and > >> add a comment. From real world testing it doesn't seem terribly > >> critical to be slightly off on CTS. ...and in any case for any clock > >> rates except the small handful in the HDMI spec we'll be slightly off > >> on CTS anyway... > > > > They're not "special cases" made up to fit something - they're from the > > tables in the HDMI specification. > > They are definitely "special cases". There is a general rule in the > code you posted (aim for 128 * freq) and these are cases for certain > clocks that are an exception to the general rule. AKA they are > special cases. Sorry, I disagree with you. > > That assumes that the audio and video clocks are coherent. On iMX6 > > hardware using this, the audio is clocked at the rate defined by the > > TDMS clock and the CTS/N values. > > I'll admit I haven't looked at the audio section of dw_hdmi much, but > I'd imagine that for all users of this controller / PHY the audio and > video clocks are coherent. Not if the audio clock comes from an I2S master rather than being sourced from the HDMI block. > I think in the perfect world we'd be able to generate exactly > 25174825.174825177 Hz and we'd use all the rates from the HDMI spec. To generate something of that accuracy, you'd need something like a caesium fountain atomic clock. > and we'd get spot on 32 kHz audio. ...but I'm simply saying that > we're not in that perfect world yet. > > Also note that there are many many rates not in the HDMI spec that > could benefit from similar optimization of trying to adjust N to make > an integral CTS. Now go and look at the HDMI spec, where it gives the CTS value for 74.25/1.001 for 32kHz. That can't be represented by an integer CTS value, so using this hardware, we can't generate that sample rate without an error. We'd use a fixed CTS value of 210937 instead, which works out at a 0.00024% error. Again, not worth worrying about. > > --- > > As a side note: I realized one part of the HDMI spec that isn't trying > to make an integral value but still uses a different value for N: 297 > MHz. From the DesignWare spec I have it appears that 594 MHz is > similar. For those cases it looks like we have: 297MHz _does_ work. 297000000 * 3072 / 222750 = 128 * 32000 exactly. > > if (pixel_clk == 297000000) { > switch (freq) { > case 32000: > return (128 * freq) / 1333; Plug the numbers in. 128 * 32000 / 1333 = 3072.96 but because we're using integer math, that's 3072. Which just happens to be the value in the HDMI spec. > case 44100: > case 48000: > case 88200: > case 96000: > case 176400: > return (128 * freq) / 1200; Do the math again. You get the spec figures for N.
Russell, On Fri, Sep 4, 2015 at 5:27 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Sep 04, 2015 at 04:50:03PM -0700, Doug Anderson wrote: >> Russell, >> >> On Fri, Sep 4, 2015 at 2:24 PM, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >> >> In your case you're probably making the value that Linux >> >> asked you to make, AKA 25.175000 MHz. >> > >> > ... which is the spec value. >> >> This is where we're not on the same page. Where in the spec does it >> say 25.17500 MHz? I see in the spec: >> 25.2 / 1.001 > > Section 4 of CEA-861-B, which defines the video clock rates and their > accuracy of 0.5%. Then perhaps you shouldn't be using a switch statement. You won't catch all values that are within .05% of (25.2 / 1.001). >> ...and this is a crucial difference here. Please double-check my math, but: >> >> (25175000 * 4576) / (128 * 32000.) >> => 28125.1953125 >> >> (25174825 * 4576) / (128 * 32000.) >> => 28125.0 >> >> This calculation is what led to my belief that the goal here is to >> make an integral CTS. If you have 25.175 MHZ clock and N of 4576 you >> _will not_ have an integral CTS. If you instead have 25.174825 MHz >> clock and N of 4576 you _will_ have an integral CTS. > > Right, but 25.175 is close enough to 25.174825. Do this calculation: > > 25175000 * 4576 / 28125 / 128 > > That'll give you the resulting audio sample rate, which is 32000.222Hz. > That's an error of... 0.00069%, which is probably around the typical > error of your average crystal oscillator. Really not worth bothering > with. OK, so do this calculation: 25175000 * 4096 / 25175 / 128 You get 32000.000000000000000000 I'm not saying there's anything terribly wrong with 32000.222 Hz and I'm sure it will work just dandy for you. I'm saying that you're adding complexity _and_ ending up with a slightly worse rate. AKA: just replace your entire "compute_n" function with: return (128 * freq) / 1000; ...and it's 100% simpler _and_ gets you a (marginally) better rate (assuming you really have 22.175000). If it was just about a 32000.222 vs 32000 I'd not be saying anything right now. It's about adding complexity. >> Said another way: >> >> 1. The reason 25174825 Hz has a different N is to make an integral CTS. >> >> 2. If you are indeed making 25175000 then there is no need for a >> different N to make an integral CTS >> >> 3. If you use 4576 for N but you're making 25175000 Hz, you end up in >> a _worse_ position than if you use the standard 4096 for N. > > Total rubbish. Sorry, but it is. > > Follow the code. Pixel clock is 25175000. For 32kHz, N will be 4576. > 25175000 * 4576 = 1.152008e11. Divide that by the audio clock rate > (128 * 32000) gives 28125.19531. Since we're using integer division, > that gets rounded down to 28125. > > DRM uses a clock rate of "25175" to represent 25.2/1.001 modes. So, > if your hardware sets a video clock rate of 25.2MHz/1.001, then you > end up with a sample rate of exactly 32kHz. If you set exactly > 25.175MHz, you end up with an approximate 32kHz sample rate - one > which is 0.00069% in error, which is (excluse the language) fuck all > different from exactly 32kHz. Agree that the difference is negligible. I will say that IMHO the kind folks who wrote the HDMI spec were still trying their best to make that error 0.00%. That's entirely the reason that they have that table and they don't just use "(128 * freq) / 1000" for everything. AKA, I can imagine something like: Person 1: Is there any reason to pick a N value that's exactly (128 * freq) / 1000? Person 2: Not really Person 1: Hrm, but I notice that I can get a tiny bit more accurate audio clock when I have a pixel clock of (25.2 / 1.001) if I use a N that's not (128 * freq) / 1000. Is that OK? Person 2: Yeah, go ahead. Add it to the spec. Person 1: OK. I've got some nifty tables I can add. Cool! Now we get exactly the right audio clock. Person 2: Nice job! ...but I have no idea if that's really true. > Are you _really_ going to continue arguing over a 0.00069% error? > If you are, I'm not going to listen anymore - it's soo damned small > that it's not worth bothering with. At all. Well, I think I've adequately expressed my opinion. If you want to land your patch, I certainly won't yell. I think it adds extra complexity and produces a (marginally) inferior audio rate, but that's up to the folks who maintain the code to deal with. >> As a side note: I realized one part of the HDMI spec that isn't trying >> to make an integral value but still uses a different value for N: 297 >> MHz. From the DesignWare spec I have it appears that 594 MHz is >> similar. For those cases it looks like we have: > > 297MHz _does_ work. > > 297000000 * 3072 / 222750 = 128 * 32000 exactly. I guess I didn't express myself clearly enough. I'm saying that: * The only reason I can discern for using non "(128 * freq) / 1000" N values for rates < 297 Mhz is to try to make an integral CTS. * For rates >= 297 MHz you could make CTS integral and still keep "(128 * freq) / 1000", but the spec still says to use something different. I don't know why. My formula accurately makes values in the spec for 297 MHz. Anyway, I'm about done commenting on this thread. Feel free to land this if folks are happy with it, but I'd prefer not to have my Reviewed-by on it given all that I've discovered. -Doug
On Fri, Sep 04, 2015 at 07:03:11PM -0700, Doug Anderson wrote: > AKA: just replace your entire "compute_n" function with: > > return (128 * freq) / 1000; > > ...and it's 100% simpler _and_ gets you a (marginally) better rate > (assuming you really have 22.175000). If it was just about a > 32000.222 vs 32000 I'd not be saying anything right now. It's about > adding complexity. No. It doesn't work for all cases. Do the calculations for every sample rate in those tables in the HDMI spec, and you'll find out why.
On Fri, Sep 04, 2015 at 07:03:11PM -0700, Doug Anderson wrote: > Then perhaps you shouldn't be using a switch statement. You won't > catch all values that are within .05% of (25.2 / 1.001). No. The clock rates you get ultimately come from the EDID via either the detailed timing modes or from the CEA mode IDs, which are then looked up in tables in the DRM EDID parsing code. Either way, you will end up with 25175 and not 25170 or something strange based on what the platform does.
Russell, On Sat, Sep 5, 2015 at 1:31 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Sep 04, 2015 at 07:03:11PM -0700, Doug Anderson wrote: >> AKA: just replace your entire "compute_n" function with: >> >> return (128 * freq) / 1000; >> >> ...and it's 100% simpler _and_ gets you a (marginally) better rate >> (assuming you really have 22.175000). If it was just about a >> 32000.222 vs 32000 I'd not be saying anything right now. It's about >> adding complexity. > > No. It doesn't work for all cases. Do the calculations for every > sample rate in those tables in the HDMI spec, and you'll find out > why. If you know the answer, just tell me. If you're talking about 74.25 vs. 32 kHz it is further evidence of what I'm saying. Note that picking only one of the two listed CTS values again puts you in a worse position for regenerating the proper audio clock then just using the default N=4096. -Doug
Russell, On Sat, Sep 5, 2015 at 1:34 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Sep 04, 2015 at 07:03:11PM -0700, Doug Anderson wrote: >> Then perhaps you shouldn't be using a switch statement. You won't >> catch all values that are within .05% of (25.2 / 1.001). > > No. > > The clock rates you get ultimately come from the EDID via either the > detailed timing modes or from the CEA mode IDs, which are then looked > up in tables in the DRM EDID parsing code. I guess in my case the (non-upsteram) code is adjusting the clock in fixup_mode. It's no longer something based on the EDID. Perhaps the fault if there, but... > Either way, you will end up with 25175 and not 25170 or something > strange based on what the platform does. I was talking to someone else about this and I guess the question is whether you should be sending a N/CTS for audio based on the theoretical or the actual clock. If you are supposed to do calculations based on the theoretical clock then you're right. If you are supposed to do calculations based on the actual clock then I'm not so sure. Note that: * I believe that you'll get better audio if you use the actual clock. * If your actual clock is an integral number of kHz, the calculations are simpler by using the actual clock. -Doug
On Sat, Sep 05, 2015 at 06:46:04AM -0700, Doug Anderson wrote: > Russell, > > On Sat, Sep 5, 2015 at 1:31 AM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Fri, Sep 04, 2015 at 07:03:11PM -0700, Doug Anderson wrote: > >> AKA: just replace your entire "compute_n" function with: > >> > >> return (128 * freq) / 1000; > >> > >> ...and it's 100% simpler _and_ gets you a (marginally) better rate > >> (assuming you really have 22.175000). If it was just about a > >> 32000.222 vs 32000 I'd not be saying anything right now. It's about > >> adding complexity. > > > > No. It doesn't work for all cases. Do the calculations for every > > sample rate in those tables in the HDMI spec, and you'll find out > > why. > > If you know the answer, just tell me. If you're talking about 74.25 > vs. 32 kHz it is further evidence of what I'm saying. Note that > picking only one of the two listed CTS values again puts you in a > worse position for regenerating the proper audio clock then just using > the default N=4096. No it doesn't. 74.25MHz/1.001 * 4096 / (128 * 32000) = 74175 (rounded down) Now do the calcuation. (74.25MHz/1.001) / 74175 * 4096 = 4096045.511 => 32000.35556Hz => error of 0.001111% Now for the calcuation using the proscribed figures. (74.25MHz/1.001) / 210937 * 11648 = 4096009.709 => 32000.07585Hz => error of 0.000237% That's significantly less error using that than your "better" idea. Now, if we take the pixel clock rate as 74.175MHz, which is just a representation of 74.25MHz/1.001: 74.175MHz / 210937 * 11648 = 4095964.198 => 31999.72029Hz => error of 0.0008741% That's still lower than your "better" idea. And as I've already said, the pixel clock rate given to us here will be the _specified_ clock rate of 74.175MHz, *not* some cocked up platform screwed crap that you think we will. It _will_ be 74175 not 74170 or some other shite like that. Right, I've had enough. I'm going to be ignoring this thread from now on, this is a waste of my time - you clearly have no understanding of what's going on here.
Hi, On Sat, Sep 5, 2015 at 7:01 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: >> If you know the answer, just tell me. If you're talking about 74.25 >> vs. 32 kHz it is further evidence of what I'm saying. Note that >> picking only one of the two listed CTS values again puts you in a >> worse position for regenerating the proper audio clock then just using >> the default N=4096. > > No it doesn't. > > 74.25MHz/1.001 * 4096 / (128 * 32000) = 74175 (rounded down) > > Now do the calcuation. > > (74.25MHz/1.001) / 74175 * 4096 = 4096045.511 => 32000.35556Hz > => error of 0.001111% > > Now for the calcuation using the proscribed figures. > > (74.25MHz/1.001) / 210937 * 11648 = 4096009.709 => 32000.07585Hz > => error of 0.000237% > Why would you round down??? Round to the closest. (74250000 / 1.001 * 4096) / (128 * 32000.) => 74175.82417582418 => 74176 (74250000 / 1.001) / 74176 * 4096 / 128 => 31999.924148327947 That's actually the same error as yours: 0.000237% You're right. Yours isn't worse, but it's also not any better.
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c index f0e6059f818a..5576cd7d7abb 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi.c +++ b/drivers/gpu/drm/bridge/dw_hdmi.c @@ -230,11 +230,11 @@ static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk, switch (freq) { case 32000: - if (pixel_clk == 25170000) + if (pixel_clk == 25175000) n = (ratio == 150) ? 9152 : 4576; - else if (pixel_clk == 27020000) + else if (pixel_clk == 27027000) n = (ratio == 150) ? 8192 : 4096; - else if (pixel_clk == 74170000 || pixel_clk == 148350000) + else if (pixel_clk == 74176000 || pixel_clk == 148352000) n = 11648; else n = 4096; @@ -242,11 +242,11 @@ static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk, break; case 44100: - if (pixel_clk == 25170000) + if (pixel_clk == 25175000) n = 7007; - else if (pixel_clk == 74170000) + else if (pixel_clk == 74176000) n = 17836; - else if (pixel_clk == 148350000) + else if (pixel_clk == 148352000) n = (ratio == 150) ? 17836 : 8918; else n = 6272; @@ -254,13 +254,13 @@ static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk, break; case 48000: - if (pixel_clk == 25170000) + if (pixel_clk == 25175000) n = (ratio == 150) ? 9152 : 6864; - else if (pixel_clk == 27020000) + else if (pixel_clk == 27027000) n = (ratio == 150) ? 8192 : 6144; - else if (pixel_clk == 74170000) + else if (pixel_clk == 74176000) n = 11648; - else if (pixel_clk == 148350000) + else if (pixel_clk == 148352000) n = (ratio == 150) ? 11648 : 5824; else n = 6144;
Adjust the pixel clock values in the N calculation to match the more accurate clock values we're given by the DRM subsystem, which are the kHz pixel rate, with any fractional kHz rounded down in the case of the non-240, non-480 line modes, or rounded up for the others. So, 25.20 / 1.001 => 25175 27.00 * 1.001 => 27027 74.25 / 1.001 => 74176 148.50 / 1.001 => 148352 Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/gpu/drm/bridge/dw_hdmi.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)