From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from galahad.ideasonboard.com ([185.26.127.97]:36531 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030189AbcK3LFx (ORCPT ); Wed, 30 Nov 2016 06:05:53 -0500 From: Laurent Pinchart To: Archit Taneja Cc: Laurent Pinchart , dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, Boris Brezillon , Jingoo Han , Inki Dae , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Stefan Agner , Alison Wang , Xinliang Liu , Rongrong Zou , Xinwei Kong , Chen Feng , Philipp Zabel , CK Hu , Rob Clark , Benjamin Gaignard , Vincent Abriou , Maxime Ripard Subject: Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code Date: Wed, 30 Nov 2016 13:05:46 +0200 Message-ID: <2651171.INF8CSizt8@avalon> In-Reply-To: <3e8816d8-d461-d010-bff8-a38011e24a96@codeaurora.org> References: <1480410283-28698-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <7224643.24NSiAqgQC@avalon> <3e8816d8-d461-d010-bff8-a38011e24a96@codeaurora.org> 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 Archit, On Wednesday 30 Nov 2016 16:30:53 Archit Taneja wrote: > On 11/30/2016 03:53 PM, Laurent Pinchart wrote: > > On Wednesday 30 Nov 2016 10:35:02 Archit Taneja wrote: > >> On 11/29/2016 11:27 PM, Laurent Pinchart wrote: > >>> On Tuesday 29 Nov 2016 15:57:06 Archit Taneja wrote: > >>>> On 11/29/2016 02:34 PM, Laurent Pinchart wrote: > >>>>> Instead of linking encoders and bridges in every driver (and getting > >>>>> it wrong half of the time, as many drivers forget to set the > >>>>> drm_bridge encoder pointer), do so in core code. The > >>>>> drm_bridge_attach() function needs the encoder and optional previous > >>>>> bridge to perform that task, update all the callers. > >>>>> > >>>>> Signed-off-by: Laurent Pinchart > >>>>> > >>>>> --- [snip] > >>>> I think we could derive previous from the encoder itself. Something > >>>> like: > >>>> > >>>> previous = encoder->bridge; > >>>> while (previous && previous->next) > >>>> previous = previous->next; > >>> > >>> That's a very good point. It would however prevent us from catching > >>> drivers that attach bridges in the wrong order, which the !previous->dev > >>> currently allows us to do (and it should be turned into a WARN_ON as > >>> Daniel proposed). > >> > >> With the simpler API, I don't think we will ever hit the case of > >> !previous->dev. The previous bridge (if it exists) in the chain would > >> already have a dev attached to it. In other words, we would remove the > >> risk of the chance of the 'previous' bridge being unattached. > >> > >> I'm a bit unclear about what you mean about the order part. If a kms > >> driver > >> wants to create a chain: encoder->bridge1->bridge2, it should ideally do: > >> > >> drm_bridge_attach(encoder, bridge1, NULL); > >> drm_bridge_attach(encoder, bridge2, bridge1); > > > > Correct. > > > >> We can't do much if the kms driver does the opposite: > >> > >> drm_bridge_attach(encoder, bridge2, NULL); > >> drm_bridge_attach(encoder, bridge2, bridge1); > > > > That would certainly be a very stupid thing for a driver to do :-) The > > problem that we could catch with my current proposal is > > > > drm_bridge_attach(encoder, bridge2, bridge1); > > ... > > drm_bridge_attach(encoder, bridge1, NULL); > > > > which I expect to happen from time to time as the two bridge can be > > attached through separate code paths sometimes a bit difficult to trace. > > It's not a big deal though, you could convince me that the advantages of > > a simpler API exceed its drawbacks. > > Having no 'previous' argument would prevent the possibility of this > altogether, won't it? > > With no 'previous' arg in the API, the driver can only do: > > drm_bridge_attach(encoder, bridge1); > drm_bridge_attach(encoder, bridge2); > > or > > drm_bridge_attach(encoder, bridge2); > drm_bridge_attach(encoder, bridge1); Correct. > For the latter, we can't do much as discussed above. Except that with the currently proposed API the code would be drm_bridge_attach(encoder, bridge2, bridge1); drm_bridge_attach(encoder, bridge1, NULL); (correct case) or drm_bridge_attach(encoder, bridge2, bridge1); drm_bridge_attach(encoder, bridge1, NULL); (incorrect case) The second one could be caught by the drm_bridge_attach() function as bridge1- >dev will be NULL when attaching bridge2 in the incorrect case. -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code Date: Wed, 30 Nov 2016 13:05:46 +0200 Message-ID: <2651171.INF8CSizt8@avalon> References: <1480410283-28698-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <7224643.24NSiAqgQC@avalon> <3e8816d8-d461-d010-bff8-a38011e24a96@codeaurora.org> 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 0C66A6E273 for ; Wed, 30 Nov 2016 11:05:32 +0000 (UTC) In-Reply-To: <3e8816d8-d461-d010-bff8-a38011e24a96@codeaurora.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Archit Taneja Cc: Laurent Pinchart , Alison Wang , Jingoo Han , Seung-Woo Kim , Xinwei Kong , dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, Kyungmin Park , Xinliang Liu , Chen Feng , Rongrong Zou , Maxime Ripard , Vincent Abriou List-Id: dri-devel@lists.freedesktop.org SGkgQXJjaGl0LAoKT24gV2VkbmVzZGF5IDMwIE5vdiAyMDE2IDE2OjMwOjUzIEFyY2hpdCBUYW5l amEgd3JvdGU6Cj4gT24gMTEvMzAvMjAxNiAwMzo1MyBQTSwgTGF1cmVudCBQaW5jaGFydCB3cm90 ZToKPiA+IE9uIFdlZG5lc2RheSAzMCBOb3YgMjAxNiAxMDozNTowMiBBcmNoaXQgVGFuZWphIHdy b3RlOgo+ID4+IE9uIDExLzI5LzIwMTYgMTE6MjcgUE0sIExhdXJlbnQgUGluY2hhcnQgd3JvdGU6 Cj4gPj4+IE9uIFR1ZXNkYXkgMjkgTm92IDIwMTYgMTU6NTc6MDYgQXJjaGl0IFRhbmVqYSB3cm90 ZToKPiA+Pj4+IE9uIDExLzI5LzIwMTYgMDI6MzQgUE0sIExhdXJlbnQgUGluY2hhcnQgd3JvdGU6 Cj4gPj4+Pj4gSW5zdGVhZCBvZiBsaW5raW5nIGVuY29kZXJzIGFuZCBicmlkZ2VzIGluIGV2ZXJ5 IGRyaXZlciAoYW5kIGdldHRpbmcKPiA+Pj4+PiBpdCB3cm9uZyBoYWxmIG9mIHRoZSB0aW1lLCBh cyBtYW55IGRyaXZlcnMgZm9yZ2V0IHRvIHNldCB0aGUKPiA+Pj4+PiBkcm1fYnJpZGdlIGVuY29k ZXIgcG9pbnRlciksIGRvIHNvIGluIGNvcmUgY29kZS4gVGhlCj4gPj4+Pj4gZHJtX2JyaWRnZV9h dHRhY2goKSBmdW5jdGlvbiBuZWVkcyB0aGUgZW5jb2RlciBhbmQgb3B0aW9uYWwgcHJldmlvdXMK PiA+Pj4+PiBicmlkZ2UgdG8gcGVyZm9ybSB0aGF0IHRhc2ssIHVwZGF0ZSBhbGwgdGhlIGNhbGxl cnMuCj4gPj4+Pj4gCj4gPj4+Pj4gU2lnbmVkLW9mZi1ieTogTGF1cmVudCBQaW5jaGFydAo+ID4+ Pj4+IDxsYXVyZW50LnBpbmNoYXJ0K3JlbmVzYXNAaWRlYXNvbmJvYXJkLmNvbT4KPiA+Pj4+PiAt LS0KCltzbmlwXQoKPiA+Pj4+IEkgdGhpbmsgd2UgY291bGQgZGVyaXZlIHByZXZpb3VzIGZyb20g dGhlIGVuY29kZXIgaXRzZWxmLiBTb21ldGhpbmcKPiA+Pj4+IGxpa2U6Cj4gPj4+Pgo+ID4+Pj4g CXByZXZpb3VzID0gZW5jb2Rlci0+YnJpZGdlOwo+ID4+Pj4gCXdoaWxlIChwcmV2aW91cyAmJiBw cmV2aW91cy0+bmV4dCkKPiA+Pj4+IAkJcHJldmlvdXMgPSBwcmV2aW91cy0+bmV4dDsKPiA+Pj4g Cj4gPj4+IFRoYXQncyBhIHZlcnkgZ29vZCBwb2ludC4gSXQgd291bGQgaG93ZXZlciBwcmV2ZW50 IHVzIGZyb20gY2F0Y2hpbmcKPiA+Pj4gZHJpdmVycyB0aGF0IGF0dGFjaCBicmlkZ2VzIGluIHRo ZSB3cm9uZyBvcmRlciwgd2hpY2ggdGhlICFwcmV2aW91cy0+ZGV2Cj4gPj4+IGN1cnJlbnRseSBh bGxvd3MgdXMgdG8gZG8gKGFuZCBpdCBzaG91bGQgYmUgdHVybmVkIGludG8gYSBXQVJOX09OIGFz Cj4gPj4+IERhbmllbCBwcm9wb3NlZCkuCj4gPj4gCj4gPj4gV2l0aCB0aGUgc2ltcGxlciBBUEks IEkgZG9uJ3QgdGhpbmsgd2Ugd2lsbCBldmVyIGhpdCB0aGUgY2FzZSBvZgo+ID4+ICFwcmV2aW91 cy0+ZGV2LiBUaGUgcHJldmlvdXMgYnJpZGdlIChpZiBpdCBleGlzdHMpIGluIHRoZSBjaGFpbiB3 b3VsZAo+ID4+IGFscmVhZHkgaGF2ZSBhIGRldiBhdHRhY2hlZCB0byBpdC4gSW4gb3RoZXIgd29y ZHMsIHdlIHdvdWxkIHJlbW92ZSB0aGUKPiA+PiByaXNrIG9mIHRoZSBjaGFuY2Ugb2YgdGhlICdw cmV2aW91cycgYnJpZGdlIGJlaW5nIHVuYXR0YWNoZWQuCj4gPj4gCj4gPj4gSSdtIGEgYml0IHVu Y2xlYXIgYWJvdXQgd2hhdCB5b3UgbWVhbiBhYm91dCB0aGUgb3JkZXIgcGFydC4gSWYgYSBrbXMK PiA+PiBkcml2ZXIKPiA+PiB3YW50cyB0byBjcmVhdGUgYSBjaGFpbjogZW5jb2Rlci0+YnJpZGdl MS0+YnJpZGdlMiwgaXQgc2hvdWxkIGlkZWFsbHkgZG86Cj4gPj4gCj4gPj4gZHJtX2JyaWRnZV9h dHRhY2goZW5jb2RlciwgYnJpZGdlMSwgTlVMTCk7Cj4gPj4gZHJtX2JyaWRnZV9hdHRhY2goZW5j b2RlciwgYnJpZGdlMiwgYnJpZGdlMSk7Cj4gPiAKPiA+IENvcnJlY3QuCj4gPiAKPiA+PiBXZSBj YW4ndCBkbyBtdWNoIGlmIHRoZSBrbXMgZHJpdmVyIGRvZXMgdGhlIG9wcG9zaXRlOgo+ID4+IAo+ ID4+IGRybV9icmlkZ2VfYXR0YWNoKGVuY29kZXIsIGJyaWRnZTIsIE5VTEwpOwo+ID4+IGRybV9i cmlkZ2VfYXR0YWNoKGVuY29kZXIsIGJyaWRnZTIsIGJyaWRnZTEpOwo+ID4gCj4gPiBUaGF0IHdv dWxkIGNlcnRhaW5seSBiZSBhIHZlcnkgc3R1cGlkIHRoaW5nIGZvciBhIGRyaXZlciB0byBkbyA6 LSkgVGhlCj4gPiBwcm9ibGVtIHRoYXQgd2UgY291bGQgY2F0Y2ggd2l0aCBteSBjdXJyZW50IHBy b3Bvc2FsIGlzCj4gPiAKPiA+IGRybV9icmlkZ2VfYXR0YWNoKGVuY29kZXIsIGJyaWRnZTIsIGJy aWRnZTEpOwo+ID4gLi4uCj4gPiBkcm1fYnJpZGdlX2F0dGFjaChlbmNvZGVyLCBicmlkZ2UxLCBO VUxMKTsKPiA+IAo+ID4gd2hpY2ggSSBleHBlY3QgdG8gaGFwcGVuIGZyb20gdGltZSB0byB0aW1l IGFzIHRoZSB0d28gYnJpZGdlIGNhbiBiZQo+ID4gYXR0YWNoZWQgdGhyb3VnaCBzZXBhcmF0ZSBj b2RlIHBhdGhzIHNvbWV0aW1lcyBhIGJpdCBkaWZmaWN1bHQgdG8gdHJhY2UuCj4gPiBJdCdzIG5v dCBhIGJpZyBkZWFsIHRob3VnaCwgeW91IGNvdWxkIGNvbnZpbmNlIG1lIHRoYXQgdGhlIGFkdmFu dGFnZXMgb2YKPiA+IGEgc2ltcGxlciBBUEkgZXhjZWVkIGl0cyBkcmF3YmFja3MuCj4gCj4gSGF2 aW5nIG5vICdwcmV2aW91cycgYXJndW1lbnQgd291bGQgcHJldmVudCB0aGUgcG9zc2liaWxpdHkg b2YgdGhpcwo+IGFsdG9nZXRoZXIsIHdvbid0IGl0Pwo+IAo+IFdpdGggbm8gJ3ByZXZpb3VzJyBh cmcgaW4gdGhlIEFQSSwgdGhlIGRyaXZlciBjYW4gb25seSBkbzoKPiAKPiBkcm1fYnJpZGdlX2F0 dGFjaChlbmNvZGVyLCBicmlkZ2UxKTsKPiBkcm1fYnJpZGdlX2F0dGFjaChlbmNvZGVyLCBicmlk Z2UyKTsKPiAKPiBvcgo+IAo+IGRybV9icmlkZ2VfYXR0YWNoKGVuY29kZXIsIGJyaWRnZTIpOwo+ IGRybV9icmlkZ2VfYXR0YWNoKGVuY29kZXIsIGJyaWRnZTEpOwoKQ29ycmVjdC4KCj4gRm9yIHRo ZSBsYXR0ZXIsIHdlIGNhbid0IGRvIG11Y2ggYXMgZGlzY3Vzc2VkIGFib3ZlLgoKRXhjZXB0IHRo YXQgd2l0aCB0aGUgY3VycmVudGx5IHByb3Bvc2VkIEFQSSB0aGUgY29kZSB3b3VsZCBiZQoKZHJt X2JyaWRnZV9hdHRhY2goZW5jb2RlciwgYnJpZGdlMiwgYnJpZGdlMSk7CmRybV9icmlkZ2VfYXR0 YWNoKGVuY29kZXIsIGJyaWRnZTEsIE5VTEwpOwoKKGNvcnJlY3QgY2FzZSkKCm9yCgpkcm1fYnJp ZGdlX2F0dGFjaChlbmNvZGVyLCBicmlkZ2UyLCBicmlkZ2UxKTsKZHJtX2JyaWRnZV9hdHRhY2go ZW5jb2RlciwgYnJpZGdlMSwgTlVMTCk7CgooaW5jb3JyZWN0IGNhc2UpCgpUaGUgc2Vjb25kIG9u ZSBjb3VsZCBiZSBjYXVnaHQgYnkgdGhlIGRybV9icmlkZ2VfYXR0YWNoKCkgZnVuY3Rpb24gYXMg YnJpZGdlMS0KPmRldiB3aWxsIGJlIE5VTEwgd2hlbiBhdHRhY2hpbmcgYnJpZGdlMiBpbiB0aGUg aW5jb3JyZWN0IGNhc2UuCgotLSAKUmVnYXJkcywKCkxhdXJlbnQgUGluY2hhcnQKCl9fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5n IGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVk ZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo=