From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Date: Mon, 19 Oct 2015 22:40:39 +0000 Subject: Re: [PATCH] drm: rcar-du: Fix plane state free in plane reset handler Message-Id: <1571374.qXLWBxx5CE@avalon> List-Id: References: <1438820580-14301-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <19460030.7jS9GcTr8h@avalon> <20150825071516.GI20434@phenom.ffwll.local> In-Reply-To: <20150825071516.GI20434@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Daniel Vetter Cc: Daniel Vetter , Laurent Pinchart , dri-devel@lists.freedesktop.org, linux-sh@vger.kernel.org Hi Daniel, On Tuesday 25 August 2015 09:15:16 Daniel Vetter wrote: > On Tue, Aug 18, 2015 at 09:35:44AM +0300, Laurent Pinchart wrote: > > On Friday 14 August 2015 09:30:15 Daniel Vetter wrote: > > > On Fri, Aug 14, 2015 at 12:19:03AM +0300, Laurent Pinchart wrote: > > > > On Friday 07 August 2015 17:30:08 Laurent Pinchart wrote: > > > > > On Friday 07 August 2015 14:53:22 Thierry Reding wrote: > > > > > > On Thu, Aug 06, 2015 at 03:23:00AM +0300, Laurent Pinchart wrote: > > > > > > > The plane reset handler frees the plane state and allocates a > > > > > > > new default state, but when doing so attempt to free the plane > > > > > > > state using the base plane state pointer instead of casting it > > > > > > > to the driver-specific state object that has been allocated. Fix > > > > > > > it by using the rcar_du_plane_atomic_destroy_state() function to > > > > > > > destroy the plane state instead of duplicating the code. > > > > > > > > > > > > > > Signed-off-by: Laurent Pinchart > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 45 +++++++++--------- > > > > > > > 1 file changed, 22 insertions(+), 23 deletions(-) > > > > > > > > > > > > > > Should the DRM core free the atomic state before calling the > > > > > > > reset operation ? That would simplify drivers. > > > > > > > > > > > > The core can't do that because drivers might have subclassed the > > > > > > state. > > > > > > > > > > But the core can call the .atomic_destroy_state() operation, can't > > > > > it ? > > > > > > > > Thierry, Daniel, any comment on this ? > > > > > > Doesn't really help you since the kzalloc is still in the helper. Btw > > > this is all helper code, core won't do here anything at all ;-) > > > > Is it ? The .reset() and .atomic_destroy_state() are core plane > > operations, not helper operations. > > Reset not being a helper func is an accident of history I think, it should > be moved. Fine with me. > > My point is that, as .reset() needs to allocate the state if no state > > exists, I'm wondering whether it wouldn't be simpler for drivers to free > > the state in the core using .atomic_destroy_state() before calling > > .reset() and always allocate a state in the driver's .reset() > > implementation. > > > > In peudo-code, drivers currently do (or at least should do) > > > > atomic_destroy_state(state) > > { > > driver_state = cast_to_driver_state(state); > > > > clean up driver_state; > > kfree(driver_state); > > } > > > > reset() > > { > > if (state) { > > driver_state = cast_to_driver_state(state); > > > > clean up driver_state; > > Why not call destroy_state here and make the kzalloc unconditional? > Simpler and with that not much point in removing copypasting ... So all drivers would have to unconditionally call their atomic_destroy_state() handler at the beginning of reset() ? Wouldn't it be simpler to move that call in the helpers before calling reset() ? > > } else { > > driver_state = kzalloc(...); > > } > > > > set all fields of driver_state to default values; > > } > > > > Wouldn't it be simpler to have the core call .atomic_destroy_state() > > before .reset() and implement .reset() as > > > > reset() > > driver_state = kzalloc(...); > > > > set all fields of driver_state to default values; > > } > > > > ? > > Well all the reset stuff was pretty much stop-gap, ->reset really > shouldn't be a core op. What I eventually wanted to do is lift the hw > state readout logic from i915 as the proper way to do this, since without > this you can't do fastboot. Eric is interested in fastboot for vc4, so we > discussed this a bit at lpc. -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH] drm: rcar-du: Fix plane state free in plane reset handler Date: Tue, 20 Oct 2015 01:40:39 +0300 Message-ID: <1571374.qXLWBxx5CE@avalon> References: <1438820580-14301-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <19460030.7jS9GcTr8h@avalon> <20150825071516.GI20434@phenom.ffwll.local> 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 [185.26.127.97]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7A35E6E66F for ; Mon, 19 Oct 2015 15:40:44 -0700 (PDT) In-Reply-To: <20150825071516.GI20434@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Daniel Vetter , Laurent Pinchart , dri-devel@lists.freedesktop.org, linux-sh@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org SGkgRGFuaWVsLAoKT24gVHVlc2RheSAyNSBBdWd1c3QgMjAxNSAwOToxNToxNiBEYW5pZWwgVmV0 dGVyIHdyb3RlOgo+IE9uIFR1ZSwgQXVnIDE4LCAyMDE1IGF0IDA5OjM1OjQ0QU0gKzAzMDAsIExh dXJlbnQgUGluY2hhcnQgd3JvdGU6Cj4gPiBPbiBGcmlkYXkgMTQgQXVndXN0IDIwMTUgMDk6MzA6 MTUgRGFuaWVsIFZldHRlciB3cm90ZToKPiA+ID4gT24gRnJpLCBBdWcgMTQsIDIwMTUgYXQgMTI6 MTk6MDNBTSArMDMwMCwgTGF1cmVudCBQaW5jaGFydCB3cm90ZToKPiA+ID4gPiBPbiBGcmlkYXkg MDcgQXVndXN0IDIwMTUgMTc6MzA6MDggTGF1cmVudCBQaW5jaGFydCB3cm90ZToKPiA+ID4gPiA+ IE9uIEZyaWRheSAwNyBBdWd1c3QgMjAxNSAxNDo1MzoyMiBUaGllcnJ5IFJlZGluZyB3cm90ZToK PiA+ID4gPiA+ID4gT24gVGh1LCBBdWcgMDYsIDIwMTUgYXQgMDM6MjM6MDBBTSArMDMwMCwgTGF1 cmVudCBQaW5jaGFydCB3cm90ZToKPiA+ID4gPiA+ID4gPiBUaGUgcGxhbmUgcmVzZXQgaGFuZGxl ciBmcmVlcyB0aGUgcGxhbmUgc3RhdGUgYW5kIGFsbG9jYXRlcyBhCj4gPiA+ID4gPiA+ID4gbmV3 IGRlZmF1bHQgc3RhdGUsIGJ1dCB3aGVuIGRvaW5nIHNvIGF0dGVtcHQgdG8gZnJlZSB0aGUgcGxh bmUKPiA+ID4gPiA+ID4gPiBzdGF0ZSB1c2luZyB0aGUgYmFzZSBwbGFuZSBzdGF0ZSBwb2ludGVy IGluc3RlYWQgb2YgY2FzdGluZyBpdAo+ID4gPiA+ID4gPiA+IHRvIHRoZSBkcml2ZXItc3BlY2lm aWMgc3RhdGUgb2JqZWN0IHRoYXQgaGFzIGJlZW4gYWxsb2NhdGVkLiBGaXgKPiA+ID4gPiA+ID4g PiBpdCBieSB1c2luZyB0aGUgcmNhcl9kdV9wbGFuZV9hdG9taWNfZGVzdHJveV9zdGF0ZSgpIGZ1 bmN0aW9uIHRvCj4gPiA+ID4gPiA+ID4gZGVzdHJveSB0aGUgcGxhbmUgc3RhdGUgaW5zdGVhZCBv ZiBkdXBsaWNhdGluZyB0aGUgY29kZS4KPiA+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gPiBTaWdu ZWQtb2ZmLWJ5OiBMYXVyZW50IFBpbmNoYXJ0Cj4gPiA+ID4gPiA+ID4gPGxhdXJlbnQucGluY2hh cnQrcmVuZXNhc0BpZGVhc29uYm9hcmQuY29tPgo+ID4gPiA+ID4gPiA+IC0tLQo+ID4gPiA+ID4g PiA+IAo+ID4gPiA+ID4gPiA+ICBkcml2ZXJzL2dwdS9kcm0vcmNhci1kdS9yY2FyX2R1X3BsYW5l LmMgfCA0NSArKysrKysrKystLS0tLS0tLS0KPiA+ID4gPiA+ID4gPiAgMSBmaWxlIGNoYW5nZWQs IDIyIGluc2VydGlvbnMoKyksIDIzIGRlbGV0aW9ucygtKQo+ID4gPiA+ID4gPiA+IAo+ID4gPiA+ ID4gPiA+IFNob3VsZCB0aGUgRFJNIGNvcmUgZnJlZSB0aGUgYXRvbWljIHN0YXRlIGJlZm9yZSBj YWxsaW5nIHRoZQo+ID4gPiA+ID4gPiA+IHJlc2V0IG9wZXJhdGlvbiA/IFRoYXQgd291bGQgc2lt cGxpZnkgZHJpdmVycy4KPiA+ID4gPiA+ID4gCj4gPiA+ID4gPiA+IFRoZSBjb3JlIGNhbid0IGRv IHRoYXQgYmVjYXVzZSBkcml2ZXJzIG1pZ2h0IGhhdmUgc3ViY2xhc3NlZCB0aGUKPiA+ID4gPiA+ ID4gc3RhdGUuCj4gPiA+ID4gPiAKPiA+ID4gPiA+IEJ1dCB0aGUgY29yZSBjYW4gY2FsbCB0aGUg LmF0b21pY19kZXN0cm95X3N0YXRlKCkgb3BlcmF0aW9uLCBjYW4ndAo+ID4gPiA+ID4gaXQgPwo+ ID4gPiA+IAo+ID4gPiA+IFRoaWVycnksIERhbmllbCwgYW55IGNvbW1lbnQgb24gdGhpcyA/Cj4g PiA+IAo+ID4gPiBEb2Vzbid0IHJlYWxseSBoZWxwIHlvdSBzaW5jZSB0aGUga3phbGxvYyBpcyBz dGlsbCBpbiB0aGUgaGVscGVyLiBCdHcKPiA+ID4gdGhpcyBpcyBhbGwgaGVscGVyIGNvZGUsIGNv cmUgd29uJ3QgZG8gaGVyZSBhbnl0aGluZyBhdCBhbGwgOy0pCj4gPiAKPiA+IElzIGl0ID8gVGhl IC5yZXNldCgpIGFuZCAuYXRvbWljX2Rlc3Ryb3lfc3RhdGUoKSBhcmUgY29yZSBwbGFuZQo+ID4g b3BlcmF0aW9ucywgbm90IGhlbHBlciBvcGVyYXRpb25zLgo+IAo+IFJlc2V0IG5vdCBiZWluZyBh IGhlbHBlciBmdW5jIGlzIGFuIGFjY2lkZW50IG9mIGhpc3RvcnkgSSB0aGluaywgaXQgc2hvdWxk Cj4gYmUgbW92ZWQuCgpGaW5lIHdpdGggbWUuCgo+ID4gTXkgcG9pbnQgaXMgdGhhdCwgYXMgLnJl c2V0KCkgbmVlZHMgdG8gYWxsb2NhdGUgdGhlIHN0YXRlIGlmIG5vIHN0YXRlCj4gPiBleGlzdHMs IEknbSB3b25kZXJpbmcgd2hldGhlciBpdCB3b3VsZG4ndCBiZSBzaW1wbGVyIGZvciBkcml2ZXJz IHRvIGZyZWUKPiA+IHRoZSBzdGF0ZSBpbiB0aGUgY29yZSB1c2luZyAuYXRvbWljX2Rlc3Ryb3lf c3RhdGUoKSBiZWZvcmUgY2FsbGluZwo+ID4gLnJlc2V0KCkgYW5kIGFsd2F5cyBhbGxvY2F0ZSBh IHN0YXRlIGluIHRoZSBkcml2ZXIncyAucmVzZXQoKQo+ID4gaW1wbGVtZW50YXRpb24uCj4gPiAK PiA+IEluIHBldWRvLWNvZGUsIGRyaXZlcnMgY3VycmVudGx5IGRvIChvciBhdCBsZWFzdCBzaG91 bGQgZG8pCj4gPiAKPiA+IGF0b21pY19kZXN0cm95X3N0YXRlKHN0YXRlKQo+ID4gewo+ID4gCWRy aXZlcl9zdGF0ZSA9IGNhc3RfdG9fZHJpdmVyX3N0YXRlKHN0YXRlKTsKPiA+IAkKPiA+IAljbGVh biB1cCBkcml2ZXJfc3RhdGU7Cj4gPiAJa2ZyZWUoZHJpdmVyX3N0YXRlKTsKPiA+IH0KPiA+IAo+ ID4gcmVzZXQoKQo+ID4gewo+ID4gCWlmIChzdGF0ZSkgewo+ID4gCQlkcml2ZXJfc3RhdGUgPSBj YXN0X3RvX2RyaXZlcl9zdGF0ZShzdGF0ZSk7Cj4gPiAJCQo+ID4gCQljbGVhbiB1cCBkcml2ZXJf c3RhdGU7Cj4gCj4gV2h5IG5vdCBjYWxsIGRlc3Ryb3lfc3RhdGUgaGVyZSBhbmQgbWFrZSB0aGUg a3phbGxvYyB1bmNvbmRpdGlvbmFsPwo+IFNpbXBsZXIgYW5kIHdpdGggdGhhdCBub3QgbXVjaCBw b2ludCBpbiByZW1vdmluZyBjb3B5cGFzdGluZyAuLi4KClNvIGFsbCBkcml2ZXJzIHdvdWxkIGhh dmUgdG8gdW5jb25kaXRpb25hbGx5IGNhbGwgdGhlaXIgYXRvbWljX2Rlc3Ryb3lfc3RhdGUoKSAK aGFuZGxlciBhdCB0aGUgYmVnaW5uaW5nIG9mIHJlc2V0KCkgPyBXb3VsZG4ndCBpdCBiZSBzaW1w bGVyIHRvIG1vdmUgdGhhdCBjYWxsIAppbiB0aGUgaGVscGVycyBiZWZvcmUgY2FsbGluZyByZXNl dCgpID8KCj4gPiAJfSBlbHNlIHsKPiA+IAkJZHJpdmVyX3N0YXRlID0ga3phbGxvYyguLi4pOwo+ ID4gCX0KPiA+IAkKPiA+IAlzZXQgYWxsIGZpZWxkcyBvZiBkcml2ZXJfc3RhdGUgdG8gZGVmYXVs dCB2YWx1ZXM7Cj4gPiB9Cj4gPiAKPiA+IFdvdWxkbid0IGl0IGJlIHNpbXBsZXIgdG8gaGF2ZSB0 aGUgY29yZSBjYWxsIC5hdG9taWNfZGVzdHJveV9zdGF0ZSgpCj4gPiBiZWZvcmUgLnJlc2V0KCkg YW5kIGltcGxlbWVudCAucmVzZXQoKSBhcwo+ID4gCj4gPiByZXNldCgpCj4gPiAJZHJpdmVyX3N0 YXRlID0ga3phbGxvYyguLi4pOwo+ID4gCQo+ID4gCXNldCBhbGwgZmllbGRzIG9mIGRyaXZlcl9z dGF0ZSB0byBkZWZhdWx0IHZhbHVlczsKPiA+IH0KPiA+IAo+ID4gPwo+IAo+IFdlbGwgYWxsIHRo ZSByZXNldCBzdHVmZiB3YXMgcHJldHR5IG11Y2ggc3RvcC1nYXAsIC0+cmVzZXQgcmVhbGx5Cj4g c2hvdWxkbid0IGJlIGEgY29yZSBvcC4gV2hhdCBJIGV2ZW50dWFsbHkgd2FudGVkIHRvIGRvIGlz IGxpZnQgdGhlIGh3Cj4gc3RhdGUgcmVhZG91dCBsb2dpYyBmcm9tIGk5MTUgYXMgdGhlIHByb3Bl ciB3YXkgdG8gZG8gdGhpcywgc2luY2Ugd2l0aG91dAo+IHRoaXMgeW91IGNhbid0IGRvIGZhc3Ri b290LiBFcmljIGlzIGludGVyZXN0ZWQgaW4gZmFzdGJvb3QgZm9yIHZjNCwgc28gd2UKPiBkaXNj dXNzZWQgdGhpcyBhIGJpdCBhdCBscGMuCgotLSAKUmVnYXJkcywKCkxhdXJlbnQgUGluY2hhcnQK Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZl bCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg==