From: Daniel Vetter <daniel@ffwll.ch> To: Boris Brezillon <boris.brezillon@collabora.com> Cc: Rob Herring <robh+dt@kernel.org>, Tomeu Vizoso <tomeu@tomeuvizoso.net>, Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>, Steven Price <steven.price@arm.com>, stable@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged Date: Fri, 29 Nov 2019 21:12:13 +0100 [thread overview] Message-ID: <20191129201213.GR624164@phenom.ffwll.local> (raw) In-Reply-To: <20191129135908.2439529-7-boris.brezillon@collabora.com> 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: <stable@vger.kernel.org> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > 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. 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. -Daniel > + if (gem_obj->dma_buf) > + ret = -EINVAL; > + else > + ret = 0; > + > + if (ret) > + goto out_unlock_object_name; > + > mutex_lock(&pfdev->shrinker_lock); > args->retained = drm_gem_shmem_madvise(gem_obj, args->madv); > > @@ -364,8 +378,11 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data, > } > mutex_unlock(&pfdev->shrinker_lock); > > +out_unlock_object_name: > + mutex_unlock(&dev->object_name_lock); > + > drm_gem_object_put_unlocked(gem_obj); > - return 0; > + return ret; > } > > int panfrost_unstable_ioctl_check(void) > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c > index 92a95210a899..31d6417dd21c 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c > @@ -99,6 +99,32 @@ void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv) > spin_unlock(&priv->mm_lock); > } > > +static struct dma_buf * > +panfrost_gem_export(struct drm_gem_object *obj, int flags) > +{ > + struct panfrost_gem_object *bo = to_panfrost_bo(obj); > + int ret; > + > + /* > + * We must make sure the BO has not been marked purgeable/purged before > + * adding the mapping. > + * Note that we don't need to protect this test with a lock because > + * &drm_gem_object_funcs.export() is called with > + * &drm_device.object_lock held, and panfrost_ioctl_madvise() takes > + * this lock before calling drm_gem_shmem_madvise() (the function that > + * modifies bo->base.madv). > + */ > + if (bo->base.madv == PANFROST_MADV_WILLNEED) > + ret = -EINVAL; > + else > + ret = 0; > + > + if (ret) > + return ERR_PTR(ret); > + > + return drm_gem_prime_export(obj, flags); > +} > + > static int panfrost_gem_pin(struct drm_gem_object *obj) > { > if (to_panfrost_bo(obj)->is_heap) > @@ -112,6 +138,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = { > .open = panfrost_gem_open, > .close = panfrost_gem_close, > .print_info = drm_gem_shmem_print_info, > + .export = panfrost_gem_export, > .pin = panfrost_gem_pin, > .unpin = drm_gem_shmem_unpin, > .get_sg_table = drm_gem_shmem_get_sg_table, > -- > 2.23.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Boris Brezillon <boris.brezillon@collabora.com> Cc: stable@vger.kernel.org, Steven Price <steven.price@arm.com>, Rob Herring <robh+dt@kernel.org>, dri-devel@lists.freedesktop.org, Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> Subject: Re: [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged Date: Fri, 29 Nov 2019 21:12:13 +0100 [thread overview] Message-ID: <20191129201213.GR624164@phenom.ffwll.local> (raw) Message-ID: <20191129201213.sw-CtZfj6Q0Dx6DWVZzSReda9pnfKbEuldT7VcvF2M4@z> (raw) In-Reply-To: <20191129135908.2439529-7-boris.brezillon@collabora.com> 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: <stable@vger.kernel.org> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > 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. 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. -Daniel > + if (gem_obj->dma_buf) > + ret = -EINVAL; > + else > + ret = 0; > + > + if (ret) > + goto out_unlock_object_name; > + > mutex_lock(&pfdev->shrinker_lock); > args->retained = drm_gem_shmem_madvise(gem_obj, args->madv); > > @@ -364,8 +378,11 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data, > } > mutex_unlock(&pfdev->shrinker_lock); > > +out_unlock_object_name: > + mutex_unlock(&dev->object_name_lock); > + > drm_gem_object_put_unlocked(gem_obj); > - return 0; > + return ret; > } > > int panfrost_unstable_ioctl_check(void) > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c > index 92a95210a899..31d6417dd21c 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c > @@ -99,6 +99,32 @@ void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv) > spin_unlock(&priv->mm_lock); > } > > +static struct dma_buf * > +panfrost_gem_export(struct drm_gem_object *obj, int flags) > +{ > + struct panfrost_gem_object *bo = to_panfrost_bo(obj); > + int ret; > + > + /* > + * We must make sure the BO has not been marked purgeable/purged before > + * adding the mapping. > + * Note that we don't need to protect this test with a lock because > + * &drm_gem_object_funcs.export() is called with > + * &drm_device.object_lock held, and panfrost_ioctl_madvise() takes > + * this lock before calling drm_gem_shmem_madvise() (the function that > + * modifies bo->base.madv). > + */ > + if (bo->base.madv == PANFROST_MADV_WILLNEED) > + ret = -EINVAL; > + else > + ret = 0; > + > + if (ret) > + return ERR_PTR(ret); > + > + return drm_gem_prime_export(obj, flags); > +} > + > static int panfrost_gem_pin(struct drm_gem_object *obj) > { > if (to_panfrost_bo(obj)->is_heap) > @@ -112,6 +138,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = { > .open = panfrost_gem_open, > .close = panfrost_gem_close, > .print_info = drm_gem_shmem_print_info, > + .export = panfrost_gem_export, > .pin = panfrost_gem_pin, > .unpin = drm_gem_shmem_unpin, > .get_sg_table = drm_gem_shmem_get_sg_table, > -- > 2.23.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-11-29 20:12 UTC|newest] Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-29 13:59 [PATCH 0/8] panfrost: Fixes for 5.4 Boris Brezillon 2019-11-29 13:59 ` [PATCH 1/8] drm/panfrost: Make panfrost_job_run() return an ERR_PTR() instead of NULL Boris Brezillon 2019-11-29 13:59 ` Boris Brezillon 2019-11-29 14:19 ` Steven Price 2019-11-29 14:19 ` Steven Price 2019-11-29 14:31 ` Boris Brezillon 2019-11-29 14:31 ` Boris Brezillon 2019-11-29 14:38 ` Steven Price 2019-11-29 14:38 ` Steven Price 2019-11-29 19:32 ` Boris Brezillon 2019-11-29 19:32 ` Boris Brezillon 2019-11-29 13:59 ` [PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise() Boris Brezillon 2019-11-29 13:59 ` Boris Brezillon 2019-11-29 14:24 ` Steven Price 2019-11-29 14:24 ` Steven Price 2019-11-29 14:33 ` Boris Brezillon 2019-11-29 14:33 ` Boris Brezillon 2019-11-29 14:40 ` Steven Price 2019-11-29 14:40 ` Steven Price 2019-11-29 20:07 ` Daniel Vetter 2019-11-29 20:07 ` Daniel Vetter 2019-11-29 21:45 ` Boris Brezillon 2019-11-29 21:45 ` Boris Brezillon 2019-12-05 23:08 ` Rob Herring 2019-12-05 23:08 ` Rob Herring 2019-12-06 7:53 ` Boris Brezillon 2019-12-06 7:53 ` Boris Brezillon 2019-12-06 8:08 ` Boris Brezillon 2019-12-06 8:08 ` Boris Brezillon 2019-11-29 13:59 ` [PATCH 3/8] drm/panfrost: Fix a BO leak in panfrost_ioctl_mmap_bo() Boris Brezillon 2019-11-29 13:59 ` Boris Brezillon 2019-11-29 14:26 ` Steven Price 2019-11-29 14:26 ` Steven Price 2019-11-29 13:59 ` [PATCH 4/8] drm/panfrost: Fix a race in panfrost_gem_free_object() Boris Brezillon 2019-11-29 13:59 ` Boris Brezillon 2019-11-29 14:28 ` Steven Price 2019-11-29 14:28 ` Steven Price 2019-11-29 13:59 ` [PATCH 5/8] drm/panfrost: Open/close the perfcnt BO Boris Brezillon 2019-11-29 13:59 ` Boris Brezillon 2019-11-29 14:34 ` Steven Price 2019-11-29 14:34 ` Steven Price 2019-11-29 13:59 ` [PATCH 6/8] drm/panfrost: Make sure imported/exported BOs are never purged Boris Brezillon 2019-11-29 13:59 ` Boris Brezillon 2019-11-29 14:14 ` Boris Brezillon 2019-11-29 14:14 ` Boris Brezillon 2019-11-29 14:45 ` Steven Price 2019-11-29 14:45 ` Steven Price 2019-11-29 14:52 ` Boris Brezillon 2019-11-29 14:52 ` Boris Brezillon 2019-11-29 20:12 ` Daniel Vetter [this message] 2019-11-29 20:12 ` Daniel Vetter 2019-11-29 21:09 ` Boris Brezillon 2019-11-29 21:09 ` Boris Brezillon 2019-12-02 8:52 ` Daniel Vetter 2019-12-02 8:52 ` Daniel Vetter 2019-12-02 9:50 ` Boris Brezillon 2019-12-02 9:50 ` Boris Brezillon 2019-11-29 13:59 ` [PATCH 7/8] drm/panfrost: Add the panfrost_gem_mapping concept Boris Brezillon 2019-11-29 13:59 ` Boris Brezillon 2019-11-29 15:37 ` Steven Price 2019-11-29 15:37 ` Steven Price 2019-11-29 20:14 ` Daniel Vetter 2019-11-29 20:14 ` Daniel Vetter 2019-11-29 21:36 ` Boris Brezillon 2019-11-29 21:36 ` Boris Brezillon 2019-12-02 8:55 ` Daniel Vetter 2019-12-02 8:55 ` Daniel Vetter 2019-12-02 9:13 ` Boris Brezillon 2019-12-02 9:13 ` Boris Brezillon 2019-12-02 9:44 ` Daniel Vetter 2019-12-02 9:44 ` Daniel Vetter 2019-12-04 11:41 ` Steven Price 2019-12-04 11:41 ` Steven Price 2019-11-29 13:59 ` [PATCH 8/8] drm/panfrost: Make sure the shrinker does not reclaim referenced BOs Boris Brezillon 2019-11-29 13:59 ` Boris Brezillon 2019-11-29 15:48 ` Steven Price 2019-11-29 15:48 ` Steven Price 2019-11-29 16:07 ` Boris Brezillon 2019-11-29 16:07 ` Boris Brezillon 2019-11-29 16:12 ` Steven Price 2019-11-29 16:12 ` Steven Price 2019-12-02 12:50 ` Robin Murphy 2019-12-02 12:50 ` Robin Murphy 2019-12-02 13:32 ` Boris Brezillon 2019-12-02 13:32 ` Boris Brezillon 2019-11-29 14:23 ` [PATCH 0/8] panfrost: Fixes for 5.4 Alyssa Rosenzweig 2019-12-06 17:16 ` Rob Herring
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191129201213.GR624164@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=alyssa.rosenzweig@collabora.com \ --cc=boris.brezillon@collabora.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=robh+dt@kernel.org \ --cc=stable@vger.kernel.org \ --cc=steven.price@arm.com \ --cc=tomeu@tomeuvizoso.net \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.