From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. Date: Tue, 08 Aug 2017 00:54:59 +0300 Message-ID: <1623957.7egAK3pKHU@avalon> References: <20170718210510.12229-1-eric@anholt.net> <30057693.2anM9zYF0F@avalon> <20170807145939.bx23xyejkcfp76ze@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 BA4D989EB8 for ; Mon, 7 Aug 2017 21:54:45 +0000 (UTC) In-Reply-To: <20170807145939.bx23xyejkcfp76ze@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: Greg Kroah-Hartman , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , Thierry Reding , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org SGkgRGFuaWVsLAoKT24gTW9uZGF5IDA3IEF1ZyAyMDE3IDE2OjU5OjM5IERhbmllbCBWZXR0ZXIg d3JvdGU6Cj4gT24gTW9uLCBBdWcgMDcsIDIwMTcgYXQgMDE6MjI6MjNQTSArMDMwMCwgTGF1cmVu dCBQaW5jaGFydCB3cm90ZToKPiA+IE9uIE1vbmRheSAwNyBBdWcgMjAxNyAxMToyNTowNyBEYW5p ZWwgVmV0dGVyIHdyb3RlOgo+ID4+IE9uIFNhdCwgQXVnIDA1LCAyMDE3IGF0IDEyOjU5OjA3UE0g KzAyMDAsIE5vcmFsZiBUcsO4bm5lcyB3cm90ZToKPiA+Pj4gRGVuIDA1LjA4LjIwMTcgMDAuMTks IHNrcmV2IElsaWEgTWlya2luOgo+ID4+Pj4gT24gRnJpLCBBdWcgNCwgMjAxNyBhdCA0OjQzIFBN LCBFcmljIEFuaG9sdCA8ZXJpY0BhbmhvbHQubmV0PiB3cm90ZToKPiA+Pj4+PiBMYXVyZW50IFBp bmNoYXJ0IDxsYXVyZW50LnBpbmNoYXJ0QGlkZWFzb25ib2FyZC5jb20+IHdyaXRlczoKPiA+Pj4+ Pj4gT24gVHVlc2RheSAxOCBKdWwgMjAxNyAxNDowNTowNiBFcmljIEFuaG9sdCB3cm90ZToKPiA+ Pj4+Pj4+IFRoaXMgd2lsbCBsZXQgZHJpdmVycyByZWR1Y2UgdGhlIGVycm9yIGNsZWFudXAgdGhl eSBuZWVkLCBpbgo+ID4+Pj4+Pj4gcGFydGljdWxhciB0aGUgImlzX3BhbmVsX2JyaWRnZSIgZmxh Zy4KPiA+Pj4+Pj4+IAo+ID4+Pj4+Pj4gdjI6IFNsaWdodCBjbGVhbnVwIG9mIHJlbW92ZSBmdW5j dGlvbiBieSBBbmRyemVqCj4gPj4+Pj4+IAo+ID4+Pj4+PiBJIGp1c3Qgd2FudCB0byBwb2ludCBv dXQgdGhhdCwgaW4gdGhlIGNvbnRleHQgb2YgRGFuaWVsJ3Mgd29yayBvbgo+ID4+Pj4+PiBob3Qt dW5wbHVnLCA5MCUgb2YgdGhlIGRldm1fKiBhbGxvY2F0aW9ucyBhcmUgd3JvbmcgYW5kIHdpbGwg Z2V0IGluCj4gPj4+Pj4+IHRoZSB3YXkuIEFsbCBEUk0gY29yZSBvYmplY3RzIHRoYXQgYXJlIGFj Y2Vzc2libGUgb25lIHdheSBvcgo+ID4+Pj4+PiBhbm90aGVyIGZyb20gdXNlcnNwYWNlIHdpbGwg bmVlZCB0byBiZSBwcm9wZXJseSByZWZlcmVuY2UtY291bnRlZAo+ID4+Pj4+PiBhbmQgZnJlZWQg b25seSB3aGVuIHRoZSBsYXN0IHJlZmVyZW5jZSBkaXNhcHBlYXJzLCB3aGljaCBjb3VsZCBiZQo+ ID4+Pj4+PiB3ZWxsIGFmdGVyIHRoZSBjb3JyZXNwb25kaW5nIGRldmljZSBpcyByZW1vdmVkLiBJ IGJlbGlldmUgdGhpcwo+ID4+Pj4+PiBjb3VsZCBiZSBvbmUgc3VjaCBvYmplY3RzIDotLwo+ID4+ Pj4+IAo+ID4+Pj4+IFN1cmUsIGlmIHlvdSdyZSBob3RwbHVnZ2luZywgeW91ciBsaWZlIGlzIHBh aW4uICBGb3IKPiA+Pj4+PiBub24taG90cGx1Z2dhYmxlIGRldmljZXMsIGxpa2Ugb3VyIFNPQyBw bGF0Zm9ybSBkZXZpY2VzIChjdXJyZW50Cj4gPj4+Pj4gcGFuZWwtYnJpZGdlIGNvbnN1bWVycyks IHRoaXMgc3RpbGwgc2VlbXMgbGlrZSBhbiBleGNlbGxlbnQKPiA+Pj4+PiBzaW1wbGlmaWNhdGlv biBvZiBtZW1vcnkgbWFuYWdlbWVudC4KPiA+Pj4+IAo+ID4+Pj4gQXQgdGhhdCBwb2ludCB5b3Ug bWF5IGFzIHdlbGwgbWFrZSB5b3VyIG1vZHVsZSBub24tdW5sb2FkYWJsZSwgYW5kCj4gPj4+PiBy ZXR1cm4gZmFpbHVyZSB3aGVuIHRyeWluZyB0byByZW1vdmUgYSBkZXZpY2UgZnJvbSBtYW5hZ2Vt ZW50IGJ5IHRoZQo+ID4+Pj4gZHJpdmVyICh3aGF0ZXZlciB0aGUgb3Bwb3NpdGUgb2YgInByb2Jl IiBpcywgSSBmb3JnZXQpLiBIb3RwbHVnZ2luZwo+ID4+Pj4gZG9lc24ndCBvbmx5IGhhcHBlbiB3 aGVuIHBoeXNpY2FsbHkgcmVtb3ZpbmcsIGl0IGNhbiBoYXBwZW4gZm9yIGFsbAo+ID4+Pj4ga2lu ZHMgb2YgcmVhc29ucy4uLiBhbmQgdXNlcnNwYWNlIG1heSBzdGlsbCBob2xkIHJlZmVyZW5jZXMg aW4gc29tZQo+ID4+Pj4gb2YgdGhvc2UgY2FzZXMuCj4gPj4+IAo+ID4+PiBJZiBkcm1fb3Blbigp IGdldHMgYSByZWYgb24gZGV2LT5kZXYgYW5kIHB1dHMgaXQgaW4gZHJtX3JlbGVhc2UoKSwKPiA+ Pj4gd29uJ3QgdGhhdCBkZWxheSBkZXZtXyogY2xlYW51cCB1bnRpbCB1c2Vyc3BhY2UgaXMgZG9u ZT8KPiA+PiAKPiA+PiBOby4gZHJtX2RldmljZSBpcyB0aGUgdGhpbmcgdGhhdCBpcyByZWZjb3Vu dGVkIGZvciB1c2Vyc3BhY2UgcmVmZXJlbmNlcwo+ID4+IGxpa2Ugb3BlbiBGRCAod2UncmUgbm90 IHBlcmZlY3QgYWJvdXQgaXQsIGUuZy4gc3lzZnMgYW5kIGRtYS1idWYvZmVuY2UKPiA+PiBkb24n dCkuCj4gPj4gCj4gPj4gZGV2bV8gb3RvaCBpcyB0aWVkIHRvIHRoZSBsaWZldGltZSBvZiB0aGUg dW5kZXJseWluZyBkZXZpY2UsIGFuZCB0aGF0Cj4gPj4gb25lIGNhbiBnZXQgb3V0bGl2ZWQgYnkg ZHJtX2RldmljZS4gT3IgYXQgbGVhc3QgYWZhaXVpLCBkZXZtXyBzdHVmZiBpcwo+ID4+IG51a2Vk IG9uIHVucGx1ZywgYW5kIG5vdCB3aGVuIHRoZSBmaW5hbCBzdyByZWZlcmVuY2Ugb2YgdGhlIHN0 cnVjdCBkZXZpY2UKPiA+PiBkaXNhcHBlYXJzLgo+ID4+IAo+ID4+IE5vdCBzdXJlIHRvdWdoLCBp dCdzIGNvbXBsaWNhdGVkLgo+ID4gCj4gPiBJdCdzIGNvbXBsaWNhdGVkIHdoZW4geW91IGhhdmUg dG8gdW5kZXJzdGFuZCB0aGUgYmVoYXZpb3VyIGJ5IHJlYWRpbmcgdGhlCj4gPiBjb2RlLCBidXQg dGhlIGJlaGF2aW91ciBpc24ndCB0aGF0IGNvbXBsZXguIGRldm0gcmVzb3VyY2VzIGFyZSByZWxl YXNlZAo+ID4gYm90aAo+ID4gCj4gPiAxLiByaWdodCBhZnRlciB0aGUgZHJpdmVyJ3MgLnJlbW92 ZSgpIG9wZXJhdGlvbiByZXR1cm5zCj4gPiAyLiB3aGVuIHRoZSBkZXZpY2UgaXMgZGVsZXRlZCAo bGFzdCByZWZlcmVuY2UgcmVsZWFzZWQpCj4gCj4gUmlnaHQsIEkgaGFkIHZhZ3VlIG1lbW9yaWVz IHdoZW4gcmVhZGluZyB0aGUgY29kZSAuLi4gQnV0IGlpcmMgdGhlcmUncwo+IGFsc28gc29tZSB3 YXkgdG8gZGVsYXkgY2xlYW51cCB1bnRpbCBpdCdzIGRlbGV0ZWQsIG1heWJlIHdlIGNvdWxkIGV4 cGxvaXQKPiB0aGF0IChhbmQgd3JhcCBpdCB1cCBpbnRvIGEgZHJtX2Rldm1fYWRkIHdyYXBwZXIp Pwo+IAo+IFBsYW4gQiwgYnV0IHRoYXQgaXMgYSBsb3QgbW9yZSB1Z2x5LCB3b3VsZCBiZSB0byBj cmVhdGUgYSBmYWtlIHN0cnVjdAo+IGRldmljZSB0aGF0IHdlIG5ldmVyIGJpbmQgKGhlbmNlIHJl bW92ZSBjYW4ndCBiZSBjYWxsZWQpIGFuZCB1c2UgdG8gdHJhY2sKPiB0aGUgbGlmZXRpbWUgb2Yg ZHJtX2RldmljZSBzdHVmZi4gV291bGQgYXZvaWQgcmUtaW52ZW50aW5nIGFsbCB0aGUgZGV2bV8K PiBmdW5jdGlvbnMsIGJ1dCB3b3VsZCBhbHNvIHJlc3VsdCBpbiBhIGR1bW15IGRldmljZSBpbiBz eXNmcy4KCklmIHdlIHdhbnRlZCB0byBnbyB0aGF0IHdheSwgd2Ugc2hvdWxkIGRlY291cGxlIGRl dm1fKiBmcm9tIHN0cnVjdCBkZXZpY2UgKG9yIApldmVuIHN0cnVjdCBrb2JqZWN0KSBhbmQgdGll IGl0IHRvIGEgbmV3IHJlc291cmNlIHRyYWNrZXIgc3RydWN0dXJlLiBUaGUgCmN1cnJlbnQgZGV2 bV8qIEFQSSB3b3VsZCByZW1haW4gdGhlIHNhbWUsIGVtYmVkZGluZyB0aGF0IHJlc291cmNlIHRy YWNrZXIgCm9iamVjdCBpbiBzdHJ1Y3QgZGV2aWNlLCBidXQgd2UgY291bGQgYWxzbyB1c2UgdGhl IG1hY2hpbmVyeSB3aXRoIHRoZSByZXNvdXJjZSAKdHJhY2tlciBvYmplY3QgZGlyZWN0bHkuCgpI b3dldmVyLCBJJ20gbm90IGNvbnZpbmNlIHdlIHNob3VsZCBkbyBzby4gV2UgZG9uJ3QgaGF2ZSBh bnkgYXV0b21hdGljIG1lbW9yeSAKYWxsb2NhdGlvbiBzeXN0ZW0gZm9yIERSTSBvYmplY3RzIChz dWNoIGFzIGZyYW1lYnVmZmVycyBvciBwbGFuZXMpLiBJbnN0ZWFkLCAKZHJpdmVycyBtdXN0IGlt cGxlbWVudCBhIGRlc3Ryb3kgb3BlcmF0aW9uIGZvciB0aGUgb2JqZWN0LCBhbmQgdGhlIGNvcmUg Y2FsbHMgCml0IHdoZW4gdGhlIGxhc3QgcmVmZXJlbmNlIHRvIHRoZSBvYmplY3QgZ29lcyBhd2F5 LiBUaGlzIHdvcmtzIGZpbmUsIGFuZCBJIApkb24ndCBzZWUgd2hhdCB3b3VsZCBwcmV2ZW50IHVz ZSBmcm9tIGltcGxlbWVudGluZyB0aGUgc2FtZSBtZWNoYW5pc20gZm9yIApicmlkZ2VzIG9yIG90 aGVyIHNpbWlsYXIgc3RydWN0dXJlcy4KCj4gV2h5IGlzIHRoaXMgc3R1ZmYgdGllZCB0byBzdHJ1 Y3QgZGV2aWNlIGFuZCBub3Qgc3RydWN0IGtvYmplY3QgYW55d2F5LiBPcgo+IGF0IGxlYXN0IHNv bWV0aGluZyB3ZSBjb3VsZCBlbWJlZCBpbiByYW5kb20gY29vbCBwbGFjZSAobGlrZSBkcm1fZGV2 aWNlKS4KPiAKPiBHcmVnLCBhbnkgaWRlYXMgaG93IHdlIGNvdWxkIHNpbXBsaWZ5IG1hbmFnZW1l bnQgb2Ygc3R1ZmYgdGhhdCdzIGNyZWF0ZWQKPiBhdCBkcml2ZXIgbG9hZCwgYnV0IGl0cyBsaWZl dGltZSB0aWVkIHRvIHNvbWV0aGluZyB3aGljaCBpc24ndCBkaXJlY3RseSBhCj4gc3RydWN0IGRl dmljZT8KPiAKPiA+IEl0J3MgdGhlIGZpcnN0IG9uZSB0aGF0IG1ha2VzIGRldm1fKiBhbGxvY2F0 aW9uIHVuc3VpdGFibGUgZm9yIGFueQo+ID4gc3RydWN0dXJlIHRoYXQgaXMgYWNjZXNzaWJsZSBm cm9tIHVzZXJzcGFjZSAoc3VjaCBhcyBpbiBmaWxlIG9wZXJhdGlvbgo+ID4gaGFuZGxlcnMpLgo+ IAo+IFllYWgsIGlmIHdlIGNvdWxkIHJlbGVhc2UgYXQgMi4gaXQgd291bGQgYmUgcGVyZmVjdCBJ IHRoaW5rIC4uLgoKSXQgd291bGRuJ3QsIGFzIHVuYmluZC9yZWJpbmQgY3ljbGVzIHdvdWxkIGFs bG9jYXRlIG5ldyBvYmplY3RzIGVhY2ggdGltZSAKd2l0aG91dCBmcmVlaW5nIHRoZSBwcmV2aW91 cyBvbmVzIHVudGlsIHRoZSBkZXZpY2UgaXMgZGVsZXRlZC4KCi0tIApSZWdhcmRzLAoKTGF1cmVu dCBQaW5jaGFydAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3Jn Cmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751950AbdHGVyq (ORCPT ); Mon, 7 Aug 2017 17:54:46 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:42743 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751662AbdHGVyp (ORCPT ); Mon, 7 Aug 2017 17:54:45 -0400 From: Laurent Pinchart To: Daniel Vetter Cc: Greg Kroah-Hartman , Noralf =?ISO-8859-1?Q?Tr=F8nnes?= , Ilia Mirkin , Eric Anholt , Daniel Vetter , Thierry Reding , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" Subject: Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. Date: Tue, 08 Aug 2017 00:54:59 +0300 Message-ID: <1623957.7egAK3pKHU@avalon> User-Agent: KMail/4.14.10 (Linux/4.9.34-gentoo; KDE/4.14.32; x86_64; ; ) In-Reply-To: <20170807145939.bx23xyejkcfp76ze@phenom.ffwll.local> References: <20170718210510.12229-1-eric@anholt.net> <30057693.2anM9zYF0F@avalon> <20170807145939.bx23xyejkcfp76ze@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v77Lsqno008037 Hi Daniel, On Monday 07 Aug 2017 16:59:39 Daniel Vetter wrote: > On Mon, Aug 07, 2017 at 01:22:23PM +0300, Laurent Pinchart wrote: > > On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote: > >> On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote: > >>> Den 05.08.2017 00.19, skrev Ilia Mirkin: > >>>> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt wrote: > >>>>> Laurent Pinchart writes: > >>>>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote: > >>>>>>> This will let drivers reduce the error cleanup they need, in > >>>>>>> particular the "is_panel_bridge" flag. > >>>>>>> > >>>>>>> v2: Slight cleanup of remove function by Andrzej > >>>>>> > >>>>>> I just want to point out that, in the context of Daniel's work on > >>>>>> hot-unplug, 90% of the devm_* allocations are wrong and will get in > >>>>>> the way. All DRM core objects that are accessible one way or > >>>>>> another from userspace will need to be properly reference-counted > >>>>>> and freed only when the last reference disappears, which could be > >>>>>> well after the corresponding device is removed. I believe this > >>>>>> could be one such objects :-/ > >>>>> > >>>>> Sure, if you're hotplugging, your life is pain. For > >>>>> non-hotpluggable devices, like our SOC platform devices (current > >>>>> panel-bridge consumers), this still seems like an excellent > >>>>> simplification of memory management. > >>>> > >>>> At that point you may as well make your module non-unloadable, and > >>>> return failure when trying to remove a device from management by the > >>>> driver (whatever the opposite of "probe" is, I forget). Hotplugging > >>>> doesn't only happen when physically removing, it can happen for all > >>>> kinds of reasons... and userspace may still hold references in some > >>>> of those cases. > >>> > >>> If drm_open() gets a ref on dev->dev and puts it in drm_release(), > >>> won't that delay devm_* cleanup until userspace is done? > >> > >> No. drm_device is the thing that is refcounted for userspace references > >> like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence > >> don't). > >> > >> devm_ otoh is tied to the lifetime of the underlying device, and that > >> one can get outlived by drm_device. Or at least afaiui, devm_ stuff is > >> nuked on unplug, and not when the final sw reference of the struct device > >> disappears. > >> > >> Not sure tough, it's complicated. > > > > It's complicated when you have to understand the behaviour by reading the > > code, but the behaviour isn't that complex. devm resources are released > > both > > > > 1. right after the driver's .remove() operation returns > > 2. when the device is deleted (last reference released) > > Right, I had vague memories when reading the code ... But iirc there's > also some way to delay cleanup until it's deleted, maybe we could exploit > that (and wrap it up into a drm_devm_add wrapper)? > > Plan B, but that is a lot more ugly, would be to create a fake struct > device that we never bind (hence remove can't be called) and use to track > the lifetime of drm_device stuff. Would avoid re-inventing all the devm_ > functions, but would also result in a dummy device in sysfs. If we wanted to go that way, we should decouple devm_* from struct device (or even struct kobject) and tie it to a new resource tracker structure. The current devm_* API would remain the same, embedding that resource tracker object in struct device, but we could also use the machinery with the resource tracker object directly. However, I'm not convince we should do so. We don't have any automatic memory allocation system for DRM objects (such as framebuffers or planes). Instead, drivers must implement a destroy operation for the object, and the core calls it when the last reference to the object goes away. This works fine, and I don't see what would prevent use from implementing the same mechanism for bridges or other similar structures. > Why is this stuff tied to struct device and not struct kobject anyway. Or > at least something we could embed in random cool place (like drm_device). > > Greg, any ideas how we could simplify management of stuff that's created > at driver load, but its lifetime tied to something which isn't directly a > struct device? > > > It's the first one that makes devm_* allocation unsuitable for any > > structure that is accessible from userspace (such as in file operation > > handlers). > > Yeah, if we could release at 2. it would be perfect I think ... It wouldn't, as unbind/rebind cycles would allocate new objects each time without freeing the previous ones until the device is deleted. -- Regards, Laurent Pinchart