From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer Date: Mon, 14 May 2018 18:28:28 +0200 Message-ID: <20180514162828.GE28661@phenom.ffwll.local> References: <20180504135212.26977-1-peda@axentia.se> <20180504135212.26977-27-peda@axentia.se> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline 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: Peter Rosin Cc: Martyn Welch , David Airlie , dri-devel@lists.freedesktop.org, Laurent Pinchart , linux-samsung-soc@vger.kernel.org, Kyungmin Park , Krzysztof Kozlowski , linux-rockchip@lists.infradead.org, Kukjin Kim , Peter Senna Tschudin , Martin Donnelly , linux-arm-msm@vger.kernel.org, Jyri Sarha , Matthias Brugger , Vincent Abriou , linux-arm-kernel@lists.infradead.org, Seung-Woo Kim , linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-mediatek@lists.infradead.org, freedreno@lists.freedesktop.org List-Id: linux-arm-msm@vger.kernel.org T24gRnJpLCBNYXkgMTEsIDIwMTggYXQgMDk6Mzc6NDdBTSArMDIwMCwgUGV0ZXIgUm9zaW4gd3Jv dGU6Cj4gT24gMjAxOC0wNS0xMCAxMDoxMCwgQW5kcnplaiBIYWpkYSB3cm90ZToKPiA+IE9uIDA0 LjA1LjIwMTggMTU6NTIsIFBldGVyIFJvc2luIHdyb3RlOgo+ID4+IElmIHRoZSBicmlkZ2Ugc3Vw cGxpZXIgaXMgdW5ib3VuZCwgdGhpcyB3aWxsIGJyaW5nIHRoZSBicmlkZ2UgY29uc3VtZXIKPiA+ PiBkb3duIGFsb25nIHdpdGggdGhlIGJyaWRnZS4gVGh1cywgdGhlcmUgd2lsbCBubyBsb25nZXIg bGluZ2VyIGFueQo+ID4+IGRhbmdsaW5nIHBvaW50ZXJzIGZyb20gdGhlIGJyaWRnZSBjb25zdW1l ciAodGhlIGRybV9kZXZpY2UpIHRvIHNvbWUKPiA+PiBub24tZXhpc3RlbnQgYnJpZGdlIHN1cHBs aWVyLgo+ID4+Cj4gPj4gU2lnbmVkLW9mZi1ieTogUGV0ZXIgUm9zaW4gPHBlZGFAYXhlbnRpYS5z ZT4KPiA+PiAtLS0KPiA+PiAgZHJpdmVycy9ncHUvZHJtL2RybV9icmlkZ2UuYyB8IDE4ICsrKysr KysrKysrKysrKysrKwo+ID4+ICBpbmNsdWRlL2RybS9kcm1fYnJpZGdlLmggICAgIHwgIDIgKysK PiA+PiAgMiBmaWxlcyBjaGFuZ2VkLCAyMCBpbnNlcnRpb25zKCspCj4gPj4KPiA+PiBkaWZmIC0t Z2l0IGEvZHJpdmVycy9ncHUvZHJtL2RybV9icmlkZ2UuYyBiL2RyaXZlcnMvZ3B1L2RybS9kcm1f YnJpZGdlLmMKPiA+PiBpbmRleCA3OGQxODZiNjgzMWIuLjAyNTlmMGEzZmYyNyAxMDA2NDQKPiA+ PiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vZHJtX2JyaWRnZS5jCj4gPj4gKysrIGIvZHJpdmVycy9n cHUvZHJtL2RybV9icmlkZ2UuYwo+ID4+IEBAIC0yNiw2ICsyNiw3IEBACj4gPj4gICNpbmNsdWRl IDxsaW51eC9tdXRleC5oPgo+ID4+ICAKPiA+PiAgI2luY2x1ZGUgPGRybS9kcm1fYnJpZGdlLmg+ Cj4gPj4gKyNpbmNsdWRlIDxkcm0vZHJtX2RldmljZS5oPgo+ID4+ICAjaW5jbHVkZSA8ZHJtL2Ry bV9lbmNvZGVyLmg+Cj4gPj4gIAo+ID4+ICAjaW5jbHVkZSAiZHJtX2NydGNfaW50ZXJuYWwuaCIK PiA+PiBAQCAtMTI3LDEyICsxMjgsMjUgQEAgaW50IGRybV9icmlkZ2VfYXR0YWNoKHN0cnVjdCBk cm1fZW5jb2RlciAqZW5jb2Rlciwgc3RydWN0IGRybV9icmlkZ2UgKmJyaWRnZSwKPiA+PiAgCWlm IChicmlkZ2UtPmRldikKPiA+PiAgCQlyZXR1cm4gLUVCVVNZOwo+ID4+ICAKPiA+PiArCWlmIChl bmNvZGVyLT5kZXYtPmRldiAhPSBicmlkZ2UtPm9kZXYpIHsKPiA+IAo+ID4gSSB3b25kZXIgd2h5 IGRldmljZV9saW5rX2FkZCBkb2VzIG5vdCBoYW5kbGUgdGhpcyBjYXNlIChzZWxmIGRlcGVuZGVu Y3kpCj4gPiBzaWxlbnRseSBhcyBub29wLCBhcyBpdCBzZWVtcyB0byBiZSBhIGNvcnJlY3QgYmVo YXZpb3IuCj4gCj4gSXQncyBraW5kLW9mIGEgc2lsbHkgY29ybmVyLWNhc2UgdGhvdWdoLCBzbyBw ZXJmZWN0bHkgdW5kZXJzdGFuZGFibGUKPiB0aGF0IGl0IGlzbid0IGhhbmRsZWQuCj4gCj4gPj4g KwkJYnJpZGdlLT5saW5rID0gZGV2aWNlX2xpbmtfYWRkKGVuY29kZXItPmRldi0+ZGV2LAo+ID4+ ICsJCQkJCSAgICAgICBicmlkZ2UtPm9kZXYsIDApOwo+ID4+ICsJCWlmICghYnJpZGdlLT5saW5r KSB7Cj4gPj4gKwkJCWRldl9lcnIoYnJpZGdlLT5vZGV2LCAiZmFpbGVkIHRvIGxpbmsgYnJpZGdl IHRvICVzXG4iLAo+ID4+ICsJCQkJZGV2X25hbWUoZW5jb2Rlci0+ZGV2LT5kZXYpKTsKPiA+PiAr CQkJcmV0dXJuIC1FSU5WQUw7Cj4gPj4gKwkJfQo+ID4+ICsJfQo+ID4+ICsKPiA+PiAgCWJyaWRn ZS0+ZGV2ID0gZW5jb2Rlci0+ZGV2Owo+ID4+ICAJYnJpZGdlLT5lbmNvZGVyID0gZW5jb2RlcjsK PiA+PiAgCj4gPj4gIAlpZiAoYnJpZGdlLT5mdW5jcy0+YXR0YWNoKSB7Cj4gPj4gIAkJcmV0ID0g YnJpZGdlLT5mdW5jcy0+YXR0YWNoKGJyaWRnZSk7Cj4gPj4gIAkJaWYgKHJldCA8IDApIHsKPiA+ PiArCQkJaWYgKGJyaWRnZS0+bGluaykKPiA+PiArCQkJCWRldmljZV9saW5rX2RlbChicmlkZ2Ut PmxpbmspOwo+ID4+ICsJCQlicmlkZ2UtPmxpbmsgPSBOVUxMOwo+ID4+ICAJCQlicmlkZ2UtPmRl diA9IE5VTEw7Cj4gPj4gIAkJCWJyaWRnZS0+ZW5jb2RlciA9IE5VTEw7Cj4gPj4gIAkJCXJldHVy biByZXQ7Cj4gPj4gQEAgLTE1OSw2ICsxNzMsMTAgQEAgdm9pZCBkcm1fYnJpZGdlX2RldGFjaChz dHJ1Y3QgZHJtX2JyaWRnZSAqYnJpZGdlKQo+ID4+ICAJaWYgKGJyaWRnZS0+ZnVuY3MtPmRldGFj aCkKPiA+PiAgCQlicmlkZ2UtPmZ1bmNzLT5kZXRhY2goYnJpZGdlKTsKPiA+PiAgCj4gPj4gKwlp ZiAoYnJpZGdlLT5saW5rKQo+ID4+ICsJCWRldmljZV9saW5rX2RlbChicmlkZ2UtPmxpbmspOwo+ ID4+ICsJYnJpZGdlLT5saW5rID0gTlVMTDsKPiA+PiArCj4gPj4gIAlicmlkZ2UtPmRldiA9IE5V TEw7Cj4gPj4gIH0KPiA+PiAgCj4gPj4gZGlmZiAtLWdpdCBhL2luY2x1ZGUvZHJtL2RybV9icmlk Z2UuaCBiL2luY2x1ZGUvZHJtL2RybV9icmlkZ2UuaAo+ID4+IGluZGV4IGI2NTZlNTA1ZDExZS4u ODA0MTg5YzYzYTRjIDEwMDY0NAo+ID4+IC0tLSBhL2luY2x1ZGUvZHJtL2RybV9icmlkZ2UuaAo+ ID4+ICsrKyBiL2luY2x1ZGUvZHJtL2RybV9icmlkZ2UuaAo+ID4+IEBAIC0yNjEsNiArMjYxLDcg QEAgc3RydWN0IGRybV9icmlkZ2VfdGltaW5ncyB7Cj4gPj4gICAqIEBsaXN0OiB0byBrZWVwIHRy YWNrIG9mIGFsbCBhZGRlZCBicmlkZ2VzCj4gPj4gICAqIEB0aW1pbmdzOiB0aGUgdGltaW5nIHNw ZWNpZmljYXRpb24gZm9yIHRoZSBicmlkZ2UsIGlmIGFueSAobWF5Cj4gPj4gICAqIGJlIE5VTEwp Cj4gPj4gKyAqIEBsaW5rOiBkcm0gY29uc3VtZXIgPC0+IGJyaWRnZSBzdXBwbGllcgo+ID4gCj4g PiBOaXRwaWNrOiAiPC0+IiBzdWdnZXN0cyBzeW1tZXRyeSwgbWF5YmUgImRldmljZSBsaW5rIGZy b20gZHJtIGNvbnN1bWVyCj4gPiB0byB0aGUgYnJpZGdlIiB3b3VsZCBiZSBiZXR0ZXIuCj4gCj4g SSBtZWFudCAiPC0+IiB0byBpbmRpY2F0ZSB0aGF0IHRoZSBsaW5rIGlzIGJpZGlyZWN0aW9uYWws IG5vdCB0aGF0IHRoZQo+IHJlbGF0aW9uc2hpcCBpcyBpbiBhbnkgd2F5IHN5bW1ldHJpYy4gSSB3 YXNuJ3QgYXdhcmUgb2YgYW55IGltcGxpY2F0aW9uCj4gb2YgYSBzeW1tZXRyaWMgcmVsYXRpb25z aGlwIHdoZW4gdXNpbmcgIjwtPiIsIGRvIHlvdSBoYXZlIGEgcmVmZXJlbmNlPwo+IEJ1dCBJIGd1 ZXNzIHRoZSBkaWZmZXJlbnQgYXJyb3cgbm90YXRpb25zIGluIG1hdGggYXJlIHNvbWV3aGF0IG92 ZXJsb2FkZWQKPiBhbmQgdGhhdCBzb21lb25lIGF0IHNvbWUgcG9pbnQgbXVzdCBoYXZlIHVzZWQg IjwtPiIgdG8gaW5kaWNhdGUgYQo+IHN5bW1ldHJpYyByZWxhdGlvbnNoaXAuLi4KClllYWggSSBh Z3JlZSB3aXRoIEFuZHJ6ZWogaGVyZSwgZm9yIG1lIDwtPiBpbXBsaWVzIGEgc3ltbWV0cmljCnJl bGF0aW9uc2hpcC4gU3BlbGxpbmcgaXQgb3V0IGxpa2UgQW5kcnplaiBzdWdnZXN0ZWQgc291bmRz IGxpa2UgdGhlCmJldHRlciBpZGVhLgotRGFuaWVsCgo+IAo+ID4gQW55d2F5Ogo+ID4gUmV2aWV3 ZWQtYnk6IEFuZHJ6ZWogSGFqZGEgPGEuaGFqZGFAc2Ftc3VuZy5jb20+Cj4gCj4gVGhhbmtzIQo+ IAo+IENoZWVycywKPiBQZXRlcgo+IAo+ID4gwqAtLQo+ID4gUmVnYXJkcwo+ID4gQW5kcnplago+ ID4gCj4gPj4gICAqIEBmdW5jczogY29udHJvbCBmdW5jdGlvbnMKPiA+PiAgICogQGRyaXZlcl9w cml2YXRlOiBwb2ludGVyIHRvIHRoZSBicmlkZ2UgZHJpdmVyJ3MgaW50ZXJuYWwgY29udGV4dAo+ ID4+ICAgKi8KPiA+PiBAQCAtMjcxLDYgKzI3Miw3IEBAIHN0cnVjdCBkcm1fYnJpZGdlIHsKPiA+ PiAgCXN0cnVjdCBkcm1fYnJpZGdlICpuZXh0Owo+ID4+ICAJc3RydWN0IGxpc3RfaGVhZCBsaXN0 Owo+ID4+ICAJY29uc3Qgc3RydWN0IGRybV9icmlkZ2VfdGltaW5ncyAqdGltaW5nczsKPiA+PiAr CXN0cnVjdCBkZXZpY2VfbGluayAqbGluazsKPiA+PiAgCj4gPj4gIAljb25zdCBzdHJ1Y3QgZHJt X2JyaWRnZV9mdW5jcyAqZnVuY3M7Cj4gPj4gIAl2b2lkICpkcml2ZXJfcHJpdmF0ZTsKPiA+IAo+ ID4gCj4gCgotLSAKRGFuaWVsIFZldHRlcgpTb2Z0d2FyZSBFbmdpbmVlciwgSW50ZWwgQ29ycG9y YXRpb24KaHR0cDovL2Jsb2cuZmZ3bGwuY2gKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMu ZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlz dGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753540AbeENQ2i (ORCPT ); Mon, 14 May 2018 12:28:38 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:38915 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753477AbeENQ2d (ORCPT ); Mon, 14 May 2018 12:28:33 -0400 X-Google-Smtp-Source: AB8JxZp8ihqXDBOfaj/qcnFkNLWpwLzwIGfaq1yww97ILFHYQ0LHkuxB+8fUkEOqK4N+99nLEJEg8w== Date: Mon, 14 May 2018 18:28:28 +0200 From: Daniel Vetter To: Peter Rosin Cc: Andrzej Hajda , linux-kernel@vger.kernel.org, Archit Taneja , Laurent Pinchart , David Airlie , Peter Senna Tschudin , Martin Donnelly , Martyn Welch , Gustavo Padovan , Maarten Lankhorst , Sean Paul , Inki Dae , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Kukjin Kim , Krzysztof Kozlowski , CK Hu , Philipp Zabel , Matthias Brugger , Rob Clark , Sandy Huang , Heiko =?iso-8859-1?Q?St=FCbner?= , Benjamin Gaignard , Vincent Abriou , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, Jyri Sarha , Daniel Vetter Subject: Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer Message-ID: <20180514162828.GE28661@phenom.ffwll.local> Mail-Followup-To: Peter Rosin , Andrzej Hajda , linux-kernel@vger.kernel.org, Archit Taneja , Laurent Pinchart , David Airlie , Peter Senna Tschudin , Martin Donnelly , Martyn Welch , Gustavo Padovan , Maarten Lankhorst , Sean Paul , Inki Dae , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Kukjin Kim , Krzysztof Kozlowski , CK Hu , Philipp Zabel , Matthias Brugger , Rob Clark , Sandy Huang , Heiko =?iso-8859-1?Q?St=FCbner?= , Benjamin Gaignard , Vincent Abriou , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, Jyri Sarha References: <20180504135212.26977-1-peda@axentia.se> <20180504135212.26977-27-peda@axentia.se> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Operating-System: Linux phenom 4.15.0-3-amd64 User-Agent: Mutt/1.9.5 (2018-04-13) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote: > On 2018-05-10 10:10, Andrzej Hajda wrote: > > On 04.05.2018 15:52, Peter Rosin wrote: > >> If the bridge supplier is unbound, this will bring the bridge consumer > >> down along with the bridge. Thus, there will no longer linger any > >> dangling pointers from the bridge consumer (the drm_device) to some > >> non-existent bridge supplier. > >> > >> Signed-off-by: Peter Rosin > >> --- > >> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ > >> include/drm/drm_bridge.h | 2 ++ > >> 2 files changed, 20 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > >> index 78d186b6831b..0259f0a3ff27 100644 > >> --- a/drivers/gpu/drm/drm_bridge.c > >> +++ b/drivers/gpu/drm/drm_bridge.c > >> @@ -26,6 +26,7 @@ > >> #include > >> > >> #include > >> +#include > >> #include > >> > >> #include "drm_crtc_internal.h" > >> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > >> if (bridge->dev) > >> return -EBUSY; > >> > >> + if (encoder->dev->dev != bridge->odev) { > > > > I wonder why device_link_add does not handle this case (self dependency) > > silently as noop, as it seems to be a correct behavior. > > It's kind-of a silly corner-case though, so perfectly understandable > that it isn't handled. > > >> + bridge->link = device_link_add(encoder->dev->dev, > >> + bridge->odev, 0); > >> + if (!bridge->link) { > >> + dev_err(bridge->odev, "failed to link bridge to %s\n", > >> + dev_name(encoder->dev->dev)); > >> + return -EINVAL; > >> + } > >> + } > >> + > >> bridge->dev = encoder->dev; > >> bridge->encoder = encoder; > >> > >> if (bridge->funcs->attach) { > >> ret = bridge->funcs->attach(bridge); > >> if (ret < 0) { > >> + if (bridge->link) > >> + device_link_del(bridge->link); > >> + bridge->link = NULL; > >> bridge->dev = NULL; > >> bridge->encoder = NULL; > >> return ret; > >> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) > >> if (bridge->funcs->detach) > >> bridge->funcs->detach(bridge); > >> > >> + if (bridge->link) > >> + device_link_del(bridge->link); > >> + bridge->link = NULL; > >> + > >> bridge->dev = NULL; > >> } > >> > >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > >> index b656e505d11e..804189c63a4c 100644 > >> --- a/include/drm/drm_bridge.h > >> +++ b/include/drm/drm_bridge.h > >> @@ -261,6 +261,7 @@ struct drm_bridge_timings { > >> * @list: to keep track of all added bridges > >> * @timings: the timing specification for the bridge, if any (may > >> * be NULL) > >> + * @link: drm consumer <-> bridge supplier > > > > Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer > > to the bridge" would be better. > > I meant "<->" to indicate that the link is bidirectional, not that the > relationship is in any way symmetric. I wasn't aware of any implication > of a symmetric relationship when using "<->", do you have a reference? > But I guess the different arrow notations in math are somewhat overloaded > and that someone at some point must have used "<->" to indicate a > symmetric relationship... Yeah I agree with Andrzej here, for me <-> implies a symmetric relationship. Spelling it out like Andrzej suggested sounds like the better idea. -Daniel > > > Anyway: > > Reviewed-by: Andrzej Hajda > > Thanks! > > Cheers, > Peter > > >  -- > > Regards > > Andrzej > > > >> * @funcs: control functions > >> * @driver_private: pointer to the bridge driver's internal context > >> */ > >> @@ -271,6 +272,7 @@ struct drm_bridge { > >> struct drm_bridge *next; > >> struct list_head list; > >> const struct drm_bridge_timings *timings; > >> + struct device_link *link; > >> > >> const struct drm_bridge_funcs *funcs; > >> void *driver_private; > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:36099 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752586AbeENQ2d (ORCPT ); Mon, 14 May 2018 12:28:33 -0400 Received: by mail-wm0-f66.google.com with SMTP id n10-v6so16319458wmc.1 for ; Mon, 14 May 2018 09:28:32 -0700 (PDT) Date: Mon, 14 May 2018 18:28:28 +0200 From: Daniel Vetter To: Peter Rosin Cc: Andrzej Hajda , linux-kernel@vger.kernel.org, Archit Taneja , Laurent Pinchart , David Airlie , Peter Senna Tschudin , Martin Donnelly , Martyn Welch , Gustavo Padovan , Maarten Lankhorst , Sean Paul , Inki Dae , Joonyoung Shim , Seung-Woo Kim , Kyungmin Park , Kukjin Kim , Krzysztof Kozlowski , CK Hu , Philipp Zabel , Matthias Brugger , Rob Clark , Sandy Huang , Heiko =?iso-8859-1?Q?St=FCbner?= , Benjamin Gaignard , Vincent Abriou , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, Jyri Sarha , Daniel Vetter Subject: Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer Message-ID: <20180514162828.GE28661@phenom.ffwll.local> References: <20180504135212.26977-1-peda@axentia.se> <20180504135212.26977-27-peda@axentia.se> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote: > On 2018-05-10 10:10, Andrzej Hajda wrote: > > On 04.05.2018 15:52, Peter Rosin wrote: > >> If the bridge supplier is unbound, this will bring the bridge consumer > >> down along with the bridge. Thus, there will no longer linger any > >> dangling pointers from the bridge consumer (the drm_device) to some > >> non-existent bridge supplier. > >> > >> Signed-off-by: Peter Rosin > >> --- > >> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ > >> include/drm/drm_bridge.h | 2 ++ > >> 2 files changed, 20 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > >> index 78d186b6831b..0259f0a3ff27 100644 > >> --- a/drivers/gpu/drm/drm_bridge.c > >> +++ b/drivers/gpu/drm/drm_bridge.c > >> @@ -26,6 +26,7 @@ > >> #include > >> > >> #include > >> +#include > >> #include > >> > >> #include "drm_crtc_internal.h" > >> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > >> if (bridge->dev) > >> return -EBUSY; > >> > >> + if (encoder->dev->dev != bridge->odev) { > > > > I wonder why device_link_add does not handle this case (self dependency) > > silently as noop, as it seems to be a correct behavior. > > It's kind-of a silly corner-case though, so perfectly understandable > that it isn't handled. > > >> + bridge->link = device_link_add(encoder->dev->dev, > >> + bridge->odev, 0); > >> + if (!bridge->link) { > >> + dev_err(bridge->odev, "failed to link bridge to %s\n", > >> + dev_name(encoder->dev->dev)); > >> + return -EINVAL; > >> + } > >> + } > >> + > >> bridge->dev = encoder->dev; > >> bridge->encoder = encoder; > >> > >> if (bridge->funcs->attach) { > >> ret = bridge->funcs->attach(bridge); > >> if (ret < 0) { > >> + if (bridge->link) > >> + device_link_del(bridge->link); > >> + bridge->link = NULL; > >> bridge->dev = NULL; > >> bridge->encoder = NULL; > >> return ret; > >> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) > >> if (bridge->funcs->detach) > >> bridge->funcs->detach(bridge); > >> > >> + if (bridge->link) > >> + device_link_del(bridge->link); > >> + bridge->link = NULL; > >> + > >> bridge->dev = NULL; > >> } > >> > >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > >> index b656e505d11e..804189c63a4c 100644 > >> --- a/include/drm/drm_bridge.h > >> +++ b/include/drm/drm_bridge.h > >> @@ -261,6 +261,7 @@ struct drm_bridge_timings { > >> * @list: to keep track of all added bridges > >> * @timings: the timing specification for the bridge, if any (may > >> * be NULL) > >> + * @link: drm consumer <-> bridge supplier > > > > Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer > > to the bridge" would be better. > > I meant "<->" to indicate that the link is bidirectional, not that the > relationship is in any way symmetric. I wasn't aware of any implication > of a symmetric relationship when using "<->", do you have a reference? > But I guess the different arrow notations in math are somewhat overloaded > and that someone at some point must have used "<->" to indicate a > symmetric relationship... Yeah I agree with Andrzej here, for me <-> implies a symmetric relationship. Spelling it out like Andrzej suggested sounds like the better idea. -Daniel > > > Anyway: > > Reviewed-by: Andrzej Hajda > > Thanks! > > Cheers, > Peter > > > �-- > > Regards > > Andrzej > > > >> * @funcs: control functions > >> * @driver_private: pointer to the bridge driver's internal context > >> */ > >> @@ -271,6 +272,7 @@ struct drm_bridge { > >> struct drm_bridge *next; > >> struct list_head list; > >> const struct drm_bridge_timings *timings; > >> + struct device_link *link; > >> > >> const struct drm_bridge_funcs *funcs; > >> void *driver_private; > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel@ffwll.ch (Daniel Vetter) Date: Mon, 14 May 2018 18:28:28 +0200 Subject: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer In-Reply-To: References: <20180504135212.26977-1-peda@axentia.se> <20180504135212.26977-27-peda@axentia.se> Message-ID: <20180514162828.GE28661@phenom.ffwll.local> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote: > On 2018-05-10 10:10, Andrzej Hajda wrote: > > On 04.05.2018 15:52, Peter Rosin wrote: > >> If the bridge supplier is unbound, this will bring the bridge consumer > >> down along with the bridge. Thus, there will no longer linger any > >> dangling pointers from the bridge consumer (the drm_device) to some > >> non-existent bridge supplier. > >> > >> Signed-off-by: Peter Rosin > >> --- > >> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ > >> include/drm/drm_bridge.h | 2 ++ > >> 2 files changed, 20 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > >> index 78d186b6831b..0259f0a3ff27 100644 > >> --- a/drivers/gpu/drm/drm_bridge.c > >> +++ b/drivers/gpu/drm/drm_bridge.c > >> @@ -26,6 +26,7 @@ > >> #include > >> > >> #include > >> +#include > >> #include > >> > >> #include "drm_crtc_internal.h" > >> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > >> if (bridge->dev) > >> return -EBUSY; > >> > >> + if (encoder->dev->dev != bridge->odev) { > > > > I wonder why device_link_add does not handle this case (self dependency) > > silently as noop, as it seems to be a correct behavior. > > It's kind-of a silly corner-case though, so perfectly understandable > that it isn't handled. > > >> + bridge->link = device_link_add(encoder->dev->dev, > >> + bridge->odev, 0); > >> + if (!bridge->link) { > >> + dev_err(bridge->odev, "failed to link bridge to %s\n", > >> + dev_name(encoder->dev->dev)); > >> + return -EINVAL; > >> + } > >> + } > >> + > >> bridge->dev = encoder->dev; > >> bridge->encoder = encoder; > >> > >> if (bridge->funcs->attach) { > >> ret = bridge->funcs->attach(bridge); > >> if (ret < 0) { > >> + if (bridge->link) > >> + device_link_del(bridge->link); > >> + bridge->link = NULL; > >> bridge->dev = NULL; > >> bridge->encoder = NULL; > >> return ret; > >> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) > >> if (bridge->funcs->detach) > >> bridge->funcs->detach(bridge); > >> > >> + if (bridge->link) > >> + device_link_del(bridge->link); > >> + bridge->link = NULL; > >> + > >> bridge->dev = NULL; > >> } > >> > >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > >> index b656e505d11e..804189c63a4c 100644 > >> --- a/include/drm/drm_bridge.h > >> +++ b/include/drm/drm_bridge.h > >> @@ -261,6 +261,7 @@ struct drm_bridge_timings { > >> * @list: to keep track of all added bridges > >> * @timings: the timing specification for the bridge, if any (may > >> * be NULL) > >> + * @link: drm consumer <-> bridge supplier > > > > Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer > > to the bridge" would be better. > > I meant "<->" to indicate that the link is bidirectional, not that the > relationship is in any way symmetric. I wasn't aware of any implication > of a symmetric relationship when using "<->", do you have a reference? > But I guess the different arrow notations in math are somewhat overloaded > and that someone at some point must have used "<->" to indicate a > symmetric relationship... Yeah I agree with Andrzej here, for me <-> implies a symmetric relationship. Spelling it out like Andrzej suggested sounds like the better idea. -Daniel > > > Anyway: > > Reviewed-by: Andrzej Hajda > > Thanks! > > Cheers, > Peter > > > ?-- > > Regards > > Andrzej > > > >> * @funcs: control functions > >> * @driver_private: pointer to the bridge driver's internal context > >> */ > >> @@ -271,6 +272,7 @@ struct drm_bridge { > >> struct drm_bridge *next; > >> struct list_head list; > >> const struct drm_bridge_timings *timings; > >> + struct device_link *link; > >> > >> const struct drm_bridge_funcs *funcs; > >> void *driver_private; > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch