From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E37A5C43381 for ; Fri, 1 Mar 2019 15:06:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B2AE320850 for ; Fri, 1 Mar 2019 15:06:37 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="W76jwSUx" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727952AbfCAPGh (ORCPT ); Fri, 1 Mar 2019 10:06:37 -0500 Received: from perceval.ideasonboard.com ([213.167.242.64]:39864 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727951AbfCAPGh (ORCPT ); Fri, 1 Mar 2019 10:06:37 -0500 Received: from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id EBF2F49; Fri, 1 Mar 2019 16:06:34 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1551452795; bh=Xk16oEbd0wGMXZcuHqcFsoN1MS66NjDxm8NC+CFt1T8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=W76jwSUx+f8XUZ19Tspwkgb2cvWUoZ+TokTZazyNq/By/OYQgSYBmyurRQGesCyjS MijMlnelKldyEhefUmHAY5Vk90gkZGRAaA/MVxz1F9NoiWoEGSkAzZfOTETMMcXc2q rUtQC3PCVKeaLO3ZF+OpfOF+CHMy8Cj5KY62JeZY= Date: Fri, 1 Mar 2019 17:06:29 +0200 From: Laurent Pinchart To: Maarten Lankhorst Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, Kieran Bingham , linux-renesas-soc@vger.kernel.org Subject: Re: [PATCH 12/17] drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset. Message-ID: <20190301150629.GE32244@pendragon.ideasonboard.com> References: <20190301125627.7285-1-maarten.lankhorst@linux.intel.com> <20190301125627.7285-13-maarten.lankhorst@linux.intel.com> <20190301131358.GC32244@pendragon.ideasonboard.com> <93a242fe-41fc-bbe5-0ccd-f26eb729fe85@linux.intel.com> <20190301143640.GD32244@pendragon.ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-renesas-soc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-renesas-soc@vger.kernel.org Hi Marteen, On Fri, Mar 01, 2019 at 03:47:02PM +0100, Maarten Lankhorst wrote: > Op 01-03-2019 om 15:36 schreef Laurent Pinchart: > > On Fri, Mar 01, 2019 at 03:08:20PM +0100, Maarten Lankhorst wrote: > >> Op 01-03-2019 om 14:13 schreef Laurent Pinchart: > >>> On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote: > >>>> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of > >>>> writing its own version. Instead of open coding destroy_state(), call > >>>> it directly for freeing the old state. > >>> I don't think the second sentence applies to this patch. > >>> > >>>> Signed-off-by: Maarten Lankhorst > >>>> Cc: Laurent Pinchart > >>>> Cc: Kieran Bingham > >>>> Cc: linux-renesas-soc@vger.kernel.org > >>>> --- > >>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++-------- > >>>> 1 file changed, 3 insertions(+), 8 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >>>> index 4cdea14d552f..7766551e67fc 100644 > >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >>>> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc) > >>>> > >>>> static void rcar_du_crtc_reset(struct drm_crtc *crtc) > >>>> { > >>>> - struct rcar_du_crtc_state *state; > >>>> + struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL); > >>>> > >>>> - if (crtc->state) { > >>>> + if (crtc->state) > >>>> rcar_du_crtc_atomic_destroy_state(crtc, crtc->state); > >>>> - crtc->state = NULL; > >>>> - } > >>>> > >>>> - state = kzalloc(sizeof(*state), GFP_KERNEL); > >>>> + __drm_atomic_helper_crtc_reset(crtc, &state->state); > >>> state may be NULL here if the above kzalloc() failed. Let's keep the > >>> original order of the function, and simply call > >>> __drm_atomic_helper_crtc_reset() after the NULL check below. > >> There were 10 different ways crtc was implemented, I felt it was good to settle on one. > >> > >> We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho. > > That's not the point. As state can be NULL, you could end up > > dereferencing a NULL pointer. The fact that the base state is the first > > field in the rcar_du_crtc_state structure is just luck, and shouldn't be > > relied on. > > Would it be ok if I changed it to state ? &state->state : NULL and let > the compiler deal with it? What's wrong with a proper implementation ? static void rcar_du_crtc_reset(struct drm_crtc *crtc) { struct rcar_du_crtc_state *state; if (crtc->state) { rcar_du_crtc_atomic_destroy_state(crtc, crtc->state); crtc->state = NULL; } state = kzalloc(sizeof(*state), GFP_KERNEL); if (state == NULL) return; __drm_atomic_helper_crtc_reset(crtc, &state->state); state->crc.source = VSP1_DU_CRC_NONE; state->crc.index = 0; } > Will probably fix up all other patches as well before committing. You won't commit this one before I ack it, right ? :-) > >> Looking more closely, it's the same way that errors in > >> rcar_du_plane_reset() are handled. :) > > It's not, the return value of kzalloc() is checked explicitly in > > rcar_du_plane_reset() before calling __drm_atomic_helper_plane_reset(). > > Please copy the code flow of rcar_du_plane_reset() to implement > > rcar_du_crtc_reset(). -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH 12/17] drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset. Date: Fri, 1 Mar 2019 17:06:29 +0200 Message-ID: <20190301150629.GE32244@pendragon.ideasonboard.com> References: <20190301125627.7285-1-maarten.lankhorst@linux.intel.com> <20190301125627.7285-13-maarten.lankhorst@linux.intel.com> <20190301131358.GC32244@pendragon.ideasonboard.com> <93a242fe-41fc-bbe5-0ccd-f26eb729fe85@linux.intel.com> <20190301143640.GD32244@pendragon.ideasonboard.com> 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: Maarten Lankhorst Cc: linux-renesas-soc@vger.kernel.org, intel-gfx@lists.freedesktop.org, Kieran Bingham , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org SGkgTWFydGVlbiwKCk9uIEZyaSwgTWFyIDAxLCAyMDE5IGF0IDAzOjQ3OjAyUE0gKzAxMDAsIE1h YXJ0ZW4gTGFua2hvcnN0IHdyb3RlOgo+IE9wIDAxLTAzLTIwMTkgb20gMTU6MzYgc2NocmVlZiBM YXVyZW50IFBpbmNoYXJ0Ogo+ID4gT24gRnJpLCBNYXIgMDEsIDIwMTkgYXQgMDM6MDg6MjBQTSAr MDEwMCwgTWFhcnRlbiBMYW5raG9yc3Qgd3JvdGU6Cj4gPj4gT3AgMDEtMDMtMjAxOSBvbSAxNDox MyBzY2hyZWVmIExhdXJlbnQgUGluY2hhcnQ6Cj4gPj4+IE9uIEZyaSwgTWFyIDAxLCAyMDE5IGF0 IDAxOjU2OjIyUE0gKzAxMDAsIE1hYXJ0ZW4gTGFua2hvcnN0IHdyb3RlOgo+ID4+Pj4gQ29udmVy dCByY2FyLWR1IHRvIHVzaW5nIF9fZHJtX2F0b21pY19oZWxwZXJfY3J0Y19yZXNldCgpLCBpbnN0 ZWFkIG9mCj4gPj4+PiB3cml0aW5nIGl0cyBvd24gdmVyc2lvbi4gSW5zdGVhZCBvZiBvcGVuIGNv ZGluZyBkZXN0cm95X3N0YXRlKCksIGNhbGwKPiA+Pj4+IGl0IGRpcmVjdGx5IGZvciBmcmVlaW5n IHRoZSBvbGQgc3RhdGUuCj4gPj4+IEkgZG9uJ3QgdGhpbmsgdGhlIHNlY29uZCBzZW50ZW5jZSBh cHBsaWVzIHRvIHRoaXMgcGF0Y2guCj4gPj4+Cj4gPj4+PiBTaWduZWQtb2ZmLWJ5OiBNYWFydGVu IExhbmtob3JzdCA8bWFhcnRlbi5sYW5raG9yc3RAbGludXguaW50ZWwuY29tPgo+ID4+Pj4gQ2M6 IExhdXJlbnQgUGluY2hhcnQgPGxhdXJlbnQucGluY2hhcnRAaWRlYXNvbmJvYXJkLmNvbT4KPiA+ Pj4+IENjOiBLaWVyYW4gQmluZ2hhbSA8a2llcmFuLmJpbmdoYW0rcmVuZXNhc0BpZGVhc29uYm9h cmQuY29tPgo+ID4+Pj4gQ2M6IGxpbnV4LXJlbmVzYXMtc29jQHZnZXIua2VybmVsLm9yZwo+ID4+ Pj4gLS0tCj4gPj4+PiAgZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV9jcnRjLmMgfCAx MSArKystLS0tLS0tLQo+ID4+Pj4gIDEgZmlsZSBjaGFuZ2VkLCAzIGluc2VydGlvbnMoKyksIDgg ZGVsZXRpb25zKC0pCj4gPj4+Pgo+ID4+Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9y Y2FyLWR1L3JjYXJfZHVfY3J0Yy5jIGIvZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV9j cnRjLmMKPiA+Pj4+IGluZGV4IDRjZGVhMTRkNTUyZi4uNzc2NjU1MWU2N2ZjIDEwMDY0NAo+ID4+ Pj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV9jcnRjLmMKPiA+Pj4+ICsr KyBiL2RyaXZlcnMvZ3B1L2RybS9yY2FyLWR1L3JjYXJfZHVfY3J0Yy5jCj4gPj4+PiBAQCAtODkx LDIyICs4OTEsMTcgQEAgc3RhdGljIHZvaWQgcmNhcl9kdV9jcnRjX2NsZWFudXAoc3RydWN0IGRy bV9jcnRjICpjcnRjKQo+ID4+Pj4gIAo+ID4+Pj4gIHN0YXRpYyB2b2lkIHJjYXJfZHVfY3J0Y19y ZXNldChzdHJ1Y3QgZHJtX2NydGMgKmNydGMpCj4gPj4+PiAgewo+ID4+Pj4gLQlzdHJ1Y3QgcmNh cl9kdV9jcnRjX3N0YXRlICpzdGF0ZTsKPiA+Pj4+ICsJc3RydWN0IHJjYXJfZHVfY3J0Y19zdGF0 ZSAqc3RhdGUgPSBremFsbG9jKHNpemVvZigqc3RhdGUpLCBHRlBfS0VSTkVMKTsKPiA+Pj4+ICAK PiA+Pj4+IC0JaWYgKGNydGMtPnN0YXRlKSB7Cj4gPj4+PiArCWlmIChjcnRjLT5zdGF0ZSkKPiA+ Pj4+ICAJCXJjYXJfZHVfY3J0Y19hdG9taWNfZGVzdHJveV9zdGF0ZShjcnRjLCBjcnRjLT5zdGF0 ZSk7Cj4gPj4+PiAtCQljcnRjLT5zdGF0ZSA9IE5VTEw7Cj4gPj4+PiAtCX0KPiA+Pj4+ICAKPiA+ Pj4+IC0Jc3RhdGUgPSBremFsbG9jKHNpemVvZigqc3RhdGUpLCBHRlBfS0VSTkVMKTsKPiA+Pj4+ ICsJX19kcm1fYXRvbWljX2hlbHBlcl9jcnRjX3Jlc2V0KGNydGMsICZzdGF0ZS0+c3RhdGUpOwo+ ID4+PiBzdGF0ZSBtYXkgYmUgTlVMTCBoZXJlIGlmIHRoZSBhYm92ZSBremFsbG9jKCkgZmFpbGVk LiBMZXQncyBrZWVwIHRoZQo+ID4+PiBvcmlnaW5hbCBvcmRlciBvZiB0aGUgZnVuY3Rpb24sIGFu ZCBzaW1wbHkgY2FsbAo+ID4+PiBfX2RybV9hdG9taWNfaGVscGVyX2NydGNfcmVzZXQoKSBhZnRl ciB0aGUgTlVMTCBjaGVjayBiZWxvdy4KPiA+PiBUaGVyZSB3ZXJlIDEwIGRpZmZlcmVudCB3YXlz IGNydGMgd2FzIGltcGxlbWVudGVkLCBJIGZlbHQgaXQgd2FzIGdvb2QgdG8gc2V0dGxlIG9uIG9u ZS4KPiA+Pgo+ID4+IFdlIGRvbid0IGhhbmRsZSBkdXJpbmcgcmVzZXQgYXQgYWxsLCB3b3VsZCBu ZWVkIHRvIHN0YXJ0IHByb3BhZ2F0aW5nIHRoaXMgZmlyc3QgYmVmb3JlIHdlIHNob3VsZCBoYW5k bGUgZXJyb3JzLCBpbWhvLgo+ID4gVGhhdCdzIG5vdCB0aGUgcG9pbnQuIEFzIHN0YXRlIGNhbiBi ZSBOVUxMLCB5b3UgY291bGQgZW5kIHVwCj4gPiBkZXJlZmVyZW5jaW5nIGEgTlVMTCBwb2ludGVy LiBUaGUgZmFjdCB0aGF0IHRoZSBiYXNlIHN0YXRlIGlzIHRoZSBmaXJzdAo+ID4gZmllbGQgaW4g dGhlIHJjYXJfZHVfY3J0Y19zdGF0ZSBzdHJ1Y3R1cmUgaXMganVzdCBsdWNrLCBhbmQgc2hvdWxk bid0IGJlCj4gPiByZWxpZWQgb24uCj4gCj4gV291bGQgaXQgYmUgb2sgaWYgSSBjaGFuZ2VkIGl0 IHRvIHN0YXRlID8gJnN0YXRlLT5zdGF0ZSA6IE5VTEwgYW5kIGxldAo+IHRoZSBjb21waWxlciBk ZWFsIHdpdGggaXQ/CgpXaGF0J3Mgd3Jvbmcgd2l0aCBhIHByb3BlciBpbXBsZW1lbnRhdGlvbiA/ CgpzdGF0aWMgdm9pZCByY2FyX2R1X2NydGNfcmVzZXQoc3RydWN0IGRybV9jcnRjICpjcnRjKQp7 CglzdHJ1Y3QgcmNhcl9kdV9jcnRjX3N0YXRlICpzdGF0ZTsKCglpZiAoY3J0Yy0+c3RhdGUpIHsK CQlyY2FyX2R1X2NydGNfYXRvbWljX2Rlc3Ryb3lfc3RhdGUoY3J0YywgY3J0Yy0+c3RhdGUpOwoJ CWNydGMtPnN0YXRlID0gTlVMTDsKCX0KCglzdGF0ZSA9IGt6YWxsb2Moc2l6ZW9mKCpzdGF0ZSks IEdGUF9LRVJORUwpOwoJaWYgKHN0YXRlID09IE5VTEwpCgkJcmV0dXJuOwoKCV9fZHJtX2F0b21p Y19oZWxwZXJfY3J0Y19yZXNldChjcnRjLCAmc3RhdGUtPnN0YXRlKTsKCglzdGF0ZS0+Y3JjLnNv dXJjZSA9IFZTUDFfRFVfQ1JDX05PTkU7CglzdGF0ZS0+Y3JjLmluZGV4ID0gMDsKfQoKPiBXaWxs IHByb2JhYmx5IGZpeCB1cCBhbGwgb3RoZXIgcGF0Y2hlcyBhcyB3ZWxsIGJlZm9yZSBjb21taXR0 aW5nLgoKWW91IHdvbid0IGNvbW1pdCB0aGlzIG9uZSBiZWZvcmUgSSBhY2sgaXQsIHJpZ2h0ID8g Oi0pCgo+ID4+IExvb2tpbmcgbW9yZSBjbG9zZWx5LCBpdCdzIHRoZSBzYW1lIHdheSB0aGF0IGVy cm9ycyBpbgo+ID4+IHJjYXJfZHVfcGxhbmVfcmVzZXQoKSBhcmUgaGFuZGxlZC4gOikKPiA+IEl0 J3Mgbm90LCB0aGUgcmV0dXJuIHZhbHVlIG9mIGt6YWxsb2MoKSBpcyBjaGVja2VkIGV4cGxpY2l0 bHkgaW4KPiA+IHJjYXJfZHVfcGxhbmVfcmVzZXQoKSBiZWZvcmUgY2FsbGluZyBfX2RybV9hdG9t aWNfaGVscGVyX3BsYW5lX3Jlc2V0KCkuCj4gPiBQbGVhc2UgY29weSB0aGUgY29kZSBmbG93IG9m IHJjYXJfZHVfcGxhbmVfcmVzZXQoKSB0byBpbXBsZW1lbnQKPiA+IHJjYXJfZHVfY3J0Y19yZXNl dCgpLgoKLS0gClJlZ2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0Cl9fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRl dmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9t YWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbA==