From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Wed, 04 Apr 2018 01:31:34 +0300 Subject: [PATCH v2 0/5] allow override of bus format in bridges In-Reply-To: <2233231.my6BbD3pcT@avalon> References: <20180326212447.7380-1-peda@axentia.se> <20180328070826.GY14155@phenom.ffwll.local> <2233231.my6BbD3pcT@avalon> Message-ID: <3049253.pVG2Z6fSJS@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi again, On Wednesday, 4 April 2018 01:28:29 EEST 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-priv > > at e-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. Reading patch 5/5 I fear this might not be so simple. I'll sleep over it and try to comment tomorrow. > > 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 01:31:34 +0300 Message-ID: <3049253.pVG2Z6fSJS@avalon> References: <20180326212447.7380-1-peda@axentia.se> <20180328070826.GY14155@phenom.ffwll.local> <2233231.my6BbD3pcT@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <2233231.my6BbD3pcT@avalon> 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@vger.kernel.org, dri-devel@lists.freedesktop.org, Nicolas Ferre , Rob Herring , Jacopo Mondi , Daniel Vetter , Peter Rosin , linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org SGkgYWdhaW4sCgpPbiBXZWRuZXNkYXksIDQgQXByaWwgMjAxOCAwMToyODoyOSBFRVNUIExhdXJl bnQgUGluY2hhcnQgd3JvdGU6Cj4gT24gV2VkbmVzZGF5LCAyOCBNYXJjaCAyMDE4IDEwOjA4OjI2 IEVFU1QgRGFuaWVsIFZldHRlciB3cm90ZToKPiA+IE9uIE1vbiwgTWFyIDI2LCAyMDE4IGF0IDEx OjI0OjQyUE0gKzAyMDAsIFBldGVyIFJvc2luIHdyb3RlOgo+ID4gPiBIaSEKPiA+ID4gCj4gPiA+ IFtJIGdvdCB0byB2MiBzb29uZXIgdGhhbiBleHBlY3RlZF0KPiA+ID4gCj4gPiA+IEkgaGF2ZSBh biBBdG1lbCBzYW1hNWQzMSBob29rZWQgdXAgdG8gYW4gbHZkcyBlbmNvZGVyIGFuZCB0aGVuCj4g PiA+IG9uIHRvIGFuIGx2ZHMgcGFuZWwuIFdoaWNoIHNlZW1zIGxpa2Ugc29tZXRoaW5nIHRoYXQg aGFzIGJlZW4KPiA+ID4gZG9uZSBvbmUgb3IgdHdvIHRpbWVzIGJlZm9yZS4uLgo+ID4gPiAKPiA+ ID4gVGhlIHByb2JsZW0gaXMgdGhhdCB0aGUgYnVzX2Zvcm1hdCBvZiB0aGUgU29DIGFuZCB0aGUg cGFuZWwgZG8KPiA+ID4gbm90IGFncmVlLiBUaGUgU29DIGRyaXZlciAoYXRtZWwtaGxjZGMpIGNh biBoYW5kbGUgdGhlCj4gPiA+IHJnYjQ0NCwgcmdiNTY1LCByZ2I2NjYgYW5kIHJnYjg4OCBidXMg Zm9ybWF0cy4gVGhlIGhhcmR3YXJlIGlzCj4gPiA+IHdpcmVkIGZvciB0aGUgcmdiNTY1IGNhc2Uu IFRoZSBsdmRzIGVuY29kZXIgc3VwcG9ydHMgcmdiODg4IG9uCj4gPiA+IGl0cyBpbnB1dCBzaWRl IHdpdGggdGhlIExTQiB3aXJlcyBmb3IgZWFjaCBjb2xvciBzaW1wbHkgcHVsbGVkCj4gPiA+IGRv d24gaW50ZXJuYWxseSBpbiB0aGUgZW5jb2RlciBpbiBteSBjYXNlIHdoaWNoIG1lYW5zIHRoYXQg dGhlCj4gPiA+IHJnYjU2NSBidXNfZm9ybWF0IGlzIHRoZSBmb3JtYXQgdGhhdCB3b3JrcyBiZXN0 LiBBbmQgdGhlIHBhbmVsCj4gPiA+IGlzIGV4cGVjdGluZyBsdmRzICh2ZXNhLTI0KSwgd2hpY2gg aXMgd2hhdCB0aGUgZW5jb2RlciBvdXRwdXRzLgo+ID4gPiAKPiA+ID4gVGhlIHJlYXNvbiBJICJi bGFtZSIgdGhlIGJ1c19mb3JtYXQgb2YgdGhlIGRybV9jb25uZWN0b3IgaXMgdGhhdAo+ID4gPiB3 aXRoIHRoZSBiZWxvdyBEVCBzbmlwcGV0LCB0aGluZ3MgZG8gbm90IHdvcmsgKmV4YWN0bHkqIGR1 ZSB0bwo+ID4gPiB0aGF0LiBBdCBsZWFzdCwgaXQgc3RhcnRzIHRvIHdvcmsgaWYgSSBoYWNrIHRo ZSBwYW5lbC1sdmRzIGRyaXZlcgo+ID4gPiB0byByZXBvcnQgdGhlIHJnYjU2NSBidXNfZm9ybWF0 IGluc3RlYWQgb2YgdmVzYS0yNC4KPiA+ID4gCj4gPiA+IAlwYW5lbDogcGFuZWwgewo+ID4gPiAJ CWNvbXBhdGlibGUgPSAicGFuZWwtbHZkcyI7Cj4gPiA+IAkJCj4gPiA+IAkJd2lkdGgtbW0gPSA8 MzA0PjsKPiA+ID4gCQloZWlnaHQtbW0gPSA8MjI4Owo+ID4gPiAJCQo+ID4gPiAJCWRhdGEtbWFw cGluZyA9ICJ2ZXNhLTI0IjsKPiA+ID4gCQkKPiA+ID4gCQlwYW5lbC10aW1pbmcgewo+ID4gPiAJ CQkvLyAxMDI0eDc2OCBAIDYwSHogKHR5cGljYWwpCj4gPiA+IAkJCWNsb2NrLWZyZXF1ZW5jeSA9 IDw1MjE0MDAwMCA2NTAwMDAwMCA3MTEwMDAwMD47Cj4gPiA+IAkJCWhhY3RpdmUgPSA8MTAyND47 Cj4gPiA+IAkJCXZhY3RpdmUgPSA8NzY4PjsKPiA+ID4gCQkJaGZyb250LXBvcmNoID0gPDQ4IDg4 IDg4PjsKPiA+ID4gCQkJaGJhY2stcG9yY2ggPSA8OTYgMTY4IDE2OD47Cj4gPiA+IAkJCWhzeW5j LWxlbiA9IDwzMiA2NCA2ND47Cj4gPiA+IAkJCXZmcm9udC1wb3JjaCA9IDw4IDEzIDE0PjsKPiA+ ID4gCQkJdmJhY2stcG9yY2ggPSA8OCAxMyAxND47Cj4gPiA+IAkJCXZzeW5jLWxlbiA9IDw4IDEy IDE0PjsKPiA+ID4gCQl9Owo+ID4gPiAJCQo+ID4gPiAJCXBvcnQgewo+ID4gPiAJCQlwYW5lbF9p bnB1dDogZW5kcG9pbnQgewo+ID4gPiAJCQkJcmVtb3RlLWVuZHBvaW50ID0gPCZsdmRzX2VuY29k ZXJfb3V0cHV0PjsKPiA+ID4gCQkJfTsKPiA+ID4gCQl9Owo+ID4gPiAJfTsKPiA+ID4gCQo+ID4g PiAJbHZkcy1lbmNvZGVyIHsKPiA+ID4gCQljb21wYXRpYmxlID0gInRpLGRzOTBjMTg1IiwgImx2 ZHMtZW5jb2RlciI7Cj4gPiA+IAkJCj4gPiA+IAkJcG9ydHMgewo+ID4gPiAJCQkjYWRkcmVzcy1j ZWxscyA9IDwxPjsKPiA+ID4gCQkJI3NpemUtY2VsbHMgPSA8MD47Cj4gPiA+IAkJCQo+ID4gPiAJ CQlwb3J0QDAgewo+ID4gPiAJCQkJcmVnID0gPDA+Owo+ID4gPiAJCQkJCj4gPiA+IAkJCQlsdmRz X2VuY29kZXJfaW5wdXQ6IGVuZHBvaW50IHsKPiA+ID4gCQkJCQlyZW1vdGUtZW5kcG9pbnQgPSA8 JmhsY2RjX291dHB1dD47Cj4gPiA+IAkJCQl9Owo+ID4gPiAJCQl9Owo+ID4gPiAJCQkKPiA+ID4g CQkJcG9ydEAxIHsKPiA+ID4gCQkJCXJlZyA9IDwxPjsKPiA+ID4gCQkJCQo+ID4gPiAJCQkJbHZk c19lbmNvZGVyX291dHB1dDogZW5kcG9pbnQgewo+ID4gPiAJCQkJCXJlbW90ZS1lbmRwb2ludCA9 IDwmcGFuZWxfaW5wdXQ+Owo+ID4gPiAJCQkJfTsKPiA+ID4gCQkJfTsKPiA+ID4gCQl9Owo+ID4g PiAJfTsKPiA+ID4gCj4gPiA+IEJ1dCwgaW5zdGVhZCBvZiBwZXJ2ZXJ0aW5nIHRoZSBwYW5lbC1s dmRzIGRyaXZlciB3aXRoIHN1cHBvcnQKPiA+ID4gZm9yIGEgdG90YWxseSBmYWtlIG5vbi1sdmRz IGJ1c19mb3JtYXQsIEkgaW50cnVkdWNlIGFuIEFQSSB0aGF0IGFsbG93cwo+ID4gPiBkaXNwbGF5 IGNvbnRyb2xsZXIgZHJpdmVycyB0byBxdWVyeSB0aGUgcmVxdWlyZWQgYnVzX2Zvcm1hdCBvZiBh bnkKPiA+ID4gaW50ZXJtZWRpYXRlIGJyaWRnZXMsIGFuZCBtYXRjaCB1cCB3aXRoIHRoYXQgaW5z dGVhZCBvZiB0aGUgZm9ybWF0cwo+ID4gPiBnaXZlbiBieSB0aGUgZHJtX2Nvbm5lY3Rvci4gSSB0 cmlnZ2VyIHRoaXMgd2l0aCB0aGlzIGFkZGl0aW9uIHRvIHRoZQo+ID4gPiAKPiA+ID4gbHZkcy1l bmNvZGVyIERUIG5vZGU6Cj4gPiA+IAkJaW50ZXJmYWNlLXBpeC1mbXQgPSAicmdiNTY1IjsKPiA+ ID4gCj4gPiA+IE5hbWluZyBpcyBoYXJkIHRob3VnaCwgc28gSSdtIG5vdCBzdXJlIGlmIHRoYXQn cyBnb29kPwo+ID4gPiAKPiA+ID4gSSB0aHJldyBpbiB0aGUgZmlyc3QgcGF0Y2gsIHNpbmNlIHRo YXQgaXMgdGhlIGFjdHVhbCBsdmRzIGVuY29kZXIKPiA+ID4gSSBoYXZlIGluIHRoaXMgY2FzZS4K PiA+ID4gCj4gPiA+IFN1Z2dlc3Rpb25zIHdlbGNvbWUuCj4gPiAKPiA+IFRvb2sgYSBxdWljayBs b29rLCBmZWVscyByYXRoZXIgdW4tYXRvbWljLiBBbmQgdGhlcmUncyBiZWVuZCBkaXNjdXNzaW5n Cj4gPiBmb3Igb3RoZXIgYnJpZGdlIHJlbGF0ZWQgc3RhdGUgdGhhdCB3ZSBtaWdodCB3YW50IHRv IHRyYWNrIChsaWtlIHRoZSBmdWxsCj4gPiBhZGp1c3RlZF9tb2RlIHRoYXQgbWlnaHQgbmVlZCB0 byBiZSBhZGp1c3RlZCBhdCBlYWNoIHN0YWdlIGluIHRoZSBjaGFpbikuCj4gPiBTbyBoZXJlJ3Mg bXkgc3VnZ2VzdGlvbnM6Cj4gPiAKPiA+IC0gQWRkIGFuIG9wdGlvbmFsIHBlci1icmlkZ2UgaW50 ZXJuYWwgc3RhdGUgc3RydWN0IHVzaW5nIHRoZSBzdXBwb3J0IGluCj4gPiAKPiA+IGh0dHBzOi8v ZHJpLmZyZWVkZXNrdG9wLm9yZy9kb2NzL2RybS9ncHUvZHJtLWttcy5odG1sI2hhbmRsaW5nLWRy aXZlci1wcml2Cj4gPiBhdCBlLXN0YXRlCj4gPiAKPiA+ICAgWWVzIGl0IHNheXMgImRyaXZlciBw cml2YXRlIiwgYnV0IHNpbmNlIGJyaWRnZSBpcyBqdXN0IGhlbHBlciBzdHVmZgo+ID4gICB0aGF0 J3MgYWxsIGluY2x1ZGVkLiAiZHJpdmVyIHByaXZhdGUiID09ICJub3QgZXhwb3NlZCBhcyB1YXBp IiBoZXJlLgo+ID4gICBJbmNsdWRlIGFsbCB0aGUgdXN1YWwgY29udmVuaWVuY2Ugd3JhcHBlcnMg dG8gZ2V0IGF0IHRoZSBzdGF0ZSBmb3IgYQo+ID4gICBicmlkZ2UuCj4gPiAKPiA+IC0gVGhlbiBz dHVmZiB5b3VyIGJ1c19mb3JtYXQgaW50byB0aGF0IG5ldyBkcm1fYnJpZGdlX3N0YXRlIHN0cnVj dC4KPiA+IAo+ID4gLSBBZGQgYSBuZXcgYnJpZGdlIGNhbGxiYWNrIGF0b21pY19jaGVjaywgd2hp Y2ggZ2V0cyB0aGF0IGJyaWRnZSBzdGF0ZSBhcwo+ID4gICBwYXJhbWV0ZXIgKHNpbWlsYXIgdG8g YWxsIHRoZSBvdGhlciBhdG9taWNfY2hlY2sgZnVuY3Rpb25zKS4KPiA+IAo+ID4gVGhpcyB3YXkg d2UgY2FuIGV2ZW4gaGFuZGxlIHRoZSBidXNfZm9ybWF0IGR5bmFtaWNhbGx5LCB0aHJvdWdoIHRo ZSBhdG9taWMKPiA+IGZyYW1ld29yayB5b3VyIGJyaWRnZSdzIGF0b21pY19jaGVjayBjYWxsYmFj ayBjYW4gbG9vayBhdCB0aGUgZW50aXJlCj4gPiBhdG9taWMgc3RhdGUgKGJvdGggdXAgYW5kIGRv d24gdGhlIGNoYWluIGlmIG5lZWRlZCksIGl0IGFsbCBuZWF0bHkgZml0cwo+ID4gaW50byBhdG9t aWMgb3ZlcmFsbCBhbmQgaXQncyBtdWNoIGVhc2llciB0byBleHRlbmQuCj4gCj4gV2hpbGUgSSB0 aGluayB3ZSdsbCBldmVudHVhbGx5IG5lZWQgYnJpZGdlIHN0YXRlcywgSSBkb24ndCB0aGluayB0 aGF0J3MgbmVlZAo+IHlldC4gVGhlIGJ1cyBmb3JtYXRzIHJlcG9ydGVkIGJ5IHRoaXMgcGF0Y2gg c2VyaWVzIGFyZSBzdGF0aWMuIFdlJ3JlIG5vdAo+IHRhbGtpbmcgYWJvdXQgdGhlIGN1cnJlbnRs eSBjb25maWd1cmVkIGZvcm1hdCBmb3IgYSBicmlkZ2UsIGJ1dCBhYm91dCB0aGUKPiBsaXN0IG9m IHN1cHBvcnRlZCBmb3JtYXRzLiBUaGlzIGlzIHNpbWlsYXIgdG8gdGhlIGJ1c19mb3JtYXRzIGZp ZWxkIHByZXNlbnQKPiBpbiB0aGUgZHJtX2Rpc3BsYXlfaW5mbyBzdHJ1Y3R1cmUuIFRoZXJlIGlz IHRodXMgaW4gbXkgb3BpbmlvbiBubyBuZWVkIHRvCj4gaW50ZXJmYWNlIHRoaXMgd2l0aCBhdG9t aWMgdW50aWwgd2UgbmVlZCB0byB0cmFjayB0aGUgY3VycmVudCBmb3JtYXQgKGFuZCBJCj4gdGhp bmsgdGhhdCB3aWxsIGluZGVlZCBoYXBwZW4gYXQgc29tZSBwb2ludCwgYnV0IEkgZG9uJ3QgdGhp bmsgUGV0ZXIgbmVlZHMKPiB0aGlzIGZlYXR1cmUgZm9yIG5vdykuIFRoYXQncyB3aHkgSSd2ZSB0 b2xkIFBldGVyIHRoYXQgSSB3b3VsZCBsaWtlIGEKPiBicmlkZ2UgQVBJIHRvIHJlcG9ydCB0aGUg aW5mb3JtYXRpb24gYW5kIGhhdmVuJ3QgcmVxdWVzdGVkIGEgc3RhdGUtYmFzZWQKPiBpbXBsZW1l bnRhdGlvbi4KClJlYWRpbmcgcGF0Y2ggNS81IEkgZmVhciB0aGlzIG1pZ2h0IG5vdCBiZSBzbyBz aW1wbGUuIEknbGwgc2xlZXAgb3ZlciBpdCBhbmQgCnRyeSB0byBjb21tZW50IHRvbW9ycm93LgoK PiA+IFBsZWFzZSBhbHNvIGNjIExhdXJlbnQgUGluY2hhcnQgb24gdGhpcy4KPiA+IAo+ID4gPiBD aGFuZ2VzIHNpbmNlIHYxIGh0dHBzOi8vbGttbC5vcmcvbGttbC8yMDE4LzMvMTcvMjIxCj4gPiA+ IC0gQWRkIGEgcHJvcGVyIGJyaWRnZSBBUEkgdG8gcXVlcnkgdGhlIGJ1c19mb3JtYXQgaW5zdGVh ZCBvZiBhYnVzaW5nCj4gPiA+ICAgdGhlIC0+Z2V0X21vZGVzIHBhcnQgb2YgdGhlIGNvZGUuIFRo aXMgaXMgY2xlYW5lciBidXQgcmVxdWlyZXMKPiA+ID4gICBjaGFuZ2VzIHRvIGFsbCBkaXNwbGF5 IGNvbnRyb2xsZXIgZHJpdmVycyB3aXNoaW5nIHRvIHBhcnRpY2lwYXRlLgo+ID4gPiAtIEFkZCBw YXRjaCB0byBhZGp1c3QgdGhlIGF0bWVsLWhsY2RjIGRyaXZlciBhY2NvcmRpbmcgdG8gdGhlIGFi b3ZlLgo+ID4gPiAtIEhvb2sgdGhlIG5ldyBpbmZvIGludG8gdGhlIGJyaWRnZSBsb2NhbCB0byB0 aGUgbHZkcy1lbmNvZGVyIGluc3RlYWQKPiA+ID4gICBvZiBtZXNzaW5nIGFib3V0IHdpdGggbmV3 IGludGVyZmFjZXMgZm9yIHRoZSBwYW5lbC1icmlkZ2UgZHJpdmVyLgo+ID4gPiAtIEFkZCBwYXRj aCB3aXRoIGEgRFQgcGFyc2luZyBmdW5jdGlvbiBvZiBidXNfZm9ybWF0cyBpbiBhIGNlbnRyYWwK PiA+ID4gICBwbGFjZS4KPiA+ID4gLSBSZXBocmFzZSB0aGUgYWRkaXRpb24gb2YgdGksZHM5MGMx ODUgdG8gdGhlIGx2ZHMtdHJhbnNtaXR0ZXIgYmluZGluZy4KPiA+ID4gCj4gPiA+IFBldGVyIFJv c2luICg1KToKPiA+ID4gICBkdC1iaW5kaW5nczogZGlzcGxheTogYnJpZGdlOiBsdmRzLXRyYW5z bWl0dGVyOiBhZGQgdGksZHM5MGMxODUKPiA+ID4gICBkcm06IGJyaWRnZTogYWRkIEFQSSB0byBx dWVyeSB0aGUgZXhwZWN0ZWQgaW5wdXQgZm9ybWF0cyBvZiBicmlkZ2VzCj4gPiA+ICAgZHJtOiBv ZjogYWRkIGRpc3BsYXkgYnVzLWZvcm1hdCBwYXJzZXIKPiA+ID4gICBkcm06IGJyaWRnZTogbHZk cy1lbmNvZGVyOiBhbGxvdyBzcGVjaWZ5aW5nIHRoZSBpbnB1dCBidXMgZm9ybWF0Cj4gPiA+ICAg ZHJtL2F0bWVsLWhsY2RjOiB0YWtlIGJyaWRnZXMgaW50byBhY2NvdW50IHdoZW4gc2VsZWN0aW5n IG91dHB1dAo+ID4gPiAgICAgZm9ybWF0Cj4gPiA+ICAKPiA+ID4gIC4uLi9iaW5kaW5ncy9kaXNw bGF5L2JyaWRnZS9sdmRzLXRyYW5zbWl0dGVyLnR4dCAgIHwgMTQgKysrKy0KPiA+ID4gIC4uLi9k ZXZpY2V0cmVlL2JpbmRpbmdzL2Rpc3BsYXkvYnVzLWZvcm1hdC50eHQgICAgIHwgMzUgKysrKysr KysrKysrKwo+ID4gPiAgZHJpdmVycy9ncHUvZHJtL2F0bWVsLWhsY2RjL2F0bWVsX2hsY2RjX2Ny dGMuYyAgICAgfCA0OSArKysrKysrKysrKysrLS0KPiA+ID4gIGRyaXZlcnMvZ3B1L2RybS9icmlk Z2UvbHZkcy1lbmNvZGVyLmMgICAgICAgICAgICAgIHwgMjUgKysrKysrKysrCj4gPiA+ICBkcml2 ZXJzL2dwdS9kcm0vZHJtX2JyaWRnZS5jICAgICAgICAgICAgICAgICAgICAgICB8IDMyICsrKysr KysrKysrKwo+ID4gPiAgZHJpdmVycy9ncHUvZHJtL2RybV9vZi5jICAgICAgICAgICAgICAgICAg ICAgICAgICAgfCA1OSArKysrKysrKysrKysrKysKPiA+ID4gIGluY2x1ZGUvZHJtL2RybV9icmlk Z2UuaCAgICAgICAgICAgICAgICAgICAgICAgICAgIHwgMTggKysrKysrKwo+ID4gPiAgaW5jbHVk ZS9kcm0vZHJtX29mLmggICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgfCAgOSArKysrCj4g PiA+ICA4IGZpbGVzIGNoYW5nZWQsIDIzNyBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9ucygtKQo+ ID4gPiAgY3JlYXRlIG1vZGUgMTAwNjQ0Cj4gPiA+ICBEb2N1bWVudGF0aW9uL2RldmljZXRyZWUv YmluZGluZ3MvZGlzcGxheS9idXMtZm9ybWF0LnR4dAoKLS0gClJlZ2FyZHMsCgpMYXVyZW50IFBp bmNoYXJ0CgoKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f CmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpo dHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756652AbeDCWb3 (ORCPT ); Tue, 3 Apr 2018 18:31:29 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:42850 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756237AbeDCWb0 (ORCPT ); Tue, 3 Apr 2018 18:31:26 -0400 From: Laurent Pinchart To: Daniel Vetter Cc: Peter Rosin , linux-kernel@vger.kernel.org, Mark Rutland , Boris Brezillon , Alexandre Belloni , devicetree@vger.kernel.org, David Airlie , Nicolas Ferre , dri-devel@lists.freedesktop.org, Rob Herring , Jacopo Mondi , Daniel Vetter , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 0/5] allow override of bus format in bridges Date: Wed, 04 Apr 2018 01:31:34 +0300 Message-ID: <3049253.pVG2Z6fSJS@avalon> Organization: Ideas on Board Oy In-Reply-To: <2233231.my6BbD3pcT@avalon> References: <20180326212447.7380-1-peda@axentia.se> <20180328070826.GY14155@phenom.ffwll.local> <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 again, On Wednesday, 4 April 2018 01:28:29 EEST 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-priv > > at e-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. Reading patch 5/5 I fear this might not be so simple. I'll sleep over it and try to comment tomorrow. > > 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