[6/9] drm: bridge/dw_hdmi: adjust pixel clock values in N calculation
diff mbox

Message ID E1ZO6hc-0003NW-55@rmk-PC.arm.linux.org.uk
State New
Headers show

Commit Message

Russell King Aug. 8, 2015, 4:10 p.m. UTC
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(-)

Comments

Doug Anderson Sept. 4, 2015, 6:21 p.m. UTC | #1
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>
Doug Anderson Sept. 4, 2015, 7:48 p.m. UTC | #2
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
Russell King - ARM Linux Sept. 4, 2015, 9:24 p.m. UTC | #3
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.
Doug Anderson Sept. 4, 2015, 11:50 p.m. UTC | #4
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;
  }
}
Russell King - ARM Linux Sept. 5, 2015, 12:27 a.m. UTC | #5
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.
Doug Anderson Sept. 5, 2015, 2:03 a.m. UTC | #6
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
Russell King - ARM Linux Sept. 5, 2015, 8:31 a.m. UTC | #7
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.
Russell King - ARM Linux Sept. 5, 2015, 8:34 a.m. UTC | #8
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.
Doug Anderson Sept. 5, 2015, 1:46 p.m. UTC | #9
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
Doug Anderson Sept. 5, 2015, 1:50 p.m. UTC | #10
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
Russell King - ARM Linux Sept. 5, 2015, 2:01 p.m. UTC | #11
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.
Doug Anderson Sept. 5, 2015, 7:44 p.m. UTC | #12
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.

Patch
diff mbox

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;