Re: [Intel-gfx] [PATCH 1/2] drm/i915/gem: Correct the locking and pin pattern for dma-buf (v5)
On Tue, Jul 13, 2021 at 9:40 AM Daniel Vetter wrote: > > On Mon, Jul 12, 2021 at 06:12:33PM -0500, Jason Ekstrand wrote: > > From: Thomas Hellström > > > > If our exported dma-bufs are imported by another instance of our driver, > > that instance will typically have the imported dma-bufs locked during > > dma_buf_map_attachment(). But the exporter also locks the same reservation > > object in the map_dma_buf() callback, which leads to recursive locking. > > > > So taking the lock inside _pin_pages_unlocked() is incorrect. > > > > Additionally, the current pinning code path is contrary to the defined > > way that pinning should occur. > > > > Remove the explicit pin/unpin from the map/umap functions and move them > > to the attach/detach allowing correct locking to occur, and to match > > the static dma-buf drm_prime pattern. > > > > Add a live selftest to exercise both dynamic and non-dynamic > > exports. > > > > v2: > > - Extend the selftest with a fake dynamic importer. > > - Provide real pin and unpin callbacks to not abuse the interface. > > v3: (ruhl) > > - Remove the dynamic export support and move the pinning into the > > attach/detach path. > > v4: (ruhl) > > - Put pages does not need to assert on the dma-resv > > v5: (jason) > > - Lock around dma_buf_unmap_attachment() when emulating a dynamic > > importer in the subtests. > > - Use pin_pages_unlocked > > > > Reported-by: Michael J. Ruhl > > Signed-off-by: Thomas Hellström > > Signed-off-by: Michael J. Ruhl > > Signed-off-by: Jason Ekstrand > > Reviewed-by: Jason Ekstrand > > --- > > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 43 +-- > > .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 118 +- > > 2 files changed, 147 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > index 616c3a2f1baf0..9a655f69a0671 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > @@ -12,6 +12,8 @@ > > #include "i915_gem_object.h" > > #include "i915_scatterlist.h" > > > > +I915_SELFTEST_DECLARE(static bool force_different_devices;) > > + > > static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf) > > { > > return to_intel_bo(buf->priv); > > @@ -25,15 +27,11 @@ static struct sg_table *i915_gem_map_dma_buf(struct > > dma_buf_attachment *attachme > > struct scatterlist *src, *dst; > > int ret, i; > > > > - ret = i915_gem_object_pin_pages_unlocked(obj); > > - if (ret) > > - goto err; > > - > > /* Copy sg so that we make an independent mapping */ > > st = kmalloc(sizeof(struct sg_table), GFP_KERNEL); > > if (st == NULL) { > > ret = -ENOMEM; > > - goto err_unpin_pages; > > + goto err; > > } > > > > ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL); > > @@ -58,8 +56,6 @@ static struct sg_table *i915_gem_map_dma_buf(struct > > dma_buf_attachment *attachme > > sg_free_table(st); > > err_free: > > kfree(st); > > -err_unpin_pages: > > - i915_gem_object_unpin_pages(obj); > > err: > > return ERR_PTR(ret); > > } > > @@ -68,13 +64,9 @@ static void i915_gem_unmap_dma_buf(struct > > dma_buf_attachment *attachment, > > struct sg_table *sg, > > enum dma_data_direction dir) > > { > > - struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); > > - > > dma_unmap_sgtable(attachment->dev, sg, dir, DMA_ATTR_SKIP_CPU_SYNC); > > sg_free_table(sg); > > kfree(sg); > > - > > - i915_gem_object_unpin_pages(obj); > > } > > > > static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct > > dma_buf_map *map) > > @@ -168,7 +160,31 @@ static int i915_gem_end_cpu_access(struct dma_buf > > *dma_buf, enum dma_data_direct > > return err; > > } > > > > +/** > > + * i915_gem_dmabuf_attach - Do any extra attach work necessary > > + * @dmabuf: imported dma-buf > > + * @attach: new attach to do work on > > + * > > + */ > > +static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf, > > + struct dma_buf_attachment *attach) > > +{ > > + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); > > + > > + return i915_gem_object_pin_pages_unlocked(obj); > > +} > > + > > +static void i915_gem_dmabuf_detach(struct dma_buf *dmabuf, > > +struct dma_buf_attachment *attach) > > +{ > > + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); > > + > > + i915_gem_object_unpin_pages(obj); > > +} > > + > > static const struct dma_buf_ops i915_dmabuf_ops = { > > + .attach = i915_gem_dmabuf_attach, > > + .detach = i915_gem_dmabuf_detach, > > .map_dma_buf = i915_gem_map_dma_buf, > > .unmap_dma_buf = i915_gem_unmap_dma_buf, > > .release = drm_gem_dmabuf_r
Re: [Intel-gfx] [PATCH 1/2] drm/i915/gem: Correct the locking and pin pattern for dma-buf (v5)
On Mon, Jul 12, 2021 at 06:12:33PM -0500, Jason Ekstrand wrote: > From: Thomas Hellström > > If our exported dma-bufs are imported by another instance of our driver, > that instance will typically have the imported dma-bufs locked during > dma_buf_map_attachment(). But the exporter also locks the same reservation > object in the map_dma_buf() callback, which leads to recursive locking. > > So taking the lock inside _pin_pages_unlocked() is incorrect. > > Additionally, the current pinning code path is contrary to the defined > way that pinning should occur. > > Remove the explicit pin/unpin from the map/umap functions and move them > to the attach/detach allowing correct locking to occur, and to match > the static dma-buf drm_prime pattern. > > Add a live selftest to exercise both dynamic and non-dynamic > exports. > > v2: > - Extend the selftest with a fake dynamic importer. > - Provide real pin and unpin callbacks to not abuse the interface. > v3: (ruhl) > - Remove the dynamic export support and move the pinning into the > attach/detach path. > v4: (ruhl) > - Put pages does not need to assert on the dma-resv > v5: (jason) > - Lock around dma_buf_unmap_attachment() when emulating a dynamic > importer in the subtests. > - Use pin_pages_unlocked > > Reported-by: Michael J. Ruhl > Signed-off-by: Thomas Hellström > Signed-off-by: Michael J. Ruhl > Signed-off-by: Jason Ekstrand > Reviewed-by: Jason Ekstrand > --- > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 43 +-- > .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 118 +- > 2 files changed, 147 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > index 616c3a2f1baf0..9a655f69a0671 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > @@ -12,6 +12,8 @@ > #include "i915_gem_object.h" > #include "i915_scatterlist.h" > > +I915_SELFTEST_DECLARE(static bool force_different_devices;) > + > static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf) > { > return to_intel_bo(buf->priv); > @@ -25,15 +27,11 @@ static struct sg_table *i915_gem_map_dma_buf(struct > dma_buf_attachment *attachme > struct scatterlist *src, *dst; > int ret, i; > > - ret = i915_gem_object_pin_pages_unlocked(obj); > - if (ret) > - goto err; > - > /* Copy sg so that we make an independent mapping */ > st = kmalloc(sizeof(struct sg_table), GFP_KERNEL); > if (st == NULL) { > ret = -ENOMEM; > - goto err_unpin_pages; > + goto err; > } > > ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL); > @@ -58,8 +56,6 @@ static struct sg_table *i915_gem_map_dma_buf(struct > dma_buf_attachment *attachme > sg_free_table(st); > err_free: > kfree(st); > -err_unpin_pages: > - i915_gem_object_unpin_pages(obj); > err: > return ERR_PTR(ret); > } > @@ -68,13 +64,9 @@ static void i915_gem_unmap_dma_buf(struct > dma_buf_attachment *attachment, > struct sg_table *sg, > enum dma_data_direction dir) > { > - struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); > - > dma_unmap_sgtable(attachment->dev, sg, dir, DMA_ATTR_SKIP_CPU_SYNC); > sg_free_table(sg); > kfree(sg); > - > - i915_gem_object_unpin_pages(obj); > } > > static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map > *map) > @@ -168,7 +160,31 @@ static int i915_gem_end_cpu_access(struct dma_buf > *dma_buf, enum dma_data_direct > return err; > } > > +/** > + * i915_gem_dmabuf_attach - Do any extra attach work necessary > + * @dmabuf: imported dma-buf > + * @attach: new attach to do work on > + * > + */ > +static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf, > + struct dma_buf_attachment *attach) > +{ > + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); > + > + return i915_gem_object_pin_pages_unlocked(obj); > +} > + > +static void i915_gem_dmabuf_detach(struct dma_buf *dmabuf, > +struct dma_buf_attachment *attach) > +{ > + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); > + > + i915_gem_object_unpin_pages(obj); > +} > + > static const struct dma_buf_ops i915_dmabuf_ops = { > + .attach = i915_gem_dmabuf_attach, > + .detach = i915_gem_dmabuf_detach, > .map_dma_buf = i915_gem_map_dma_buf, > .unmap_dma_buf = i915_gem_unmap_dma_buf, > .release = drm_gem_dmabuf_release, > @@ -204,6 +220,8 @@ static int i915_gem_object_get_pages_dmabuf(struct > drm_i915_gem_object *obj) > struct sg_table *pages; > unsigned int sg_page_sizes; > > + assert_object_held(obj); > + > pages = dma_buf_map_attachment(obj->base.import_attach, >
[Intel-gfx] [PATCH 1/2] drm/i915/gem: Correct the locking and pin pattern for dma-buf (v5)
From: Thomas Hellström If our exported dma-bufs are imported by another instance of our driver, that instance will typically have the imported dma-bufs locked during dma_buf_map_attachment(). But the exporter also locks the same reservation object in the map_dma_buf() callback, which leads to recursive locking. So taking the lock inside _pin_pages_unlocked() is incorrect. Additionally, the current pinning code path is contrary to the defined way that pinning should occur. Remove the explicit pin/unpin from the map/umap functions and move them to the attach/detach allowing correct locking to occur, and to match the static dma-buf drm_prime pattern. Add a live selftest to exercise both dynamic and non-dynamic exports. v2: - Extend the selftest with a fake dynamic importer. - Provide real pin and unpin callbacks to not abuse the interface. v3: (ruhl) - Remove the dynamic export support and move the pinning into the attach/detach path. v4: (ruhl) - Put pages does not need to assert on the dma-resv v5: (jason) - Lock around dma_buf_unmap_attachment() when emulating a dynamic importer in the subtests. - Use pin_pages_unlocked Reported-by: Michael J. Ruhl Signed-off-by: Thomas Hellström Signed-off-by: Michael J. Ruhl Signed-off-by: Jason Ekstrand Reviewed-by: Jason Ekstrand --- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 43 +-- .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 118 +- 2 files changed, 147 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf0..9a655f69a0671 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -12,6 +12,8 @@ #include "i915_gem_object.h" #include "i915_scatterlist.h" +I915_SELFTEST_DECLARE(static bool force_different_devices;) + static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf) { return to_intel_bo(buf->priv); @@ -25,15 +27,11 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i; - ret = i915_gem_object_pin_pages_unlocked(obj); - if (ret) - goto err; - /* Copy sg so that we make an independent mapping */ st = kmalloc(sizeof(struct sg_table), GFP_KERNEL); if (st == NULL) { ret = -ENOMEM; - goto err_unpin_pages; + goto err; } ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL); @@ -58,8 +56,6 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme sg_free_table(st); err_free: kfree(st); -err_unpin_pages: - i915_gem_object_unpin_pages(obj); err: return ERR_PTR(ret); } @@ -68,13 +64,9 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *sg, enum dma_data_direction dir) { - struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); - dma_unmap_sgtable(attachment->dev, sg, dir, DMA_ATTR_SKIP_CPU_SYNC); sg_free_table(sg); kfree(sg); - - i915_gem_object_unpin_pages(obj); } static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map) @@ -168,7 +160,31 @@ static int i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direct return err; } +/** + * i915_gem_dmabuf_attach - Do any extra attach work necessary + * @dmabuf: imported dma-buf + * @attach: new attach to do work on + * + */ +static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf, + struct dma_buf_attachment *attach) +{ + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); + + return i915_gem_object_pin_pages_unlocked(obj); +} + +static void i915_gem_dmabuf_detach(struct dma_buf *dmabuf, + struct dma_buf_attachment *attach) +{ + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); + + i915_gem_object_unpin_pages(obj); +} + static const struct dma_buf_ops i915_dmabuf_ops = { + .attach = i915_gem_dmabuf_attach, + .detach = i915_gem_dmabuf_detach, .map_dma_buf = i915_gem_map_dma_buf, .unmap_dma_buf = i915_gem_unmap_dma_buf, .release = drm_gem_dmabuf_release, @@ -204,6 +220,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) struct sg_table *pages; unsigned int sg_page_sizes; + assert_object_held(obj); + pages = dma_buf_map_attachment(obj->base.import_attach, DMA_BIDIRECTIONAL); if (IS_ERR(pages)) @@ -241,7 +259,8 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, if (dma_buf->ops == &i915_dmabuf_ops) { obj = dma_buf_to_obj(dma_buf);
Re: [Intel-gfx] [PATCH 1/2] drm/i915/gem: Correct the locking and pin pattern for dma-buf
Am 02.07.21 um 19:09 schrieb Daniel Vetter: On Thu, Jul 1, 2021 at 4:24 PM Michael J. Ruhl wrote: From: Thomas Hellström If our exported dma-bufs are imported by another instance of our driver, that instance will typically have the imported dma-bufs locked during dma_buf_map_attachment(). But the exporter also locks the same reservation object in the map_dma_buf() callback, which leads to recursive locking. So taking the lock inside _pin_pages_unlocked() is incorrect. Additionally, the current pinning code path is contrary to the defined way that pinning should occur. Remove the explicit pin/unpin from the map/umap functions and move them to the attach/detach allowing correct locking to occur, and to match the static dma-buf drm_prime pattern. Add a live selftest to exercise both dynamic and non-dynamic exports. v2: - Extend the selftest with a fake dynamic importer. - Provide real pin and unpin callbacks to not abuse the interface. v3: (ruhl) - Remove the dynamic export support and move the pinning into the attach/detach path. Reported-by: Michael J. Ruhl Signed-off-by: Thomas Hellström Signed-off-by: Michael J. Ruhl CI splat is because I got the locking rules wrong, I thought ->attach/detach are called under the dma_resv_lock, because when we used the old dma_buf->lock those calls where protected by that lock under the same critical section as adding/removing from the list. But we changed that in f45f57cce584 ("dma-buf: stop using the dmabuf->lock so much v2") 15fd552d186c ("dma-buf: change DMA-buf locking convention v3") Because keeping dma_resv_lock over ->attach/detach would go boom on all the ttm drivers, which pin/unpin the buffer in there. Iow we need the unlocked version there, but also having this split up is a bit awkward and might be good to patch up so that it's atomic again. Would mean updating a bunch of drivers. Christian, any thoughts? Yeah, we already discussed that when we switched from dma_buf->lock to dma_resv->lock. In general completely agree to do this and stopping using the dma_buf->lock was a first step towards this, but IIRC we postponed that switch till later. Regards, Christian. PS: I'm currently on sick leave, so response might be delayed. Mike, for now I'd just keep using the _unlocked variants and we should be fine. -Daniel --- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 46 ++-- .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 111 +- 2 files changed, 143 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf..00338c8d3739 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -12,6 +12,8 @@ #include "i915_gem_object.h" #include "i915_scatterlist.h" +I915_SELFTEST_DECLARE(static bool force_different_devices;) + static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf) { return to_intel_bo(buf->priv); @@ -25,15 +27,11 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i; - ret = i915_gem_object_pin_pages_unlocked(obj); - if (ret) - goto err; - /* Copy sg so that we make an independent mapping */ st = kmalloc(sizeof(struct sg_table), GFP_KERNEL); if (st == NULL) { ret = -ENOMEM; - goto err_unpin_pages; + goto err; } ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL); @@ -58,8 +56,6 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme sg_free_table(st); err_free: kfree(st); -err_unpin_pages: - i915_gem_object_unpin_pages(obj); err: return ERR_PTR(ret); } @@ -68,13 +64,9 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *sg, enum dma_data_direction dir) { - struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); - dma_unmap_sgtable(attachment->dev, sg, dir, DMA_ATTR_SKIP_CPU_SYNC); sg_free_table(sg); kfree(sg); - - i915_gem_object_unpin_pages(obj); } static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map) @@ -168,7 +160,32 @@ static int i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direct return err; } +/** + * i915_gem_dmabuf_attach - Do any extra attach work necessary + * @dmabuf: imported dma-buf + * @attach: new attach to do work on + * + */ +static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf, + struct dma_buf_attachment *attach) +{ + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); + + assert_object_held(obj); + return i915_gem_object_pin_pages(obj); +}
Re: [Intel-gfx] [PATCH 1/2] drm/i915/gem: Correct the locking and pin pattern for dma-buf
On Thu, Jul 1, 2021 at 4:24 PM Michael J. Ruhl wrote: > > From: Thomas Hellström > > If our exported dma-bufs are imported by another instance of our driver, > that instance will typically have the imported dma-bufs locked during > dma_buf_map_attachment(). But the exporter also locks the same reservation > object in the map_dma_buf() callback, which leads to recursive locking. > > So taking the lock inside _pin_pages_unlocked() is incorrect. > > Additionally, the current pinning code path is contrary to the defined > way that pinning should occur. > > Remove the explicit pin/unpin from the map/umap functions and move them > to the attach/detach allowing correct locking to occur, and to match > the static dma-buf drm_prime pattern. > > Add a live selftest to exercise both dynamic and non-dynamic > exports. > > v2: > - Extend the selftest with a fake dynamic importer. > - Provide real pin and unpin callbacks to not abuse the interface. > v3: (ruhl) > - Remove the dynamic export support and move the pinning into the > attach/detach path. > > Reported-by: Michael J. Ruhl > Signed-off-by: Thomas Hellström > Signed-off-by: Michael J. Ruhl CI splat is because I got the locking rules wrong, I thought ->attach/detach are called under the dma_resv_lock, because when we used the old dma_buf->lock those calls where protected by that lock under the same critical section as adding/removing from the list. But we changed that in f45f57cce584 ("dma-buf: stop using the dmabuf->lock so much v2") 15fd552d186c ("dma-buf: change DMA-buf locking convention v3") Because keeping dma_resv_lock over ->attach/detach would go boom on all the ttm drivers, which pin/unpin the buffer in there. Iow we need the unlocked version there, but also having this split up is a bit awkward and might be good to patch up so that it's atomic again. Would mean updating a bunch of drivers. Christian, any thoughts? Mike, for now I'd just keep using the _unlocked variants and we should be fine. -Daniel > --- > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 46 ++-- > .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 111 +- > 2 files changed, 143 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > index 616c3a2f1baf..00338c8d3739 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > @@ -12,6 +12,8 @@ > #include "i915_gem_object.h" > #include "i915_scatterlist.h" > > +I915_SELFTEST_DECLARE(static bool force_different_devices;) > + > static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf) > { > return to_intel_bo(buf->priv); > @@ -25,15 +27,11 @@ static struct sg_table *i915_gem_map_dma_buf(struct > dma_buf_attachment *attachme > struct scatterlist *src, *dst; > int ret, i; > > - ret = i915_gem_object_pin_pages_unlocked(obj); > - if (ret) > - goto err; > - > /* Copy sg so that we make an independent mapping */ > st = kmalloc(sizeof(struct sg_table), GFP_KERNEL); > if (st == NULL) { > ret = -ENOMEM; > - goto err_unpin_pages; > + goto err; > } > > ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL); > @@ -58,8 +56,6 @@ static struct sg_table *i915_gem_map_dma_buf(struct > dma_buf_attachment *attachme > sg_free_table(st); > err_free: > kfree(st); > -err_unpin_pages: > - i915_gem_object_unpin_pages(obj); > err: > return ERR_PTR(ret); > } > @@ -68,13 +64,9 @@ static void i915_gem_unmap_dma_buf(struct > dma_buf_attachment *attachment, >struct sg_table *sg, >enum dma_data_direction dir) > { > - struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); > - > dma_unmap_sgtable(attachment->dev, sg, dir, DMA_ATTR_SKIP_CPU_SYNC); > sg_free_table(sg); > kfree(sg); > - > - i915_gem_object_unpin_pages(obj); > } > > static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map > *map) > @@ -168,7 +160,32 @@ static int i915_gem_end_cpu_access(struct dma_buf > *dma_buf, enum dma_data_direct > return err; > } > > +/** > + * i915_gem_dmabuf_attach - Do any extra attach work necessary > + * @dmabuf: imported dma-buf > + * @attach: new attach to do work on > + * > + */ > +static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf, > + struct dma_buf_attachment *attach) > +{ > + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); > + > + assert_object_held(obj); > + return i915_gem_object_pin_pages(obj); > +} > + > +static void i915_gem_dmabuf_detach(struct dma_buf *dmabuf, > + struct dma_buf_attachment *attach) > +{ > + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf
[Intel-gfx] [PATCH 1/2] drm/i915/gem: Correct the locking and pin pattern for dma-buf
From: Thomas Hellström If our exported dma-bufs are imported by another instance of our driver, that instance will typically have the imported dma-bufs locked during dma_buf_map_attachment(). But the exporter also locks the same reservation object in the map_dma_buf() callback, which leads to recursive locking. So taking the lock inside _pin_pages_unlocked() is incorrect. Additionally, the current pinning code path is contrary to the defined way that pinning should occur. Remove the explicit pin/unpin from the map/umap functions and move them to the attach/detach allowing correct locking to occur, and to match the static dma-buf drm_prime pattern. Add a live selftest to exercise both dynamic and non-dynamic exports. v2: - Extend the selftest with a fake dynamic importer. - Provide real pin and unpin callbacks to not abuse the interface. v3: (ruhl) - Remove the dynamic export support and move the pinning into the attach/detach path. Reported-by: Michael J. Ruhl Signed-off-by: Thomas Hellström Signed-off-by: Michael J. Ruhl --- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 46 +-- .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 116 +- 2 files changed, 148 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf..8c528b693a30 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -12,6 +12,8 @@ #include "i915_gem_object.h" #include "i915_scatterlist.h" +I915_SELFTEST_DECLARE(static bool force_different_devices;) + static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf) { return to_intel_bo(buf->priv); @@ -25,15 +27,11 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i; - ret = i915_gem_object_pin_pages_unlocked(obj); - if (ret) - goto err; - /* Copy sg so that we make an independent mapping */ st = kmalloc(sizeof(struct sg_table), GFP_KERNEL); if (st == NULL) { ret = -ENOMEM; - goto err_unpin_pages; + goto err; } ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL); @@ -58,8 +56,6 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme sg_free_table(st); err_free: kfree(st); -err_unpin_pages: - i915_gem_object_unpin_pages(obj); err: return ERR_PTR(ret); } @@ -68,13 +64,9 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *sg, enum dma_data_direction dir) { - struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); - dma_unmap_sgtable(attachment->dev, sg, dir, DMA_ATTR_SKIP_CPU_SYNC); sg_free_table(sg); kfree(sg); - - i915_gem_object_unpin_pages(obj); } static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map) @@ -168,7 +160,32 @@ static int i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direct return err; } +/** + * i915_gem_dmabuf_attach - Do any extra attach work necessary + * @dmabuf: imported dma-buf + * @attach: new attach to do work on + * + */ +static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf, + struct dma_buf_attachment *attach) +{ + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); + + assert_object_held(obj); + return i915_gem_object_pin_pages(obj); +} + +static void i915_gem_dmabuf_detach(struct dma_buf *dmabuf, + struct dma_buf_attachment *attach) +{ + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); + + i915_gem_object_unpin_pages(obj); +} + static const struct dma_buf_ops i915_dmabuf_ops = { + .attach = i915_gem_dmabuf_attach, + .detach = i915_gem_dmabuf_detach, .map_dma_buf = i915_gem_map_dma_buf, .unmap_dma_buf = i915_gem_unmap_dma_buf, .release = drm_gem_dmabuf_release, @@ -204,6 +221,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) struct sg_table *pages; unsigned int sg_page_sizes; + assert_object_held(obj); + pages = dma_buf_map_attachment(obj->base.import_attach, DMA_BIDIRECTIONAL); if (IS_ERR(pages)) @@ -219,6 +238,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) static void i915_gem_object_put_pages_dmabuf(struct drm_i915_gem_object *obj, struct sg_table *pages) { + assert_object_held(obj); + dma_buf_unmap_attachment(obj->base.import_attach, pages, DMA_BIDIRECTIONAL); } @@ -241,7 +262,8 @@ struct d
[Intel-gfx] [PATCH 1/2] drm/i915/gem: Correct the locking and pin pattern for dma-buf
From: Thomas Hellström If our exported dma-bufs are imported by another instance of our driver, that instance will typically have the imported dma-bufs locked during dma_buf_map_attachment(). But the exporter also locks the same reservation object in the map_dma_buf() callback, which leads to recursive locking. So taking the lock inside _pin_pages_unlocked() is incorrect. Additionally, the current pinning code path is contrary to the defined way that pinning should occur. Remove the explicit pin/unpin from the map/umap functions and move them to the attach/detach allowing correct locking to occur, and to match the static dma-buf drm_prime pattern. Add a live selftest to exercise both dynamic and non-dynamic exports. v2: - Extend the selftest with a fake dynamic importer. - Provide real pin and unpin callbacks to not abuse the interface. v3: (ruhl) - Remove the dynamic export support and move the pinning into the attach/detach path. Reported-by: Michael J. Ruhl Signed-off-by: Thomas Hellström Signed-off-by: Michael J. Ruhl --- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 46 ++-- .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 111 +- 2 files changed, 143 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf..00338c8d3739 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -12,6 +12,8 @@ #include "i915_gem_object.h" #include "i915_scatterlist.h" +I915_SELFTEST_DECLARE(static bool force_different_devices;) + static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf) { return to_intel_bo(buf->priv); @@ -25,15 +27,11 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i; - ret = i915_gem_object_pin_pages_unlocked(obj); - if (ret) - goto err; - /* Copy sg so that we make an independent mapping */ st = kmalloc(sizeof(struct sg_table), GFP_KERNEL); if (st == NULL) { ret = -ENOMEM; - goto err_unpin_pages; + goto err; } ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL); @@ -58,8 +56,6 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme sg_free_table(st); err_free: kfree(st); -err_unpin_pages: - i915_gem_object_unpin_pages(obj); err: return ERR_PTR(ret); } @@ -68,13 +64,9 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *sg, enum dma_data_direction dir) { - struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); - dma_unmap_sgtable(attachment->dev, sg, dir, DMA_ATTR_SKIP_CPU_SYNC); sg_free_table(sg); kfree(sg); - - i915_gem_object_unpin_pages(obj); } static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map) @@ -168,7 +160,32 @@ static int i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direct return err; } +/** + * i915_gem_dmabuf_attach - Do any extra attach work necessary + * @dmabuf: imported dma-buf + * @attach: new attach to do work on + * + */ +static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf, + struct dma_buf_attachment *attach) +{ + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); + + assert_object_held(obj); + return i915_gem_object_pin_pages(obj); +} + +static void i915_gem_dmabuf_detach(struct dma_buf *dmabuf, + struct dma_buf_attachment *attach) +{ + struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf); + + i915_gem_object_unpin_pages(obj); +} + static const struct dma_buf_ops i915_dmabuf_ops = { + .attach = i915_gem_dmabuf_attach, + .detach = i915_gem_dmabuf_detach, .map_dma_buf = i915_gem_map_dma_buf, .unmap_dma_buf = i915_gem_unmap_dma_buf, .release = drm_gem_dmabuf_release, @@ -204,6 +221,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) struct sg_table *pages; unsigned int sg_page_sizes; + assert_object_held(obj); + pages = dma_buf_map_attachment(obj->base.import_attach, DMA_BIDIRECTIONAL); if (IS_ERR(pages)) @@ -219,6 +238,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) static void i915_gem_object_put_pages_dmabuf(struct drm_i915_gem_object *obj, struct sg_table *pages) { + assert_object_held(obj); + dma_buf_unmap_attachment(obj->base.import_attach, pages, DMA_BIDIRECTIONAL); } @@ -241,7 +262,8 @@ struct d