From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Mon, 18 Dec 2017 13:01:06 +0200 Subject: [PATCH 0/4 v5] Support bridge timings In-Reply-To: <20171215155415.GT26573@phenom.ffwll.local> References: <20171215121047.3650-1-linus.walleij@linaro.org> <20171215155415.GT26573@phenom.ffwll.local> Message-ID: <2682424.BlFQCyBzxh@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Daniel, On Friday, 15 December 2017 17:54:15 EET Daniel Vetter wrote: > On Fri, Dec 15, 2017 at 01:30:24PM +0100, Linus Walleij wrote: > > On Fri, Dec 15, 2017 at 1:10 PM, Linus Walleij wrote: > >> - The connector is apparently not the right abstraction to carry > >> the detailed timings specification between DRI drivers and bridge > >> drivers. > >> > >> - Instead put detailed timing data into the bridge itself as an > >> optional information pointer. > > > > Notice that this is just my fumbling attempts to deal with the situation. > > > > Laurent made me understand what the actual technical problem was, > > how come my pixels were flickering. > > > > Both Laurent and DVetter mentioned that we may need to convey > > information between the bridge and the display engine in some > > way. > > > > Alternatively I could go and hack on adding this to e.g. drm_display_info > > which was used in the previous patch sets by setting the negede flag > > in bus_formats. > > > > I don't know. struct drm_display_info is getting a bit heavy as > > container of misc settings related to "some kind of display". > > The bridge isn't even a display itself, that is on the other side > > of it. So using the connector and treating a bridge as "some kind > > of display" seems wrong too. > > > > Is there a third way? > > If you don't plan to nest bridges too deeply, there is. Atm we have 2 > modes in drm_crtc_state: > > - mode, which is what userspace requested, and what it expects logically > to be the actual real thing. I.e. timing, resolution and all that that > userspace can observe (through plane positioning and vblank timestamps) > should match this mode. For external screens this should also match > what's physically going over the cable. > > - adjusted_mode, which is something entirely undefined and to be used by > drivers internally. Most drivers use it as the thing that's actually > transported between the CRTC and the encoder. > > There's a few common reasons for adjusted mode to be different: > - integrated panel, and your CRTC has a scaler. In that case the > userspace-requested mode is what you feed into into the scaler, and the > adjusted mode is what comes out of your scaler and then goes down the > wire to the panel. > > - your encoder is funky, and e.g. transcodes to the output mode itself, > but expects that you program the input mode always the same. Usual > reasons for this are transcoders that always want non-interlaced mode > (and do the interlacing themselves), if the transcoder has some scaler > itself (some TV-out transcoders had that), or if it has a strict > expectation about signalling edges and stuff (and then transcodes the > signal again). DACs are common doing that. > > Anyway, sounds like your bridge is of the 2nd kind, so all you have to do > is > - in your bridge->mode_fixup function, adjust the adjusted_mode as needed > - in your pl111 driver, program the adjusted mode, not the originally > requested mode > > adjusted mode is set to be a copy of the requested mode by atomic helpers, > so this should keep working as-is on any other bridge driver. I don't think that's the right fix. The problem here is that the display engine has to output data in a way that doesn't violate the DAC setup and hold times. Depending on the display engine, you can just select the output clock edge, or adjust the phase of the data compared to the pixel clock by a fraction of a clock cycle (1/4 is common, I've seen smaller steps too). Note that selecting the opposite clock edge is simple a 1/2 clock cycle delay. The delay has to be computed based on the receiver's setup and hold times, but also take into account other components on the board (such as buffer or voltage shifters, or even inverters) and the PCB delay itself. This computation doesn't belong to the bridge driver, especially given than its goal is to compute a delay that depends on the display engine's capabilities (inverting the clock vs. smaller step delays for instance). For this reason I think the bridge driver should expose its timing parameters, and the display engine should then decide how to output its data accordingly. > No idea why I didn't tell you this right away, or maybe I'm missing > something this time around. > > > I'm just a bit lost. > > Once your un-lost, pls review the docs for drm_crtc_state and the various > mode_fixup functions, to make sure they're clear on how this is supposed > to work. Might need a new overview DOC: comment that ties it all together. -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH 0/4 v5] Support bridge timings Date: Mon, 18 Dec 2017 13:01:06 +0200 Message-ID: <2682424.BlFQCyBzxh@avalon> References: <20171215121047.3650-1-linus.walleij@linaro.org> <20171215155415.GT26573@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [185.26.127.97]) by gabe.freedesktop.org (Postfix) with ESMTPS id 88ED86E0A0 for ; Mon, 18 Dec 2017 11:00:57 +0000 (UTC) In-Reply-To: <20171215155415.GT26573@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: "open list:DRM PANEL DRIVERS" , Linux ARM List-Id: dri-devel@lists.freedesktop.org SGkgRGFuaWVsLAoKT24gRnJpZGF5LCAxNSBEZWNlbWJlciAyMDE3IDE3OjU0OjE1IEVFVCBEYW5p ZWwgVmV0dGVyIHdyb3RlOgo+IE9uIEZyaSwgRGVjIDE1LCAyMDE3IGF0IDAxOjMwOjI0UE0gKzAx MDAsIExpbnVzIFdhbGxlaWogd3JvdGU6Cj4gPiBPbiBGcmksIERlYyAxNSwgMjAxNyBhdCAxOjEw IFBNLCBMaW51cyBXYWxsZWlqIDxsaW51cy53YWxsZWlqQGxpbmFyby5vcmc+IAp3cm90ZToKPiA+ PiAtIFRoZSBjb25uZWN0b3IgaXMgYXBwYXJlbnRseSBub3QgdGhlIHJpZ2h0IGFic3RyYWN0aW9u IHRvIGNhcnJ5Cj4gPj4gICB0aGUgZGV0YWlsZWQgdGltaW5ncyBzcGVjaWZpY2F0aW9uIGJldHdl ZW4gRFJJIGRyaXZlcnMgYW5kIGJyaWRnZQo+ID4+ICAgZHJpdmVycy4KPiA+PiAKPiA+PiAtIElu c3RlYWQgcHV0IGRldGFpbGVkIHRpbWluZyBkYXRhIGludG8gdGhlIGJyaWRnZSBpdHNlbGYgYXMg YW4KPiA+PiAgIG9wdGlvbmFsIGluZm9ybWF0aW9uIHBvaW50ZXIuCj4gPiAKPiA+IE5vdGljZSB0 aGF0IHRoaXMgaXMganVzdCBteSBmdW1ibGluZyBhdHRlbXB0cyB0byBkZWFsIHdpdGggdGhlIHNp dHVhdGlvbi4KPiA+IAo+ID4gTGF1cmVudCBtYWRlIG1lIHVuZGVyc3RhbmQgd2hhdCB0aGUgYWN0 dWFsIHRlY2huaWNhbCBwcm9ibGVtIHdhcywKPiA+IGhvdyBjb21lIG15IHBpeGVscyB3ZXJlIGZs aWNrZXJpbmcuCj4gPiAKPiA+IEJvdGggTGF1cmVudCBhbmQgRFZldHRlciBtZW50aW9uZWQgdGhh dCB3ZSBtYXkgbmVlZCB0byBjb252ZXkKPiA+IGluZm9ybWF0aW9uIGJldHdlZW4gdGhlIGJyaWRn ZSBhbmQgdGhlIGRpc3BsYXkgZW5naW5lIGluIHNvbWUKPiA+IHdheS4KPiA+IAo+ID4gQWx0ZXJu YXRpdmVseSBJIGNvdWxkIGdvIGFuZCBoYWNrIG9uIGFkZGluZyB0aGlzIHRvIGUuZy4gZHJtX2Rp c3BsYXlfaW5mbwo+ID4gd2hpY2ggd2FzIHVzZWQgaW4gdGhlIHByZXZpb3VzIHBhdGNoIHNldHMg Ynkgc2V0dGluZyB0aGUgbmVnZWRlIGZsYWcKPiA+IGluIGJ1c19mb3JtYXRzLgo+ID4gCj4gPiBJ IGRvbid0IGtub3cuIHN0cnVjdCBkcm1fZGlzcGxheV9pbmZvIGlzIGdldHRpbmcgYSBiaXQgaGVh dnkgYXMKPiA+IGNvbnRhaW5lciBvZiBtaXNjIHNldHRpbmdzIHJlbGF0ZWQgdG8gInNvbWUga2lu ZCBvZiBkaXNwbGF5Ii4KPiA+IFRoZSBicmlkZ2UgaXNuJ3QgZXZlbiBhIGRpc3BsYXkgaXRzZWxm LCB0aGF0IGlzIG9uIHRoZSBvdGhlciBzaWRlCj4gPiBvZiBpdC4gU28gdXNpbmcgdGhlIGNvbm5l Y3RvciBhbmQgdHJlYXRpbmcgYSBicmlkZ2UgYXMgInNvbWUga2luZAo+ID4gb2YgZGlzcGxheSIg c2VlbXMgd3JvbmcgdG9vLgo+ID4gCj4gPiBJcyB0aGVyZSBhIHRoaXJkIHdheT8KPiAKPiBJZiB5 b3UgZG9uJ3QgcGxhbiB0byBuZXN0IGJyaWRnZXMgdG9vIGRlZXBseSwgdGhlcmUgaXMuIEF0bSB3 ZSBoYXZlIDIKPiBtb2RlcyBpbiBkcm1fY3J0Y19zdGF0ZToKPiAKPiAtIG1vZGUsIHdoaWNoIGlz IHdoYXQgdXNlcnNwYWNlIHJlcXVlc3RlZCwgYW5kIHdoYXQgaXQgZXhwZWN0cyBsb2dpY2FsbHkK PiAgIHRvIGJlIHRoZSBhY3R1YWwgcmVhbCB0aGluZy4gSS5lLiB0aW1pbmcsIHJlc29sdXRpb24g YW5kIGFsbCB0aGF0IHRoYXQKPiAgIHVzZXJzcGFjZSBjYW4gb2JzZXJ2ZSAodGhyb3VnaCBwbGFu ZSBwb3NpdGlvbmluZyBhbmQgdmJsYW5rIHRpbWVzdGFtcHMpCj4gICBzaG91bGQgbWF0Y2ggdGhp cyBtb2RlLiBGb3IgZXh0ZXJuYWwgc2NyZWVucyB0aGlzIHNob3VsZCBhbHNvIG1hdGNoCj4gICB3 aGF0J3MgcGh5c2ljYWxseSBnb2luZyBvdmVyIHRoZSBjYWJsZS4KPiAKPiAtIGFkanVzdGVkX21v ZGUsIHdoaWNoIGlzIHNvbWV0aGluZyBlbnRpcmVseSB1bmRlZmluZWQgYW5kIHRvIGJlIHVzZWQg YnkKPiAgIGRyaXZlcnMgaW50ZXJuYWxseS4gTW9zdCBkcml2ZXJzIHVzZSBpdCBhcyB0aGUgdGhp bmcgdGhhdCdzIGFjdHVhbGx5Cj4gICB0cmFuc3BvcnRlZCBiZXR3ZWVuIHRoZSBDUlRDIGFuZCB0 aGUgZW5jb2Rlci4KPiAKPiBUaGVyZSdzIGEgZmV3IGNvbW1vbiByZWFzb25zIGZvciBhZGp1c3Rl ZCBtb2RlIHRvIGJlIGRpZmZlcmVudDoKPiAtIGludGVncmF0ZWQgcGFuZWwsIGFuZCB5b3VyIENS VEMgaGFzIGEgc2NhbGVyLiBJbiB0aGF0IGNhc2UgdGhlCj4gICB1c2Vyc3BhY2UtcmVxdWVzdGVk IG1vZGUgaXMgd2hhdCB5b3UgZmVlZCBpbnRvIGludG8gdGhlIHNjYWxlciwgYW5kIHRoZQo+ICAg YWRqdXN0ZWQgbW9kZSBpcyB3aGF0IGNvbWVzIG91dCBvZiB5b3VyIHNjYWxlciBhbmQgdGhlbiBn b2VzIGRvd24gdGhlCj4gICB3aXJlIHRvIHRoZSBwYW5lbC4KPiAKPiAtIHlvdXIgZW5jb2RlciBp cyBmdW5reSwgYW5kIGUuZy4gdHJhbnNjb2RlcyB0byB0aGUgb3V0cHV0IG1vZGUgaXRzZWxmLAo+ ICAgYnV0IGV4cGVjdHMgdGhhdCB5b3UgcHJvZ3JhbSB0aGUgaW5wdXQgbW9kZSBhbHdheXMgdGhl IHNhbWUuIFVzdWFsCj4gICByZWFzb25zIGZvciB0aGlzIGFyZSB0cmFuc2NvZGVycyB0aGF0IGFs d2F5cyB3YW50IG5vbi1pbnRlcmxhY2VkIG1vZGUKPiAgIChhbmQgZG8gdGhlIGludGVybGFjaW5n IHRoZW1zZWx2ZXMpLCBpZiB0aGUgdHJhbnNjb2RlciBoYXMgc29tZSBzY2FsZXIKPiAgIGl0c2Vs ZiAoc29tZSBUVi1vdXQgdHJhbnNjb2RlcnMgaGFkIHRoYXQpLCBvciBpZiBpdCBoYXMgYSBzdHJp Y3QKPiAgIGV4cGVjdGF0aW9uIGFib3V0IHNpZ25hbGxpbmcgZWRnZXMgYW5kIHN0dWZmIChhbmQg dGhlbiB0cmFuc2NvZGVzIHRoZQo+ICAgc2lnbmFsIGFnYWluKS4gREFDcyBhcmUgY29tbW9uIGRv aW5nIHRoYXQuCj4gCj4gQW55d2F5LCBzb3VuZHMgbGlrZSB5b3VyIGJyaWRnZSBpcyBvZiB0aGUg Mm5kIGtpbmQsIHNvIGFsbCB5b3UgaGF2ZSB0byBkbwo+IGlzCj4gLSBpbiB5b3VyIGJyaWRnZS0+ bW9kZV9maXh1cCBmdW5jdGlvbiwgYWRqdXN0IHRoZSBhZGp1c3RlZF9tb2RlIGFzIG5lZWRlZAo+ IC0gaW4geW91ciBwbDExMSBkcml2ZXIsIHByb2dyYW0gdGhlIGFkanVzdGVkIG1vZGUsIG5vdCB0 aGUgb3JpZ2luYWxseQo+ICAgcmVxdWVzdGVkIG1vZGUKPiAKPiBhZGp1c3RlZCBtb2RlIGlzIHNl dCB0byBiZSBhIGNvcHkgb2YgdGhlIHJlcXVlc3RlZCBtb2RlIGJ5IGF0b21pYyBoZWxwZXJzLAo+ IHNvIHRoaXMgc2hvdWxkIGtlZXAgd29ya2luZyBhcy1pcyBvbiBhbnkgb3RoZXIgYnJpZGdlIGRy aXZlci4KCkkgZG9uJ3QgdGhpbmsgdGhhdCdzIHRoZSByaWdodCBmaXguCgpUaGUgcHJvYmxlbSBo ZXJlIGlzIHRoYXQgdGhlIGRpc3BsYXkgZW5naW5lIGhhcyB0byBvdXRwdXQgZGF0YSBpbiBhIHdh eSB0aGF0IApkb2Vzbid0IHZpb2xhdGUgdGhlIERBQyBzZXR1cCBhbmQgaG9sZCB0aW1lcy4gRGVw ZW5kaW5nIG9uIHRoZSBkaXNwbGF5IGVuZ2luZSwgCnlvdSBjYW4ganVzdCBzZWxlY3QgdGhlIG91 dHB1dCBjbG9jayBlZGdlLCBvciBhZGp1c3QgdGhlIHBoYXNlIG9mIHRoZSBkYXRhIApjb21wYXJl ZCB0byB0aGUgcGl4ZWwgY2xvY2sgYnkgYSBmcmFjdGlvbiBvZiBhIGNsb2NrIGN5Y2xlICgxLzQg aXMgY29tbW9uLCAKSSd2ZSBzZWVuIHNtYWxsZXIgc3RlcHMgdG9vKS4gTm90ZSB0aGF0IHNlbGVj dGluZyB0aGUgb3Bwb3NpdGUgY2xvY2sgZWRnZSBpcyAKc2ltcGxlIGEgMS8yIGNsb2NrIGN5Y2xl IGRlbGF5LgoKVGhlIGRlbGF5IGhhcyB0byBiZSBjb21wdXRlZCBiYXNlZCBvbiB0aGUgcmVjZWl2 ZXIncyBzZXR1cCBhbmQgaG9sZCB0aW1lcywgYnV0IAphbHNvIHRha2UgaW50byBhY2NvdW50IG90 aGVyIGNvbXBvbmVudHMgb24gdGhlIGJvYXJkIChzdWNoIGFzIGJ1ZmZlciBvciAKdm9sdGFnZSBz aGlmdGVycywgb3IgZXZlbiBpbnZlcnRlcnMpIGFuZCB0aGUgUENCIGRlbGF5IGl0c2VsZi4gVGhp cyAKY29tcHV0YXRpb24gZG9lc24ndCBiZWxvbmcgdG8gdGhlIGJyaWRnZSBkcml2ZXIsIGVzcGVj aWFsbHkgZ2l2ZW4gdGhhbiBpdHMgCmdvYWwgaXMgdG8gY29tcHV0ZSBhIGRlbGF5IHRoYXQgZGVw ZW5kcyBvbiB0aGUgZGlzcGxheSBlbmdpbmUncyBjYXBhYmlsaXRpZXMgCihpbnZlcnRpbmcgdGhl IGNsb2NrIHZzLiBzbWFsbGVyIHN0ZXAgZGVsYXlzIGZvciBpbnN0YW5jZSkuIEZvciB0aGlzIHJl YXNvbiBJIAp0aGluayB0aGUgYnJpZGdlIGRyaXZlciBzaG91bGQgZXhwb3NlIGl0cyB0aW1pbmcg cGFyYW1ldGVycywgYW5kIHRoZSBkaXNwbGF5IAplbmdpbmUgc2hvdWxkIHRoZW4gZGVjaWRlIGhv dyB0byBvdXRwdXQgaXRzIGRhdGEgYWNjb3JkaW5nbHkuCgo+IE5vIGlkZWEgd2h5IEkgZGlkbid0 IHRlbGwgeW91IHRoaXMgcmlnaHQgYXdheSwgb3IgbWF5YmUgSSdtIG1pc3NpbmcKPiBzb21ldGhp bmcgdGhpcyB0aW1lIGFyb3VuZC4KPiAKPiA+IEknbSBqdXN0IGEgYml0IGxvc3QuCj4gCj4gT25j ZSB5b3VyIHVuLWxvc3QsIHBscyByZXZpZXcgdGhlIGRvY3MgZm9yIGRybV9jcnRjX3N0YXRlIGFu ZCB0aGUgdmFyaW91cwo+IG1vZGVfZml4dXAgZnVuY3Rpb25zLCB0byBtYWtlIHN1cmUgdGhleSdy ZSBjbGVhciBvbiBob3cgdGhpcyBpcyBzdXBwb3NlZAo+IHRvIHdvcmsuIE1pZ2h0IG5lZWQgYSBu ZXcgb3ZlcnZpZXcgRE9DOiBjb21tZW50IHRoYXQgdGllcyBpdCBhbGwgdG9nZXRoZXIuCgotLSAK UmVnYXJkcywKCkxhdXJlbnQgUGluY2hhcnQKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3Rz LmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xp c3RpbmZvL2RyaS1kZXZlbAo=