From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760834AbbIECDX (ORCPT ); Fri, 4 Sep 2015 22:03:23 -0400 Received: from mail-yk0-f169.google.com ([209.85.160.169]:34460 "EHLO mail-yk0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760350AbbIECDN (ORCPT ); Fri, 4 Sep 2015 22:03:13 -0400 MIME-Version: 1.0 In-Reply-To: <20150905002733.GJ21084@n2100.arm.linux.org.uk> References: <20150808160936.GN7557@n2100.arm.linux.org.uk> <20150904212401.GI21084@n2100.arm.linux.org.uk> <20150905002733.GJ21084@n2100.arm.linux.org.uk> Date: Fri, 4 Sep 2015 19:03:11 -0700 X-Google-Sender-Auth: fphPB3_dvWgKqgX4hDegiN4q1lM 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 5:27 PM, Russell King - ARM Linux 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 >> 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 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 19:03:11 -0700 Message-ID: References: <20150808160936.GN7557@n2100.arm.linux.org.uk> <20150904212401.GI21084@n2100.arm.linux.org.uk> <20150905002733.GJ21084@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: <20150905002733.GJ21084@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 UnVzc2VsbCwKCk9uIEZyaSwgU2VwIDQsIDIwMTUgYXQgNToyNyBQTSwgUnVzc2VsbCBLaW5nIC0g QVJNIExpbnV4CjxsaW51eEBhcm0ubGludXgub3JnLnVrPiB3cm90ZToKPiBPbiBGcmksIFNlcCAw NCwgMjAxNSBhdCAwNDo1MDowM1BNIC0wNzAwLCBEb3VnIEFuZGVyc29uIHdyb3RlOgo+PiBSdXNz ZWxsLAo+Pgo+PiBPbiBGcmksIFNlcCA0LCAyMDE1IGF0IDI6MjQgUE0sIFJ1c3NlbGwgS2luZyAt IEFSTSBMaW51eAo+PiA8bGludXhAYXJtLmxpbnV4Lm9yZy51az4gd3JvdGU6Cj4+ID4+IEluIHlv dXIgY2FzZSB5b3UncmUgcHJvYmFibHkgbWFraW5nIHRoZSB2YWx1ZSB0aGF0IExpbnV4Cj4+ID4+ IGFza2VkIHlvdSB0byBtYWtlLCBBS0EgMjUuMTc1MDAwIE1Iei4KPj4gPgo+PiA+IC4uLiB3aGlj aCBpcyB0aGUgc3BlYyB2YWx1ZS4KPj4KPj4gVGhpcyBpcyB3aGVyZSB3ZSdyZSBub3Qgb24gdGhl IHNhbWUgcGFnZS4gIFdoZXJlIGluIHRoZSBzcGVjIGRvZXMgaXQKPj4gc2F5IDI1LjE3NTAwIE1I ej8gIEkgc2VlIGluIHRoZSBzcGVjOgo+PiAgMjUuMiAvIDEuMDAxCj4KPiBTZWN0aW9uIDQgb2Yg Q0VBLTg2MS1CLCB3aGljaCBkZWZpbmVzIHRoZSB2aWRlbyBjbG9jayByYXRlcyBhbmQgdGhlaXIK PiBhY2N1cmFjeSBvZiAwLjUlLgoKVGhlbiBwZXJoYXBzIHlvdSBzaG91bGRuJ3QgYmUgdXNpbmcg YSBzd2l0Y2ggc3RhdGVtZW50LiAgWW91IHdvbid0CmNhdGNoIGFsbCB2YWx1ZXMgdGhhdCBhcmUg d2l0aGluIC4wNSUgb2YgKDI1LjIgLyAxLjAwMSkuCgoKPj4gLi4uYW5kIHRoaXMgaXMgYSBjcnVj aWFsIGRpZmZlcmVuY2UgaGVyZS4gIFBsZWFzZSBkb3VibGUtY2hlY2sgbXkgbWF0aCwgYnV0Ogo+ Pgo+PiAoMjUxNzUwMDAgKiA0NTc2KSAvICgxMjggKiAzMjAwMC4pCj4+ID0+IDI4MTI1LjE5NTMx MjUKPj4KPj4gKDI1MTc0ODI1ICogNDU3NikgLyAoMTI4ICogMzIwMDAuKQo+PiA9PiAyODEyNS4w Cj4+Cj4+IFRoaXMgY2FsY3VsYXRpb24gaXMgd2hhdCBsZWQgdG8gbXkgYmVsaWVmIHRoYXQgdGhl IGdvYWwgaGVyZSBpcyB0bwo+PiBtYWtlIGFuIGludGVncmFsIENUUy4gIElmIHlvdSBoYXZlIDI1 LjE3NSBNSFogY2xvY2sgYW5kIE4gb2YgNDU3NiB5b3UKPj4gX3dpbGwgbm90XyBoYXZlIGFuIGlu dGVncmFsIENUUy4gIElmIHlvdSBpbnN0ZWFkIGhhdmUgMjUuMTc0ODI1IE1Iego+PiBjbG9jayBh bmQgTiBvZiA0NTc2IHlvdSBfd2lsbF8gaGF2ZSBhbiBpbnRlZ3JhbCBDVFMuCj4KPiBSaWdodCwg YnV0IDI1LjE3NSBpcyBjbG9zZSBlbm91Z2ggdG8gMjUuMTc0ODI1LiAgRG8gdGhpcyBjYWxjdWxh dGlvbjoKPgo+IDI1MTc1MDAwICogNDU3NiAvIDI4MTI1IC8gMTI4Cj4KPiBUaGF0J2xsIGdpdmUg eW91IHRoZSByZXN1bHRpbmcgYXVkaW8gc2FtcGxlIHJhdGUsIHdoaWNoIGlzIDMyMDAwLjIyMkh6 Lgo+IFRoYXQncyBhbiBlcnJvciBvZi4uLiAwLjAwMDY5JSwgd2hpY2ggaXMgcHJvYmFibHkgYXJv dW5kIHRoZSB0eXBpY2FsCj4gZXJyb3Igb2YgeW91ciBhdmVyYWdlIGNyeXN0YWwgb3NjaWxsYXRv ci4gIFJlYWxseSBub3Qgd29ydGggYm90aGVyaW5nCj4gd2l0aC4KCk9LLCBzbyBkbyB0aGlzIGNh bGN1bGF0aW9uOgoKMjUxNzUwMDAgKiA0MDk2IC8gMjUxNzUgLyAxMjgKCllvdSBnZXQgMzIwMDAu MDAwMDAwMDAwMDAwMDAwMDAwCgpJJ20gbm90IHNheWluZyB0aGVyZSdzIGFueXRoaW5nIHRlcnJp Ymx5IHdyb25nIHdpdGggMzIwMDAuMjIyIEh6IGFuZApJJ20gc3VyZSBpdCB3aWxsIHdvcmsganVz dCBkYW5keSBmb3IgeW91LiAgSSdtIHNheWluZyB0aGF0IHlvdSdyZQphZGRpbmcgY29tcGxleGl0 eSBfYW5kXyBlbmRpbmcgdXAgd2l0aCBhIHNsaWdodGx5IHdvcnNlIHJhdGUuCgpBS0E6IGp1c3Qg cmVwbGFjZSB5b3VyIGVudGlyZSAiY29tcHV0ZV9uIiBmdW5jdGlvbiB3aXRoOgoKcmV0dXJuICgx MjggKiBmcmVxKSAvIDEwMDA7CgouLi5hbmQgaXQncyAxMDAlIHNpbXBsZXIgX2FuZF8gZ2V0cyB5 b3UgYSAobWFyZ2luYWxseSkgYmV0dGVyIHJhdGUKKGFzc3VtaW5nIHlvdSByZWFsbHkgaGF2ZSAy Mi4xNzUwMDApLiAgSWYgaXQgd2FzIGp1c3QgYWJvdXQgYQozMjAwMC4yMjIgdnMgMzIwMDAgSSdk IG5vdCBiZSBzYXlpbmcgYW55dGhpbmcgcmlnaHQgbm93LiAgSXQncyBhYm91dAphZGRpbmcgY29t cGxleGl0eS4KCgo+PiBTYWlkIGFub3RoZXIgd2F5Ogo+Pgo+PiAxLiBUaGUgcmVhc29uIDI1MTc0 ODI1IEh6IGhhcyBhIGRpZmZlcmVudCBOIGlzIHRvIG1ha2UgYW4gaW50ZWdyYWwgQ1RTLgo+Pgo+ PiAyLiBJZiB5b3UgYXJlIGluZGVlZCBtYWtpbmcgMjUxNzUwMDAgdGhlbiB0aGVyZSBpcyBubyBu ZWVkIGZvciBhCj4+IGRpZmZlcmVudCBOIHRvIG1ha2UgYW4gaW50ZWdyYWwgQ1RTCj4+Cj4+IDMu IElmIHlvdSB1c2UgNDU3NiBmb3IgTiBidXQgeW91J3JlIG1ha2luZyAyNTE3NTAwMCBIeiwgeW91 IGVuZCB1cCBpbgo+PiBhIF93b3JzZV8gcG9zaXRpb24gdGhhbiBpZiB5b3UgdXNlIHRoZSBzdGFu ZGFyZCA0MDk2IGZvciBOLgo+Cj4gVG90YWwgcnViYmlzaC4gIFNvcnJ5LCBidXQgaXQgaXMuCj4K PiBGb2xsb3cgdGhlIGNvZGUuICBQaXhlbCBjbG9jayBpcyAyNTE3NTAwMC4gIEZvciAzMmtIeiwg TiB3aWxsIGJlIDQ1NzYuCj4gMjUxNzUwMDAgKiA0NTc2ID0gMS4xNTIwMDhlMTEuICBEaXZpZGUg dGhhdCBieSB0aGUgYXVkaW8gY2xvY2sgcmF0ZQo+ICgxMjggKiAzMjAwMCkgZ2l2ZXMgMjgxMjUu MTk1MzEuICBTaW5jZSB3ZSdyZSB1c2luZyBpbnRlZ2VyIGRpdmlzaW9uLAo+IHRoYXQgZ2V0cyBy b3VuZGVkIGRvd24gdG8gMjgxMjUuCj4KPiBEUk0gdXNlcyBhIGNsb2NrIHJhdGUgb2YgIjI1MTc1 IiB0byByZXByZXNlbnQgMjUuMi8xLjAwMSBtb2Rlcy4gIFNvLAo+IGlmIHlvdXIgaGFyZHdhcmUg c2V0cyBhIHZpZGVvIGNsb2NrIHJhdGUgb2YgMjUuMk1Iei8xLjAwMSwgdGhlbiB5b3UKPiBlbmQg dXAgd2l0aCBhIHNhbXBsZSByYXRlIG9mIGV4YWN0bHkgMzJrSHouICBJZiB5b3Ugc2V0IGV4YWN0 bHkKPiAyNS4xNzVNSHosIHlvdSBlbmQgdXAgd2l0aCBhbiBhcHByb3hpbWF0ZSAzMmtIeiBzYW1w bGUgcmF0ZSAtIG9uZQo+IHdoaWNoIGlzIDAuMDAwNjklIGluIGVycm9yLCB3aGljaCBpcyAoZXhj bHVzZSB0aGUgbGFuZ3VhZ2UpIGZ1Y2sgYWxsCj4gZGlmZmVyZW50IGZyb20gZXhhY3RseSAzMmtI ei4KCkFncmVlIHRoYXQgdGhlIGRpZmZlcmVuY2UgaXMgbmVnbGlnaWJsZS4KCkkgd2lsbCBzYXkg dGhhdCBJTUhPIHRoZSBraW5kIGZvbGtzIHdobyB3cm90ZSB0aGUgSERNSSBzcGVjIHdlcmUgc3Rp bGwKdHJ5aW5nIHRoZWlyIGJlc3QgdG8gbWFrZSB0aGF0IGVycm9yIDAuMDAlLiAgVGhhdCdzIGVu dGlyZWx5IHRoZQpyZWFzb24gdGhhdCB0aGV5IGhhdmUgdGhhdCB0YWJsZSBhbmQgdGhleSBkb24n dCBqdXN0IHVzZSAiKDEyOCAqIGZyZXEpCi8gMTAwMCIgZm9yIGV2ZXJ5dGhpbmcuCgpBS0EsIEkg Y2FuIGltYWdpbmUgc29tZXRoaW5nIGxpa2U6CgpQZXJzb24gMTogSXMgdGhlcmUgYW55IHJlYXNv biB0byBwaWNrIGEgTiB2YWx1ZSB0aGF0J3MgZXhhY3RseSAoMTI4ICoKZnJlcSkgLyAxMDAwPwoK UGVyc29uIDI6IE5vdCByZWFsbHkKClBlcnNvbiAxOiBIcm0sIGJ1dCBJIG5vdGljZSB0aGF0IEkg Y2FuIGdldCBhIHRpbnkgYml0IG1vcmUgYWNjdXJhdGUKYXVkaW8gY2xvY2sgd2hlbiBJIGhhdmUg YSBwaXhlbCBjbG9jayBvZiAoMjUuMiAvIDEuMDAxKSBpZiBJIHVzZSBhIE4KdGhhdCdzIG5vdCAo MTI4ICogZnJlcSkgLyAxMDAwLiAgSXMgdGhhdCBPSz8KClBlcnNvbiAyOiBZZWFoLCBnbyBhaGVh ZC4gIEFkZCBpdCB0byB0aGUgc3BlYy4KClBlcnNvbiAxOiBPSy4gIEkndmUgZ290IHNvbWUgbmlm dHkgdGFibGVzIEkgY2FuIGFkZC4gIENvb2whICBOb3cgd2UKZ2V0IGV4YWN0bHkgdGhlIHJpZ2h0 IGF1ZGlvIGNsb2NrLgoKUGVyc29uIDI6IE5pY2Ugam9iIQoKLi4uYnV0IEkgaGF2ZSBubyBpZGVh IGlmIHRoYXQncyByZWFsbHkgdHJ1ZS4KCgo+IEFyZSB5b3UgX3JlYWxseV8gZ29pbmcgdG8gY29u dGludWUgYXJndWluZyBvdmVyIGEgMC4wMDA2OSUgZXJyb3I/Cj4gSWYgeW91IGFyZSwgSSdtIG5v dCBnb2luZyB0byBsaXN0ZW4gYW55bW9yZSAtIGl0J3Mgc29vIGRhbW5lZCBzbWFsbAo+IHRoYXQg aXQncyBub3Qgd29ydGggYm90aGVyaW5nIHdpdGguICBBdCBhbGwuCgpXZWxsLCBJIHRoaW5rIEkn dmUgYWRlcXVhdGVseSBleHByZXNzZWQgbXkgb3Bpbmlvbi4gIElmIHlvdSB3YW50IHRvCmxhbmQg eW91ciBwYXRjaCwgSSBjZXJ0YWlubHkgd29uJ3QgeWVsbC4gIEkgdGhpbmsgaXQgYWRkcyBleHRy YQpjb21wbGV4aXR5IGFuZCBwcm9kdWNlcyBhIChtYXJnaW5hbGx5KSBpbmZlcmlvciBhdWRpbyBy YXRlLCBidXQgdGhhdCdzCnVwIHRvIHRoZSBmb2xrcyB3aG8gbWFpbnRhaW4gdGhlIGNvZGUgdG8g ZGVhbCB3aXRoLgoKCj4+IEFzIGEgc2lkZSBub3RlOiBJIHJlYWxpemVkIG9uZSBwYXJ0IG9mIHRo ZSBIRE1JIHNwZWMgdGhhdCBpc24ndCB0cnlpbmcKPj4gdG8gbWFrZSBhbiBpbnRlZ3JhbCB2YWx1 ZSBidXQgc3RpbGwgdXNlcyBhIGRpZmZlcmVudCB2YWx1ZSBmb3IgTjogMjk3Cj4+IE1Iei4gIEZy b20gdGhlIERlc2lnbldhcmUgc3BlYyBJIGhhdmUgaXQgYXBwZWFycyB0aGF0IDU5NCBNSHogaXMK Pj4gc2ltaWxhci4gIEZvciB0aG9zZSBjYXNlcyBpdCBsb29rcyBsaWtlIHdlIGhhdmU6Cj4KPiAy OTdNSHogX2RvZXNfIHdvcmsuCj4KPiAyOTcwMDAwMDAgKiAzMDcyIC8gMjIyNzUwID0gMTI4ICog MzIwMDAgZXhhY3RseS4KCkkgZ3Vlc3MgSSBkaWRuJ3QgZXhwcmVzcyBteXNlbGYgY2xlYXJseSBl bm91Z2guICBJJ20gc2F5aW5nIHRoYXQ6CgoqIFRoZSBvbmx5IHJlYXNvbiBJIGNhbiBkaXNjZXJu IGZvciB1c2luZyBub24gIigxMjggKiBmcmVxKSAvIDEwMDAiIE4KdmFsdWVzIGZvciByYXRlcyA8 IDI5NyBNaHogaXMgdG8gdHJ5IHRvIG1ha2UgYW4gaW50ZWdyYWwgQ1RTLgoKKiBGb3IgcmF0ZXMg Pj0gMjk3IE1IeiB5b3UgY291bGQgbWFrZSBDVFMgaW50ZWdyYWwgYW5kIHN0aWxsIGtlZXAKIigx MjggKiBmcmVxKSAvIDEwMDAiLCBidXQgdGhlIHNwZWMgc3RpbGwgc2F5cyB0byB1c2Ugc29tZXRo aW5nCmRpZmZlcmVudC4gIEkgZG9uJ3Qga25vdyB3aHkuICBNeSBmb3JtdWxhIGFjY3VyYXRlbHkg bWFrZXMgdmFsdWVzIGluCnRoZSBzcGVjIGZvciAyOTcgTUh6LgoKCkFueXdheSwgSSdtIGFib3V0 IGRvbmUgY29tbWVudGluZyBvbiB0aGlzIHRocmVhZC4gIEZlZWwgZnJlZSB0byBsYW5kCnRoaXMg aWYgZm9sa3MgYXJlIGhhcHB5IHdpdGggaXQsIGJ1dCBJJ2QgcHJlZmVyIG5vdCB0byBoYXZlIG15 ClJldmlld2VkLWJ5IG9uIGl0IGdpdmVuIGFsbCB0aGF0IEkndmUgZGlzY292ZXJlZC4KCi1Eb3Vn Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZl bCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: dianders@chromium.org (Doug Anderson) Date: Fri, 4 Sep 2015 19:03:11 -0700 Subject: [PATCH 6/9] drm: bridge/dw_hdmi: adjust pixel clock values in N calculation In-Reply-To: <20150905002733.GJ21084@n2100.arm.linux.org.uk> References: <20150808160936.GN7557@n2100.arm.linux.org.uk> <20150904212401.GI21084@n2100.arm.linux.org.uk> <20150905002733.GJ21084@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 5:27 PM, Russell King - ARM Linux 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 >> 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