Arnaud,
On Tue, Jun 7, 2016 at 1:41 AM, Arnaud Pouliquen arnaud.pouliquen@st.com wrote:
hi Doug,
Thanks for this very interesting feed back.
On my side i'm quite busy on some other topics, and on my platform, CTS is hardware computed. So if you have the experience and the hardware for coherent N and CTS calculations, you are welcome to improve my patch.
Sure. I'm not working on anything related to this at the moment, but I happened to stumble upon your patch and I figured I'd at least post my observations / history in case it was useful to anyone. I know it took me quite some time before I understood where all these magic numbers came from and I figured I'd save others the effort.
If nobody has improved this by the next time I end up working on it, I will definitely post patches.
On 06/06/2016 06:34 PM, Doug Anderson wrote:
Hi,
On Thu, Apr 21, 2016 at 8:29 AM, Arnaud Pouliquen dianders@chromium.org wrote:
Add helper functions to compute HDMI CTS and N parameters. Implementation is based on HDMI 1.4b specification.
It would be super nice to have this somewhere common. Any idea who would land this?
I discussed with Daniel Vetter on DRM IRC, he requests more adherence/commitment on it. So if you are interested in using helpers in your driver that should help :-)
I'm not actively working on any drivers that would use this. In the past I had to dive deep into dw_hdmi on Rockchip SoCs to help fix a bunch of bugs, but it's not something I usually work on. I would have posted my changes upstream but we have enough non-upstream stuff in our dw_hdmi code that it was difficult to really do that. Hopefully next time around...
In general, though, I would support this going someplace common so we didn't need to keep reinventing it. It seems like I've seen this same code several times...
One thing to note is that for all but the non-integral clock rates and the rates >= ~297MHz, all of this can be done programmatically. ...the function I came up with to do that is pretty slow, so a table is still useful in general unless you want to try to optimize things, but it might be nice to have the function available as a fallback? Specifically many TVs will allow audio to work with rates other than the ones in the HDMI spec.
You can see the full implementation we used on some devices I worked on at https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/drivers/gpu/drm/bridge/dw_hdmi.c. Specifically the function for computing N:
static unsigned int hdmi_compute_n(struct dw_hdmi *hdmi, unsigned long pixel_clk) { unsigned int freq = hdmi->sample_rate; unsigned int min_n = DIV_ROUND_UP((128 * freq), 1500); unsigned int max_n = (128 * freq) / 300; unsigned int ideal_n = (128 * freq) / 1000; unsigned int best_n_distance = ideal_n; unsigned int best_n = 0; u64 best_diff = U64_MAX; int n; /* If the ideal N could satisfy the audio math, then just take it */ if (hdmi_audio_math_diff(freq, ideal_n, pixel_clk) == 0) return ideal_n; for (n = min_n; n <= max_n; n++) { u64 diff = hdmi_audio_math_diff(freq, n, pixel_clk); if (diff < best_diff || (diff == best_diff && abs(n - ideal_n) < best_n_distance)) { best_n = n; best_diff = diff; best_n_distance = abs(best_n - ideal_n); } /* * The best N already satisfy the audio math, and also be * the closest value to ideal N, so just cut the loop. */ if ((best_diff == 0) && (abs(n - ideal_n) > best_n_distance)) break; } return best_n; }
Right, I have based my default case algorithm, on HDMI recommendation,
val = (u64)tmds_clk * n_cts->n;n_cts->cts = div64_u64(val, 128UL * audio_fs);but yours seems more accurate. if too slow, a parameter could allows to select between accurate and fast calculation...
Yeah, the HDMI docs I found didn't totally explain what we were trying to accomplish with all their magic numbers and I agree that they suggested just falling back as you say.
My calculations, of course, assume we know the real clock and not just the integral-rounded version of it...
/** Pre-defined frequency not found. Compute CTS using formula:* CTS = (Ftdms_clk * N) / (128 * audio_fs)*/val = (u64)tmds_clk * n_cts->n;n_cts->cts = div64_u64(val, 128UL * audio_fs);n_cts->cts_1_ratio = 0;min = (u64)n_cts->cts * 128UL * audio_fs;if (min < val) {/** Non-accurate value for CTS* compute ratio, needed by user to alternate in ACR* between CTS and CTS + 1 value.*/n_cts->cts_1_ratio = ((u32)(val - min)) * 100 /(128 * audio_fs);}This fallback isn't nearly as nice and will likely lead to audio reconstitution problems. IIRC the problem was periodic audio cutouts of you listened long enough.
This fallback that provides a ratio between the use of the CTS and (CTS+1) value was proposed by Russell, when no CTS accurate value is found. I think it is also interesting to keep it, in addition of your algorithm. This is another way to allow driver to implement a compensation, to avoid audio cut.
Ah, that actually makes tons of sense. Thanks! Yeah, then I agree this is a good idea.
-Doug