From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965142AbcHaQ2K (ORCPT ); Wed, 31 Aug 2016 12:28:10 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:34828 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964995AbcHaQ1N (ORCPT ); Wed, 31 Aug 2016 12:27:13 -0400 Date: Wed, 31 Aug 2016 18:27:08 +0200 From: Daniel Vetter To: Maxime Ripard Cc: Chen-Yu Tsai , linux-arm-kernel , dri-devel , linux-kernel Subject: Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found Message-ID: <20160831162708.GD20761@phenom.ffwll.local> Mail-Followup-To: Maxime Ripard , Chen-Yu Tsai , linux-arm-kernel , dri-devel , linux-kernel References: <20160830122223.21377-1-wens@csie.org> <20160830125622.GF18605@lukather> <20160831154002.GC14379@lukather> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160831154002.GC14379@lukather> X-Operating-System: Linux phenom 4.6.0-1-amd64 User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Aug 31, 2016 at 05:40:02PM +0200, Maxime Ripard wrote: > On Tue, Aug 30, 2016 at 11:51:26PM +0800, Chen-Yu Tsai wrote: > > On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard > > wrote: > > > Hi, > > > > > > On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > > >> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > > >> encoder->bridge directly to drm_bridge_mode_fixup, which expects a > > >> valid pointer, or NULL (in which case it just returns). > > >> > > >> Clear encoder->bridge if a bridge is not found, instead of keeping > > >> the ERR_PTR value. > > >> > > >> Since other drm_bridge functions also follow this pattern of checking > > >> for a non-NULL pointer, we can drop the ifs around the calls and just > > >> pass the pointer directly. > > >> > > >> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > > >> Signed-off-by: Chen-Yu Tsai > > >> --- > > >> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > > >> 1 file changed, 6 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> index d4e52522ec53..ee0795152a33 100644 > > >> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) > > >> if (!IS_ERR(tcon->panel)) > > >> drm_panel_enable(tcon->panel); > > >> > > >> - if (!IS_ERR(encoder->bridge)) > > >> - drm_bridge_enable(encoder->bridge); > > >> + drm_bridge_enable(encoder->bridge); > > >> > > >> sun4i_tcon_channel_enable(tcon, 0); > > >> } > > >> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) > > >> > > >> sun4i_tcon_channel_disable(tcon, 0); > > >> > > >> - if (!IS_ERR(encoder->bridge)) > > >> - drm_bridge_disable(encoder->bridge); > > >> + drm_bridge_disable(encoder->bridge); > > > > > > I'd rather keep those changes, it makes it obvious that it's something > > > optionnal, that can be set to NULL. > > > > OK. > > > > >> if (!IS_ERR(tcon->panel)) > > >> drm_panel_disable(tcon->panel); > > >> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > > >> return 0; > > >> } > > >> > > >> + if (IS_ERR(encoder->bridge)) > > >> + encoder->bridge = NULL; > > >> + > > > > > > And that could be the else condition of the if statement below. > > > > That would be a bit confusing, changing it after calling drm_encoder_init. > > The code says it ok to do though. > > The magic really happens only after the encoder has been attached to > something, so it's really safe. s/attached/registered using drm_dev_register(). Which should happen _way_ later for all drivers which have gotten rid of their ->load callback and implemented the recommend driver load sequence. -Daniel -- 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: Wed, 31 Aug 2016 18:27:08 +0200 Subject: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found In-Reply-To: <20160831154002.GC14379@lukather> References: <20160830122223.21377-1-wens@csie.org> <20160830125622.GF18605@lukather> <20160831154002.GC14379@lukather> Message-ID: <20160831162708.GD20761@phenom.ffwll.local> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Aug 31, 2016 at 05:40:02PM +0200, Maxime Ripard wrote: > On Tue, Aug 30, 2016 at 11:51:26PM +0800, Chen-Yu Tsai wrote: > > On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard > > wrote: > > > Hi, > > > > > > On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > > >> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > > >> encoder->bridge directly to drm_bridge_mode_fixup, which expects a > > >> valid pointer, or NULL (in which case it just returns). > > >> > > >> Clear encoder->bridge if a bridge is not found, instead of keeping > > >> the ERR_PTR value. > > >> > > >> Since other drm_bridge functions also follow this pattern of checking > > >> for a non-NULL pointer, we can drop the ifs around the calls and just > > >> pass the pointer directly. > > >> > > >> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > > >> Signed-off-by: Chen-Yu Tsai > > >> --- > > >> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > > >> 1 file changed, 6 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> index d4e52522ec53..ee0795152a33 100644 > > >> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > > >> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) > > >> if (!IS_ERR(tcon->panel)) > > >> drm_panel_enable(tcon->panel); > > >> > > >> - if (!IS_ERR(encoder->bridge)) > > >> - drm_bridge_enable(encoder->bridge); > > >> + drm_bridge_enable(encoder->bridge); > > >> > > >> sun4i_tcon_channel_enable(tcon, 0); > > >> } > > >> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) > > >> > > >> sun4i_tcon_channel_disable(tcon, 0); > > >> > > >> - if (!IS_ERR(encoder->bridge)) > > >> - drm_bridge_disable(encoder->bridge); > > >> + drm_bridge_disable(encoder->bridge); > > > > > > I'd rather keep those changes, it makes it obvious that it's something > > > optionnal, that can be set to NULL. > > > > OK. > > > > >> if (!IS_ERR(tcon->panel)) > > >> drm_panel_disable(tcon->panel); > > >> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > > >> return 0; > > >> } > > >> > > >> + if (IS_ERR(encoder->bridge)) > > >> + encoder->bridge = NULL; > > >> + > > > > > > And that could be the else condition of the if statement below. > > > > That would be a bit confusing, changing it after calling drm_encoder_init. > > The code says it ok to do though. > > The magic really happens only after the encoder has been attached to > something, so it's really safe. s/attached/registered using drm_dev_register(). Which should happen _way_ later for all drivers which have gotten rid of their ->load callback and implemented the recommend driver load sequence. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found Date: Wed, 31 Aug 2016 18:27:08 +0200 Message-ID: <20160831162708.GD20761@phenom.ffwll.local> References: <20160830122223.21377-1-wens@csie.org> <20160830125622.GF18605@lukather> <20160831154002.GC14379@lukather> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-wm0-x243.google.com (mail-wm0-x243.google.com [IPv6:2a00:1450:400c:c09::243]) by gabe.freedesktop.org (Postfix) with ESMTPS id 61DB36E7CC for ; Wed, 31 Aug 2016 16:27:13 +0000 (UTC) Received: by mail-wm0-x243.google.com with SMTP id w207so4152115wmw.0 for ; Wed, 31 Aug 2016 09:27:13 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20160831154002.GC14379@lukather> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Maxime Ripard Cc: Chen-Yu Tsai , dri-devel , linux-arm-kernel , linux-kernel List-Id: dri-devel@lists.freedesktop.org T24gV2VkLCBBdWcgMzEsIDIwMTYgYXQgMDU6NDA6MDJQTSArMDIwMCwgTWF4aW1lIFJpcGFyZCB3 cm90ZToKPiBPbiBUdWUsIEF1ZyAzMCwgMjAxNiBhdCAxMTo1MToyNlBNICswODAwLCBDaGVuLVl1 IFRzYWkgd3JvdGU6Cj4gPiBPbiBUdWUsIEF1ZyAzMCwgMjAxNiBhdCA4OjU2IFBNLCBNYXhpbWUg UmlwYXJkCj4gPiA8bWF4aW1lLnJpcGFyZEBmcmVlLWVsZWN0cm9ucy5jb20+IHdyb3RlOgo+ID4g PiBIaSwKPiA+ID4KPiA+ID4gT24gVHVlLCBBdWcgMzAsIDIwMTYgYXQgMDg6MjI6MjNQTSArMDgw MCwgQ2hlbi1ZdSBUc2FpIHdyb3RlOgo+ID4gPj4gVGhlIEtNUyBoZWxwZXJzIChkcm1fYXRvbWlj X2hlbHBlcl9jaGVja19tb2Rlc2V0L21vZGVfZml4dXApIHBhc3MKPiA+ID4+IGVuY29kZXItPmJy aWRnZSBkaXJlY3RseSB0byBkcm1fYnJpZGdlX21vZGVfZml4dXAsIHdoaWNoIGV4cGVjdHMgYQo+ ID4gPj4gdmFsaWQgcG9pbnRlciwgb3IgTlVMTCAoaW4gd2hpY2ggY2FzZSBpdCBqdXN0IHJldHVy bnMpLgo+ID4gPj4KPiA+ID4+IENsZWFyIGVuY29kZXItPmJyaWRnZSBpZiBhIGJyaWRnZSBpcyBu b3QgZm91bmQsIGluc3RlYWQgb2Yga2VlcGluZwo+ID4gPj4gdGhlIEVSUl9QVFIgdmFsdWUuCj4g PiA+Pgo+ID4gPj4gU2luY2Ugb3RoZXIgZHJtX2JyaWRnZSBmdW5jdGlvbnMgYWxzbyBmb2xsb3cg dGhpcyBwYXR0ZXJuIG9mIGNoZWNraW5nCj4gPiA+PiBmb3IgYSBub24tTlVMTCBwb2ludGVyLCB3 ZSBjYW4gZHJvcCB0aGUgaWZzIGFyb3VuZCB0aGUgY2FsbHMgYW5kIGp1c3QKPiA+ID4+IHBhc3Mg dGhlIHBvaW50ZXIgZGlyZWN0bHkuCj4gPiA+Pgo+ID4gPj4gRml4ZXM6IDg5NGY1YTlmNGI0YSAo ImRybS9zdW40aTogQWRkIGJyaWRnZSBzdXBwb3J0IikKPiA+ID4+IFNpZ25lZC1vZmYtYnk6IENo ZW4tWXUgVHNhaSA8d2Vuc0Bjc2llLm9yZz4KPiA+ID4+IC0tLQo+ID4gPj4gIGRyaXZlcnMvZ3B1 L2RybS9zdW40aS9zdW40aV9yZ2IuYyB8IDExICsrKysrKy0tLS0tCj4gPiA+PiAgMSBmaWxlIGNo YW5nZWQsIDYgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMoLSkKPiA+ID4+Cj4gPiA+PiBkaWZm IC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL3N1bjRpL3N1bjRpX3JnYi5jIGIvZHJpdmVycy9ncHUv ZHJtL3N1bjRpL3N1bjRpX3JnYi5jCj4gPiA+PiBpbmRleCBkNGU1MjUyMmVjNTMuLmVlMDc5NTE1 MmEzMyAxMDA2NDQKPiA+ID4+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9zdW40aS9zdW40aV9yZ2Iu Ywo+ID4gPj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL3N1bjRpL3N1bjRpX3JnYi5jCj4gPiA+PiBA QCAtMTU0LDggKzE1NCw3IEBAIHN0YXRpYyB2b2lkIHN1bjRpX3JnYl9lbmNvZGVyX2VuYWJsZShz dHJ1Y3QgZHJtX2VuY29kZXIgKmVuY29kZXIpCj4gPiA+PiAgICAgICBpZiAoIUlTX0VSUih0Y29u LT5wYW5lbCkpCj4gPiA+PiAgICAgICAgICAgICAgIGRybV9wYW5lbF9lbmFibGUodGNvbi0+cGFu ZWwpOwo+ID4gPj4KPiA+ID4+IC0gICAgIGlmICghSVNfRVJSKGVuY29kZXItPmJyaWRnZSkpCj4g PiA+PiAtICAgICAgICAgICAgIGRybV9icmlkZ2VfZW5hYmxlKGVuY29kZXItPmJyaWRnZSk7Cj4g PiA+PiArICAgICBkcm1fYnJpZGdlX2VuYWJsZShlbmNvZGVyLT5icmlkZ2UpOwo+ID4gPj4KPiA+ ID4+ICAgICAgIHN1bjRpX3Rjb25fY2hhbm5lbF9lbmFibGUodGNvbiwgMCk7Cj4gPiA+PiAgfQo+ ID4gPj4gQEAgLTE3MCw4ICsxNjksNyBAQCBzdGF0aWMgdm9pZCBzdW40aV9yZ2JfZW5jb2Rlcl9k aXNhYmxlKHN0cnVjdCBkcm1fZW5jb2RlciAqZW5jb2RlcikKPiA+ID4+Cj4gPiA+PiAgICAgICBz dW40aV90Y29uX2NoYW5uZWxfZGlzYWJsZSh0Y29uLCAwKTsKPiA+ID4+Cj4gPiA+PiAtICAgICBp ZiAoIUlTX0VSUihlbmNvZGVyLT5icmlkZ2UpKQo+ID4gPj4gLSAgICAgICAgICAgICBkcm1fYnJp ZGdlX2Rpc2FibGUoZW5jb2Rlci0+YnJpZGdlKTsKPiA+ID4+ICsgICAgIGRybV9icmlkZ2VfZGlz YWJsZShlbmNvZGVyLT5icmlkZ2UpOwo+ID4gPgo+ID4gPiBJJ2QgcmF0aGVyIGtlZXAgdGhvc2Ug Y2hhbmdlcywgaXQgbWFrZXMgaXQgb2J2aW91cyB0aGF0IGl0J3Mgc29tZXRoaW5nCj4gPiA+IG9w dGlvbm5hbCwgdGhhdCBjYW4gYmUgc2V0IHRvIE5VTEwuCj4gPiAKPiA+IE9LLgo+ID4gCj4gPiA+ PiAgICAgICBpZiAoIUlTX0VSUih0Y29uLT5wYW5lbCkpCj4gPiA+PiAgICAgICAgICAgICAgIGRy bV9wYW5lbF9kaXNhYmxlKHRjb24tPnBhbmVsKTsKPiA+ID4+IEBAIC0yMzAsNiArMjI4LDkgQEAg aW50IHN1bjRpX3JnYl9pbml0KHN0cnVjdCBkcm1fZGV2aWNlICpkcm0pCj4gPiA+PiAgICAgICAg ICAgICAgIHJldHVybiAwOwo+ID4gPj4gICAgICAgfQo+ID4gPj4KPiA+ID4+ICsgICAgIGlmIChJ U19FUlIoZW5jb2Rlci0+YnJpZGdlKSkKPiA+ID4+ICsgICAgICAgICAgICAgZW5jb2Rlci0+YnJp ZGdlID0gTlVMTDsKPiA+ID4+ICsKPiA+ID4KPiA+ID4gQW5kIHRoYXQgY291bGQgYmUgdGhlIGVs c2UgY29uZGl0aW9uIG9mIHRoZSBpZiBzdGF0ZW1lbnQgYmVsb3cuCj4gPiAKPiA+IFRoYXQgd291 bGQgYmUgYSBiaXQgY29uZnVzaW5nLCBjaGFuZ2luZyBpdCBhZnRlciBjYWxsaW5nIGRybV9lbmNv ZGVyX2luaXQuCj4gPiBUaGUgY29kZSBzYXlzIGl0IG9rIHRvIGRvIHRob3VnaC4KPiAKPiBUaGUg bWFnaWMgcmVhbGx5IGhhcHBlbnMgb25seSBhZnRlciB0aGUgZW5jb2RlciBoYXMgYmVlbiBhdHRh Y2hlZCB0bwo+IHNvbWV0aGluZywgc28gaXQncyByZWFsbHkgc2FmZS4KCnMvYXR0YWNoZWQvcmVn aXN0ZXJlZCB1c2luZyBkcm1fZGV2X3JlZ2lzdGVyKCkuIFdoaWNoIHNob3VsZApoYXBwZW4gX3dh eV8gbGF0ZXIgZm9yIGFsbCBkcml2ZXJzIHdoaWNoIGhhdmUgZ290dGVuIHJpZCBvZiB0aGVpciAt PmxvYWQKY2FsbGJhY2sgYW5kIGltcGxlbWVudGVkIHRoZSByZWNvbW1lbmQgZHJpdmVyIGxvYWQg c2VxdWVuY2UuCi1EYW5pZWwKLS0gCkRhbmllbCBWZXR0ZXIKU29mdHdhcmUgRW5naW5lZXIsIElu dGVsIENvcnBvcmF0aW9uCmh0dHA6Ly9ibG9nLmZmd2xsLmNoCl9fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRl dmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9t YWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo=