From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760775AbbIDXuJ (ORCPT ); Fri, 4 Sep 2015 19:50:09 -0400 Received: from mail-vk0-f52.google.com ([209.85.213.52]:35642 "EHLO mail-vk0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760680AbbIDXuE (ORCPT ); Fri, 4 Sep 2015 19:50:04 -0400 MIME-Version: 1.0 In-Reply-To: <20150904212401.GI21084@n2100.arm.linux.org.uk> References: <20150808160936.GN7557@n2100.arm.linux.org.uk> <20150904212401.GI21084@n2100.arm.linux.org.uk> Date: Fri, 4 Sep 2015 16:50:03 -0700 X-Google-Sender-Auth: dNEQJ1O0i4hr1IIdGGPMiEYwuAo Message-ID: Subject: Re: [PATCH 6/9] drm: bridge/dw_hdmi: adjust pixel clock values in N calculation From: Doug Anderson To: Russell King - ARM Linux Cc: "open list:ARM/Rockchip SoC..." , alsa-devel@alsa-project.org, "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Philipp Zabel , Andy Yan , Yakir Yang , Fabio Estevam , Mark Brown , Takashi Iwai , Jaroslav Kysela , Sascha Hauer , Jon Nettleton , David Airlie Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Russell, On Fri, Sep 4, 2015 at 2:24 PM, Russell King - ARM Linux 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; } } From mboxrd@z Thu Jan 1 00:00:00 1970 From: Doug Anderson Subject: Re: [PATCH 6/9] drm: bridge/dw_hdmi: adjust pixel clock values in N calculation Date: Fri, 4 Sep 2015 16:50:03 -0700 Message-ID: References: <20150808160936.GN7557@n2100.arm.linux.org.uk> <20150904212401.GI21084@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20150904212401.GI21084@n2100.arm.linux.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Russell King - ARM Linux Cc: Fabio Estevam , alsa-devel@alsa-project.org, "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , Jaroslav Kysela , "open list:ARM/Rockchip SoC..." , Mark Brown , Yakir Yang , Andy Yan , "linux-arm-kernel@lists.infradead.org" List-Id: alsa-devel@alsa-project.org UnVzc2VsbCwKCk9uIEZyaSwgU2VwIDQsIDIwMTUgYXQgMjoyNCBQTSwgUnVzc2VsbCBLaW5nIC0g QVJNIExpbnV4CjxsaW51eEBhcm0ubGludXgub3JnLnVrPiB3cm90ZToKPj4gQmFzaWNhbGx5IHRo ZSBzcGVjIGlzIHNheWluZyB0aGF0IHlvdSB3YW50IGJvdGggTiBhbmQgQ1RTIHRvIGJlCj4+IGlu dGVncmFsLiAgLi4uYXMgeW91IHNheSB5b3UgcmVhbGx5IHdhbnQ6Cj4+ICAgQ1RTID0gKFRNRFMg KiBOKSAvICgxMjggKiBhdWRpb19mcmVxKQo+Cj4gSW4gdGhlIGNhc2Ugb2Ygc29mdHdhcmUtcHJv Z3JhbW1lZCBDVFMgYW5kIE4gdmFsdWVzLCB0aGV5IGhhdmUgdG8gYmUKPiBpbnRlZ3JhbCBiZWNh dXNlIHRoZXJlJ3Mgbm8gc3VjaCB0aGluZyBhcyBmcmFjdGlvbmFsIGRpdmlzaW9uIGhlcmUuCj4g VGhlIENUUyBhbmQgTiB2YWx1ZXMgZ2V0IHNlbnQgYWNyb3NzIHRoZSBIRE1JIGxpbmsgdG8gdGhl IHNpbmssIGFuZAo+IHRoZXkgdXNlIHRob3NlIGluIGEgUExMIGxpa2UgYXJyYW5nZW1lbnQgdG8g ZGVyaXZlIHRoZSBhdWRpbyBjbG9jay4KPgo+IE1vcmUgImludGVsaWdlbnQiIGhhcmR3YXJlIGF1 dG9tYXRpY2FsbHkgbWVhc3VyZXMgdGhlIENUUyBudW1iZXIgYW5kCj4gY29udGludWFsbHkgdXBk YXRlcyB0aGUgc2luaywgd2hpY2ggYWxsb3dzIHRoZSBzaW5rIHRvIHJlbWFpbiBpbgo+IHN5bmMg d2l0aCB0aGUgYXVkaW8gYXQgbm9uLWNvaGVyZW50IHJhdGVzLgo+Cj4+IC4uLkNUUyBoYXMgbm8g b3RoZXIgcmVzdHJpY3Rpb25zIChvdGhlciB0aGFuIGJlaW5nIGludGVncmFsKSBhbmQKPj4geW91 J3JlIGFsbG93ZWQgYSBiaXQgb2Ygc2xvcCBmb3IgTiAoeW91IGFpbSBmb3IgMTI4ICogYXVkaW9f ZnJlcSBidXQKPj4gY2FuIGdvIHVwIG9yIGRvd24gYSBiaXQpLgo+Cj4gTm8uICBCb3RoIENUUyBh bmQgTiBoYXZlIHRvIGJlIGFjY3VyYXRlIHRvIGdlbmVyYXRlIHRoZSBjb3JyZWN0Cj4gc2FtcGxl IHJhdGUgZnJvbSB0aGUgVERNUyBjbG9jay4KCkkgZ3Vlc3MgYnkgIm90aGVyIiBJIG1lYW50IG5v IHJlc3RyaWN0aW9ucyBvdGhlciB0aGFuIHRoYXQsIHdoaWNoIGlzCmxpc3RlZCBhYm92ZSB0aGF0 ICJDVFMgPSAoVE1EUyAqIE4pIC8gKDEyOCAqIGF1ZGlvX2ZyZXEpIi4gIEFueXdheSwKc291bmRz IGxpa2Ugd2UncmUgb24gdGhlIHNhbWUgcGFnZSBoZXJlLi4uCgoKPj4gQXBwYXJlbnRseSBpdCdz IG1vcmUgaW1wb3J0YW50IHRvIG9wdGltaXplIGZvciB0aGUgQ1RTIGZvcm11bGEgd29ya2luZwo+ PiBvdXQgdGhlbiBpdCBpcyBmb3IgZ2V0dGluZyBjbG9zZSB0byAiMTI4ICogYXVkaW8gZnJlcSIu ICAuLi5hbmQgdGhhdCdzCj4+IHRoZSByZWFzb24gZm9yIHRoZXNlIHNwZWNpYWwgY2FzZSBOIHZh bHVlcy4uLgo+Cj4gVGhlICIxMjggKiBhdWRpbyBmcmVxIiBpcyBqdXN0IGEgcmVjb21tZW5kYXRp b24uICBHb2luZyB0aHJvdWdoIHRoZSBIRE1JCj4gc3BlYydzIHJlY29tbWVuZGVkIHZhbHVlcyBm b3IgdmFyaW91cyBjbG9jayByYXRlcyBhbmQgc2FtcGxlIHJhdGVzCj4gcmV2ZWFscyB0aGF0IHF1 aXRlIGEgbnVtYmVyIG9mIHRoZW0gYXJlIGZhciBmcm9tIHRoaXMgInJlY29tbWVuZGF0aW9uIi4K Pgo+IFNvIEkgd291bGRuJ3QgcmVhZCB0b28gbXVjaCBpbnRvIHRoZSAiMTI4ICogYXVkaW8gZnJl cSIgdGhpbmcuCgpBZ2Fpbiwgc2FtZSBwYWdlLgoKCj4+IFNvIHRvIHB1dCBzb21lIG51bWJlcnM6 Cj4+Cj4+IFdlJ3JlIHBlcmZlY3Qgd2hlbiB3ZSBoYXZlIGV4YWN0bHkgMjUuMjoKPj4gICAyNTIw MCAqIDQwOTYgLyAoMTI4ICogMzIpCj4+ICAgPT4gMjUyMDAsIHNvIENUUyBmb3IgMjUuMiBNSHog aXMgMjUyMDAuICBQZXJmZWN0Cj4+Cj4+IC4uLmJ1dCB3aGVuIHdlIGhhdmUgMjUuMiAvIDEuMDAx IHdlIGdldCBhIG5vbi1pbnRlZ3JhbCBDVFM6Cj4+ICAgKDI1MjAwIC8gMS4wMDEpICogNDA5NiAv ICgxMjggKiAzMikKPj4gICA9PiAyNTE3NC44MjUxNzQ4MjUxOAo+Pgo+PiAuLi53ZSBjYW4gZ2V0 IGFuIGludGVncmFsIENUUyBhbmQgc3RpbGwgcmVtYWluIGluIHJhbmdlIGlmOgo+PiAgICgyNTIw MCAvIDEuMDAxKSAqIDQ1NzYgLyAoMTI4ICogMzIpCj4+ICAgPT4gMjgxMjUKPgo+IENvcnJlY3Qu ICBUaGVzZSBhcmUgdGhlIHZhbHVlcyBnaXZlbiBpbiB0aGUgSERNSSBzcGVjaWZpY2F0aW9uIGZv ciBlYWNoCj4gb2YgeW91ciBjbG9jayByYXRlcyB5b3UgbWVudGlvbiBhYm92ZS4KPgo+IFlvdSBj YW4gZXZlbiB1c2UgNDA5NiBmb3IgTiBfcHJvdmlkZWRfIHRoZSBzb3VyY2UgbWVhc3VyZXMgYW5k IHNlbmRzCj4gdGhlIENUUyB2YWx1ZSAodGhhdCdzIGJhc2ljYWxseSB3aGF0IGhhcHBlbnMgaW4g dGhlIGNhc2Ugb2YKPiAibm9uLWNvaGVyZW50IiBjbG9ja3MuKQo+Cj4+IEluIHRoZSBjYXNlIG9m IExpbnV4LCBJJ20gYWZyYWlkIHdlIGp1c3QgZG9uJ3QgaGF2ZSB0aGlzIHR5cGUgb2YKPj4gYWNj dXJhY3kgaW4gb3VyIEFQSXMuCj4KPiBXZSBkb24ndCBoYXZlIHRoYXQga2luZCBvZiBwcmVjaXNp b24gaW4gdGhlIERSTSBBUEksIGJ1dCB3ZSBkbyBoYXZlIHRoZQo+IHByZWNpc2lvbiBpbiB0aGUg Y2xvY2sgQVBJLgoKWXVwLiAgT24gdGhlIHNhbWUgcGFnZS4gIFNlZSBteSBzdWdnZXN0aW9ucyBv ZiB1c2luZyB0aGUgY29tbW9uIGNsb2NrIGZyYW1ld29yay4KCgo+PiBUaGUgc3BlYyBpcyB0YWxr aW5nIGFib3V0IG1ha2luZyAyNS4xNzQ4MjUxNzQ4MjUxOCBNSHouCj4KPiArLy0gMC41JSwgYWNj b3JkaW5nIHRvIENFQS04NjEtQi4KPgo+PiBBcyBJIHNhaWQsIGluIG15IGNhc2UgSSdtIGFjdHVh bGx5IG1ha2luZyAyNTE3MDczMi4KPgo+IC4uLiB3aGljaCBpcyB3aXRoaW4gMC4wMiUsIHNvIGlz IHdpdGhpbiBzcGVjLgoKWXVwLCB0aGF0J3Mgd2h5IHdlJ3JlIGRvaW5nIGl0LiAgTm90ZSB0aGF0 IHRvdGFsIGppdHRlciBoYXMgdG8gYmUKdW5kZXIgKy8tIDAuNSUgcmlnaHQ/ICAuLi5zbyBpZiB5 b3UndmUgZ290IGVycm9yIGhlcmUgeW91J3ZlIGdvdCB0bwptYWtlIHN1cmUgeW91ciBjbG9jayBp cyBleHRyYSBjbGVhbiBJIHRoaW5rLgoKCj4+IEluIHlvdXIgY2FzZSB5b3UncmUgcHJvYmFibHkg bWFraW5nIHRoZSB2YWx1ZSB0aGF0IExpbnV4Cj4+IGFza2VkIHlvdSB0byBtYWtlLCBBS0EgMjUu MTc1MDAwIE1Iei4KPgo+IC4uLiB3aGljaCBpcyB0aGUgc3BlYyB2YWx1ZS4KClRoaXMgaXMgd2hl cmUgd2UncmUgbm90IG9uIHRoZSBzYW1lIHBhZ2UuICBXaGVyZSBpbiB0aGUgc3BlYyBkb2VzIGl0 CnNheSAyNS4xNzUwMCBNSHo/ICBJIHNlZSBpbiB0aGUgc3BlYzoKIDI1LjIgLyAxLjAwMQoKLi4u YW5kIHRoaXMgaXMgYSBjcnVjaWFsIGRpZmZlcmVuY2UgaGVyZS4gIFBsZWFzZSBkb3VibGUtY2hl Y2sgbXkgbWF0aCwgYnV0OgoKKDI1MTc1MDAwICogNDU3NikgLyAoMTI4ICogMzIwMDAuKQo9PiAy ODEyNS4xOTUzMTI1CgooMjUxNzQ4MjUgKiA0NTc2KSAvICgxMjggKiAzMjAwMC4pCj0+IDI4MTI1 LjAKClRoaXMgY2FsY3VsYXRpb24gaXMgd2hhdCBsZWQgdG8gbXkgYmVsaWVmIHRoYXQgdGhlIGdv YWwgaGVyZSBpcyB0bwptYWtlIGFuIGludGVncmFsIENUUy4gIElmIHlvdSBoYXZlIDI1LjE3NSBN SFogY2xvY2sgYW5kIE4gb2YgNDU3NiB5b3UKX3dpbGwgbm90XyBoYXZlIGFuIGludGVncmFsIENU Uy4gIElmIHlvdSBpbnN0ZWFkIGhhdmUgMjUuMTc0ODI1IE1IegpjbG9jayBhbmQgTiBvZiA0NTc2 IHlvdSBfd2lsbF8gaGF2ZSBhbiBpbnRlZ3JhbCBDVFMuCgpTYWlkIGFub3RoZXIgd2F5OgoKMS4g VGhlIHJlYXNvbiAyNTE3NDgyNSBIeiBoYXMgYSBkaWZmZXJlbnQgTiBpcyB0byBtYWtlIGFuIGlu dGVncmFsIENUUy4KCjIuIElmIHlvdSBhcmUgaW5kZWVkIG1ha2luZyAyNTE3NTAwMCB0aGVuIHRo ZXJlIGlzIG5vIG5lZWQgZm9yIGEKZGlmZmVyZW50IE4gdG8gbWFrZSBhbiBpbnRlZ3JhbCBDVFMK CjMuIElmIHlvdSB1c2UgNDU3NiBmb3IgTiBidXQgeW91J3JlIG1ha2luZyAyNTE3NTAwMCBIeiwg eW91IGVuZCB1cCBpbgphIF93b3JzZV8gcG9zaXRpb24gdGhhbiBpZiB5b3UgdXNlIHRoZSBzdGFu ZGFyZCA0MDk2IGZvciBOLgoKCj4+IFVuc3VycHJpc2luZ2x5LCBpZiB5b3UgZG8gdGhlCj4+IGNh bGN1bGF0aW9ucyB3aXRoIDI1LjE3NSBNSHogKG9yIGFueSBpbnRlZ3JhbCBrSHogdmFsdWUpIHlv dSBkb24ndAo+PiBoYXZlIHRvIGRvIGFueSBzcGVjaWFsIG9wdGltaXphdGlvbiB0byBzdGF5IGlu dGVncmFsOgo+Pgo+PiAgIDI1MTc1ICogNDA5NiAvICgxMjggKiAzMikKPj4gICA9PiAyNTE3NQo+ Pgo+Pgo+PiBTbyB1bmxlc3MgeW91IGhhdmUgc29tZSB3YXkgdG8ga25vdyB0aGF0IHRoZSB1bmRl cmx5aW5nIGNsb2NrIGlzCj4+IGFjdHVhbGx5ICgyNS4yIC8gMS4wMDEpIE1IeiBhbmQgbm90IGp1 c3QgMjUuMTc1IE1IeiB0aGVuIHlvdXIgcGF0Y2gKPj4gbG9va3Mgd3JvbmcuCj4KPiBJIGRvbid0 IGJlbGlldmUgeW91IGNhbiBtYWtlIHRoYXQgc3RhdGVtZW50LiAgSWYgeW91IHdpc2ggdG8gdGFr ZSB0aGUKPiBsYWNrIG9mIHByZWNpc2lvbiB1cCB3aXRoIHRoZSBhdXRob3JzIG9mIHRoZSBDRUEt ODYxIGFuZCBIRE1JCj4gc3BlY2lmaWNhdGlvbnMsIHNpbmNlIHRoZXkgImFwcHJveGltYXRlIiB0 byB0aGUgdmFsdWVzIEkgaGF2ZSBpbiB0aGlzCj4gcGF0Y2gsIGFuZCBhcmUgd2hhdCB1c2Vyc3Bh Y2UgcGFzc2VzIGluIHRoZSBtb2RlIHN0cnVjdHVyZXMgdG8ga2VybmVsCj4gc3BhY2UuCgpJIHdp bGwgcmVwZWF0IG15IG1hbnRyYTogSSdtIGEgdmlzaXRvciBoZXJlIGFuZCBkZWNpZGVkbHkgbm90 IGFuCmV4cGVydC4gIEhvd2V2ZXIsIGZyb20gbXkgcmVhZGluZyBvZiB0aGUgSERNSSBzcGVjIHNo b3dzIHRoYXQgdGhlIHNwZWMKaXRzZWxmIGlzIGZpbmUuICBUaGV5IGFyZSBqdXN0IGFzc3VtaW5n IHRoYXQgeW91J3JlIHByb3ZpZGluZyBhCjI1LjE3NDgyNSBNSHogY2xvY2sgYW5kIGdpdmluZyB5 b3Ugb3B0aW1pemVkIHZhbHVlcyBmb3Igc2FpZCBjbG9jay4KCklmIHRoZSBjdXJyZW50IGRyaXZl ciBzYXlzIHRoYXQgaXQncyBwcm92aWRpbmcgMjUuMTc1MDAwIE1IeiB0aGVuIHlvdQpzaG91bGRu J3QgYXNzdW1lIHRoYXQgaXQncyBhY3R1YWxseSBtYWtpbmcgMjUuMTc0ODI1IE1IegoKCj4+IEFz IGEgZmlyc3Qgc3RlcCBJJ2Qgc3VnZ2VzdCBqdXN0IHJlbW92aW5nIGFsbCB0aGUgc3BlY2lhbCBj YXNlcyBhbmQKPj4gYWRkIGEgY29tbWVudC4gIEZyb20gcmVhbCB3b3JsZCB0ZXN0aW5nIGl0IGRv ZXNuJ3Qgc2VlbSB0ZXJyaWJseQo+PiBjcml0aWNhbCB0byBiZSBzbGlnaHRseSBvZmYgb24gQ1RT LiAgLi4uYW5kIGluIGFueSBjYXNlIGZvciBhbnkgY2xvY2sKPj4gcmF0ZXMgZXhjZXB0IHRoZSBz bWFsbCBoYW5kZnVsIGluIHRoZSBIRE1JIHNwZWMgd2UnbGwgYmUgc2xpZ2h0bHkgb2ZmCj4+IG9u IENUUyBhbnl3YXkuLi4KPgo+IFRoZXkncmUgbm90ICJzcGVjaWFsIGNhc2VzIiBtYWRlIHVwIHRv IGZpdCBzb21ldGhpbmcgLSB0aGV5J3JlIGZyb20gdGhlCj4gdGFibGVzIGluIHRoZSBIRE1JIHNw ZWNpZmljYXRpb24uCgpUaGV5IGFyZSBkZWZpbml0ZWx5ICJzcGVjaWFsIGNhc2VzIi4gIFRoZXJl IGlzIGEgZ2VuZXJhbCBydWxlIGluIHRoZQpjb2RlIHlvdSBwb3N0ZWQgKGFpbSBmb3IgMTI4ICog ZnJlcSkgYW5kIHRoZXNlIGFyZSBjYXNlcyBmb3IgY2VydGFpbgpjbG9ja3MgdGhhdCBhcmUgYW4g ZXhjZXB0aW9uIHRvIHRoZSBnZW5lcmFsIHJ1bGUuICBBS0EgdGhleSBhcmUKc3BlY2lhbCBjYXNl cy4KCkknbSBub3QgYXJndWluZyB0aGF0IHRoZXJlJ3Mgbm90IGEgdmFsaWQgcmVhc29uIGZvciB0 aGVzZSBzcGVjaWFsCmNhc2VzLiAgSSdtIHNpbXBseSBhcmd1aW5nIHRoYXQgdGhlIHNwZWNpYWwg Y2FzZXMgYXJlIGxpa2VseSBmb3IgYQpkaWZmZXJlbnQgc2l0dWF0aW9uIHRoYW4gdGhlIG9uZSB3 ZSdyZSBpbi4KClRoZSBIRE1JIHNwZWMgaXRzZWxmIChsb29zZWx5IGludGVycHJldGVkKSBwcmV0 dHkgbXVjaCBzYXlzOiBpZgp0aGVyZSdzIGFueSBkb3VidCwganVzdCB1c2UgdGhlIGVxdWF0aW9u cy0tZG9uJ3QgdXNlIHRoZSB0YWJsZXMuCgoKPiBUaGF0IGFzc3VtZXMgdGhhdCB0aGUgYXVkaW8g YW5kIHZpZGVvIGNsb2NrcyBhcmUgY29oZXJlbnQuICBPbiBpTVg2Cj4gaGFyZHdhcmUgdXNpbmcg dGhpcywgdGhlIGF1ZGlvIGlzIGNsb2NrZWQgYXQgdGhlIHJhdGUgZGVmaW5lZCBieSB0aGUKPiBU RE1TIGNsb2NrIGFuZCB0aGUgQ1RTL04gdmFsdWVzLgoKSSdsbCBhZG1pdCBJIGhhdmVuJ3QgbG9v a2VkIGF0IHRoZSBhdWRpbyBzZWN0aW9uIG9mIGR3X2hkbWkgbXVjaCwgYnV0CkknZCBpbWFnaW5l IHRoYXQgZm9yIGFsbCB1c2VycyBvZiB0aGlzIGNvbnRyb2xsZXIgLyBQSFkgdGhlIGF1ZGlvIGFu ZAp2aWRlbyBjbG9ja3MgYXJlIGNvaGVyZW50LgoKSSB0aGluayBpbiB0aGUgcGVyZmVjdCB3b3Js ZCB3ZSdkIGJlIGFibGUgdG8gZ2VuZXJhdGUgZXhhY3RseQoyNTE3NDgyNS4xNzQ4MjUxNzcgSHog YW5kIHdlJ2QgdXNlIGFsbCB0aGUgcmF0ZXMgZnJvbSB0aGUgSERNSSBzcGVjLgphbmQgd2UnZCBn ZXQgc3BvdCBvbiAzMiBrSHogYXVkaW8uICAuLi5idXQgSSdtIHNpbXBseSBzYXlpbmcgdGhhdAp3 ZSdyZSBub3QgaW4gdGhhdCBwZXJmZWN0IHdvcmxkIHlldC4KCkFsc28gbm90ZSB0aGF0IHRoZXJl IGFyZSBtYW55IG1hbnkgcmF0ZXMgbm90IGluIHRoZSBIRE1JIHNwZWMgdGhhdApjb3VsZCBiZW5l Zml0IGZyb20gc2ltaWxhciBvcHRpbWl6YXRpb24gb2YgdHJ5aW5nIHRvIGFkanVzdCBOIHRvIG1h a2UKYW4gaW50ZWdyYWwgQ1RTLgoKLS0tCgpBcyBhIHNpZGUgbm90ZTogSSByZWFsaXplZCBvbmUg cGFydCBvZiB0aGUgSERNSSBzcGVjIHRoYXQgaXNuJ3QgdHJ5aW5nCnRvIG1ha2UgYW4gaW50ZWdy YWwgdmFsdWUgYnV0IHN0aWxsIHVzZXMgYSBkaWZmZXJlbnQgdmFsdWUgZm9yIE46IDI5NwpNSHou ICBGcm9tIHRoZSBEZXNpZ25XYXJlIHNwZWMgSSBoYXZlIGl0IGFwcGVhcnMgdGhhdCA1OTQgTUh6 IGlzCnNpbWlsYXIuICBGb3IgdGhvc2UgY2FzZXMgaXQgbG9va3MgbGlrZSB3ZSBoYXZlOgoKaWYg KHBpeGVsX2NsayA9PSAyOTcwMDAwMDApIHsKICBzd2l0Y2ggKGZyZXEpIHsKICBjYXNlIDMyMDAw OgogICAgcmV0dXJuICgxMjggKiBmcmVxKSAvIDEzMzM7CiAgY2FzZSA0NDEwMDoKICBjYXNlIDQ4 MDAwOgogIGNhc2UgODgyMDA6CiAgY2FzZSA5NjAwMDoKICBjYXNlIDE3NjQwMDoKICAgIHJldHVy biAoMTI4ICogZnJlcSkgLyAxMjAwOwogIH0KfSBlbHNlIGlmIChwaXhlbF9jbGsgPT0gNTk0MDAw MDAwKSB7CiAgc3dpdGNoIChmcmVxKSB7CiAgY2FzZSAzMjAwMDoKICAgIHJldHVybiAoMTI4ICog ZnJlcSkgLyAxMzMzOwogIGNhc2UgNDQxMDA6CiAgY2FzZSA4ODIwMDoKICBjYXNlIDE3NjQwMDoK ICAgIHJldHVybiAoMTI4ICogZnJlcSkgLyA2MDA7CiAgfQp9Cl9fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRl dmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21h aWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: dianders@chromium.org (Doug Anderson) Date: Fri, 4 Sep 2015 16:50:03 -0700 Subject: [PATCH 6/9] drm: bridge/dw_hdmi: adjust pixel clock values in N calculation In-Reply-To: <20150904212401.GI21084@n2100.arm.linux.org.uk> References: <20150808160936.GN7557@n2100.arm.linux.org.uk> <20150904212401.GI21084@n2100.arm.linux.org.uk> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Russell, On Fri, Sep 4, 2015 at 2:24 PM, Russell King - ARM Linux 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; } }