From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged Date: Mon, 2 Dec 2019 10:50:44 +0100 Message-ID: <20191202105044.108549fa@collabora.com> References: <20191129135908.2439529-1-boris.brezillon@collabora.com> <20191129135908.2439529-7-boris.brezillon@collabora.com> <20191129201213.GR624164@phenom.ffwll.local> <20191129220924.7982a350@collabora.com> <20191202085243.GX624164@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20191202085243.GX624164@phenom.ffwll.local> Sender: stable-owner@vger.kernel.org To: Daniel Vetter Cc: Rob Herring , Tomeu Vizoso , Alyssa Rosenzweig , Steven Price , stable@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Mon, 2 Dec 2019 09:52:43 +0100 Daniel Vetter wrote: > On Fri, Nov 29, 2019 at 10:09:24PM +0100, Boris Brezillon wrote: > > On Fri, 29 Nov 2019 21:12:13 +0100 > > Daniel Vetter wrote: > > > > > On Fri, Nov 29, 2019 at 02:59:06PM +0100, Boris Brezillon wrote: > > > > We don't want imported/exported BOs to be purges, as those are shared > > > > with other processes that might still use them. We should also refuse > > > > to export a BO if it's been marked purgeable or has already been purged. > > > > > > > > Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support") > > > > Cc: > > > > Signed-off-by: Boris Brezillon > > > > --- > > > > drivers/gpu/drm/panfrost/panfrost_drv.c | 19 ++++++++++++++++- > > > > drivers/gpu/drm/panfrost/panfrost_gem.c | 27 +++++++++++++++++++++++++ > > > > 2 files changed, 45 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c > > > > index 1c67ac434e10..751df975534f 100644 > > > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > > > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > > > > @@ -343,6 +343,7 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data, > > > > struct drm_panfrost_madvise *args = data; > > > > struct panfrost_device *pfdev = dev->dev_private; > > > > struct drm_gem_object *gem_obj; > > > > + int ret; > > > > > > > > gem_obj = drm_gem_object_lookup(file_priv, args->handle); > > > > if (!gem_obj) { > > > > @@ -350,6 +351,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data, > > > > return -ENOENT; > > > > } > > > > > > > > + /* > > > > + * We don't want to mark exported/imported BOs as purgeable: we're not > > > > + * the only owner in that case. > > > > + */ > > > > + mutex_lock(&dev->object_name_lock); > > > > > > Kinda not awesome that you have to take this core lock here and encumber > > > core drm locking with random driver stuff. > > > > Looks like drm_gem_shmem_is_purgeable() already does the !imported && > > !exported check. For the imported case, I think we're good, since > > userspace can't change the madv value before ->import_attach has been > > set. For the exporter case, we need to make sure there's no race > > between the export and madvise operations, which I can probably do from > > the gem->export() hook by taking the shrinker or bo->mappings lock. Okay, I tried that, and I actually need an extra panfrost_gem_object->exported field that's set to true from the ->export() hook with the mappings lock held, otherwise the code is still racy (->dma_buf is assigned after ->export() returns). > > > > > > > > Can't this be solved with your own locking only and some reasonable > > > ordering of checks? big locks because it's easy is endless long-term pain. > > > > > > Also exporting purgeable objects is kinda a userspace bug, all you have to > > > do is not oops in dma_buf_attachment_map. No need to prevent the damage > > > here imo. > > > > I feel like making sure an exported BO can't be purged or a purged BO > > can't be exported would be much simpler than making sure all importers > > are ready to have the sgt freed. > > If you free the sgt while someone is using it, that's kinda a different > bug I think. You already have a pages refcount, that should be enough to > prevent this? My bad, I thought drm_gem_shmem_get_pages_sgt() was used as the ->get_sg_table() implem, but we actually use drm_gem_shmem_get_sg_table() which allocates a new sgt. I still need to make sure we're safe against sgt destruction in panfrost. 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.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_2 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 1030EC432C0 for ; Mon, 2 Dec 2019 09:50:52 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DD60620661 for ; Mon, 2 Dec 2019 09:50:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DD60620661 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=collabora.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C907E89954; Mon, 2 Dec 2019 09:50:50 +0000 (UTC) Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7A55289954 for ; Mon, 2 Dec 2019 09:50:48 +0000 (UTC) Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: bbrezillon) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id DE55E28DE03; Mon, 2 Dec 2019 09:50:46 +0000 (GMT) Date: Mon, 2 Dec 2019 10:50:44 +0100 From: Boris Brezillon To: Daniel Vetter Subject: Re: [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged Message-ID: <20191202105044.108549fa@collabora.com> In-Reply-To: <20191202085243.GX624164@phenom.ffwll.local> References: <20191129135908.2439529-1-boris.brezillon@collabora.com> <20191129135908.2439529-7-boris.brezillon@collabora.com> <20191129201213.GR624164@phenom.ffwll.local> <20191129220924.7982a350@collabora.com> <20191202085243.GX624164@phenom.ffwll.local> Organization: Collabora X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: stable@vger.kernel.org, Steven Price , Rob Herring , dri-devel@lists.freedesktop.org, Alyssa Rosenzweig Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Message-ID: <20191202095044.nLFw3mdUqrwawBFDPDBrRpIIUkZRLCDf_R5DH-lvfYc@z> T24gTW9uLCAyIERlYyAyMDE5IDA5OjUyOjQzICswMTAwCkRhbmllbCBWZXR0ZXIgPGRhbmllbEBm ZndsbC5jaD4gd3JvdGU6Cgo+IE9uIEZyaSwgTm92IDI5LCAyMDE5IGF0IDEwOjA5OjI0UE0gKzAx MDAsIEJvcmlzIEJyZXppbGxvbiB3cm90ZToKPiA+IE9uIEZyaSwgMjkgTm92IDIwMTkgMjE6MTI6 MTMgKzAxMDAKPiA+IERhbmllbCBWZXR0ZXIgPGRhbmllbEBmZndsbC5jaD4gd3JvdGU6Cj4gPiAg IAo+ID4gPiBPbiBGcmksIE5vdiAyOSwgMjAxOSBhdCAwMjo1OTowNlBNICswMTAwLCBCb3JpcyBC cmV6aWxsb24gd3JvdGU6ICAKPiA+ID4gPiBXZSBkb24ndCB3YW50IGltcG9ydGVkL2V4cG9ydGVk IEJPcyB0byBiZSBwdXJnZXMsIGFzIHRob3NlIGFyZSBzaGFyZWQKPiA+ID4gPiB3aXRoIG90aGVy IHByb2Nlc3NlcyB0aGF0IG1pZ2h0IHN0aWxsIHVzZSB0aGVtLiBXZSBzaG91bGQgYWxzbyByZWZ1 c2UKPiA+ID4gPiB0byBleHBvcnQgYSBCTyBpZiBpdCdzIGJlZW4gbWFya2VkIHB1cmdlYWJsZSBv ciBoYXMgYWxyZWFkeSBiZWVuIHB1cmdlZC4KPiA+ID4gPiAKPiA+ID4gPiBGaXhlczogMDEzYjY1 MTAxMzE1ICgiZHJtL3BhbmZyb3N0OiBBZGQgbWFkdmlzZSBhbmQgc2hyaW5rZXIgc3VwcG9ydCIp Cj4gPiA+ID4gQ2M6IDxzdGFibGVAdmdlci5rZXJuZWwub3JnPgo+ID4gPiA+IFNpZ25lZC1vZmYt Ynk6IEJvcmlzIEJyZXppbGxvbiA8Ym9yaXMuYnJlemlsbG9uQGNvbGxhYm9yYS5jb20+Cj4gPiA+ ID4gLS0tCj4gPiA+ID4gIGRyaXZlcnMvZ3B1L2RybS9wYW5mcm9zdC9wYW5mcm9zdF9kcnYuYyB8 IDE5ICsrKysrKysrKysrKysrKystCj4gPiA+ID4gIGRyaXZlcnMvZ3B1L2RybS9wYW5mcm9zdC9w YW5mcm9zdF9nZW0uYyB8IDI3ICsrKysrKysrKysrKysrKysrKysrKysrKysKPiA+ID4gPiAgMiBm aWxlcyBjaGFuZ2VkLCA0NSBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0pCj4gPiA+ID4gCj4g PiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9wYW5mcm9zdC9wYW5mcm9zdF9kcnYu YyBiL2RyaXZlcnMvZ3B1L2RybS9wYW5mcm9zdC9wYW5mcm9zdF9kcnYuYwo+ID4gPiA+IGluZGV4 IDFjNjdhYzQzNGUxMC4uNzUxZGY5NzU1MzRmIDEwMDY0NAo+ID4gPiA+IC0tLSBhL2RyaXZlcnMv Z3B1L2RybS9wYW5mcm9zdC9wYW5mcm9zdF9kcnYuYwo+ID4gPiA+ICsrKyBiL2RyaXZlcnMvZ3B1 L2RybS9wYW5mcm9zdC9wYW5mcm9zdF9kcnYuYwo+ID4gPiA+IEBAIC0zNDMsNiArMzQzLDcgQEAg c3RhdGljIGludCBwYW5mcm9zdF9pb2N0bF9tYWR2aXNlKHN0cnVjdCBkcm1fZGV2aWNlICpkZXYs IHZvaWQgKmRhdGEsCj4gPiA+ID4gIAlzdHJ1Y3QgZHJtX3BhbmZyb3N0X21hZHZpc2UgKmFyZ3Mg PSBkYXRhOwo+ID4gPiA+ICAJc3RydWN0IHBhbmZyb3N0X2RldmljZSAqcGZkZXYgPSBkZXYtPmRl dl9wcml2YXRlOwo+ID4gPiA+ICAJc3RydWN0IGRybV9nZW1fb2JqZWN0ICpnZW1fb2JqOwo+ID4g PiA+ICsJaW50IHJldDsKPiA+ID4gPiAgCj4gPiA+ID4gIAlnZW1fb2JqID0gZHJtX2dlbV9vYmpl Y3RfbG9va3VwKGZpbGVfcHJpdiwgYXJncy0+aGFuZGxlKTsKPiA+ID4gPiAgCWlmICghZ2VtX29i aikgewo+ID4gPiA+IEBAIC0zNTAsNiArMzUxLDE5IEBAIHN0YXRpYyBpbnQgcGFuZnJvc3RfaW9j dGxfbWFkdmlzZShzdHJ1Y3QgZHJtX2RldmljZSAqZGV2LCB2b2lkICpkYXRhLAo+ID4gPiA+ICAJ CXJldHVybiAtRU5PRU5UOwo+ID4gPiA+ICAJfQo+ID4gPiA+ICAKPiA+ID4gPiArCS8qCj4gPiA+ ID4gKwkgKiBXZSBkb24ndCB3YW50IHRvIG1hcmsgZXhwb3J0ZWQvaW1wb3J0ZWQgQk9zIGFzIHB1 cmdlYWJsZTogd2UncmUgbm90Cj4gPiA+ID4gKwkgKiB0aGUgb25seSBvd25lciBpbiB0aGF0IGNh c2UuCj4gPiA+ID4gKwkgKi8KPiA+ID4gPiArCW11dGV4X2xvY2soJmRldi0+b2JqZWN0X25hbWVf bG9jayk7ICAgIAo+ID4gPiAKPiA+ID4gS2luZGEgbm90IGF3ZXNvbWUgdGhhdCB5b3UgaGF2ZSB0 byB0YWtlIHRoaXMgY29yZSBsb2NrIGhlcmUgYW5kIGVuY3VtYmVyCj4gPiA+IGNvcmUgZHJtIGxv Y2tpbmcgd2l0aCByYW5kb20gZHJpdmVyIHN0dWZmLiAgCj4gPiAKPiA+IExvb2tzIGxpa2UgZHJt X2dlbV9zaG1lbV9pc19wdXJnZWFibGUoKSBhbHJlYWR5IGRvZXMgdGhlICFpbXBvcnRlZCAmJgo+ ID4gIWV4cG9ydGVkIGNoZWNrLiBGb3IgdGhlIGltcG9ydGVkIGNhc2UsIEkgdGhpbmsgd2UncmUg Z29vZCwgc2luY2UKPiA+IHVzZXJzcGFjZSBjYW4ndCBjaGFuZ2UgdGhlIG1hZHYgdmFsdWUgYmVm b3JlIC0+aW1wb3J0X2F0dGFjaCBoYXMgYmVlbgo+ID4gc2V0LiBGb3IgdGhlIGV4cG9ydGVyIGNh c2UsIHdlIG5lZWQgdG8gbWFrZSBzdXJlIHRoZXJlJ3Mgbm8gcmFjZQo+ID4gYmV0d2VlbiB0aGUg ZXhwb3J0IGFuZCBtYWR2aXNlIG9wZXJhdGlvbnMsIHdoaWNoIEkgY2FuIHByb2JhYmx5IGRvIGZy b20KPiA+IHRoZSBnZW0tPmV4cG9ydCgpIGhvb2sgYnkgdGFraW5nIHRoZSBzaHJpbmtlciBvciBi by0+bWFwcGluZ3MgbG9jay4KCk9rYXksIEkgdHJpZWQgdGhhdCwgYW5kIEkgYWN0dWFsbHkgbmVl ZCBhbiBleHRyYQpwYW5mcm9zdF9nZW1fb2JqZWN0LT5leHBvcnRlZCBmaWVsZCB0aGF0J3Mgc2V0 IHRvIHRydWUgZnJvbSB0aGUKLT5leHBvcnQoKSBob29rIHdpdGggdGhlIG1hcHBpbmdzIGxvY2sg aGVsZCwgb3RoZXJ3aXNlIHRoZSBjb2RlIGlzCnN0aWxsIHJhY3kgKC0+ZG1hX2J1ZiBpcyBhc3Np Z25lZCBhZnRlciAtPmV4cG9ydCgpIHJldHVybnMpLgoKPiA+ICAgCj4gPiA+IAo+ID4gPiBDYW4n dCB0aGlzIGJlIHNvbHZlZCB3aXRoIHlvdXIgb3duIGxvY2tpbmcgb25seSBhbmQgc29tZSByZWFz b25hYmxlCj4gPiA+IG9yZGVyaW5nIG9mIGNoZWNrcz8gYmlnIGxvY2tzIGJlY2F1c2UgaXQncyBl YXN5IGlzIGVuZGxlc3MgbG9uZy10ZXJtIHBhaW4uCj4gPiA+IAo+ID4gPiBBbHNvIGV4cG9ydGlu ZyBwdXJnZWFibGUgb2JqZWN0cyBpcyBraW5kYSBhIHVzZXJzcGFjZSBidWcsIGFsbCB5b3UgaGF2 ZSB0bwo+ID4gPiBkbyBpcyBub3Qgb29wcyBpbiBkbWFfYnVmX2F0dGFjaG1lbnRfbWFwLiBObyBu ZWVkIHRvIHByZXZlbnQgdGhlIGRhbWFnZQo+ID4gPiBoZXJlIGltby4gIAo+ID4gCj4gPiBJIGZl ZWwgbGlrZSBtYWtpbmcgc3VyZSBhbiBleHBvcnRlZCBCTyBjYW4ndCBiZSBwdXJnZWQgb3IgYSBw dXJnZWQgQk8KPiA+IGNhbid0IGJlIGV4cG9ydGVkIHdvdWxkIGJlIG11Y2ggc2ltcGxlciB0aGFu IG1ha2luZyBzdXJlIGFsbCBpbXBvcnRlcnMKPiA+IGFyZSByZWFkeSB0byBoYXZlIHRoZSBzZ3Qg ZnJlZWQuICAKPiAKPiBJZiB5b3UgZnJlZSB0aGUgc2d0IHdoaWxlIHNvbWVvbmUgaXMgdXNpbmcg aXQsIHRoYXQncyBraW5kYSBhIGRpZmZlcmVudAo+IGJ1ZyBJIHRoaW5rLiBZb3UgYWxyZWFkeSBo YXZlIGEgcGFnZXMgcmVmY291bnQsIHRoYXQgc2hvdWxkIGJlIGVub3VnaCB0bwo+IHByZXZlbnQg dGhpcz8KCk15IGJhZCwgSSB0aG91Z2h0IGRybV9nZW1fc2htZW1fZ2V0X3BhZ2VzX3NndCgpIHdh cyB1c2VkIGFzIHRoZQotPmdldF9zZ190YWJsZSgpIGltcGxlbSwgYnV0IHdlIGFjdHVhbGx5IHVz ZQpkcm1fZ2VtX3NobWVtX2dldF9zZ190YWJsZSgpIHdoaWNoIGFsbG9jYXRlcyBhIG5ldyBzZ3Qu IEkgc3RpbGwgbmVlZCB0bwptYWtlIHN1cmUgd2UncmUgc2FmZSBhZ2FpbnN0IHNndCBkZXN0cnVj dGlvbiBpbiBwYW5mcm9zdC4KX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0 b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJp LWRldmVs