From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from galahad.ideasonboard.com ([185.26.127.97]:33584 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936054AbdADOdx (ORCPT ); Wed, 4 Jan 2017 09:33:53 -0500 From: Laurent Pinchart To: Daniel Vetter Cc: Laurent Pinchart , dri-devel , "open list:DRM DRIVERS FOR RENESAS" Subject: Re: [PATCH v3 06/13] drm: bridge: Add LVDS encoder driver Date: Wed, 04 Jan 2017 16:33:57 +0200 Message-ID: <2223998.87cnrdv6Vi@avalon> In-Reply-To: References: <1480410283-28698-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <5172242.1xI6ry2aS1@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: Hi Daniel, On Wednesday 04 Jan 2017 14:51:48 Daniel Vetter wrote: > On Wed, Jan 4, 2017 at 2:08 PM, Laurent Pinchart wrote: > > On Wednesday 04 Jan 2017 09:18:18 Daniel Vetter wrote: > >> On Tue, Nov 29, 2016 at 10:57:07PM +0200, Laurent Pinchart wrote: > >>> On Tuesday 29 Nov 2016 10:54:09 Daniel Vetter wrote: > >>>> On Tue, Nov 29, 2016 at 11:04:36AM +0200, Laurent Pinchart wrote: > >>>>> The LVDS encoder driver is a DRM bridge driver that supports the > >>>>> parallel to LVDS encoders that don't require any configuration. The > >>>>> driver thus doesn't interact with the device, but creates an LVDS > >>>>> connector for the panel and exposes its size and timing based on > >>>>> information retrieved from DT. > >>>>> > >>>>> Signed-off-by: Laurent Pinchart > >>>>> > >>>> > >>>> Since it's 100% dummy, why put LVDS into the name? This little thing > >>>> here could be our generic "wrap drm_panel and attach it to a chain" > >>>> helper. So what about calling this _The_ drm_panel_bridge, and also > >>>> linking it into docs to feature it a bit more prominently. > >>> > >>> I'm not opposed to that, except that this driver should not be > >>> considered as just a helper to link a panel. It should only be used to > >>> support a real hardware bridge that requires no control. > >>> > >>>> I came up with this because I spotted some refactoring belows for > >>>> building this helper, until I realized that this driver _is_ the > >>>> helper I think we want ;-) Only thing missing is an exported function > >>>> to instantiate a bridge with just a drm_panel as the parameter. And > >>>> putting it into the drm_kms_helper.ko module. > >>> > >>> What would that be used for ? The bridge should be instantiated by this > >>> bridge driver, bound to a bridge device instantiated from DT (or the > >>> platform firmware of your choice). > >> > >> Atm every driver using drm_panel needs a bit of glue to bind it into it's > >> display chain. With this code, and bridge chaining, you'd get that for > >> free, and I think that would be rather useful. > > > > You would trade the bit of panel glue for a bit of bridge glue. Bridge > > chaining could perhaps help slightly at runtime there, but at init time > > the > > amount of glue would be similar. > > Hm, something like drm_bridge_panel_bridge_init(dev, panel) should be > enough, or not? My idea is to use this for the case where the only > thing in dt is the panel, with no real bridge chip. And I think we > don't need anything beyond that one _init function, plus maybe some > additional paramaters ... There should be no bridge then. If you want the DRM core to manage panels automatically, then we should create specific helpers for that, not abuse the bridge infrastructure. Bridges should be instantiated from a hardware device and bound to drivers as usual. > > I've noticed one piece of code that is LVDS-specific in this driver, and > > that's connector creation. The connector type is hardcoded to LVDS. To > > make the driver more generic, we would need a way to find the connector > > type at runtime. I'm wondering whether I should do this now, or keep the > > driver LVDS- specific until someone comes up with a similar need. Without > > several potential users it's often hard to design a properly generic API. > > ... like whether the panel is dsi or lvds or soemthing else. Or maybe > we should just use LVDS for everything, because it's a panel, and > userspace shouldn't really care beyond that ;-) Feel free to propose a panel connector type :-) > >> And from a software point of view I'd say if it quacks like a bridge, and > >> walks like a bridge, it probably _is_ a bridge. Even if no one calls > >> that, or if physical the only thing on the board at that spot are a bunch > >> of dumb wires. > > > > I dream of moving all encoders to DRM bridge, and then unifying drm_bridge > > and drm_panel with a common set of operations that could be invoked on > > both objects. That way the core could chain bridges and panels quite > > easily. I plan to experiment with this when moving the omapdrm driver to > > DRM bridge and DRM panel (it currently includes a bunch of custom encoder > > and panel drivers). > > Agreed on that dream, auto-wrapping panels in dummy bridges would be a > first step towards that. But I think that's a step in the wrong direction. Ideally I'd like the encoders/bridges not to have to care about connectors and panels. There are multiple options, but a dummy bridge isn't a good idea in my opinion. > The other one is making drm_encoders entirely dummies, and I think we're 90% > there already. We need to convert everything to drm_bridge first, we're not quite there yet :-) > End result isn't quite as clean as a complete rewrite, but probably > good enough. And because uapi we can't get rid of drm_encoder anyway, > and keeping drm_panel as the internal thing embedded into a > drm_panel_bridge struct seems reasonable too. That way panel drivers > can cope with a slightly simpler interface than full bridges. -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v3 06/13] drm: bridge: Add LVDS encoder driver Date: Wed, 04 Jan 2017 16:33:57 +0200 Message-ID: <2223998.87cnrdv6Vi@avalon> References: <1480410283-28698-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <5172242.1xI6ry2aS1@avalon> 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 [IPv6:2001:4b98:dc2:45:216:3eff:febb:480d]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0BCF96E71B for ; Wed, 4 Jan 2017 14:33:53 +0000 (UTC) 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: "open list:DRM DRIVERS FOR RENESAS" , Laurent Pinchart , dri-devel List-Id: dri-devel@lists.freedesktop.org SGkgRGFuaWVsLAoKT24gV2VkbmVzZGF5IDA0IEphbiAyMDE3IDE0OjUxOjQ4IERhbmllbCBWZXR0 ZXIgd3JvdGU6Cj4gT24gV2VkLCBKYW4gNCwgMjAxNyBhdCAyOjA4IFBNLCBMYXVyZW50IFBpbmNo YXJ0IHdyb3RlOgo+ID4gT24gV2VkbmVzZGF5IDA0IEphbiAyMDE3IDA5OjE4OjE4IERhbmllbCBW ZXR0ZXIgd3JvdGU6Cj4gPj4gT24gVHVlLCBOb3YgMjksIDIwMTYgYXQgMTA6NTc6MDdQTSArMDIw MCwgTGF1cmVudCBQaW5jaGFydCB3cm90ZToKPiA+Pj4gT24gVHVlc2RheSAyOSBOb3YgMjAxNiAx MDo1NDowOSBEYW5pZWwgVmV0dGVyIHdyb3RlOgo+ID4+Pj4gT24gVHVlLCBOb3YgMjksIDIwMTYg YXQgMTE6MDQ6MzZBTSArMDIwMCwgTGF1cmVudCBQaW5jaGFydCB3cm90ZToKPiA+Pj4+PiBUaGUg TFZEUyBlbmNvZGVyIGRyaXZlciBpcyBhIERSTSBicmlkZ2UgZHJpdmVyIHRoYXQgc3VwcG9ydHMg dGhlCj4gPj4+Pj4gcGFyYWxsZWwgdG8gTFZEUyBlbmNvZGVycyB0aGF0IGRvbid0IHJlcXVpcmUg YW55IGNvbmZpZ3VyYXRpb24uIFRoZQo+ID4+Pj4+IGRyaXZlciB0aHVzIGRvZXNuJ3QgaW50ZXJh Y3Qgd2l0aCB0aGUgZGV2aWNlLCBidXQgY3JlYXRlcyBhbiBMVkRTCj4gPj4+Pj4gY29ubmVjdG9y IGZvciB0aGUgcGFuZWwgYW5kIGV4cG9zZXMgaXRzIHNpemUgYW5kIHRpbWluZyBiYXNlZCBvbgo+ ID4+Pj4+IGluZm9ybWF0aW9uIHJldHJpZXZlZCBmcm9tIERULgo+ID4+Pj4+IAo+ID4+Pj4+IFNp Z25lZC1vZmYtYnk6IExhdXJlbnQgUGluY2hhcnQKPiA+Pj4+PiA8bGF1cmVudC5waW5jaGFydCty ZW5lc2FzQGlkZWFzb25ib2FyZC5jb20+Cj4gPj4+PiAKPiA+Pj4+IFNpbmNlIGl0J3MgMTAwJSBk dW1teSwgd2h5IHB1dCBMVkRTIGludG8gdGhlIG5hbWU/IFRoaXMgbGl0dGxlIHRoaW5nCj4gPj4+ PiBoZXJlIGNvdWxkIGJlIG91ciBnZW5lcmljICJ3cmFwIGRybV9wYW5lbCBhbmQgYXR0YWNoIGl0 IHRvIGEgY2hhaW4iCj4gPj4+PiBoZWxwZXIuIFNvIHdoYXQgYWJvdXQgY2FsbGluZyB0aGlzIF9U aGVfIGRybV9wYW5lbF9icmlkZ2UsIGFuZCBhbHNvCj4gPj4+PiBsaW5raW5nIGl0IGludG8gZG9j cyB0byBmZWF0dXJlIGl0IGEgYml0IG1vcmUgcHJvbWluZW50bHkuCj4gPj4+IAo+ID4+PiBJJ20g bm90IG9wcG9zZWQgdG8gdGhhdCwgZXhjZXB0IHRoYXQgdGhpcyBkcml2ZXIgc2hvdWxkIG5vdCBi ZQo+ID4+PiBjb25zaWRlcmVkIGFzIGp1c3QgYSBoZWxwZXIgdG8gbGluayBhIHBhbmVsLiBJdCBz aG91bGQgb25seSBiZSB1c2VkIHRvCj4gPj4+IHN1cHBvcnQgYSByZWFsIGhhcmR3YXJlIGJyaWRn ZSB0aGF0IHJlcXVpcmVzIG5vIGNvbnRyb2wuCj4gPj4+IAo+ID4+Pj4gSSBjYW1lIHVwIHdpdGgg dGhpcyBiZWNhdXNlIEkgc3BvdHRlZCBzb21lIHJlZmFjdG9yaW5nIGJlbG93cyBmb3IKPiA+Pj4+ IGJ1aWxkaW5nIHRoaXMgaGVscGVyLCB1bnRpbCBJIHJlYWxpemVkIHRoYXQgdGhpcyBkcml2ZXIg X2lzXyB0aGUKPiA+Pj4+IGhlbHBlciBJIHRoaW5rIHdlIHdhbnQgOy0pIE9ubHkgdGhpbmcgbWlz c2luZyBpcyBhbiBleHBvcnRlZCBmdW5jdGlvbgo+ID4+Pj4gdG8gaW5zdGFudGlhdGUgYSBicmlk Z2Ugd2l0aCBqdXN0IGEgZHJtX3BhbmVsIGFzIHRoZSBwYXJhbWV0ZXIuIEFuZAo+ID4+Pj4gcHV0 dGluZyBpdCBpbnRvIHRoZSBkcm1fa21zX2hlbHBlci5rbyBtb2R1bGUuCj4gPj4+IAo+ID4+PiBX aGF0IHdvdWxkIHRoYXQgYmUgdXNlZCBmb3IgPyBUaGUgYnJpZGdlIHNob3VsZCBiZSBpbnN0YW50 aWF0ZWQgYnkgdGhpcwo+ID4+PiBicmlkZ2UgZHJpdmVyLCBib3VuZCB0byBhIGJyaWRnZSBkZXZp Y2UgaW5zdGFudGlhdGVkIGZyb20gRFQgKG9yIHRoZQo+ID4+PiBwbGF0Zm9ybSBmaXJtd2FyZSBv ZiB5b3VyIGNob2ljZSkuCj4gPj4gCj4gPj4gQXRtIGV2ZXJ5IGRyaXZlciB1c2luZyBkcm1fcGFu ZWwgbmVlZHMgYSBiaXQgb2YgZ2x1ZSB0byBiaW5kIGl0IGludG8gaXQncwo+ID4+IGRpc3BsYXkg Y2hhaW4uIFdpdGggdGhpcyBjb2RlLCBhbmQgYnJpZGdlIGNoYWluaW5nLCB5b3UnZCBnZXQgdGhh dCBmb3IKPiA+PiBmcmVlLCBhbmQgSSB0aGluayB0aGF0IHdvdWxkIGJlIHJhdGhlciB1c2VmdWwu Cj4gPiAKPiA+IFlvdSB3b3VsZCB0cmFkZSB0aGUgYml0IG9mIHBhbmVsIGdsdWUgZm9yIGEgYml0 IG9mIGJyaWRnZSBnbHVlLiBCcmlkZ2UKPiA+IGNoYWluaW5nIGNvdWxkIHBlcmhhcHMgaGVscCBz bGlnaHRseSBhdCBydW50aW1lIHRoZXJlLCBidXQgYXQgaW5pdCB0aW1lCj4gPiB0aGUKPiA+IGFt b3VudCBvZiBnbHVlIHdvdWxkIGJlIHNpbWlsYXIuCj4gCj4gSG0sIHNvbWV0aGluZyBsaWtlIGRy bV9icmlkZ2VfcGFuZWxfYnJpZGdlX2luaXQoZGV2LCBwYW5lbCkgc2hvdWxkIGJlCj4gZW5vdWdo LCBvciBub3Q/IE15IGlkZWEgaXMgdG8gdXNlIHRoaXMgZm9yIHRoZSBjYXNlIHdoZXJlIHRoZSBv bmx5Cj4gdGhpbmcgaW4gZHQgaXMgdGhlIHBhbmVsLCB3aXRoIG5vIHJlYWwgYnJpZGdlIGNoaXAu IEFuZCBJIHRoaW5rIHdlCj4gZG9uJ3QgbmVlZCBhbnl0aGluZyBiZXlvbmQgdGhhdCBvbmUgX2lu aXQgZnVuY3Rpb24sIHBsdXMgbWF5YmUgc29tZQo+IGFkZGl0aW9uYWwgcGFyYW1hdGVycyAuLi4K ClRoZXJlIHNob3VsZCBiZSBubyBicmlkZ2UgdGhlbi4gSWYgeW91IHdhbnQgdGhlIERSTSBjb3Jl IHRvIG1hbmFnZSBwYW5lbHMgCmF1dG9tYXRpY2FsbHksIHRoZW4gd2Ugc2hvdWxkIGNyZWF0ZSBz cGVjaWZpYyBoZWxwZXJzIGZvciB0aGF0LCBub3QgYWJ1c2UgdGhlIApicmlkZ2UgaW5mcmFzdHJ1 Y3R1cmUuIEJyaWRnZXMgc2hvdWxkIGJlIGluc3RhbnRpYXRlZCBmcm9tIGEgaGFyZHdhcmUgZGV2 aWNlIAphbmQgYm91bmQgdG8gZHJpdmVycyBhcyB1c3VhbC4KCj4gPiBJJ3ZlIG5vdGljZWQgb25l IHBpZWNlIG9mIGNvZGUgdGhhdCBpcyBMVkRTLXNwZWNpZmljIGluIHRoaXMgZHJpdmVyLCBhbmQK PiA+IHRoYXQncyBjb25uZWN0b3IgY3JlYXRpb24uIFRoZSBjb25uZWN0b3IgdHlwZSBpcyBoYXJk Y29kZWQgdG8gTFZEUy4gVG8KPiA+IG1ha2UgdGhlIGRyaXZlciBtb3JlIGdlbmVyaWMsIHdlIHdv dWxkIG5lZWQgYSB3YXkgdG8gZmluZCB0aGUgY29ubmVjdG9yCj4gPiB0eXBlIGF0IHJ1bnRpbWUu IEknbSB3b25kZXJpbmcgd2hldGhlciBJIHNob3VsZCBkbyB0aGlzIG5vdywgb3Iga2VlcCB0aGUK PiA+IGRyaXZlciBMVkRTLSBzcGVjaWZpYyB1bnRpbCBzb21lb25lIGNvbWVzIHVwIHdpdGggYSBz aW1pbGFyIG5lZWQuIFdpdGhvdXQKPiA+IHNldmVyYWwgcG90ZW50aWFsIHVzZXJzIGl0J3Mgb2Z0 ZW4gaGFyZCB0byBkZXNpZ24gYSBwcm9wZXJseSBnZW5lcmljIEFQSS4KPiAKPiAuLi4gbGlrZSB3 aGV0aGVyIHRoZSBwYW5lbCBpcyBkc2kgb3IgbHZkcyBvciBzb2VtdGhpbmcgZWxzZS4gT3IgbWF5 YmUKPiB3ZSBzaG91bGQganVzdCB1c2UgTFZEUyBmb3IgZXZlcnl0aGluZywgYmVjYXVzZSBpdCdz IGEgcGFuZWwsIGFuZAo+IHVzZXJzcGFjZSBzaG91bGRuJ3QgcmVhbGx5IGNhcmUgYmV5b25kIHRo YXQgOy0pCgpGZWVsIGZyZWUgdG8gcHJvcG9zZSBhIHBhbmVsIGNvbm5lY3RvciB0eXBlIDotKQoK PiA+PiBBbmQgZnJvbSBhIHNvZnR3YXJlIHBvaW50IG9mIHZpZXcgSSdkIHNheSBpZiBpdCBxdWFj a3MgbGlrZSBhIGJyaWRnZSwgYW5kCj4gPj4gd2Fsa3MgbGlrZSBhIGJyaWRnZSwgaXQgcHJvYmFi bHkgX2lzXyBhIGJyaWRnZS4gRXZlbiBpZiBubyBvbmUgY2FsbHMKPiA+PiB0aGF0LCBvciBpZiBw aHlzaWNhbCB0aGUgb25seSB0aGluZyBvbiB0aGUgYm9hcmQgYXQgdGhhdCBzcG90IGFyZSBhIGJ1 bmNoCj4gPj4gb2YgZHVtYiB3aXJlcy4KPiA+IAo+ID4gSSBkcmVhbSBvZiBtb3ZpbmcgYWxsIGVu Y29kZXJzIHRvIERSTSBicmlkZ2UsIGFuZCB0aGVuIHVuaWZ5aW5nIGRybV9icmlkZ2UKPiA+IGFu ZCBkcm1fcGFuZWwgd2l0aCBhIGNvbW1vbiBzZXQgb2Ygb3BlcmF0aW9ucyB0aGF0IGNvdWxkIGJl IGludm9rZWQgb24KPiA+IGJvdGggb2JqZWN0cy4gVGhhdCB3YXkgdGhlIGNvcmUgY291bGQgY2hh aW4gYnJpZGdlcyBhbmQgcGFuZWxzIHF1aXRlCj4gPiBlYXNpbHkuIEkgcGxhbiB0byBleHBlcmlt ZW50IHdpdGggdGhpcyB3aGVuIG1vdmluZyB0aGUgb21hcGRybSBkcml2ZXIgdG8KPiA+IERSTSBi cmlkZ2UgYW5kIERSTSBwYW5lbCAoaXQgY3VycmVudGx5IGluY2x1ZGVzIGEgYnVuY2ggb2YgY3Vz dG9tIGVuY29kZXIKPiA+IGFuZCBwYW5lbCBkcml2ZXJzKS4KPgo+IEFncmVlZCBvbiB0aGF0IGRy ZWFtLCBhdXRvLXdyYXBwaW5nIHBhbmVscyBpbiBkdW1teSBicmlkZ2VzIHdvdWxkIGJlIGEKPiBm aXJzdCBzdGVwIHRvd2FyZHMgdGhhdC4KCkJ1dCBJIHRoaW5rIHRoYXQncyBhIHN0ZXAgaW4gdGhl IHdyb25nIGRpcmVjdGlvbi4gSWRlYWxseSBJJ2QgbGlrZSB0aGUgCmVuY29kZXJzL2JyaWRnZXMg bm90IHRvIGhhdmUgdG8gY2FyZSBhYm91dCBjb25uZWN0b3JzIGFuZCBwYW5lbHMuIFRoZXJlIGFy ZSAKbXVsdGlwbGUgb3B0aW9ucywgYnV0IGEgZHVtbXkgYnJpZGdlIGlzbid0IGEgZ29vZCBpZGVh IGluIG15IG9waW5pb24uCgo+IFRoZSBvdGhlciBvbmUgaXMgbWFraW5nIGRybV9lbmNvZGVycyBl bnRpcmVseSBkdW1taWVzLCBhbmQgSSB0aGluayB3ZSdyZSA5MCUKPiB0aGVyZSBhbHJlYWR5LgoK V2UgbmVlZCB0byBjb252ZXJ0IGV2ZXJ5dGhpbmcgdG8gZHJtX2JyaWRnZSBmaXJzdCwgd2UncmUg bm90IHF1aXRlIHRoZXJlIHlldCAKOi0pCgo+IEVuZCByZXN1bHQgaXNuJ3QgcXVpdGUgYXMgY2xl YW4gYXMgYSBjb21wbGV0ZSByZXdyaXRlLCBidXQgcHJvYmFibHkKPiBnb29kIGVub3VnaC4gQW5k IGJlY2F1c2UgdWFwaSB3ZSBjYW4ndCBnZXQgcmlkIG9mIGRybV9lbmNvZGVyIGFueXdheSwKPiBh bmQga2VlcGluZyBkcm1fcGFuZWwgYXMgdGhlIGludGVybmFsIHRoaW5nIGVtYmVkZGVkIGludG8g YQo+IGRybV9wYW5lbF9icmlkZ2Ugc3RydWN0IHNlZW1zIHJlYXNvbmFibGUgdG9vLiBUaGF0IHdh eSBwYW5lbCBkcml2ZXJzCj4gY2FuIGNvcGUgd2l0aCBhIHNsaWdodGx5IHNpbXBsZXIgaW50ZXJm YWNlIHRoYW4gZnVsbCBicmlkZ2VzLgoKLS0gClJlZ2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0Cgpf X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwg bWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK