From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Wed, 04 Apr 2018 12:07:15 +0300 Subject: [PATCH v2 0/5] allow override of bus format in bridges In-Reply-To: References: <20180326212447.7380-1-peda@axentia.se> <2233231.my6BbD3pcT@avalon> Message-ID: <1563250.XEJxFDuW0h@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Daniel, On Wednesday, 4 April 2018 09:34:41 EEST Daniel Vetter wrote: > On Wed, Apr 4, 2018 at 12:28 AM, Laurent Pinchart wrote: > > On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote: > >> On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote: > >>> Hi! > >>> > >>> [I got to v2 sooner than expected] > >>> > >>> I have an Atmel sama5d31 hooked up to an lvds encoder and then > >>> on to an lvds panel. Which seems like something that has been > >>> done one or two times before... > >>> > >>> The problem is that the bus_format of the SoC and the panel do > >>> not agree. The SoC driver (atmel-hlcdc) can handle the > >>> rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is > >>> wired for the rgb565 case. The lvds encoder supports rgb888 on > >>> its input side with the LSB wires for each color simply pulled > >>> down internally in the encoder in my case which means that the > >>> rgb565 bus_format is the format that works best. And the panel > >>> is expecting lvds (vesa-24), which is what the encoder outputs. > >>> > >>> The reason I "blame" the bus_format of the drm_connector is that > >>> with the below DT snippet, things do not work *exactly* due to > >>> that. At least, it starts to work if I hack the panel-lvds driver > >>> to report the rgb565 bus_format instead of vesa-24. > >>> > >>> panel: panel { > >>> compatible = "panel-lvds"; > >>> > >>> width-mm = <304>; > >>> height-mm = <228; > >>> > >>> data-mapping = "vesa-24"; > >>> > >>> panel-timing { > >>> // 1024x768 @ 60Hz (typical) > >>> clock-frequency = <52140000 65000000 71100000>; > >>> hactive = <1024>; > >>> vactive = <768>; > >>> hfront-porch = <48 88 88>; > >>> hback-porch = <96 168 168>; > >>> hsync-len = <32 64 64>; > >>> vfront-porch = <8 13 14>; > >>> vback-porch = <8 13 14>; > >>> vsync-len = <8 12 14>; > >>> }; > >>> > >>> port { > >>> panel_input: endpoint { > >>> remote-endpoint = <&lvds_encoder_output>; > >>> }; > >>> }; > >>> }; > >>> > >>> lvds-encoder { > >>> compatible = "ti,ds90c185", "lvds-encoder"; > >>> > >>> ports { > >>> #address-cells = <1>; > >>> #size-cells = <0>; > >>> > >>> port at 0 { > >>> reg = <0>; > >>> > >>> lvds_encoder_input: endpoint { > >>> remote-endpoint = <&hlcdc_output>; > >>> }; > >>> }; > >>> > >>> port at 1 { > >>> reg = <1>; > >>> > >>> lvds_encoder_output: endpoint { > >>> remote-endpoint = <&panel_input>; > >>> }; > >>> }; > >>> }; > >>> }; > >>> > >>> But, instead of perverting the panel-lvds driver with support > >>> for a totally fake non-lvds bus_format, I intruduce an API that allows > >>> display controller drivers to query the required bus_format of any > >>> intermediate bridges, and match up with that instead of the formats > >>> given by the drm_connector. I trigger this with this addition to the > >>> > >>> lvds-encoder DT node: > >>> interface-pix-fmt = "rgb565"; > >>> > >>> Naming is hard though, so I'm not sure if that's good? > >>> > >>> I threw in the first patch, since that is the actual lvds encoder > >>> I have in this case. > >>> > >>> Suggestions welcome. > >> > >> Took a quick look, feels rather un-atomic. And there's beend discussing > >> for other bridge related state that we might want to track (like the full > >> adjusted_mode that might need to be adjusted at each stage in the chain). > >> So here's my suggestions: > >> > >> - Add an optional per-bridge internal state struct using the support in > >> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-> >> private-state > >> > >> Yes it says "driver private", but since bridge is just helper stuff > >> that's all included. "driver private" == "not exposed as uapi" here. > >> Include all the usual convenience wrappers to get at the state for a > >> bridge. > >> > >> - Then stuff your bus_format into that new drm_bridge_state struct. > >> > >> - Add a new bridge callback atomic_check, which gets that bridge state as > >> parameter (similar to all the other atomic_check functions). > >> > >> This way we can even handle the bus_format dynamically, through the > >> atomic framework your bridge's atomic_check callback can look at the > >> entire atomic state (both up and down the chain if needed), it all neatly > >> fits into atomic overall and it's much easier to extend. > > > > While I think we'll eventually need bridge states, I don't think that's > > need yet. The bus formats reported by this patch series are static. We're > > not talking about the currently configured format for a bridge, but about > > the list of supported formats. This is similar to the bus_formats field > > present in the drm_display_info structure. There is thus in my opinion no > > need to interface this with atomic until we need to track the current > > format (and I think that will indeed happen at some point, but I don't > > think Peter needs this feature for now). That's why I've told Peter that > > I would like a bridge API to report the information and haven't requested > > a state-based implementation. > > If it's static, why a dynamic callback? Just add it to struct > drm_bridge, require it's filled out before registering the bridge, > done. If I remember correctly I mentioned both options in my initial proposal, without a personal preference. A new field in struct drm_bridge would certainly work for me. > >> Please also cc Laurent Pinchart on this. > >> > >>> Changes since v1 https://lkml.org/lkml/2018/3/17/221 > >>> - Add a proper bridge API to query the bus_format instead of abusing > >>> the ->get_modes part of the code. This is cleaner but requires > >>> changes to all display controller drivers wishing to participate. > >>> - Add patch to adjust the atmel-hlcdc driver according to the above. > >>> - Hook the new info into the bridge local to the lvds-encoder instead > >>> of messing about with new interfaces for the panel-bridge driver. > >>> - Add patch with a DT parsing function of bus_formats in a central > >>> place. > >>> - Rephrase the addition of ti,ds90c185 to the lvds-transmitter binding. > >>> > >>> Peter Rosin (5): > >>> dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185 > >>> drm: bridge: add API to query the expected input formats of bridges > >>> drm: of: add display bus-format parser > >>> drm: bridge: lvds-encoder: allow specifying the input bus format > >>> drm/atmel-hlcdc: take bridges into account when selecting output > >>> format > >>> > >>> .../bindings/display/bridge/lvds-transmitter.txt | 14 ++++- > >>> .../devicetree/bindings/display/bus-format.txt | 35 +++++++++++++ > >>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 49 +++++++++++++-- > >>> drivers/gpu/drm/bridge/lvds-encoder.c | 25 +++++++++ > >>> drivers/gpu/drm/drm_bridge.c | 32 ++++++++++++ > >>> drivers/gpu/drm/drm_of.c | 59 +++++++++++++++ > >>> include/drm/drm_bridge.h | 18 +++++++ > >>> include/drm/drm_of.h | 9 ++++ > >>> 8 files changed, 237 insertions(+), 4 deletions(-) > >>> create mode 100644 > >>> Documentation/devicetree/bindings/display/bus-format.txt -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v2 0/5] allow override of bus format in bridges Date: Wed, 04 Apr 2018 12:07:15 +0300 Message-ID: <1563250.XEJxFDuW0h@avalon> References: <20180326212447.7380-1-peda@axentia.se> <2233231.my6BbD3pcT@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: 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: Mark Rutland , Boris Brezillon , Alexandre Belloni , devicetree@vger.kernel.org, David Airlie , Linux Kernel Mailing List , dri-devel , Nicolas Ferre , Rob Herring , Jacopo Mondi , Daniel Vetter , Peter Rosin , Linux ARM List-Id: devicetree@vger.kernel.org SGkgRGFuaWVsLAoKT24gV2VkbmVzZGF5LCA0IEFwcmlsIDIwMTggMDk6MzQ6NDEgRUVTVCBEYW5p ZWwgVmV0dGVyIHdyb3RlOgo+IE9uIFdlZCwgQXByIDQsIDIwMTggYXQgMTI6MjggQU0sIExhdXJl bnQgUGluY2hhcnQgd3JvdGU6Cj4gPiBPbiBXZWRuZXNkYXksIDI4IE1hcmNoIDIwMTggMTA6MDg6 MjYgRUVTVCBEYW5pZWwgVmV0dGVyIHdyb3RlOgo+ID4+IE9uIE1vbiwgTWFyIDI2LCAyMDE4IGF0 IDExOjI0OjQyUE0gKzAyMDAsIFBldGVyIFJvc2luIHdyb3RlOgo+ID4+PiBIaSEKPiA+Pj4gCj4g Pj4+IFtJIGdvdCB0byB2MiBzb29uZXIgdGhhbiBleHBlY3RlZF0KPiA+Pj4gCj4gPj4+IEkgaGF2 ZSBhbiBBdG1lbCBzYW1hNWQzMSBob29rZWQgdXAgdG8gYW4gbHZkcyBlbmNvZGVyIGFuZCB0aGVu Cj4gPj4+IG9uIHRvIGFuIGx2ZHMgcGFuZWwuIFdoaWNoIHNlZW1zIGxpa2Ugc29tZXRoaW5nIHRo YXQgaGFzIGJlZW4KPiA+Pj4gZG9uZSBvbmUgb3IgdHdvIHRpbWVzIGJlZm9yZS4uLgo+ID4+PiAK PiA+Pj4gVGhlIHByb2JsZW0gaXMgdGhhdCB0aGUgYnVzX2Zvcm1hdCBvZiB0aGUgU29DIGFuZCB0 aGUgcGFuZWwgZG8KPiA+Pj4gbm90IGFncmVlLiBUaGUgU29DIGRyaXZlciAoYXRtZWwtaGxjZGMp IGNhbiBoYW5kbGUgdGhlCj4gPj4+IHJnYjQ0NCwgcmdiNTY1LCByZ2I2NjYgYW5kIHJnYjg4OCBi dXMgZm9ybWF0cy4gVGhlIGhhcmR3YXJlIGlzCj4gPj4+IHdpcmVkIGZvciB0aGUgcmdiNTY1IGNh c2UuIFRoZSBsdmRzIGVuY29kZXIgc3VwcG9ydHMgcmdiODg4IG9uCj4gPj4+IGl0cyBpbnB1dCBz aWRlIHdpdGggdGhlIExTQiB3aXJlcyBmb3IgZWFjaCBjb2xvciBzaW1wbHkgcHVsbGVkCj4gPj4+ IGRvd24gaW50ZXJuYWxseSBpbiB0aGUgZW5jb2RlciBpbiBteSBjYXNlIHdoaWNoIG1lYW5zIHRo YXQgdGhlCj4gPj4+IHJnYjU2NSBidXNfZm9ybWF0IGlzIHRoZSBmb3JtYXQgdGhhdCB3b3JrcyBi ZXN0LiBBbmQgdGhlIHBhbmVsCj4gPj4+IGlzIGV4cGVjdGluZyBsdmRzICh2ZXNhLTI0KSwgd2hp Y2ggaXMgd2hhdCB0aGUgZW5jb2RlciBvdXRwdXRzLgo+ID4+PiAKPiA+Pj4gVGhlIHJlYXNvbiBJ ICJibGFtZSIgdGhlIGJ1c19mb3JtYXQgb2YgdGhlIGRybV9jb25uZWN0b3IgaXMgdGhhdAo+ID4+ PiB3aXRoIHRoZSBiZWxvdyBEVCBzbmlwcGV0LCB0aGluZ3MgZG8gbm90IHdvcmsgKmV4YWN0bHkq IGR1ZSB0bwo+ID4+PiB0aGF0LiBBdCBsZWFzdCwgaXQgc3RhcnRzIHRvIHdvcmsgaWYgSSBoYWNr IHRoZSBwYW5lbC1sdmRzIGRyaXZlcgo+ID4+PiB0byByZXBvcnQgdGhlIHJnYjU2NSBidXNfZm9y bWF0IGluc3RlYWQgb2YgdmVzYS0yNC4KPiA+Pj4gCj4gPj4+ICAgICBwYW5lbDogcGFuZWwgewo+ ID4+PiAgICAgICAgICAgICBjb21wYXRpYmxlID0gInBhbmVsLWx2ZHMiOwo+ID4+PiAgICAgICAg ICAgICAKPiA+Pj4gICAgICAgICAgICAgd2lkdGgtbW0gPSA8MzA0PjsKPiA+Pj4gICAgICAgICAg ICAgaGVpZ2h0LW1tID0gPDIyODsKPiA+Pj4gICAgICAgICAgICAgCj4gPj4+ICAgICAgICAgICAg IGRhdGEtbWFwcGluZyA9ICJ2ZXNhLTI0IjsKPiA+Pj4gICAgICAgICAgICAgCj4gPj4+ICAgICAg ICAgICAgIHBhbmVsLXRpbWluZyB7Cj4gPj4+ICAgICAgICAgICAgICAgICAgICAgLy8gMTAyNHg3 NjggQCA2MEh6ICh0eXBpY2FsKQo+ID4+PiAgICAgICAgICAgICAgICAgICAgIGNsb2NrLWZyZXF1 ZW5jeSA9IDw1MjE0MDAwMCA2NTAwMDAwMCA3MTEwMDAwMD47Cj4gPj4+ICAgICAgICAgICAgICAg ICAgICAgaGFjdGl2ZSA9IDwxMDI0PjsKPiA+Pj4gICAgICAgICAgICAgICAgICAgICB2YWN0aXZl ID0gPDc2OD47Cj4gPj4+ICAgICAgICAgICAgICAgICAgICAgaGZyb250LXBvcmNoID0gPDQ4IDg4 IDg4PjsKPiA+Pj4gICAgICAgICAgICAgICAgICAgICBoYmFjay1wb3JjaCA9IDw5NiAxNjggMTY4 PjsKPiA+Pj4gICAgICAgICAgICAgICAgICAgICBoc3luYy1sZW4gPSA8MzIgNjQgNjQ+Owo+ID4+ PiAgICAgICAgICAgICAgICAgICAgIHZmcm9udC1wb3JjaCA9IDw4IDEzIDE0PjsKPiA+Pj4gICAg ICAgICAgICAgICAgICAgICB2YmFjay1wb3JjaCA9IDw4IDEzIDE0PjsKPiA+Pj4gICAgICAgICAg ICAgICAgICAgICB2c3luYy1sZW4gPSA8OCAxMiAxND47Cj4gPj4+ICAgICAgICAgICAgIH07Cj4g Pj4+ICAgICAgICAgICAgIAo+ID4+PiAgICAgICAgICAgICBwb3J0IHsKPiA+Pj4gICAgICAgICAg ICAgICAgICAgICBwYW5lbF9pbnB1dDogZW5kcG9pbnQgewo+ID4+PiAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgcmVtb3RlLWVuZHBvaW50ID0gPCZsdmRzX2VuY29kZXJfb3V0cHV0PjsKPiA+ Pj4gICAgICAgICAgICAgICAgICAgICB9Owo+ID4+PiAgICAgICAgICAgICB9Owo+ID4+PiAgICAg fTsKPiA+Pj4gICAgIAo+ID4+PiAgICAgbHZkcy1lbmNvZGVyIHsKPiA+Pj4gICAgICAgICAgICAg Y29tcGF0aWJsZSA9ICJ0aSxkczkwYzE4NSIsICJsdmRzLWVuY29kZXIiOwo+ID4+PiAgICAgICAg ICAgICAKPiA+Pj4gICAgICAgICAgICAgcG9ydHMgewo+ID4+PiAgICAgICAgICAgICAgICAgICAg ICNhZGRyZXNzLWNlbGxzID0gPDE+Owo+ID4+PiAgICAgICAgICAgICAgICAgICAgICNzaXplLWNl bGxzID0gPDA+Owo+ID4+PiAgICAgICAgICAgICAgICAgICAgIAo+ID4+PiAgICAgICAgICAgICAg ICAgICAgIHBvcnRAMCB7Cj4gPj4+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICByZWcgPSA8 MD47Cj4gPj4+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAKPiA+Pj4gICAgICAgICAgICAg ICAgICAgICAgICAgICAgIGx2ZHNfZW5jb2Rlcl9pbnB1dDogZW5kcG9pbnQgewo+ID4+PiAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICByZW1vdGUtZW5kcG9pbnQgPSA8JmhsY2Rj X291dHB1dD47Cj4gPj4+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICB9Owo+ID4+PiAgICAg ICAgICAgICAgICAgICAgIH07Cj4gPj4+ICAgICAgICAgICAgICAgICAgICAgCj4gPj4+ICAgICAg ICAgICAgICAgICAgICAgcG9ydEAxIHsKPiA+Pj4gICAgICAgICAgICAgICAgICAgICAgICAgICAg IHJlZyA9IDwxPjsKPiA+Pj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgIAo+ID4+PiAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgbHZkc19lbmNvZGVyX291dHB1dDogZW5kcG9pbnQgewo+ ID4+PiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICByZW1vdGUtZW5kcG9pbnQg PSA8JnBhbmVsX2lucHV0PjsKPiA+Pj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgIH07Cj4g Pj4+ICAgICAgICAgICAgICAgICAgICAgfTsKPiA+Pj4gICAgICAgICAgICAgfTsKPiA+Pj4gICAg IH07Cj4gPj4+IAo+ID4+PiBCdXQsIGluc3RlYWQgb2YgcGVydmVydGluZyB0aGUgcGFuZWwtbHZk cyBkcml2ZXIgd2l0aCBzdXBwb3J0Cj4gPj4+IGZvciBhIHRvdGFsbHkgZmFrZSBub24tbHZkcyBi dXNfZm9ybWF0LCBJIGludHJ1ZHVjZSBhbiBBUEkgdGhhdCBhbGxvd3MKPiA+Pj4gZGlzcGxheSBj b250cm9sbGVyIGRyaXZlcnMgdG8gcXVlcnkgdGhlIHJlcXVpcmVkIGJ1c19mb3JtYXQgb2YgYW55 Cj4gPj4+IGludGVybWVkaWF0ZSBicmlkZ2VzLCBhbmQgbWF0Y2ggdXAgd2l0aCB0aGF0IGluc3Rl YWQgb2YgdGhlIGZvcm1hdHMKPiA+Pj4gZ2l2ZW4gYnkgdGhlIGRybV9jb25uZWN0b3IuIEkgdHJp Z2dlciB0aGlzIHdpdGggdGhpcyBhZGRpdGlvbiB0byB0aGUKPiA+Pj4gCj4gPj4+IGx2ZHMtZW5j b2RlciBEVCBub2RlOgo+ID4+PiAgICAgICAgICAgICBpbnRlcmZhY2UtcGl4LWZtdCA9ICJyZ2I1 NjUiOwo+ID4+PiAKPiA+Pj4gTmFtaW5nIGlzIGhhcmQgdGhvdWdoLCBzbyBJJ20gbm90IHN1cmUg aWYgdGhhdCdzIGdvb2Q/Cj4gPj4+IAo+ID4+PiBJIHRocmV3IGluIHRoZSBmaXJzdCBwYXRjaCwg c2luY2UgdGhhdCBpcyB0aGUgYWN0dWFsIGx2ZHMgZW5jb2Rlcgo+ID4+PiBJIGhhdmUgaW4gdGhp cyBjYXNlLgo+ID4+PiAKPiA+Pj4gU3VnZ2VzdGlvbnMgd2VsY29tZS4KPiA+PiAKPiA+PiBUb29r IGEgcXVpY2sgbG9vaywgZmVlbHMgcmF0aGVyIHVuLWF0b21pYy4gQW5kIHRoZXJlJ3MgYmVlbmQg ZGlzY3Vzc2luZwo+ID4+IGZvciBvdGhlciBicmlkZ2UgcmVsYXRlZCBzdGF0ZSB0aGF0IHdlIG1p Z2h0IHdhbnQgdG8gdHJhY2sgKGxpa2UgdGhlIGZ1bGwKPiA+PiBhZGp1c3RlZF9tb2RlIHRoYXQg bWlnaHQgbmVlZCB0byBiZSBhZGp1c3RlZCBhdCBlYWNoIHN0YWdlIGluIHRoZSBjaGFpbikuCj4g Pj4gU28gaGVyZSdzIG15IHN1Z2dlc3Rpb25zOgo+ID4+IAo+ID4+IC0gQWRkIGFuIG9wdGlvbmFs IHBlci1icmlkZ2UgaW50ZXJuYWwgc3RhdGUgc3RydWN0IHVzaW5nIHRoZSBzdXBwb3J0IGluCj4g Pj4gICBodHRwczovL2RyaS5mcmVlZGVza3RvcC5vcmcvZG9jcy9kcm0vZ3B1L2RybS1rbXMuaHRt bCNoYW5kbGluZy1kcml2ZXItPiA+PiAgIHByaXZhdGUtc3RhdGUKPiA+PiAKPiA+PiAgIFllcyBp dCBzYXlzICJkcml2ZXIgcHJpdmF0ZSIsIGJ1dCBzaW5jZSBicmlkZ2UgaXMganVzdCBoZWxwZXIg c3R1ZmYKPiA+PiAgIHRoYXQncyBhbGwgaW5jbHVkZWQuICJkcml2ZXIgcHJpdmF0ZSIgPT0gIm5v dCBleHBvc2VkIGFzIHVhcGkiIGhlcmUuCj4gPj4gICBJbmNsdWRlIGFsbCB0aGUgdXN1YWwgY29u dmVuaWVuY2Ugd3JhcHBlcnMgdG8gZ2V0IGF0IHRoZSBzdGF0ZSBmb3IgYQo+ID4+ICAgYnJpZGdl Lgo+ID4+IAo+ID4+IC0gVGhlbiBzdHVmZiB5b3VyIGJ1c19mb3JtYXQgaW50byB0aGF0IG5ldyBk cm1fYnJpZGdlX3N0YXRlIHN0cnVjdC4KPiA+PiAKPiA+PiAtIEFkZCBhIG5ldyBicmlkZ2UgY2Fs bGJhY2sgYXRvbWljX2NoZWNrLCB3aGljaCBnZXRzIHRoYXQgYnJpZGdlIHN0YXRlIGFzCj4gPj4g ICBwYXJhbWV0ZXIgKHNpbWlsYXIgdG8gYWxsIHRoZSBvdGhlciBhdG9taWNfY2hlY2sgZnVuY3Rp b25zKS4KPiA+PiAKPiA+PiBUaGlzIHdheSB3ZSBjYW4gZXZlbiBoYW5kbGUgdGhlIGJ1c19mb3Jt YXQgZHluYW1pY2FsbHksIHRocm91Z2ggdGhlCj4gPj4gYXRvbWljIGZyYW1ld29yayB5b3VyIGJy aWRnZSdzIGF0b21pY19jaGVjayBjYWxsYmFjayBjYW4gbG9vayBhdCB0aGUKPiA+PiBlbnRpcmUg YXRvbWljIHN0YXRlIChib3RoIHVwIGFuZCBkb3duIHRoZSBjaGFpbiBpZiBuZWVkZWQpLCBpdCBh bGwgbmVhdGx5Cj4gPj4gZml0cyBpbnRvIGF0b21pYyBvdmVyYWxsIGFuZCBpdCdzIG11Y2ggZWFz aWVyIHRvIGV4dGVuZC4KPiA+IAo+ID4gV2hpbGUgSSB0aGluayB3ZSdsbCBldmVudHVhbGx5IG5l ZWQgYnJpZGdlIHN0YXRlcywgSSBkb24ndCB0aGluayB0aGF0J3MKPiA+IG5lZWQgeWV0LiBUaGUg YnVzIGZvcm1hdHMgcmVwb3J0ZWQgYnkgdGhpcyBwYXRjaCBzZXJpZXMgYXJlIHN0YXRpYy4gV2Un cmUKPiA+IG5vdCB0YWxraW5nIGFib3V0IHRoZSBjdXJyZW50bHkgY29uZmlndXJlZCBmb3JtYXQg Zm9yIGEgYnJpZGdlLCBidXQgYWJvdXQKPiA+IHRoZSBsaXN0IG9mIHN1cHBvcnRlZCBmb3JtYXRz LiBUaGlzIGlzIHNpbWlsYXIgdG8gdGhlIGJ1c19mb3JtYXRzIGZpZWxkCj4gPiBwcmVzZW50IGlu IHRoZSBkcm1fZGlzcGxheV9pbmZvIHN0cnVjdHVyZS4gVGhlcmUgaXMgdGh1cyBpbiBteSBvcGlu aW9uIG5vCj4gPiBuZWVkIHRvIGludGVyZmFjZSB0aGlzIHdpdGggYXRvbWljIHVudGlsIHdlIG5l ZWQgdG8gdHJhY2sgdGhlIGN1cnJlbnQKPiA+IGZvcm1hdCAoYW5kIEkgdGhpbmsgdGhhdCB3aWxs IGluZGVlZCBoYXBwZW4gYXQgc29tZSBwb2ludCwgYnV0IEkgZG9uJ3QKPiA+IHRoaW5rIFBldGVy IG5lZWRzIHRoaXMgZmVhdHVyZSBmb3Igbm93KS4gVGhhdCdzIHdoeSBJJ3ZlIHRvbGQgUGV0ZXIg dGhhdAo+ID4gSSB3b3VsZCBsaWtlIGEgYnJpZGdlIEFQSSB0byByZXBvcnQgdGhlIGluZm9ybWF0 aW9uIGFuZCBoYXZlbid0IHJlcXVlc3RlZAo+ID4gYSBzdGF0ZS1iYXNlZCBpbXBsZW1lbnRhdGlv bi4KPiAKPiBJZiBpdCdzIHN0YXRpYywgd2h5IGEgZHluYW1pYyBjYWxsYmFjaz8gSnVzdCBhZGQg aXQgdG8gc3RydWN0Cj4gZHJtX2JyaWRnZSwgcmVxdWlyZSBpdCdzIGZpbGxlZCBvdXQgYmVmb3Jl IHJlZ2lzdGVyaW5nIHRoZSBicmlkZ2UsCj4gZG9uZS4KCklmIEkgcmVtZW1iZXIgY29ycmVjdGx5 IEkgbWVudGlvbmVkIGJvdGggb3B0aW9ucyBpbiBteSBpbml0aWFsIHByb3Bvc2FsLCAKd2l0aG91 dCBhIHBlcnNvbmFsIHByZWZlcmVuY2UuIEEgbmV3IGZpZWxkIGluIHN0cnVjdCBkcm1fYnJpZGdl IHdvdWxkIApjZXJ0YWlubHkgd29yayBmb3IgbWUuCgo+ID4+IFBsZWFzZSBhbHNvIGNjIExhdXJl bnQgUGluY2hhcnQgb24gdGhpcy4KPiA+PiAKPiA+Pj4gQ2hhbmdlcyBzaW5jZSB2MSBodHRwczov L2xrbWwub3JnL2xrbWwvMjAxOC8zLzE3LzIyMQo+ID4+PiAtIEFkZCBhIHByb3BlciBicmlkZ2Ug QVBJIHRvIHF1ZXJ5IHRoZSBidXNfZm9ybWF0IGluc3RlYWQgb2YgYWJ1c2luZwo+ID4+PiAgIHRo ZSAtPmdldF9tb2RlcyBwYXJ0IG9mIHRoZSBjb2RlLiBUaGlzIGlzIGNsZWFuZXIgYnV0IHJlcXVp cmVzCj4gPj4+ICAgY2hhbmdlcyB0byBhbGwgZGlzcGxheSBjb250cm9sbGVyIGRyaXZlcnMgd2lz aGluZyB0byBwYXJ0aWNpcGF0ZS4KPiA+Pj4gLSBBZGQgcGF0Y2ggdG8gYWRqdXN0IHRoZSBhdG1l bC1obGNkYyBkcml2ZXIgYWNjb3JkaW5nIHRvIHRoZSBhYm92ZS4KPiA+Pj4gLSBIb29rIHRoZSBu ZXcgaW5mbyBpbnRvIHRoZSBicmlkZ2UgbG9jYWwgdG8gdGhlIGx2ZHMtZW5jb2RlciBpbnN0ZWFk Cj4gPj4+ICAgb2YgbWVzc2luZyBhYm91dCB3aXRoIG5ldyBpbnRlcmZhY2VzIGZvciB0aGUgcGFu ZWwtYnJpZGdlIGRyaXZlci4KPiA+Pj4gLSBBZGQgcGF0Y2ggd2l0aCBhIERUIHBhcnNpbmcgZnVu Y3Rpb24gb2YgYnVzX2Zvcm1hdHMgaW4gYSBjZW50cmFsCj4gPj4+ICAgcGxhY2UuCj4gPj4+IC0g UmVwaHJhc2UgdGhlIGFkZGl0aW9uIG9mIHRpLGRzOTBjMTg1IHRvIHRoZSBsdmRzLXRyYW5zbWl0 dGVyIGJpbmRpbmcuCj4gPj4+IAo+ID4+PiBQZXRlciBSb3NpbiAoNSk6Cj4gPj4+ICAgZHQtYmlu ZGluZ3M6IGRpc3BsYXk6IGJyaWRnZTogbHZkcy10cmFuc21pdHRlcjogYWRkIHRpLGRzOTBjMTg1 Cj4gPj4+ICAgZHJtOiBicmlkZ2U6IGFkZCBBUEkgdG8gcXVlcnkgdGhlIGV4cGVjdGVkIGlucHV0 IGZvcm1hdHMgb2YgYnJpZGdlcwo+ID4+PiAgIGRybTogb2Y6IGFkZCBkaXNwbGF5IGJ1cy1mb3Jt YXQgcGFyc2VyCj4gPj4+ICAgZHJtOiBicmlkZ2U6IGx2ZHMtZW5jb2RlcjogYWxsb3cgc3BlY2lm eWluZyB0aGUgaW5wdXQgYnVzIGZvcm1hdAo+ID4+PiAgIGRybS9hdG1lbC1obGNkYzogdGFrZSBi cmlkZ2VzIGludG8gYWNjb3VudCB3aGVuIHNlbGVjdGluZyBvdXRwdXQKPiA+Pj4gICAgIGZvcm1h dAo+ID4+PiAgCj4gPj4+ICAuLi4vYmluZGluZ3MvZGlzcGxheS9icmlkZ2UvbHZkcy10cmFuc21p dHRlci50eHQgICB8IDE0ICsrKystCj4gPj4+ICAuLi4vZGV2aWNldHJlZS9iaW5kaW5ncy9kaXNw bGF5L2J1cy1mb3JtYXQudHh0ICAgICB8IDM1ICsrKysrKysrKysrKysKPiA+Pj4gIGRyaXZlcnMv Z3B1L2RybS9hdG1lbC1obGNkYy9hdG1lbF9obGNkY19jcnRjLmMgICAgIHwgNDkgKysrKysrKysr KysrKy0tCj4gPj4+ICBkcml2ZXJzL2dwdS9kcm0vYnJpZGdlL2x2ZHMtZW5jb2Rlci5jICAgICAg ICAgICAgICB8IDI1ICsrKysrKysrKwo+ID4+PiAgZHJpdmVycy9ncHUvZHJtL2RybV9icmlkZ2Uu YyAgICAgICAgICAgICAgICAgICAgICAgfCAzMiArKysrKysrKysrKysKPiA+Pj4gIGRyaXZlcnMv Z3B1L2RybS9kcm1fb2YuYyAgICAgICAgICAgICAgICAgICAgICAgICAgIHwgNTkgKysrKysrKysr KysrKysrCj4gPj4+ICBpbmNsdWRlL2RybS9kcm1fYnJpZGdlLmggICAgICAgICAgICAgICAgICAg ICAgICAgICB8IDE4ICsrKysrKysKPiA+Pj4gIGluY2x1ZGUvZHJtL2RybV9vZi5oICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgIHwgIDkgKysrKwo+ID4+PiAgOCBmaWxlcyBjaGFuZ2VkLCAy MzcgaW5zZXJ0aW9ucygrKSwgNCBkZWxldGlvbnMoLSkKPiA+Pj4gIGNyZWF0ZSBtb2RlIDEwMDY0 NAo+ID4+PiAgRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVlL2JpbmRpbmdzL2Rpc3BsYXkvYnVzLWZv cm1hdC50eHQKCi0tIApSZWdhcmRzLAoKTGF1cmVudCBQaW5jaGFydAoKCgpfX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0 CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3Rv cC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751722AbeDDJHJ (ORCPT ); Wed, 4 Apr 2018 05:07:09 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:45350 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751251AbeDDJHI (ORCPT ); Wed, 4 Apr 2018 05:07:08 -0400 From: Laurent Pinchart To: Daniel Vetter Cc: Peter Rosin , Linux Kernel Mailing List , Mark Rutland , Boris Brezillon , Alexandre Belloni , devicetree@vger.kernel.org, David Airlie , Nicolas Ferre , dri-devel , Rob Herring , Jacopo Mondi , Daniel Vetter , Linux ARM Subject: Re: [PATCH v2 0/5] allow override of bus format in bridges Date: Wed, 04 Apr 2018 12:07:15 +0300 Message-ID: <1563250.XEJxFDuW0h@avalon> Organization: Ideas on Board Oy In-Reply-To: References: <20180326212447.7380-1-peda@axentia.se> <2233231.my6BbD3pcT@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Daniel, On Wednesday, 4 April 2018 09:34:41 EEST Daniel Vetter wrote: > On Wed, Apr 4, 2018 at 12:28 AM, Laurent Pinchart wrote: > > On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote: > >> On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote: > >>> Hi! > >>> > >>> [I got to v2 sooner than expected] > >>> > >>> I have an Atmel sama5d31 hooked up to an lvds encoder and then > >>> on to an lvds panel. Which seems like something that has been > >>> done one or two times before... > >>> > >>> The problem is that the bus_format of the SoC and the panel do > >>> not agree. The SoC driver (atmel-hlcdc) can handle the > >>> rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is > >>> wired for the rgb565 case. The lvds encoder supports rgb888 on > >>> its input side with the LSB wires for each color simply pulled > >>> down internally in the encoder in my case which means that the > >>> rgb565 bus_format is the format that works best. And the panel > >>> is expecting lvds (vesa-24), which is what the encoder outputs. > >>> > >>> The reason I "blame" the bus_format of the drm_connector is that > >>> with the below DT snippet, things do not work *exactly* due to > >>> that. At least, it starts to work if I hack the panel-lvds driver > >>> to report the rgb565 bus_format instead of vesa-24. > >>> > >>> panel: panel { > >>> compatible = "panel-lvds"; > >>> > >>> width-mm = <304>; > >>> height-mm = <228; > >>> > >>> data-mapping = "vesa-24"; > >>> > >>> panel-timing { > >>> // 1024x768 @ 60Hz (typical) > >>> clock-frequency = <52140000 65000000 71100000>; > >>> hactive = <1024>; > >>> vactive = <768>; > >>> hfront-porch = <48 88 88>; > >>> hback-porch = <96 168 168>; > >>> hsync-len = <32 64 64>; > >>> vfront-porch = <8 13 14>; > >>> vback-porch = <8 13 14>; > >>> vsync-len = <8 12 14>; > >>> }; > >>> > >>> port { > >>> panel_input: endpoint { > >>> remote-endpoint = <&lvds_encoder_output>; > >>> }; > >>> }; > >>> }; > >>> > >>> lvds-encoder { > >>> compatible = "ti,ds90c185", "lvds-encoder"; > >>> > >>> ports { > >>> #address-cells = <1>; > >>> #size-cells = <0>; > >>> > >>> port@0 { > >>> reg = <0>; > >>> > >>> lvds_encoder_input: endpoint { > >>> remote-endpoint = <&hlcdc_output>; > >>> }; > >>> }; > >>> > >>> port@1 { > >>> reg = <1>; > >>> > >>> lvds_encoder_output: endpoint { > >>> remote-endpoint = <&panel_input>; > >>> }; > >>> }; > >>> }; > >>> }; > >>> > >>> But, instead of perverting the panel-lvds driver with support > >>> for a totally fake non-lvds bus_format, I intruduce an API that allows > >>> display controller drivers to query the required bus_format of any > >>> intermediate bridges, and match up with that instead of the formats > >>> given by the drm_connector. I trigger this with this addition to the > >>> > >>> lvds-encoder DT node: > >>> interface-pix-fmt = "rgb565"; > >>> > >>> Naming is hard though, so I'm not sure if that's good? > >>> > >>> I threw in the first patch, since that is the actual lvds encoder > >>> I have in this case. > >>> > >>> Suggestions welcome. > >> > >> Took a quick look, feels rather un-atomic. And there's beend discussing > >> for other bridge related state that we might want to track (like the full > >> adjusted_mode that might need to be adjusted at each stage in the chain). > >> So here's my suggestions: > >> > >> - Add an optional per-bridge internal state struct using the support in > >> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-> >> private-state > >> > >> Yes it says "driver private", but since bridge is just helper stuff > >> that's all included. "driver private" == "not exposed as uapi" here. > >> Include all the usual convenience wrappers to get at the state for a > >> bridge. > >> > >> - Then stuff your bus_format into that new drm_bridge_state struct. > >> > >> - Add a new bridge callback atomic_check, which gets that bridge state as > >> parameter (similar to all the other atomic_check functions). > >> > >> This way we can even handle the bus_format dynamically, through the > >> atomic framework your bridge's atomic_check callback can look at the > >> entire atomic state (both up and down the chain if needed), it all neatly > >> fits into atomic overall and it's much easier to extend. > > > > While I think we'll eventually need bridge states, I don't think that's > > need yet. The bus formats reported by this patch series are static. We're > > not talking about the currently configured format for a bridge, but about > > the list of supported formats. This is similar to the bus_formats field > > present in the drm_display_info structure. There is thus in my opinion no > > need to interface this with atomic until we need to track the current > > format (and I think that will indeed happen at some point, but I don't > > think Peter needs this feature for now). That's why I've told Peter that > > I would like a bridge API to report the information and haven't requested > > a state-based implementation. > > If it's static, why a dynamic callback? Just add it to struct > drm_bridge, require it's filled out before registering the bridge, > done. If I remember correctly I mentioned both options in my initial proposal, without a personal preference. A new field in struct drm_bridge would certainly work for me. > >> Please also cc Laurent Pinchart on this. > >> > >>> Changes since v1 https://lkml.org/lkml/2018/3/17/221 > >>> - Add a proper bridge API to query the bus_format instead of abusing > >>> the ->get_modes part of the code. This is cleaner but requires > >>> changes to all display controller drivers wishing to participate. > >>> - Add patch to adjust the atmel-hlcdc driver according to the above. > >>> - Hook the new info into the bridge local to the lvds-encoder instead > >>> of messing about with new interfaces for the panel-bridge driver. > >>> - Add patch with a DT parsing function of bus_formats in a central > >>> place. > >>> - Rephrase the addition of ti,ds90c185 to the lvds-transmitter binding. > >>> > >>> Peter Rosin (5): > >>> dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185 > >>> drm: bridge: add API to query the expected input formats of bridges > >>> drm: of: add display bus-format parser > >>> drm: bridge: lvds-encoder: allow specifying the input bus format > >>> drm/atmel-hlcdc: take bridges into account when selecting output > >>> format > >>> > >>> .../bindings/display/bridge/lvds-transmitter.txt | 14 ++++- > >>> .../devicetree/bindings/display/bus-format.txt | 35 +++++++++++++ > >>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 49 +++++++++++++-- > >>> drivers/gpu/drm/bridge/lvds-encoder.c | 25 +++++++++ > >>> drivers/gpu/drm/drm_bridge.c | 32 ++++++++++++ > >>> drivers/gpu/drm/drm_of.c | 59 +++++++++++++++ > >>> include/drm/drm_bridge.h | 18 +++++++ > >>> include/drm/drm_of.h | 9 ++++ > >>> 8 files changed, 237 insertions(+), 4 deletions(-) > >>> create mode 100644 > >>> Documentation/devicetree/bindings/display/bus-format.txt -- Regards, Laurent Pinchart