From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH 1/2] drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge. Date: Wed, 03 May 2017 17:44:53 +0300 Message-ID: <2463758.9HmFMyJbcQ@avalon> References: <20170427163601.7313-1-eric@anholt.net> <2556350.BYUa4rMt48@avalon> <20170503142856.bmazihqoj6rgvbwq@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 [IPv6:2001:4b98:dc2:45:216:3eff:febb:480d]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9FCBF6E0E9 for ; Wed, 3 May 2017 14:43:37 +0000 (UTC) In-Reply-To: <20170503142856.bmazihqoj6rgvbwq@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: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Yannick Fertre , Thierry Reding List-Id: dri-devel@lists.freedesktop.org SGkgRGFuaWVsLAoKT24gV2VkbmVzZGF5IDAzIE1heSAyMDE3IDE2OjI4OjU2IERhbmllbCBWZXR0 ZXIgd3JvdGU6Cj4gT24gV2VkLCBNYXkgMDMsIDIwMTcgYXQgMTI6MzY6MDZQTSArMDMwMCwgTGF1 cmVudCBQaW5jaGFydCB3cm90ZToKPiA+IE9uIFdlZG5lc2RheSAwMyBNYXkgMjAxNyAxMTozMjox NyBEYW5pZWwgVmV0dGVyIHdyb3RlOgo+ID4+IE9uIFdlZCwgTWF5IDAzLCAyMDE3IGF0IDAyOjUz OjAwUE0gKzA1MzAsIEFyY2hpdCBUYW5lamEgd3JvdGU6Cj4gPj4+ICtwYW5lbC9icmlkZ2UgcmV2 aWV3ZXJzLgo+ID4+PiAKPiA+Pj4gVGhpcyBkb2VzIG1ha2UgdGhpbmdzIG11Y2ggY2xlYW5lciwg YnV0IGl0IHNlZW1zIGEgYml0IHN0cmFuZ2UgdG8KPiA+Pj4gY3JlYXRlIGEgZHJtX2JyaWRnZSB3 aGVuIHRoZXJlIGlzbid0IHJlYWxseSBhIEhXIGJyaWRnZSBpbiB0aGUgZGlzcGxheQo+ID4+PiBj aGFpbiAoaS5lLCB3aGVuIHRoZSBEU0kgZW5jb2RlciBpcyBkaXJlY3RseSBjb25uZWN0ZWQgdG8g YSBEU0kgcGFuZWwpLgo+ID4+PiAKPiA+Pj4gVGhlcmUgYXJlIGttcyBkcml2ZXJzIHRoYXQgdXNl IGRybV9wYW5lbCwgYnV0IGRvbid0IGhhdmUgc2ltcGxlIHN0dWIKPiA+Pj4gY29ubmVjdG9ycyB0 aGF0IHdyYXAgYXJvdW5kIGEgZHJtX3BhbmVsLiBUaGV5IGhhdmUgbW9yZSBjb21wbGljYXRlZAo+ ID4+PiBjb25uZWN0b3Igb3BzLCBhbmQgbWF5IGNhbGwgZHJtX3BhbmVsX3ByZXBhcmUoKSBhbmQg cmVsYXRlZCBmdW5jdGlvbnMKPiA+Pj4gYSBiaXQgZGlmZmVyZW50bHkuIFdlIHdvbid0IGJlIGFi bGUgdG8gdXNlIGRybV9wYW5lbF9icmlkZ2UgZm9yIHRob3NlCj4gPj4+IGRyaXZlcnMuCj4gPj4+ IAo+ID4+PiBGb3IgbXNtLCB3ZSBjaGVjayB3aGV0aGVyIHRoZSBEU0kgZW5jb2RlciBpcyBjb25u ZWN0ZWQgZGlyZWN0bHkgdG8gYQo+ID4+PiBwYW5lbCBvciBhbiBleHRlcm5hbCBicmlkZ2UuIElm IGl0J3MgY29ubmVjdGVkIHRvIGFuIGV4dGVybmFsIGJyaWRnZSwKPiA+Pj4gd2Ugc2tpcCB0aGUg Y3JlYXRpb24gb2YgdGhlIHN0dWIgY29ubmVjdG9yLCBhbmQgcmVseSBvbiB0aGUgZXh0ZXJuYWwK PiA+Pj4gYnJpZGdlIGRyaXZlciB0byBjcmVhdGUgdGhlIGNvbm5lY3RvcjoKPiA+Pj4gCj4gPj4+ IGh0dHA6Ly9seHIuZnJlZS1lbGVjdHJvbnMuY29tL3NvdXJjZS9kcml2ZXJzL2dwdS9kcm0vbXNt L2RzaS9kc2kuYyNMMjIKPiA+Pj4gNwo+ID4+PiAKPiA+Pj4gVGhlIG1zbSBzb2x1dGlvbiBpc24n dCB2ZXJ5IG5lYXQsIGJ1dCBpdCBhdm9pZHMgdGhlIG5lZWQgdG8gY3JlYXRlCj4gPj4+IGFub3Ro ZXIgYnJpZGdlIHRvIGdsdWUgdGhpbmdzIHRvZ2V0aGVyLgo+ID4+IAo+ID4+IFNpbmNlIEkgc3Vn Z2VzdGVkIHRoaXMsIHllcyBJIGxpa2UgaXQuIEFuZCBJIHRoaW5rIGp1c3QgdW5jb25kaXRpb25h bGx5Cj4gPj4gY3JlYXRpbmcgdGhlIHBhbmVsIGJyaWRnZSBpcyBwcm9iYWJseSBldmVuIHNpbXBs ZXIsIGFmdGVyIGFsbCBicmlkZ2VzCj4gPj4gYXJlIHN1cHBvc2VkIHRvIGJlIGNoYWluYWJsZS4g SSBndWVzcyB0aGVyZSdzIGFsd2F5cyBnb2luZyB0byBiZSBkcml2ZXJzCj4gPj4gd2hlcmUgd2Ug bmVlZCBzcGVjaWFsIGhhbmRsaW5nLCBidXQgSSdtIGtpbmRhIGhvcGluZyB0aGF0IGZvciBtb3N0 IGNhc2VzCj4gPj4gc2ltcGx5IHBsdWdnaW5nIGluIGEgcGFuZWwgYnJpZGdlIGlzIGFsbCB0aGF0 J3MgbmVlZCB0byBnbHVlIGRybV9wYW5lbAo+ID4+IHN1cHBvcnQgaW50byBhIGRyaXZlci4gVGhl IHNpbXBsZSBwaXBlIGhlbHBlcnMgZG8gc3VwcG9ydCBicmlkZ2VzLCBhbmQKPiA+PiBwYXJ0IG9m IHRoZSBnb2FsIHRoZXJlIHZlcnkgbXVjaCB3YXMgdG8gbWFrZSBpdCBlYXN5IHRvIGdsdWUgaW4g cGFuZWwKPiA+PiBkcml2ZXJzLgo+ID4gCj4gPiBBcyBJJ3ZlIGp1c3QgZXhwbGFpbmVkIGluIGFu b3RoZXIgcmVwbHksIEkgZG9uJ3Qgc2VlIHRoZSBwb2ludCBpbiBkb2luZwo+ID4gdGhpcyB3aGVu IHdlIGNhbiBpbnN0ZWFkIHJlZmFjdG9yIHRoZSBicmlkZ2UgYW5kIHBhbmVsIG9wZXJhdGlvbnMg dG8KPiA+IGV4cG9zZSBhIGNvbW1vbiBiYXNlIG9iamVjdCB0aGF0IHdpbGwgdGhlbiBiZSBlYXN5 IHRvIGhhbmRsZSBpbiBjb3JlCj4gPiBjb2RlLiBUaGlzIGlzbid0IGp1c3QgZm9yIHBhbmVscywg YXMgY29ubmVjdG9ycyBzaG91bGQgaGF2ZSBEVCBub2RlcywgaXQKPiA+IG1ha2VzIHNlbnNlIHRv IGluc3RhbnRpYXRlIGFuIG9iamVjdCBmb3IgdGhlbSB0aGF0IGNhbiBiZSBoYW5kbGVkIGJ5IHRo ZQo+ID4gRFJNIGNvcmUsIHdpdGhvdXQgaGF2aW5nIHRvIHB1c2ggY29ubmVjdG9yIGhhbmRsaW5n IGluIGFsbCBicmlkZ2UKPiA+IGRyaXZlcnMuCj4gCj4gSW1vIHlvdSdyZSBhaW1pbmcgdG9vIGhp Z2guIFdlIGhhdmUgMjEgYnJpZGdlIGRyaXZlcnMgYW5kIDExIHBhbmVsCj4gZHJpdmVycy4gQXNr aW5nIHNvbWVvbmUgdG8gcmVmYWN0b3IgdGhlbSBhbGwgKHBsdXMgYWxsIHRoZSBjYWxsZXJzIGFu ZAo+IGV2ZXJ5dGhpbmcpIG1lYW5zIGl0IHdvbid0IGhhcHBlbi4gQXQgbGVhc3QgSSBwZXJzb25h bGx5IHdpbGwgZGVmaW5pdGVseQo+IG5vdCBibG9jayBhIGNvbnRyaWJ1dGlvbiBvbiB0aGlzIGhh cHBlbmluZywgdGhhdCdzIGEgdG90YWxseSBvdXRzaXplZAo+IGRlbWFuZC4KCkkgdGhpbmsgeW91 J3JlIGFpbWluZyB0b28gbG93LiBXaGVuIHRoZSBhdG9taWMgdXBkYXRlIEFQSSB3YXMgaW50cm9k dWNlZCBJIApjb3VsZCBoYXZlIHRvbGQgeW91IHRoYXQgY29udmVydGluZyBhbGwgZHJpdmVycyB3 YXMgYW4gaW1wb3NzaWJsZSB0YXNrIDstKQoKSm9rZXMgYXNpZGUsIEkgYmVsaWV2ZSBpdCBtaWdo dCBiZSBwb3NzaWJsZSB0byBpbXBsZW1lbnQgc29tZXRoaW5nIHNpbXBsZS4gSSdtIApmbGV4aWJs ZSBhYm91dCB0aGUgbmFtaW5nLCBzbyBpbnN0ZWFkIG9mIGNyZWF0aW5nIGEgbmV3IGJhc2Ugc3Ry dWN0dXJlIGFuZCAKcmVmYWN0b3IgZHJtX2JyaWRnZSBhbmQgZHJtX3BhbmVsIHRvIGVtYmVkIGl0 LCB3ZSBjb3VsZCBhcyBhIGZpcnN0IHN0ZXAgdXNlIApkcm1fYnJpZGdlIGFzIHRoYXQgYmFzZSBz dHJ1Y3R1cmUuIFdlIHdvdWxkIG9ubHkgbmVlZCB0byBlbWJlZCBhIGRybV9icmlkZ2UgCmluc3Rh bmNlIGluIGRybV9wYW5lbCBhbmQgYWRkIGEgZmV3IGNvbm5lY3Rvci1yZWxhdGVkIG9wZXJhdGlv bnMgdG8gdGhlIGJyaWRnZSAKb3BzIHN0cnVjdHVyZS4gQXMgZXhpc3RpbmcgYnJpZGdlIGRyaXZl cnMgd291bGRuJ3QgbmVlZCB0byBwcm92aWRlIHRob3NlIG5ldyAKb3BzLCB0aGV5IHdvdWxkbid0 IG5lZWQgdG8gYmUgdG91Y2hlZC4KCj4gV2hhdCB3ZSBjb3VsZCBkbyBpbnN0ZWFkIGlzIHNsb3ds eSBtZXJnZSB0aGVzZSB0d28gd29ybGRzIHRvZ2V0aGVyLCBhbmQKPiB0aGlzIGhlcmUgaXMgZGVm aW5pdGVseSBhIHN0ZXAgaW50byB0aGF0IGRpcmVjdGlvbi4gTGV0J3MgcGxlYXNlIG5vdCB0aHJv dwo+IG91dCB1c2VmdWwgaW1wcm92ZW1lbnRzIGJ5IGluc2lzdGluZyB0aGF0IHdlIG9ubHkgbWVy Z2UgcGVyZmVjdCBjb2RlLiBXZQo+IGFscmVhZHkgZGlkIG1lcmdlIGJvdGggZHJtX3BhbmVsIGFu ZCBkcm1fYnJpZGdlIChwbHVzIGEgZmV3IG1vcmUgZWFybGllcgo+IGF0dGVtcHRzKSwgY2xlYXJs eSB3ZSdyZSBub3Qgb25seSBtZXJnaW5nIHBlcmZlY3QgY29kZSA6LSkKPiAKPiBPciB5b3UgZ28g YWhlYWQgYW5kIGRlbGl2ZXIgdGhhdCByZWZhY3RvcmluZywgdGhhdCdzIGFub3RoZXIgb3B0aW9u IG9mYwo+IC4uLgoKSXQncyBvbiBteSB0by1kbyBsaXN0IGZvciB0aGUgbmVhciBmdXR1cmUgYWN0 dWFsbHksIGluIG9yZGVyIHRvIGNvbnZlcnQgdGhlIApvbWFwZHJtLXNwZWNpZmljIGJyaWRnZSBh bmQgcGFuZWwgZHJpdmVycyBpbnRvIHN0YW5kYXJkIERSTSBkcml2ZXJzLiBJJ2QgbGlrZSAKdG8g Z2V0IGEgZ2VuZXJhbCBhZ3JlZW1lbnQgb24gdGhlIGRpcmVjdGlvbiBJJ2QgbGlrZSB0byB0YWtl IGJlZm9yZSBjb252ZXJ0aW5nIApldmVyeXRoaW5nIHRob3VnaCwgc28gSSdkIGFwcHJlY2lhdGUg eW91ciBmZWVkYmFjayBvbiB0aGUgdGhvdWdodHMgYWJvdmUuCgotLSAKUmVnYXJkcywKCkxhdXJl bnQgUGluY2hhcnQKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9y ZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZl bAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753492AbdECOnr (ORCPT ); Wed, 3 May 2017 10:43:47 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:49593 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753666AbdECOni (ORCPT ); Wed, 3 May 2017 10:43:38 -0400 From: Laurent Pinchart To: Daniel Vetter Cc: Archit Taneja , Eric Anholt , dri-devel@lists.freedesktop.org, Yannick Fertre , Thierry Reding , linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge. Date: Wed, 03 May 2017 17:44:53 +0300 Message-ID: <2463758.9HmFMyJbcQ@avalon> User-Agent: KMail/4.14.10 (Linux/4.9.16-gentoo; KDE/4.14.29; x86_64; ; ) In-Reply-To: <20170503142856.bmazihqoj6rgvbwq@phenom.ffwll.local> References: <20170427163601.7313-1-eric@anholt.net> <2556350.BYUa4rMt48@avalon> <20170503142856.bmazihqoj6rgvbwq@phenom.ffwll.local> 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 03 May 2017 16:28:56 Daniel Vetter wrote: > On Wed, May 03, 2017 at 12:36:06PM +0300, Laurent Pinchart wrote: > > On Wednesday 03 May 2017 11:32:17 Daniel Vetter wrote: > >> On Wed, May 03, 2017 at 02:53:00PM +0530, Archit Taneja wrote: > >>> +panel/bridge reviewers. > >>> > >>> This does make things much cleaner, but it seems a bit strange to > >>> create a drm_bridge when there isn't really a HW bridge in the display > >>> chain (i.e, when the DSI encoder is directly connected to a DSI panel). > >>> > >>> There are kms drivers that use drm_panel, but don't have simple stub > >>> connectors that wrap around a drm_panel. They have more complicated > >>> connector ops, and may call drm_panel_prepare() and related functions > >>> a bit differently. We won't be able to use drm_panel_bridge for those > >>> drivers. > >>> > >>> For msm, we check whether the DSI encoder is connected directly to a > >>> panel or an external bridge. If it's connected to an external bridge, > >>> we skip the creation of the stub connector, and rely on the external > >>> bridge driver to create the connector: > >>> > >>> http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L22 > >>> 7 > >>> > >>> The msm solution isn't very neat, but it avoids the need to create > >>> another bridge to glue things together. > >> > >> Since I suggested this, yes I like it. And I think just unconditionally > >> creating the panel bridge is probably even simpler, after all bridges > >> are supposed to be chainable. I guess there's always going to be drivers > >> where we need special handling, but I'm kinda hoping that for most cases > >> simply plugging in a panel bridge is all that's need to glue drm_panel > >> support into a driver. The simple pipe helpers do support bridges, and > >> part of the goal there very much was to make it easy to glue in panel > >> drivers. > > > > As I've just explained in another reply, I don't see the point in doing > > this when we can instead refactor the bridge and panel operations to > > expose a common base object that will then be easy to handle in core > > code. This isn't just for panels, as connectors should have DT nodes, it > > makes sense to instantiate an object for them that can be handled by the > > DRM core, without having to push connector handling in all bridge > > drivers. > > Imo you're aiming too high. We have 21 bridge drivers and 11 panel > drivers. Asking someone to refactor them all (plus all the callers and > everything) means it won't happen. At least I personally will definitely > not block a contribution on this happening, that's a totally outsized > demand. I think you're aiming too low. When the atomic update API was introduced I could have told you that converting all drivers was an impossible task ;-) Jokes aside, I believe it might be possible to implement something simple. I'm flexible about the naming, so instead of creating a new base structure and refactor drm_bridge and drm_panel to embed it, we could as a first step use drm_bridge as that base structure. We would only need to embed a drm_bridge instance in drm_panel and add a few connector-related operations to the bridge ops structure. As existing bridge drivers wouldn't need to provide those new ops, they wouldn't need to be touched. > What we could do instead is slowly merge these two worlds together, and > this here is definitely a step into that direction. Let's please not throw > out useful improvements by insisting that we only merge perfect code. We > already did merge both drm_panel and drm_bridge (plus a few more earlier > attempts), clearly we're not only merging perfect code :-) > > Or you go ahead and deliver that refactoring, that's another option ofc > ... It's on my to-do list for the near future actually, in order to convert the omapdrm-specific bridge and panel drivers into standard DRM drivers. I'd like to get a general agreement on the direction I'd like to take before converting everything though, so I'd appreciate your feedback on the thoughts above. -- Regards, Laurent Pinchart