From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.codeaurora.org ([198.145.29.96]:47856 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750922AbcK3N2u (ORCPT ); Wed, 30 Nov 2016 08:28:50 -0500 Subject: Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code To: Laurent Pinchart References: <1480410283-28698-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <7224643.24NSiAqgQC@avalon> <3e8816d8-d461-d010-bff8-a38011e24a96@codeaurora.org> <2651171.INF8CSizt8@avalon> 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 From: Archit Taneja Message-ID: <265154a1-2f23-2544-4203-db97a909bb7b@codeaurora.org> Date: Wed, 30 Nov 2016 18:57:30 +0530 MIME-Version: 1.0 In-Reply-To: <2651171.INF8CSizt8@avalon> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: On 11/30/2016 4:35 PM, Laurent Pinchart wrote: > 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. Okay, I got it now. As you said, it does make sense for cases like analogix_dp, where one attach is in the bridge driver, and the other is in the kms driver. It makes things more legible too. Passing 'previous' as NULL makes it clear in the code that we're attaching first bridge in the chain. Let's stick to your proposal. One additional thing we could do is to compare the 'previous' arg passed by the API with the last bridge in the chain, and return an error if they aren't the same, just as an additional safety measure. Archit > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project From mboxrd@z Thu Jan 1 00:00:00 1970 From: Archit Taneja Subject: Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code Date: Wed, 30 Nov 2016 18:57:30 +0530 Message-ID: <265154a1-2f23-2544-4203-db97a909bb7b@codeaurora.org> References: <1480410283-28698-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <7224643.24NSiAqgQC@avalon> <3e8816d8-d461-d010-bff8-a38011e24a96@codeaurora.org> <2651171.INF8CSizt8@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from smtp.codeaurora.org (smtp.codeaurora.org [198.145.29.96]) by gabe.freedesktop.org (Postfix) with ESMTPS id 75FD96E331 for ; Wed, 30 Nov 2016 13:28:49 +0000 (UTC) In-Reply-To: <2651171.INF8CSizt8@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart 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 CgpPbiAxMS8zMC8yMDE2IDQ6MzUgUE0sIExhdXJlbnQgUGluY2hhcnQgd3JvdGU6Cj4gSGkgQXJj aGl0LAo+Cj4gT24gV2VkbmVzZGF5IDMwIE5vdiAyMDE2IDE2OjMwOjUzIEFyY2hpdCBUYW5lamEg d3JvdGU6Cj4+IE9uIDExLzMwLzIwMTYgMDM6NTMgUE0sIExhdXJlbnQgUGluY2hhcnQgd3JvdGU6 Cj4+PiBPbiBXZWRuZXNkYXkgMzAgTm92IDIwMTYgMTA6MzU6MDIgQXJjaGl0IFRhbmVqYSB3cm90 ZToKPj4+PiBPbiAxMS8yOS8yMDE2IDExOjI3IFBNLCBMYXVyZW50IFBpbmNoYXJ0IHdyb3RlOgo+ Pj4+PiBPbiBUdWVzZGF5IDI5IE5vdiAyMDE2IDE1OjU3OjA2IEFyY2hpdCBUYW5lamEgd3JvdGU6 Cj4+Pj4+PiBPbiAxMS8yOS8yMDE2IDAyOjM0IFBNLCBMYXVyZW50IFBpbmNoYXJ0IHdyb3RlOgo+ Pj4+Pj4+IEluc3RlYWQgb2YgbGlua2luZyBlbmNvZGVycyBhbmQgYnJpZGdlcyBpbiBldmVyeSBk cml2ZXIgKGFuZCBnZXR0aW5nCj4+Pj4+Pj4gaXQgd3JvbmcgaGFsZiBvZiB0aGUgdGltZSwgYXMg bWFueSBkcml2ZXJzIGZvcmdldCB0byBzZXQgdGhlCj4+Pj4+Pj4gZHJtX2JyaWRnZSBlbmNvZGVy IHBvaW50ZXIpLCBkbyBzbyBpbiBjb3JlIGNvZGUuIFRoZQo+Pj4+Pj4+IGRybV9icmlkZ2VfYXR0 YWNoKCkgZnVuY3Rpb24gbmVlZHMgdGhlIGVuY29kZXIgYW5kIG9wdGlvbmFsIHByZXZpb3VzCj4+ Pj4+Pj4gYnJpZGdlIHRvIHBlcmZvcm0gdGhhdCB0YXNrLCB1cGRhdGUgYWxsIHRoZSBjYWxsZXJz Lgo+Pj4+Pj4+Cj4+Pj4+Pj4gU2lnbmVkLW9mZi1ieTogTGF1cmVudCBQaW5jaGFydAo+Pj4+Pj4+ IDxsYXVyZW50LnBpbmNoYXJ0K3JlbmVzYXNAaWRlYXNvbmJvYXJkLmNvbT4KPj4+Pj4+PiAtLS0K Pgo+IFtzbmlwXQo+Cj4+Pj4+PiBJIHRoaW5rIHdlIGNvdWxkIGRlcml2ZSBwcmV2aW91cyBmcm9t IHRoZSBlbmNvZGVyIGl0c2VsZi4gU29tZXRoaW5nCj4+Pj4+PiBsaWtlOgo+Pj4+Pj4KPj4+Pj4+ IAlwcmV2aW91cyA9IGVuY29kZXItPmJyaWRnZTsKPj4+Pj4+IAl3aGlsZSAocHJldmlvdXMgJiYg cHJldmlvdXMtPm5leHQpCj4+Pj4+PiAJCXByZXZpb3VzID0gcHJldmlvdXMtPm5leHQ7Cj4+Pj4+ Cj4+Pj4+IFRoYXQncyBhIHZlcnkgZ29vZCBwb2ludC4gSXQgd291bGQgaG93ZXZlciBwcmV2ZW50 IHVzIGZyb20gY2F0Y2hpbmcKPj4+Pj4gZHJpdmVycyB0aGF0IGF0dGFjaCBicmlkZ2VzIGluIHRo ZSB3cm9uZyBvcmRlciwgd2hpY2ggdGhlICFwcmV2aW91cy0+ZGV2Cj4+Pj4+IGN1cnJlbnRseSBh bGxvd3MgdXMgdG8gZG8gKGFuZCBpdCBzaG91bGQgYmUgdHVybmVkIGludG8gYSBXQVJOX09OIGFz Cj4+Pj4+IERhbmllbCBwcm9wb3NlZCkuCj4+Pj4KPj4+PiBXaXRoIHRoZSBzaW1wbGVyIEFQSSwg SSBkb24ndCB0aGluayB3ZSB3aWxsIGV2ZXIgaGl0IHRoZSBjYXNlIG9mCj4+Pj4gIXByZXZpb3Vz LT5kZXYuIFRoZSBwcmV2aW91cyBicmlkZ2UgKGlmIGl0IGV4aXN0cykgaW4gdGhlIGNoYWluIHdv dWxkCj4+Pj4gYWxyZWFkeSBoYXZlIGEgZGV2IGF0dGFjaGVkIHRvIGl0LiBJbiBvdGhlciB3b3Jk cywgd2Ugd291bGQgcmVtb3ZlIHRoZQo+Pj4+IHJpc2sgb2YgdGhlIGNoYW5jZSBvZiB0aGUgJ3By ZXZpb3VzJyBicmlkZ2UgYmVpbmcgdW5hdHRhY2hlZC4KPj4+Pgo+Pj4+IEknbSBhIGJpdCB1bmNs ZWFyIGFib3V0IHdoYXQgeW91IG1lYW4gYWJvdXQgdGhlIG9yZGVyIHBhcnQuIElmIGEga21zCj4+ Pj4gZHJpdmVyCj4+Pj4gd2FudHMgdG8gY3JlYXRlIGEgY2hhaW46IGVuY29kZXItPmJyaWRnZTEt PmJyaWRnZTIsIGl0IHNob3VsZCBpZGVhbGx5IGRvOgo+Pj4+Cj4+Pj4gZHJtX2JyaWRnZV9hdHRh Y2goZW5jb2RlciwgYnJpZGdlMSwgTlVMTCk7Cj4+Pj4gZHJtX2JyaWRnZV9hdHRhY2goZW5jb2Rl ciwgYnJpZGdlMiwgYnJpZGdlMSk7Cj4+Pgo+Pj4gQ29ycmVjdC4KPj4+Cj4+Pj4gV2UgY2FuJ3Qg ZG8gbXVjaCBpZiB0aGUga21zIGRyaXZlciBkb2VzIHRoZSBvcHBvc2l0ZToKPj4+Pgo+Pj4+IGRy bV9icmlkZ2VfYXR0YWNoKGVuY29kZXIsIGJyaWRnZTIsIE5VTEwpOwo+Pj4+IGRybV9icmlkZ2Vf YXR0YWNoKGVuY29kZXIsIGJyaWRnZTIsIGJyaWRnZTEpOwo+Pj4KPj4+IFRoYXQgd291bGQgY2Vy dGFpbmx5IGJlIGEgdmVyeSBzdHVwaWQgdGhpbmcgZm9yIGEgZHJpdmVyIHRvIGRvIDotKSBUaGUK Pj4+IHByb2JsZW0gdGhhdCB3ZSBjb3VsZCBjYXRjaCB3aXRoIG15IGN1cnJlbnQgcHJvcG9zYWwg aXMKPj4+Cj4+PiBkcm1fYnJpZGdlX2F0dGFjaChlbmNvZGVyLCBicmlkZ2UyLCBicmlkZ2UxKTsK Pj4+IC4uLgo+Pj4gZHJtX2JyaWRnZV9hdHRhY2goZW5jb2RlciwgYnJpZGdlMSwgTlVMTCk7Cj4+ Pgo+Pj4gd2hpY2ggSSBleHBlY3QgdG8gaGFwcGVuIGZyb20gdGltZSB0byB0aW1lIGFzIHRoZSB0 d28gYnJpZGdlIGNhbiBiZQo+Pj4gYXR0YWNoZWQgdGhyb3VnaCBzZXBhcmF0ZSBjb2RlIHBhdGhz IHNvbWV0aW1lcyBhIGJpdCBkaWZmaWN1bHQgdG8gdHJhY2UuCj4+PiBJdCdzIG5vdCBhIGJpZyBk ZWFsIHRob3VnaCwgeW91IGNvdWxkIGNvbnZpbmNlIG1lIHRoYXQgdGhlIGFkdmFudGFnZXMgb2YK Pj4+IGEgc2ltcGxlciBBUEkgZXhjZWVkIGl0cyBkcmF3YmFja3MuCj4+Cj4+IEhhdmluZyBubyAn cHJldmlvdXMnIGFyZ3VtZW50IHdvdWxkIHByZXZlbnQgdGhlIHBvc3NpYmlsaXR5IG9mIHRoaXMK Pj4gYWx0b2dldGhlciwgd29uJ3QgaXQ/Cj4+Cj4+IFdpdGggbm8gJ3ByZXZpb3VzJyBhcmcgaW4g dGhlIEFQSSwgdGhlIGRyaXZlciBjYW4gb25seSBkbzoKPj4KPj4gZHJtX2JyaWRnZV9hdHRhY2go ZW5jb2RlciwgYnJpZGdlMSk7Cj4+IGRybV9icmlkZ2VfYXR0YWNoKGVuY29kZXIsIGJyaWRnZTIp Owo+Pgo+PiBvcgo+Pgo+PiBkcm1fYnJpZGdlX2F0dGFjaChlbmNvZGVyLCBicmlkZ2UyKTsKPj4g ZHJtX2JyaWRnZV9hdHRhY2goZW5jb2RlciwgYnJpZGdlMSk7Cj4KPiBDb3JyZWN0Lgo+Cj4+IEZv ciB0aGUgbGF0dGVyLCB3ZSBjYW4ndCBkbyBtdWNoIGFzIGRpc2N1c3NlZCBhYm92ZS4KPgo+IEV4 Y2VwdCB0aGF0IHdpdGggdGhlIGN1cnJlbnRseSBwcm9wb3NlZCBBUEkgdGhlIGNvZGUgd291bGQg YmUKPgo+IGRybV9icmlkZ2VfYXR0YWNoKGVuY29kZXIsIGJyaWRnZTIsIGJyaWRnZTEpOwo+IGRy bV9icmlkZ2VfYXR0YWNoKGVuY29kZXIsIGJyaWRnZTEsIE5VTEwpOwo+Cj4gKGNvcnJlY3QgY2Fz ZSkKPgo+IG9yCj4KPiBkcm1fYnJpZGdlX2F0dGFjaChlbmNvZGVyLCBicmlkZ2UyLCBicmlkZ2Ux KTsKPiBkcm1fYnJpZGdlX2F0dGFjaChlbmNvZGVyLCBicmlkZ2UxLCBOVUxMKTsKPgo+IChpbmNv cnJlY3QgY2FzZSkKPgo+IFRoZSBzZWNvbmQgb25lIGNvdWxkIGJlIGNhdWdodCBieSB0aGUgZHJt X2JyaWRnZV9hdHRhY2goKSBmdW5jdGlvbiBhcyBicmlkZ2UxLQo+PiBkZXYgd2lsbCBiZSBOVUxM IHdoZW4gYXR0YWNoaW5nIGJyaWRnZTIgaW4gdGhlIGluY29ycmVjdCBjYXNlLgoKT2theSwgSSBn b3QgaXQgbm93LgoKQXMgeW91IHNhaWQsIGl0IGRvZXMgbWFrZSBzZW5zZSBmb3IgY2FzZXMgbGlr ZSBhbmFsb2dpeF9kcCwgd2hlcmUgb25lCmF0dGFjaCBpcyBpbiB0aGUgYnJpZGdlIGRyaXZlciwg YW5kIHRoZSBvdGhlciBpcyBpbiB0aGUga21zIGRyaXZlci4KSXQgbWFrZXMgdGhpbmdzIG1vcmUg bGVnaWJsZSB0b28uIFBhc3NpbmcgJ3ByZXZpb3VzJyBhcyBOVUxMIG1ha2VzIGl0CmNsZWFyIGlu IHRoZSBjb2RlIHRoYXQgd2UncmUgYXR0YWNoaW5nIGZpcnN0IGJyaWRnZSBpbiB0aGUgY2hhaW4u CkxldCdzIHN0aWNrIHRvIHlvdXIgcHJvcG9zYWwuCgpPbmUgYWRkaXRpb25hbCB0aGluZyB3ZSBj b3VsZCBkbyBpcyB0byBjb21wYXJlIHRoZSAncHJldmlvdXMnIGFyZwpwYXNzZWQgYnkgdGhlIEFQ SSB3aXRoIHRoZSBsYXN0IGJyaWRnZSBpbiB0aGUgY2hhaW4sIGFuZCByZXR1cm4KYW4gZXJyb3Ig aWYgdGhleSBhcmVuJ3QgdGhlIHNhbWUsIGp1c3QgYXMgYW4gYWRkaXRpb25hbCBzYWZldHkKbWVh c3VyZS4KCkFyY2hpdAoKPgoKLS0gClF1YWxjb21tIElubm92YXRpb24gQ2VudGVyLCBJbmMuIGlz IGEgbWVtYmVyIG9mIENvZGUgQXVyb3JhIEZvcnVtLAphIExpbnV4IEZvdW5kYXRpb24gQ29sbGFi b3JhdGl2ZSBQcm9qZWN0Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9w Lm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1k ZXZlbAo=