[v4,1/2] video: hdmi: add helper functions for N and CTS

[Doug Anderson]

Arnaud,

On Tue, Jun 7, 2016 at 1:41 AM, Arnaud Pouliquen
<arnaud.pouliquen at 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 at 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