Hello,
This patchset introduces memory shrinker for the VirtIO-GPU DRM driver.
During OOM, the shrinker will release BOs that are marked as "not needed"
by userspace using the new madvise IOCTL, it will also evict idling BOs
to SWAP. The userspace in this case is the Mesa VirGL driver, it will mark
the cached BOs as "not needed", allowing kernel driver to release memory
of the cached shmem BOs on lowmem situations, preventing OOM kills.
This patchset adds memory purging and eviction support to VirtIO-GPU driver.
The Panfrost driver is switched to use generic memory shrinker. Eviction
support will come later on, after resolving the blocker bug in Panfrost.
This patchset also includes couple improvements and fixes for various
minor things that I found while was working on the shrinker.
The Mesa and IGT patches will be kept on hold until this kernel series
will be approved and merged.
This patchset was tested using Qemu and crosvm, including both cases of
IOMMU off/on.
Mesa: https://gitlab.freedesktop.org/digetx/mesa/-/commits/virgl-madvise
IGT: https://gitlab.freedesktop.org/digetx/igt-gpu-tools/-/commits/virtio-madvise
https://gitlab.freedesktop.org/digetx/igt-gpu-tools/-/commits/panfrost-madvise
Changelog:
v4: - Corrected minor W=1 warnings reported by kernel test robot for v3.
- Renamed DRM_GEM_SHMEM_PAGES_STATE_ACTIVE/INACTIVE to PINNED/UNPINNED,
for more clarity.
v3: - Hardened shrinker's count() with usage of READ_ONCE() since we don't
use atomic type for counting and technically compiler is free to
re-fetch counter's variable.
- "Correct drm_gem_shmem_get_sg_table() error handling" now uses
PTR_ERR_OR_ZERO(), fixing typo that was made in v2.
- Removed obsoleted shrinker from the Panfrost driver, which I missed to
do in v2 by accident and Alyssa Rosenzweig managed to notice it.
- CCed stable kernels in all patches that make fixes, even the minor ones,
like was suggested by Emil Velikov and added his r-b to the patches.
- Added t-b from Steven Price to the Panfrost's shrinker patch.
- Corrected doc-comment of drm_gem_shmem_object.madv, like was suggested
by Steven Price. Comment now says that madv=1 means "object is purged"
instead of saying that value is unused.
- Added more doc-comments to the new shmem shrinker API.
- The "Improve DMA API usage for shmem BOs" patch got more improvements
by removing the obsoleted drm_dev_set_unique() quirk and its comment.
- Added patch that makes Virtio-GPU driver to use common dev_is_pci()
helper, which was suggested by Robin Murphy.
- Added new "drm/shmem-helper: Take GEM reservation lock instead of
drm_gem_shmem locks" patch, which was suggested by Daniel Vetter.
- Added new "drm/virtio: Simplify error handling of
virtio_gpu_object_create()" patch.
- Improved "Correct doc-comment of drm_gem_shmem_get_sg_table()" patch,
like was suggested by Daniel Vetter, by saying that function returns
ERR_PTR() and not errno.
- virtio_gpu_purge_object() is fenced properly now, turned out
virtio_gpu_notify() doesn't do fencing as I was supposing before.
Stress testing of memory eviction revealed that.
- Added new patch that corrects virtio_gpu_plane_cleanup_fb() to use
appropriate atomic plane state.
- SHMEM shrinker got eviction support.
- VirtIO-GPU driver now supports memory eviction. It's enabled for a
non-blob GEMs only, i.e. for VirGL. The blobs don't support dynamic
attaching/detaching of guest's memory, so it's not trivial to enable
them.
- Added patch that removes obsoleted drm_gem_shmem_purge()
- Added patch that makes drm_gem_shmem_get_pages() private.
- Added patch that fixes lockup on dma_resv_reserve_fences() error.
v2: - Improved shrinker by using a more fine-grained locking to reduce
contention during scan of objects and dropped locking from the
'counting' callback by tracking count of shrinkable pages. This
was suggested by Rob Clark in the comment to v1.
- Factored out common shrinker code into drm_gem_shmem_helper.c
and switched Panfrost driver to use the new common memory shrinker.
This was proposed by Thomas Zimmermann in his prototype series that
he shared with us in the comment to v1. Note that I only compile-tested
the Panfrost driver.
- Shrinker now takes object_name_lock during scan to prevent racing
with dma-buf exporting.
- Shrinker now takes vmap_lock during scan to prevent racing with shmem
vmap/unmap code.
- Added "Correct doc-comment of drm_gem_shmem_get_sg_table()" patch,
which I sent out previously as a standalone change, since the
drm_gem_shmem_helper.c is now touched by this patchset anyways and
it doesn't hurt to group all the patches together.
Dmitry Osipenko (15):
drm/virtio: Correct drm_gem_shmem_get_sg_table() error handling
drm/virtio: Check whether transferred 2D BO is shmem
drm/virtio: Unlock GEM reservations on virtio_gpu_object_shmem_init()
error
drm/virtio: Unlock reservations on dma_resv_reserve_fences() error
drm/virtio: Use appropriate atomic state in
virtio_gpu_plane_cleanup_fb()
drm/virtio: Simplify error handling of virtio_gpu_object_create()
drm/virtio: Improve DMA API usage for shmem BOs
drm/virtio: Use dev_is_pci()
drm/shmem-helper: Correct doc-comment of drm_gem_shmem_get_sg_table()
drm/shmem-helper: Take reservation lock instead of drm_gem_shmem locks
drm/shmem-helper: Add generic memory shrinker
drm/virtio: Support memory shrinking
drm/panfrost: Switch to generic memory shrinker
drm/shmem-helper: Make drm_gem_shmem_get_pages() private
drm/shmem-helper: Remove drm_gem_shmem_purge()
drivers/gpu/drm/drm_gem_shmem_helper.c | 814 ++++++++++++++++--
drivers/gpu/drm/lima/lima_gem.c | 8 +-
drivers/gpu/drm/panfrost/Makefile | 1 -
drivers/gpu/drm/panfrost/panfrost_device.h | 4 -
drivers/gpu/drm/panfrost/panfrost_drv.c | 19 +-
drivers/gpu/drm/panfrost/panfrost_gem.c | 30 +-
drivers/gpu/drm/panfrost/panfrost_gem.h | 9 -
.../gpu/drm/panfrost/panfrost_gem_shrinker.c | 122 ---
drivers/gpu/drm/panfrost/panfrost_job.c | 18 +-
drivers/gpu/drm/panfrost/panfrost_mmu.c | 15 +-
drivers/gpu/drm/virtio/virtgpu_drv.c | 53 +-
drivers/gpu/drm/virtio/virtgpu_drv.h | 20 +-
drivers/gpu/drm/virtio/virtgpu_gem.c | 50 +-
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 37 +
drivers/gpu/drm/virtio/virtgpu_kms.c | 16 +-
drivers/gpu/drm/virtio/virtgpu_object.c | 204 +++--
drivers/gpu/drm/virtio/virtgpu_plane.c | 23 +-
drivers/gpu/drm/virtio/virtgpu_vq.c | 55 +-
include/drm/drm_device.h | 4 +
include/drm/drm_gem.h | 35 +
include/drm/drm_gem_shmem_helper.h | 114 ++-
include/uapi/drm/virtgpu_drm.h | 14 +
22 files changed, 1277 insertions(+), 388 deletions(-)
delete mode 100644 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
--
2.35.1
Unlock reservations on dma_resv_reserve_fences() error to fix recursive
locking of the reservations when this error happens.
Cc: [email protected]
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/virtio/virtgpu_gem.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 580a78809836..7db48d17ee3a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -228,8 +228,10 @@ int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs)
for (i = 0; i < objs->nents; ++i) {
ret = dma_resv_reserve_fences(objs->objs[i]->resv, 1);
- if (ret)
+ if (ret) {
+ virtio_gpu_array_unlock_resv(objs);
return ret;
+ }
}
return ret;
}
--
2.35.1
VirtIO-GPU driver was the only user of drm_gem_shmem_get_pages()
and it now uses drm_gem_shmem_get_pages_sgt(). Make the get_pages()
private to drm_gem_shmem_helper.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 3 +--
include/drm/drm_gem_shmem_helper.h | 1 -
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 3838fb8d6f3a..6d83d11211fc 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -537,7 +537,7 @@ static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
* Returns:
* 0 on success or a negative error code on failure.
*/
-int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
+static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
{
int ret;
@@ -554,7 +554,6 @@ int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
return ret;
}
-EXPORT_SYMBOL(drm_gem_shmem_get_pages);
static void drm_gem_shmem_get_pages_no_fail(struct drm_gem_shmem_object *shmem)
{
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index a65557b446e6..84316c281292 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -144,7 +144,6 @@ struct drm_gem_shmem_object {
struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size);
void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem);
-int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem);
void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem);
int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem);
void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem);
--
2.35.1
drm_gem_shmem_get_sg_table() never ever returned NULL on error. Correct
the error handling to avoid crash on OOM.
Cc: [email protected]
Reviewed-by: Emil Velikov <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/virtio/virtgpu_object.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index f293e6ad52da..3d0c8d4d1c20 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -168,9 +168,11 @@ static int virtio_gpu_object_shmem_init(struct virtio_gpu_device *vgdev,
* since virtio_gpu doesn't support dma-buf import from other devices.
*/
shmem->pages = drm_gem_shmem_get_sg_table(&bo->base);
- if (!shmem->pages) {
+ ret = PTR_ERR_OR_ZERO(shmem->pages);
+ if (ret) {
drm_gem_shmem_unpin(&bo->base);
- return -EINVAL;
+ shmem->pages = NULL;
+ return ret;
}
if (use_dma_api) {
--
2.35.1
DRM API requires the DRM's driver to be backed with the device that can
be used for generic DMA operations. The VirtIO-GPU device can't perform
DMA operations if it uses PCI transport because PCI device driver creates
a virtual VirtIO-GPU device that isn't associated with the PCI. Use PCI's
GPU device for the DRM's device instead of the VirtIO-GPU device and drop
DMA-related hacks from the VirtIO-GPU driver.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/virtio/virtgpu_drv.c | 51 ++++++----------------
drivers/gpu/drm/virtio/virtgpu_drv.h | 5 +--
drivers/gpu/drm/virtio/virtgpu_kms.c | 7 ++--
drivers/gpu/drm/virtio/virtgpu_object.c | 56 +++++--------------------
drivers/gpu/drm/virtio/virtgpu_vq.c | 13 +++---
5 files changed, 32 insertions(+), 100 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 5f25a8d15464..0141b7df97ec 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -46,12 +46,11 @@ static int virtio_gpu_modeset = -1;
MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
module_param_named(modeset, virtio_gpu_modeset, int, 0400);
-static int virtio_gpu_pci_quirk(struct drm_device *dev, struct virtio_device *vdev)
+static int virtio_gpu_pci_quirk(struct drm_device *dev)
{
- struct pci_dev *pdev = to_pci_dev(vdev->dev.parent);
+ struct pci_dev *pdev = to_pci_dev(dev->dev);
const char *pname = dev_name(&pdev->dev);
bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
- char unique[20];
int ret;
DRM_INFO("pci: %s detected at %s\n",
@@ -63,39 +62,7 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev, struct virtio_device *vd
return ret;
}
- /*
- * Normally the drm_dev_set_unique() call is done by core DRM.
- * The following comment covers, why virtio cannot rely on it.
- *
- * Unlike the other virtual GPU drivers, virtio abstracts the
- * underlying bus type by using struct virtio_device.
- *
- * Hence the dev_is_pci() check, used in core DRM, will fail
- * and the unique returned will be the virtio_device "virtio0",
- * while a "pci:..." one is required.
- *
- * A few other ideas were considered:
- * - Extend the dev_is_pci() check [in drm_set_busid] to
- * consider virtio.
- * Seems like a bigger hack than what we have already.
- *
- * - Point drm_device::dev to the parent of the virtio_device
- * Semantic changes:
- * * Using the wrong device for i2c, framebuffer_alloc and
- * prime import.
- * Visual changes:
- * * Helpers such as DRM_DEV_ERROR, dev_info, drm_printer,
- * will print the wrong information.
- *
- * We could address the latter issues, by introducing
- * drm_device::bus_dev, ... which would be used solely for this.
- *
- * So for the moment keep things as-is, with a bulky comment
- * for the next person who feels like removing this
- * drm_dev_set_unique() quirk.
- */
- snprintf(unique, sizeof(unique), "pci:%s", pname);
- return drm_dev_set_unique(dev, unique);
+ return 0;
}
static int virtio_gpu_probe(struct virtio_device *vdev)
@@ -109,18 +76,24 @@ static int virtio_gpu_probe(struct virtio_device *vdev)
if (virtio_gpu_modeset == 0)
return -EINVAL;
- dev = drm_dev_alloc(&driver, &vdev->dev);
+ /*
+ * The virtio-gpu device is a virtual device that doesn't have DMA
+ * ops assigned to it, nor DMA mask set and etc. Its parent device
+ * is actual GPU device we want to use it for the DRM's device in
+ * order to benefit from using generic DRM APIs.
+ */
+ dev = drm_dev_alloc(&driver, vdev->dev.parent);
if (IS_ERR(dev))
return PTR_ERR(dev);
vdev->priv = dev;
if (!strcmp(vdev->dev.parent->bus->name, "pci")) {
- ret = virtio_gpu_pci_quirk(dev, vdev);
+ ret = virtio_gpu_pci_quirk(dev);
if (ret)
goto err_free;
}
- ret = virtio_gpu_init(dev);
+ ret = virtio_gpu_init(vdev, dev);
if (ret)
goto err_free;
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 0a194aaad419..b2d93cb12ebf 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -100,8 +100,6 @@ struct virtio_gpu_object {
struct virtio_gpu_object_shmem {
struct virtio_gpu_object base;
- struct sg_table *pages;
- uint32_t mapped;
};
struct virtio_gpu_object_vram {
@@ -214,7 +212,6 @@ struct virtio_gpu_drv_cap_cache {
};
struct virtio_gpu_device {
- struct device *dev;
struct drm_device *ddev;
struct virtio_device *vdev;
@@ -282,7 +279,7 @@ extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
void virtio_gpu_create_context(struct drm_device *dev, struct drm_file *file);
/* virtgpu_kms.c */
-int virtio_gpu_init(struct drm_device *dev);
+int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev);
void virtio_gpu_deinit(struct drm_device *dev);
void virtio_gpu_release(struct drm_device *dev);
int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 3313b92db531..0d1e3eb61bee 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -110,7 +110,7 @@ static void virtio_gpu_get_capsets(struct virtio_gpu_device *vgdev,
vgdev->num_capsets = num_capsets;
}
-int virtio_gpu_init(struct drm_device *dev)
+int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
{
static vq_callback_t *callbacks[] = {
virtio_gpu_ctrl_ack, virtio_gpu_cursor_ack
@@ -123,7 +123,7 @@ int virtio_gpu_init(struct drm_device *dev)
u32 num_scanouts, num_capsets;
int ret = 0;
- if (!virtio_has_feature(dev_to_virtio(dev->dev), VIRTIO_F_VERSION_1))
+ if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
return -ENODEV;
vgdev = kzalloc(sizeof(struct virtio_gpu_device), GFP_KERNEL);
@@ -132,8 +132,7 @@ int virtio_gpu_init(struct drm_device *dev)
vgdev->ddev = dev;
dev->dev_private = vgdev;
- vgdev->vdev = dev_to_virtio(dev->dev);
- vgdev->dev = dev->dev;
+ vgdev->vdev = vdev;
spin_lock_init(&vgdev->display_info_lock);
spin_lock_init(&vgdev->resource_export_lock);
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index 18f70ef6b4d0..8d7728181de0 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -67,21 +67,6 @@ void virtio_gpu_cleanup_object(struct virtio_gpu_object *bo)
virtio_gpu_resource_id_put(vgdev, bo->hw_res_handle);
if (virtio_gpu_is_shmem(bo)) {
- struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);
-
- if (shmem->pages) {
- if (shmem->mapped) {
- dma_unmap_sgtable(vgdev->vdev->dev.parent,
- shmem->pages, DMA_TO_DEVICE, 0);
- shmem->mapped = 0;
- }
-
- sg_free_table(shmem->pages);
- kfree(shmem->pages);
- shmem->pages = NULL;
- drm_gem_shmem_unpin(&bo->base);
- }
-
drm_gem_shmem_free(&bo->base);
} else if (virtio_gpu_is_vram(bo)) {
struct virtio_gpu_object_vram *vram = to_virtio_gpu_vram(bo);
@@ -153,37 +138,18 @@ static int virtio_gpu_object_shmem_init(struct virtio_gpu_device *vgdev,
unsigned int *nents)
{
bool use_dma_api = !virtio_has_dma_quirk(vgdev->vdev);
- struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);
struct scatterlist *sg;
- int si, ret;
+ struct sg_table *pages;
+ int si;
- ret = drm_gem_shmem_pin(&bo->base);
- if (ret < 0)
- return -EINVAL;
-
- /*
- * virtio_gpu uses drm_gem_shmem_get_sg_table instead of
- * drm_gem_shmem_get_pages_sgt because virtio has it's own set of
- * dma-ops. This is discouraged for other drivers, but should be fine
- * since virtio_gpu doesn't support dma-buf import from other devices.
- */
- shmem->pages = drm_gem_shmem_get_sg_table(&bo->base);
- ret = PTR_ERR_OR_ZERO(shmem->pages);
- if (ret) {
- drm_gem_shmem_unpin(&bo->base);
- shmem->pages = NULL;
- return ret;
- }
+ pages = drm_gem_shmem_get_pages_sgt(&bo->base);
+ if (IS_ERR(pages))
+ return PTR_ERR(pages);
- if (use_dma_api) {
- ret = dma_map_sgtable(vgdev->vdev->dev.parent,
- shmem->pages, DMA_TO_DEVICE, 0);
- if (ret)
- return ret;
- *nents = shmem->mapped = shmem->pages->nents;
- } else {
- *nents = shmem->pages->orig_nents;
- }
+ if (use_dma_api)
+ *nents = pages->nents;
+ else
+ *nents = pages->orig_nents;
*ents = kvmalloc_array(*nents,
sizeof(struct virtio_gpu_mem_entry),
@@ -194,13 +160,13 @@ static int virtio_gpu_object_shmem_init(struct virtio_gpu_device *vgdev,
}
if (use_dma_api) {
- for_each_sgtable_dma_sg(shmem->pages, sg, si) {
+ for_each_sgtable_dma_sg(pages, sg, si) {
(*ents)[si].addr = cpu_to_le64(sg_dma_address(sg));
(*ents)[si].length = cpu_to_le32(sg_dma_len(sg));
(*ents)[si].padding = 0;
}
} else {
- for_each_sgtable_sg(shmem->pages, sg, si) {
+ for_each_sgtable_sg(pages, sg, si) {
(*ents)[si].addr = cpu_to_le64(sg_phys(sg));
(*ents)[si].length = cpu_to_le32(sg->length);
(*ents)[si].padding = 0;
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 2edf31806b74..06566e44307d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -593,11 +593,10 @@ void virtio_gpu_cmd_transfer_to_host_2d(struct virtio_gpu_device *vgdev,
struct virtio_gpu_transfer_to_host_2d *cmd_p;
struct virtio_gpu_vbuffer *vbuf;
bool use_dma_api = !virtio_has_dma_quirk(vgdev->vdev);
- struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);
if (virtio_gpu_is_shmem(bo) && use_dma_api)
- dma_sync_sgtable_for_device(vgdev->vdev->dev.parent,
- shmem->pages, DMA_TO_DEVICE);
+ dma_sync_sgtable_for_device(&vgdev->vdev->dev,
+ bo->base.sgt, DMA_TO_DEVICE);
cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
memset(cmd_p, 0, sizeof(*cmd_p));
@@ -1017,11 +1016,9 @@ void virtio_gpu_cmd_transfer_to_host_3d(struct virtio_gpu_device *vgdev,
struct virtio_gpu_vbuffer *vbuf;
bool use_dma_api = !virtio_has_dma_quirk(vgdev->vdev);
- if (virtio_gpu_is_shmem(bo) && use_dma_api) {
- struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);
- dma_sync_sgtable_for_device(vgdev->vdev->dev.parent,
- shmem->pages, DMA_TO_DEVICE);
- }
+ if (virtio_gpu_is_shmem(bo) && use_dma_api)
+ dma_sync_sgtable_for_device(&vgdev->vdev->dev,
+ bo->base.sgt, DMA_TO_DEVICE);
cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
memset(cmd_p, 0, sizeof(*cmd_p));
--
2.35.1
Change the order of SHMEM initialization and reservation locking to
make code cleaner a tad and to prepare to transitioning of the common
GEM SHMEM code to use the GEM's reservation lock instead of the
shmem.page_lock.
There is no need to lock reservation during allocation of the SHMEM pages
because the lock is needed only to avoid racing with the async host-side
allocation. Hence we can safely move the SHMEM initialization out of the
reservation lock.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/virtio/virtgpu_object.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index 21c19cdedce0..18f70ef6b4d0 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -236,6 +236,10 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
bo->dumb = params->dumb;
+ ret = virtio_gpu_object_shmem_init(vgdev, bo, &ents, &nents);
+ if (ret != 0)
+ goto err_put_id;
+
if (fence) {
ret = -ENOMEM;
objs = virtio_gpu_array_alloc(1);
@@ -248,15 +252,6 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
goto err_put_objs;
}
- ret = virtio_gpu_object_shmem_init(vgdev, bo, &ents, &nents);
- if (ret != 0) {
- if (fence)
- virtio_gpu_array_unlock_resv(objs);
- virtio_gpu_array_put_free(objs);
- virtio_gpu_free_object(&shmem_obj->base);
- return ret;
- }
-
if (params->blob) {
if (params->blob_mem == VIRTGPU_BLOB_MEM_GUEST)
bo->guest_blob = true;
--
2.35.1
Replace drm_gem_shmem locks with the reservation lock to make GEM
lockings more consistent.
Previously drm_gem_shmem_vmap() and drm_gem_shmem_get_pages() were
protected by separate locks, now it's the same lock, but it doesn't
make any difference for the current GEM SHMEM users. Only Panfrost
and Lima drivers use vmap() and they do it in the slow code paths,
hence there was no practical justification for the usage of separate
lock in the vmap().
Suggested-by: Daniel Vetter <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 38 ++++++++++++-------------
drivers/gpu/drm/lima/lima_gem.c | 8 +++---
drivers/gpu/drm/panfrost/panfrost_mmu.c | 15 ++++++----
include/drm/drm_gem_shmem_helper.h | 10 -------
4 files changed, 31 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 30ee46348a99..3ecef571eff3 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -86,8 +86,6 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
if (ret)
goto err_release;
- mutex_init(&shmem->pages_lock);
- mutex_init(&shmem->vmap_lock);
INIT_LIST_HEAD(&shmem->madv_list);
if (!private) {
@@ -157,8 +155,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
WARN_ON(shmem->pages_use_count);
drm_gem_object_release(obj);
- mutex_destroy(&shmem->pages_lock);
- mutex_destroy(&shmem->vmap_lock);
kfree(shmem);
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
@@ -209,11 +205,11 @@ int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
WARN_ON(shmem->base.import_attach);
- ret = mutex_lock_interruptible(&shmem->pages_lock);
+ ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
if (ret)
return ret;
ret = drm_gem_shmem_get_pages_locked(shmem);
- mutex_unlock(&shmem->pages_lock);
+ dma_resv_unlock(shmem->base.resv);
return ret;
}
@@ -248,9 +244,9 @@ static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem)
*/
void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
{
- mutex_lock(&shmem->pages_lock);
+ dma_resv_lock(shmem->base.resv, NULL);
drm_gem_shmem_put_pages_locked(shmem);
- mutex_unlock(&shmem->pages_lock);
+ dma_resv_unlock(shmem->base.resv);
}
EXPORT_SYMBOL(drm_gem_shmem_put_pages);
@@ -310,7 +306,7 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
} else {
pgprot_t prot = PAGE_KERNEL;
- ret = drm_gem_shmem_get_pages(shmem);
+ ret = drm_gem_shmem_get_pages_locked(shmem);
if (ret)
goto err_zero_use;
@@ -360,11 +356,11 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
{
int ret;
- ret = mutex_lock_interruptible(&shmem->vmap_lock);
+ ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
if (ret)
return ret;
ret = drm_gem_shmem_vmap_locked(shmem, map);
- mutex_unlock(&shmem->vmap_lock);
+ dma_resv_unlock(shmem->base.resv);
return ret;
}
@@ -385,7 +381,7 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
dma_buf_vunmap(obj->import_attach->dmabuf, map);
} else {
vunmap(shmem->vaddr);
- drm_gem_shmem_put_pages(shmem);
+ drm_gem_shmem_put_pages_locked(shmem);
}
shmem->vaddr = NULL;
@@ -406,9 +402,11 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
struct iosys_map *map)
{
- mutex_lock(&shmem->vmap_lock);
+ dma_resv_lock(shmem->base.resv, NULL);
drm_gem_shmem_vunmap_locked(shmem, map);
- mutex_unlock(&shmem->vmap_lock);
+ dma_resv_unlock(shmem->base.resv);
+
+ drm_gem_shmem_update_purgeable_status(shmem);
}
EXPORT_SYMBOL(drm_gem_shmem_vunmap);
@@ -442,14 +440,14 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
*/
int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
{
- mutex_lock(&shmem->pages_lock);
+ dma_resv_lock(shmem->base.resv, NULL);
if (shmem->madv >= 0)
shmem->madv = madv;
madv = shmem->madv;
- mutex_unlock(&shmem->pages_lock);
+ dma_resv_unlock(shmem->base.resv);
return (madv >= 0);
}
@@ -487,10 +485,10 @@ EXPORT_SYMBOL(drm_gem_shmem_purge_locked);
bool drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem)
{
- if (!mutex_trylock(&shmem->pages_lock))
+ if (!dma_resv_trylock(shmem->base.resv))
return false;
drm_gem_shmem_purge_locked(shmem);
- mutex_unlock(&shmem->pages_lock);
+ dma_resv_unlock(shmem->base.resv);
return true;
}
@@ -549,7 +547,7 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
/* We don't use vmf->pgoff since that has the fake offset */
page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
- mutex_lock(&shmem->pages_lock);
+ dma_resv_lock(shmem->base.resv, NULL);
if (page_offset >= num_pages ||
WARN_ON_ONCE(!shmem->pages) ||
@@ -561,7 +559,7 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
ret = vmf_insert_pfn(vma, vmf->address, page_to_pfn(page));
}
- mutex_unlock(&shmem->pages_lock);
+ dma_resv_unlock(shmem->base.resv);
return ret;
}
diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
index 0f1ca0b0db49..5008f0c2428f 100644
--- a/drivers/gpu/drm/lima/lima_gem.c
+++ b/drivers/gpu/drm/lima/lima_gem.c
@@ -34,7 +34,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
new_size = min(new_size, bo->base.base.size);
- mutex_lock(&bo->base.pages_lock);
+ dma_resv_lock(bo->base.base.resv, NULL);
if (bo->base.pages) {
pages = bo->base.pages;
@@ -42,7 +42,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
pages = kvmalloc_array(bo->base.base.size >> PAGE_SHIFT,
sizeof(*pages), GFP_KERNEL | __GFP_ZERO);
if (!pages) {
- mutex_unlock(&bo->base.pages_lock);
+ dma_resv_unlock(bo->base.base.resv);
return -ENOMEM;
}
@@ -56,13 +56,13 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
struct page *page = shmem_read_mapping_page(mapping, i);
if (IS_ERR(page)) {
- mutex_unlock(&bo->base.pages_lock);
+ dma_resv_unlock(bo->base.base.resv);
return PTR_ERR(page);
}
pages[i] = page;
}
- mutex_unlock(&bo->base.pages_lock);
+ dma_resv_unlock(bo->base.base.resv);
ret = sg_alloc_table_from_pages(&sgt, pages, i, 0,
new_size, GFP_KERNEL);
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index d3f82b26a631..404b8f67e2df 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -424,6 +424,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
struct panfrost_gem_mapping *bomapping;
struct panfrost_gem_object *bo;
struct address_space *mapping;
+ struct drm_gem_object *obj;
pgoff_t page_offset;
struct sg_table *sgt;
struct page **pages;
@@ -446,13 +447,15 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
page_offset = addr >> PAGE_SHIFT;
page_offset -= bomapping->mmnode.start;
- mutex_lock(&bo->base.pages_lock);
+ obj = &bo->base.base;
+
+ dma_resv_lock(obj->resv, NULL);
if (!bo->base.pages) {
bo->sgts = kvmalloc_array(bo->base.base.size / SZ_2M,
sizeof(struct sg_table), GFP_KERNEL | __GFP_ZERO);
if (!bo->sgts) {
- mutex_unlock(&bo->base.pages_lock);
+ dma_resv_unlock(obj->resv);
ret = -ENOMEM;
goto err_bo;
}
@@ -462,7 +465,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
if (!pages) {
kvfree(bo->sgts);
bo->sgts = NULL;
- mutex_unlock(&bo->base.pages_lock);
+ dma_resv_unlock(obj->resv);
ret = -ENOMEM;
goto err_bo;
}
@@ -472,7 +475,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
pages = bo->base.pages;
if (pages[page_offset]) {
/* Pages are already mapped, bail out. */
- mutex_unlock(&bo->base.pages_lock);
+ dma_resv_unlock(obj->resv);
goto out;
}
}
@@ -483,13 +486,13 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
for (i = page_offset; i < page_offset + NUM_FAULT_PAGES; i++) {
pages[i] = shmem_read_mapping_page(mapping, i);
if (IS_ERR(pages[i])) {
- mutex_unlock(&bo->base.pages_lock);
+ dma_resv_unlock(obj->resv);
ret = PTR_ERR(pages[i]);
goto err_pages;
}
}
- mutex_unlock(&bo->base.pages_lock);
+ dma_resv_unlock(obj->resv);
sgt = &bo->sgts[page_offset / (SZ_2M / PAGE_SIZE)];
ret = sg_alloc_table_from_pages(sgt, pages + page_offset,
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index d0a57853c188..70889533962a 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -26,11 +26,6 @@ struct drm_gem_shmem_object {
*/
struct drm_gem_object base;
- /**
- * @pages_lock: Protects the page table and use count
- */
- struct mutex pages_lock;
-
/**
* @pages: Page table
*/
@@ -79,11 +74,6 @@ struct drm_gem_shmem_object {
*/
struct sg_table *sgt;
- /**
- * @vmap_lock: Protects the vmap address and use count
- */
- struct mutex vmap_lock;
-
/**
* @vaddr: Kernel virtual address of the backing memory
*/
--
2.35.1
Hi
Am 18.04.22 um 00:37 schrieb Dmitry Osipenko:
> Replace drm_gem_shmem locks with the reservation lock to make GEM
> lockings more consistent.
>
> Previously drm_gem_shmem_vmap() and drm_gem_shmem_get_pages() were
> protected by separate locks, now it's the same lock, but it doesn't
> make any difference for the current GEM SHMEM users. Only Panfrost
> and Lima drivers use vmap() and they do it in the slow code paths,
> hence there was no practical justification for the usage of separate
> lock in the vmap().
>
> Suggested-by: Daniel Vetter <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 38 ++++++++++++-------------
> drivers/gpu/drm/lima/lima_gem.c | 8 +++---
> drivers/gpu/drm/panfrost/panfrost_mmu.c | 15 ++++++----
> include/drm/drm_gem_shmem_helper.h | 10 -------
> 4 files changed, 31 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 30ee46348a99..3ecef571eff3 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -86,8 +86,6 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
> if (ret)
> goto err_release;
>
> - mutex_init(&shmem->pages_lock);
> - mutex_init(&shmem->vmap_lock);
> INIT_LIST_HEAD(&shmem->madv_list);
>
> if (!private) {
> @@ -157,8 +155,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
> WARN_ON(shmem->pages_use_count);
>
> drm_gem_object_release(obj);
> - mutex_destroy(&shmem->pages_lock);
> - mutex_destroy(&shmem->vmap_lock);
> kfree(shmem);
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
> @@ -209,11 +205,11 @@ int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
>
> WARN_ON(shmem->base.import_attach);
>
> - ret = mutex_lock_interruptible(&shmem->pages_lock);
> + ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
> if (ret)
> return ret;
> ret = drm_gem_shmem_get_pages_locked(shmem);
> - mutex_unlock(&shmem->pages_lock);
> + dma_resv_unlock(shmem->base.resv);
>
> return ret;
> }
> @@ -248,9 +244,9 @@ static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem)
> */
> void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
> {
> - mutex_lock(&shmem->pages_lock);
> + dma_resv_lock(shmem->base.resv, NULL);
> drm_gem_shmem_put_pages_locked(shmem);
> - mutex_unlock(&shmem->pages_lock);
> + dma_resv_unlock(shmem->base.resv);
> }
> EXPORT_SYMBOL(drm_gem_shmem_put_pages);
>
> @@ -310,7 +306,7 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
> } else {
> pgprot_t prot = PAGE_KERNEL;
>
> - ret = drm_gem_shmem_get_pages(shmem);
> + ret = drm_gem_shmem_get_pages_locked(shmem);
> if (ret)
> goto err_zero_use;
>
> @@ -360,11 +356,11 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
> {
> int ret;
>
> - ret = mutex_lock_interruptible(&shmem->vmap_lock);
> + ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
> if (ret)
> return ret;
> ret = drm_gem_shmem_vmap_locked(shmem, map);
Within drm_gem_shmem_vmap_locked(), there's a call to dma_buf_vmap() for
imported pages. If the exporter side also holds/acquires the same
reservation lock as our object, the whole thing can deadlock. We cannot
move dma_buf_vmap() out of the CS, because we still need to increment
the reference counter. I honestly don't know how to easily fix this
problem. There's a TODO item about replacing these locks at [1]. As
Daniel suggested this patch, we should talk to him about the issue.
Best regards
Thomas
[1]
https://www.kernel.org/doc/html/latest/gpu/todo.html#move-buffer-object-locking-to-dma-resv-lock
> - mutex_unlock(&shmem->vmap_lock);
> + dma_resv_unlock(shmem->base.resv);
>
> return ret;
> }
> @@ -385,7 +381,7 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
> dma_buf_vunmap(obj->import_attach->dmabuf, map);
> } else {
> vunmap(shmem->vaddr);
> - drm_gem_shmem_put_pages(shmem);
> + drm_gem_shmem_put_pages_locked(shmem);
> }
>
> shmem->vaddr = NULL;
> @@ -406,9 +402,11 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
> void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
> struct iosys_map *map)
> {
> - mutex_lock(&shmem->vmap_lock);
> + dma_resv_lock(shmem->base.resv, NULL);
> drm_gem_shmem_vunmap_locked(shmem, map);
> - mutex_unlock(&shmem->vmap_lock);
> + dma_resv_unlock(shmem->base.resv);
> +
> + drm_gem_shmem_update_purgeable_status(shmem);
> }
> EXPORT_SYMBOL(drm_gem_shmem_vunmap);
>
> @@ -442,14 +440,14 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
> */
> int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
> {
> - mutex_lock(&shmem->pages_lock);
> + dma_resv_lock(shmem->base.resv, NULL);
>
> if (shmem->madv >= 0)
> shmem->madv = madv;
>
> madv = shmem->madv;
>
> - mutex_unlock(&shmem->pages_lock);
> + dma_resv_unlock(shmem->base.resv);
>
> return (madv >= 0);
> }
> @@ -487,10 +485,10 @@ EXPORT_SYMBOL(drm_gem_shmem_purge_locked);
>
> bool drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem)
> {
> - if (!mutex_trylock(&shmem->pages_lock))
> + if (!dma_resv_trylock(shmem->base.resv))
> return false;
> drm_gem_shmem_purge_locked(shmem);
> - mutex_unlock(&shmem->pages_lock);
> + dma_resv_unlock(shmem->base.resv);
>
> return true;
> }
> @@ -549,7 +547,7 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
> /* We don't use vmf->pgoff since that has the fake offset */
> page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
>
> - mutex_lock(&shmem->pages_lock);
> + dma_resv_lock(shmem->base.resv, NULL);
>
> if (page_offset >= num_pages ||
> WARN_ON_ONCE(!shmem->pages) ||
> @@ -561,7 +559,7 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
> ret = vmf_insert_pfn(vma, vmf->address, page_to_pfn(page));
> }
>
> - mutex_unlock(&shmem->pages_lock);
> + dma_resv_unlock(shmem->base.resv);
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
> index 0f1ca0b0db49..5008f0c2428f 100644
> --- a/drivers/gpu/drm/lima/lima_gem.c
> +++ b/drivers/gpu/drm/lima/lima_gem.c
> @@ -34,7 +34,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
>
> new_size = min(new_size, bo->base.base.size);
>
> - mutex_lock(&bo->base.pages_lock);
> + dma_resv_lock(bo->base.base.resv, NULL);
>
> if (bo->base.pages) {
> pages = bo->base.pages;
> @@ -42,7 +42,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
> pages = kvmalloc_array(bo->base.base.size >> PAGE_SHIFT,
> sizeof(*pages), GFP_KERNEL | __GFP_ZERO);
> if (!pages) {
> - mutex_unlock(&bo->base.pages_lock);
> + dma_resv_unlock(bo->base.base.resv);
> return -ENOMEM;
> }
>
> @@ -56,13 +56,13 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
> struct page *page = shmem_read_mapping_page(mapping, i);
>
> if (IS_ERR(page)) {
> - mutex_unlock(&bo->base.pages_lock);
> + dma_resv_unlock(bo->base.base.resv);
> return PTR_ERR(page);
> }
> pages[i] = page;
> }
>
> - mutex_unlock(&bo->base.pages_lock);
> + dma_resv_unlock(bo->base.base.resv);
>
> ret = sg_alloc_table_from_pages(&sgt, pages, i, 0,
> new_size, GFP_KERNEL);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index d3f82b26a631..404b8f67e2df 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -424,6 +424,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
> struct panfrost_gem_mapping *bomapping;
> struct panfrost_gem_object *bo;
> struct address_space *mapping;
> + struct drm_gem_object *obj;
> pgoff_t page_offset;
> struct sg_table *sgt;
> struct page **pages;
> @@ -446,13 +447,15 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
> page_offset = addr >> PAGE_SHIFT;
> page_offset -= bomapping->mmnode.start;
>
> - mutex_lock(&bo->base.pages_lock);
> + obj = &bo->base.base;
> +
> + dma_resv_lock(obj->resv, NULL);
>
> if (!bo->base.pages) {
> bo->sgts = kvmalloc_array(bo->base.base.size / SZ_2M,
> sizeof(struct sg_table), GFP_KERNEL | __GFP_ZERO);
> if (!bo->sgts) {
> - mutex_unlock(&bo->base.pages_lock);
> + dma_resv_unlock(obj->resv);
> ret = -ENOMEM;
> goto err_bo;
> }
> @@ -462,7 +465,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
> if (!pages) {
> kvfree(bo->sgts);
> bo->sgts = NULL;
> - mutex_unlock(&bo->base.pages_lock);
> + dma_resv_unlock(obj->resv);
> ret = -ENOMEM;
> goto err_bo;
> }
> @@ -472,7 +475,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
> pages = bo->base.pages;
> if (pages[page_offset]) {
> /* Pages are already mapped, bail out. */
> - mutex_unlock(&bo->base.pages_lock);
> + dma_resv_unlock(obj->resv);
> goto out;
> }
> }
> @@ -483,13 +486,13 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
> for (i = page_offset; i < page_offset + NUM_FAULT_PAGES; i++) {
> pages[i] = shmem_read_mapping_page(mapping, i);
> if (IS_ERR(pages[i])) {
> - mutex_unlock(&bo->base.pages_lock);
> + dma_resv_unlock(obj->resv);
> ret = PTR_ERR(pages[i]);
> goto err_pages;
> }
> }
>
> - mutex_unlock(&bo->base.pages_lock);
> + dma_resv_unlock(obj->resv);
>
> sgt = &bo->sgts[page_offset / (SZ_2M / PAGE_SIZE)];
> ret = sg_alloc_table_from_pages(sgt, pages + page_offset,
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index d0a57853c188..70889533962a 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -26,11 +26,6 @@ struct drm_gem_shmem_object {
> */
> struct drm_gem_object base;
>
> - /**
> - * @pages_lock: Protects the page table and use count
> - */
> - struct mutex pages_lock;
> -
> /**
> * @pages: Page table
> */
> @@ -79,11 +74,6 @@ struct drm_gem_shmem_object {
> */
> struct sg_table *sgt;
>
> - /**
> - * @vmap_lock: Protects the vmap address and use count
> - */
> - struct mutex vmap_lock;
> -
> /**
> * @vaddr: Kernel virtual address of the backing memory
> */
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Hello,
On 4/18/22 21:38, Thomas Zimmermann wrote:
> Hi
>
> Am 18.04.22 um 00:37 schrieb Dmitry Osipenko:
>> Replace drm_gem_shmem locks with the reservation lock to make GEM
>> lockings more consistent.
>>
>> Previously drm_gem_shmem_vmap() and drm_gem_shmem_get_pages() were
>> protected by separate locks, now it's the same lock, but it doesn't
>> make any difference for the current GEM SHMEM users. Only Panfrost
>> and Lima drivers use vmap() and they do it in the slow code paths,
>> hence there was no practical justification for the usage of separate
>> lock in the vmap().
>>
>> Suggested-by: Daniel Vetter <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
...
>> @@ -310,7 +306,7 @@ static int drm_gem_shmem_vmap_locked(struct
>> drm_gem_shmem_object *shmem,
>> } else {
>> pgprot_t prot = PAGE_KERNEL;
>> - ret = drm_gem_shmem_get_pages(shmem);
>> + ret = drm_gem_shmem_get_pages_locked(shmem);
>> if (ret)
>> goto err_zero_use;
>> @@ -360,11 +356,11 @@ int drm_gem_shmem_vmap(struct
>> drm_gem_shmem_object *shmem,
>> {
>> int ret;
>> - ret = mutex_lock_interruptible(&shmem->vmap_lock);
>> + ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
>> if (ret)
>> return ret;
>> ret = drm_gem_shmem_vmap_locked(shmem, map);
>
> Within drm_gem_shmem_vmap_locked(), there's a call to dma_buf_vmap() for
> imported pages. If the exporter side also holds/acquires the same
> reservation lock as our object, the whole thing can deadlock. We cannot
> move dma_buf_vmap() out of the CS, because we still need to increment
> the reference counter. I honestly don't know how to easily fix this
> problem. There's a TODO item about replacing these locks at [1]. As
> Daniel suggested this patch, we should talk to him about the issue.
>
> Best regards
> Thomas
>
> [1]
> https://www.kernel.org/doc/html/latest/gpu/todo.html#move-buffer-object-locking-to-dma-resv-lock
Indeed, good catch! Perhaps we could simply use a separate lock for the
vmapping of the *imported* GEMs? The vmap_use_count is used only by
vmap/vunmap, so it doesn't matter which lock is used by these functions
in the case of imported GEMs since we only need to protect the
vmap_use_count.
On Mon, Apr 18, 2022 at 10:18:54PM +0300, Dmitry Osipenko wrote:
> Hello,
>
> On 4/18/22 21:38, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 18.04.22 um 00:37 schrieb Dmitry Osipenko:
> >> Replace drm_gem_shmem locks with the reservation lock to make GEM
> >> lockings more consistent.
> >>
> >> Previously drm_gem_shmem_vmap() and drm_gem_shmem_get_pages() were
> >> protected by separate locks, now it's the same lock, but it doesn't
> >> make any difference for the current GEM SHMEM users. Only Panfrost
> >> and Lima drivers use vmap() and they do it in the slow code paths,
> >> hence there was no practical justification for the usage of separate
> >> lock in the vmap().
> >>
> >> Suggested-by: Daniel Vetter <[email protected]>
> >> Signed-off-by: Dmitry Osipenko <[email protected]>
> >> ---
> ...
> >> ? @@ -310,7 +306,7 @@ static int drm_gem_shmem_vmap_locked(struct
> >> drm_gem_shmem_object *shmem,
> >> ????? } else {
> >> ????????? pgprot_t prot = PAGE_KERNEL;
> >> ? -??????? ret = drm_gem_shmem_get_pages(shmem);
> >> +??????? ret = drm_gem_shmem_get_pages_locked(shmem);
> >> ????????? if (ret)
> >> ????????????? goto err_zero_use;
> >> ? @@ -360,11 +356,11 @@ int drm_gem_shmem_vmap(struct
> >> drm_gem_shmem_object *shmem,
> >> ? {
> >> ????? int ret;
> >> ? -??? ret = mutex_lock_interruptible(&shmem->vmap_lock);
> >> +??? ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
> >> ????? if (ret)
> >> ????????? return ret;
> >> ????? ret = drm_gem_shmem_vmap_locked(shmem, map);
> >
> > Within drm_gem_shmem_vmap_locked(), there's a call to dma_buf_vmap() for
> > imported pages. If the exporter side also holds/acquires the same
> > reservation lock as our object, the whole thing can deadlock. We cannot
> > move dma_buf_vmap() out of the CS, because we still need to increment
> > the reference counter. I honestly don't know how to easily fix this
> > problem. There's a TODO item about replacing these locks at [1]. As
> > Daniel suggested this patch, we should talk to him about the issue.
> >
> > Best regards
> > Thomas
> >
> > [1]
> > https://www.kernel.org/doc/html/latest/gpu/todo.html#move-buffer-object-locking-to-dma-resv-lock
>
> Indeed, good catch! Perhaps we could simply use a separate lock for the
> vmapping of the *imported* GEMs? The vmap_use_count is used only by
> vmap/vunmap, so it doesn't matter which lock is used by these functions
> in the case of imported GEMs since we only need to protect the
> vmap_use_count.
Apologies for the late reply, I'm flooded.
I discussed this with Daniel Stone last week in a chat, roughly what we
need to do is:
1. Pick a function from shmem helpers.
2. Go through all drivers that call this, and make sure that we acquire
dma_resv_lock in the top level driver entry point for this.
3. Once all driver code paths are converted, add a dma_resv_assert_held()
call to that function to make sure you have it all correctly.
4. Repeate 1-3 until all shmem helper functions are converted over.
5. Ditch the 3 different shmem helper locks.
The trouble is that I forgot that vmap is a thing, so that needs more
work. I think there's two approaches here:
- Do the vmap at import time. This is the trick we used to untangle the
dma_resv_lock issues around dma_buf_attachment_map()
- Change the dma_buf_vmap rules that callers must hold the dma_resv_lock.
- Maybe also do what you suggest and keep a separate lock for this, but
the fundamental issue is that this doesn't really work - if you share
buffers both ways with two drivers using shmem helpers, then the
ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and
you can get some nice deadlocks. So not a great approach (and also the
reason why we really need to get everyone to move towards dma_resv_lock
as _the_ buffer object lock, since otherwise we'll never get a
consistent lock nesting hierarchy).
The trouble here is that trying to be clever and doing the conversion just
in shmem helpers wont work, because there's a lot of cases where the
drivers are all kinds of inconsistent with their locking.
Adding Daniel S, also maybe for questions it'd be fastest to chat on irc?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hello Daniel,
27.04.2022 17:50, Daniel Vetter пишет:
> On Mon, Apr 18, 2022 at 10:18:54PM +0300, Dmitry Osipenko wrote:
>> Hello,
>>
>> On 4/18/22 21:38, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 18.04.22 um 00:37 schrieb Dmitry Osipenko:
>>>> Replace drm_gem_shmem locks with the reservation lock to make GEM
>>>> lockings more consistent.
>>>>
>>>> Previously drm_gem_shmem_vmap() and drm_gem_shmem_get_pages() were
>>>> protected by separate locks, now it's the same lock, but it doesn't
>>>> make any difference for the current GEM SHMEM users. Only Panfrost
>>>> and Lima drivers use vmap() and they do it in the slow code paths,
>>>> hence there was no practical justification for the usage of separate
>>>> lock in the vmap().
>>>>
>>>> Suggested-by: Daniel Vetter <[email protected]>
>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>>> ---
>> ...
>>>> @@ -310,7 +306,7 @@ static int drm_gem_shmem_vmap_locked(struct
>>>> drm_gem_shmem_object *shmem,
>>>> } else {
>>>> pgprot_t prot = PAGE_KERNEL;
>>>> - ret = drm_gem_shmem_get_pages(shmem);
>>>> + ret = drm_gem_shmem_get_pages_locked(shmem);
>>>> if (ret)
>>>> goto err_zero_use;
>>>> @@ -360,11 +356,11 @@ int drm_gem_shmem_vmap(struct
>>>> drm_gem_shmem_object *shmem,
>>>> {
>>>> int ret;
>>>> - ret = mutex_lock_interruptible(&shmem->vmap_lock);
>>>> + ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
>>>> if (ret)
>>>> return ret;
>>>> ret = drm_gem_shmem_vmap_locked(shmem, map);
>>>
>>> Within drm_gem_shmem_vmap_locked(), there's a call to dma_buf_vmap() for
>>> imported pages. If the exporter side also holds/acquires the same
>>> reservation lock as our object, the whole thing can deadlock. We cannot
>>> move dma_buf_vmap() out of the CS, because we still need to increment
>>> the reference counter. I honestly don't know how to easily fix this
>>> problem. There's a TODO item about replacing these locks at [1]. As
>>> Daniel suggested this patch, we should talk to him about the issue.
>>>
>>> Best regards
>>> Thomas
>>>
>>> [1]
>>> https://www.kernel.org/doc/html/latest/gpu/todo.html#move-buffer-object-locking-to-dma-resv-lock
>>
>> Indeed, good catch! Perhaps we could simply use a separate lock for the
>> vmapping of the *imported* GEMs? The vmap_use_count is used only by
>> vmap/vunmap, so it doesn't matter which lock is used by these functions
>> in the case of imported GEMs since we only need to protect the
>> vmap_use_count.
>
> Apologies for the late reply, I'm flooded.
>
> I discussed this with Daniel Stone last week in a chat, roughly what we
> need to do is:
>
> 1. Pick a function from shmem helpers.
>
> 2. Go through all drivers that call this, and make sure that we acquire
> dma_resv_lock in the top level driver entry point for this.
>
> 3. Once all driver code paths are converted, add a dma_resv_assert_held()
> call to that function to make sure you have it all correctly.
> 4. Repeate 1-3 until all shmem helper functions are converted over.
Somehow I didn't notice the existence of dma_resv_assert_held(), thank
you for the suggestion :)
>
> 5. Ditch the 3 different shmem helper locks.
>
> The trouble is that I forgot that vmap is a thing, so that needs more
> work. I think there's two approaches here:
> - Do the vmap at import time. This is the trick we used to untangle the
> dma_resv_lock issues around dma_buf_attachment_map()
> - Change the dma_buf_vmap rules that callers must hold the dma_resv_lock.
I'll consider this option for v6, thank you.
I see now that you actually want to define the new rules for the
dma-bufs in general and not only in the context of the DRM code, this
now makes much more sense to me.
> - Maybe also do what you suggest and keep a separate lock for this, but
> the fundamental issue is that this doesn't really work - if you share
> buffers both ways with two drivers using shmem helpers, then the
> ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and
> you can get some nice deadlocks. So not a great approach (and also the
> reason why we really need to get everyone to move towards dma_resv_lock
> as _the_ buffer object lock, since otherwise we'll never get a
> consistent lock nesting hierarchy).
The separate locks should work okay because it will be always the
exporter that takes the dma_resv_lock. But I agree that it's less ideal
than defining the new rules for dma-bufs since sometime you will take
the resv lock and sometime not, potentially hiding bugs related to lockings.
> The trouble here is that trying to be clever and doing the conversion just
> in shmem helpers wont work, because there's a lot of cases where the
> drivers are all kinds of inconsistent with their locking.
>
> Adding Daniel S, also maybe for questions it'd be fastest to chat on irc?
My nickname is digetx on the #dri-devel channel, feel free to ping me if
needed. Right now yours suggestions are clear to me, hence no extra
questions.
Thank you for the review.
On Thu, Apr 28, 2022 at 09:31:00PM +0300, Dmitry Osipenko wrote:
> Hello Daniel,
>
> 27.04.2022 17:50, Daniel Vetter пишет:
> > On Mon, Apr 18, 2022 at 10:18:54PM +0300, Dmitry Osipenko wrote:
> >> Hello,
> >>
> >> On 4/18/22 21:38, Thomas Zimmermann wrote:
> >>> Hi
> >>>
> >>> Am 18.04.22 um 00:37 schrieb Dmitry Osipenko:
> >>>> Replace drm_gem_shmem locks with the reservation lock to make GEM
> >>>> lockings more consistent.
> >>>>
> >>>> Previously drm_gem_shmem_vmap() and drm_gem_shmem_get_pages() were
> >>>> protected by separate locks, now it's the same lock, but it doesn't
> >>>> make any difference for the current GEM SHMEM users. Only Panfrost
> >>>> and Lima drivers use vmap() and they do it in the slow code paths,
> >>>> hence there was no practical justification for the usage of separate
> >>>> lock in the vmap().
> >>>>
> >>>> Suggested-by: Daniel Vetter <[email protected]>
> >>>> Signed-off-by: Dmitry Osipenko <[email protected]>
> >>>> ---
> >> ...
> >>>> @@ -310,7 +306,7 @@ static int drm_gem_shmem_vmap_locked(struct
> >>>> drm_gem_shmem_object *shmem,
> >>>> } else {
> >>>> pgprot_t prot = PAGE_KERNEL;
> >>>> - ret = drm_gem_shmem_get_pages(shmem);
> >>>> + ret = drm_gem_shmem_get_pages_locked(shmem);
> >>>> if (ret)
> >>>> goto err_zero_use;
> >>>> @@ -360,11 +356,11 @@ int drm_gem_shmem_vmap(struct
> >>>> drm_gem_shmem_object *shmem,
> >>>> {
> >>>> int ret;
> >>>> - ret = mutex_lock_interruptible(&shmem->vmap_lock);
> >>>> + ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
> >>>> if (ret)
> >>>> return ret;
> >>>> ret = drm_gem_shmem_vmap_locked(shmem, map);
> >>>
> >>> Within drm_gem_shmem_vmap_locked(), there's a call to dma_buf_vmap() for
> >>> imported pages. If the exporter side also holds/acquires the same
> >>> reservation lock as our object, the whole thing can deadlock. We cannot
> >>> move dma_buf_vmap() out of the CS, because we still need to increment
> >>> the reference counter. I honestly don't know how to easily fix this
> >>> problem. There's a TODO item about replacing these locks at [1]. As
> >>> Daniel suggested this patch, we should talk to him about the issue.
> >>>
> >>> Best regards
> >>> Thomas
> >>>
> >>> [1]
> >>> https://www.kernel.org/doc/html/latest/gpu/todo.html#move-buffer-object-locking-to-dma-resv-lock
> >>
> >> Indeed, good catch! Perhaps we could simply use a separate lock for the
> >> vmapping of the *imported* GEMs? The vmap_use_count is used only by
> >> vmap/vunmap, so it doesn't matter which lock is used by these functions
> >> in the case of imported GEMs since we only need to protect the
> >> vmap_use_count.
> >
> > Apologies for the late reply, I'm flooded.
> >
> > I discussed this with Daniel Stone last week in a chat, roughly what we
> > need to do is:
> >
> > 1. Pick a function from shmem helpers.
> >
> > 2. Go through all drivers that call this, and make sure that we acquire
> > dma_resv_lock in the top level driver entry point for this.
> >
> > 3. Once all driver code paths are converted, add a dma_resv_assert_held()
> > call to that function to make sure you have it all correctly.
> > 4. Repeate 1-3 until all shmem helper functions are converted over.
> Somehow I didn't notice the existence of dma_resv_assert_held(), thank
> you for the suggestion :)
>
> >
> > 5. Ditch the 3 different shmem helper locks.
> >
> > The trouble is that I forgot that vmap is a thing, so that needs more
> > work. I think there's two approaches here:
> > - Do the vmap at import time. This is the trick we used to untangle the
> > dma_resv_lock issues around dma_buf_attachment_map()
>
> > - Change the dma_buf_vmap rules that callers must hold the dma_resv_lock.
>
> I'll consider this option for v6, thank you.
>
> I see now that you actually want to define the new rules for the
> dma-bufs in general and not only in the context of the DRM code, this
> now makes much more sense to me.
Yeah dma-buf is a cross driver interface, so we should try to be
consistent here. We didn't do this in the past, where the only reason you
didn't get lockdep splats was because you normally didn't run all possible
combinations of drivers and importer/exporter relationships in one system.
But that means it becomes very tricky to reason about how dma-buf really
works.
> > - Maybe also do what you suggest and keep a separate lock for this, but
> > the fundamental issue is that this doesn't really work - if you share
> > buffers both ways with two drivers using shmem helpers, then the
> > ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and
> > you can get some nice deadlocks. So not a great approach (and also the
> > reason why we really need to get everyone to move towards dma_resv_lock
> > as _the_ buffer object lock, since otherwise we'll never get a
> > consistent lock nesting hierarchy).
>
> The separate locks should work okay because it will be always the
> exporter that takes the dma_resv_lock. But I agree that it's less ideal
> than defining the new rules for dma-bufs since sometime you will take
> the resv lock and sometime not, potentially hiding bugs related to lockings.
That's the issue, some importers need to take the dma_resv_lock for
dma_buf_vmap too (e.g. to first nail the buffer in place when it's a
dynamic memory manager). In practice it'll work as well as what we have
currently, which is similarly inconsistent, except with per-driver locks
instead of shared locks from shmem helpers or dma-buf, so less obvious
that things are inconsistent.
So yeah if it's too messy maybe the approach is to have a separate lock
for vmap for now, land things, and then fix up dma_buf_vmap in a follow up
series.
-Daniel
> > The trouble here is that trying to be clever and doing the conversion just
> > in shmem helpers wont work, because there's a lot of cases where the
> > drivers are all kinds of inconsistent with their locking.
> >
> > Adding Daniel S, also maybe for questions it'd be fastest to chat on irc?
>
> My nickname is digetx on the #dri-devel channel, feel free to ping me if
> needed. Right now yours suggestions are clear to me, hence no extra
> questions.
>
> Thank you for the review.
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 5/4/22 11:21, Daniel Vetter wrote:
...
>>> - Maybe also do what you suggest and keep a separate lock for this, but
>>> the fundamental issue is that this doesn't really work - if you share
>>> buffers both ways with two drivers using shmem helpers, then the
>>> ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and
>>> you can get some nice deadlocks. So not a great approach (and also the
>>> reason why we really need to get everyone to move towards dma_resv_lock
>>> as _the_ buffer object lock, since otherwise we'll never get a
>>> consistent lock nesting hierarchy).
>>
>> The separate locks should work okay because it will be always the
>> exporter that takes the dma_resv_lock. But I agree that it's less ideal
>> than defining the new rules for dma-bufs since sometime you will take
>> the resv lock and sometime not, potentially hiding bugs related to lockings.
>
> That's the issue, some importers need to take the dma_resv_lock for
> dma_buf_vmap too (e.g. to first nail the buffer in place when it's a
> dynamic memory manager). In practice it'll work as well as what we have
> currently, which is similarly inconsistent, except with per-driver locks
> instead of shared locks from shmem helpers or dma-buf, so less obvious
> that things are inconsistent.
>
> So yeah if it's too messy maybe the approach is to have a separate lock
> for vmap for now, land things, and then fix up dma_buf_vmap in a follow up
> series.
The amdgpu driver was the fist who introduced the concept of movable
memory for dma-bufs. Now we want to support it for DRM SHMEM too. For
both amdgpu ttm and shmem drivers we will want to hold the reservation
lock when we're touching moveable buffers. The current way of denoting
that dma-buf is movable is to implement the pin/unpin callbacks of the
dma-buf ops, should be doable for shmem.
A day ago I found that mapping of imported dma-bufs is broken at least
for the Tegra DRM driver (and likely for others too) because driver
doesn't assume that anyone will try to mmap imported buffer and just
doesn't handle this case at all, so we're getting a hard lockup on
touching mapped memory because we're mapping something else than the
dma-buf.
My plan is to move the dma-buf management code to the level of DRM core
and make it aware of the reservation locks for the dynamic dma-bufs.
This way we will get the proper locking for dma-bufs and fix mapping of
imported dma-bufs for Tegra and other drivers.
--
Best regards,
Dmitry
On Wed, May 04, 2022 at 06:56:09PM +0300, Dmitry Osipenko wrote:
> On 5/4/22 11:21, Daniel Vetter wrote:
> ...
> >>> - Maybe also do what you suggest and keep a separate lock for this, but
> >>> the fundamental issue is that this doesn't really work - if you share
> >>> buffers both ways with two drivers using shmem helpers, then the
> >>> ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and
> >>> you can get some nice deadlocks. So not a great approach (and also the
> >>> reason why we really need to get everyone to move towards dma_resv_lock
> >>> as _the_ buffer object lock, since otherwise we'll never get a
> >>> consistent lock nesting hierarchy).
> >>
> >> The separate locks should work okay because it will be always the
> >> exporter that takes the dma_resv_lock. But I agree that it's less ideal
> >> than defining the new rules for dma-bufs since sometime you will take
> >> the resv lock and sometime not, potentially hiding bugs related to lockings.
> >
> > That's the issue, some importers need to take the dma_resv_lock for
> > dma_buf_vmap too (e.g. to first nail the buffer in place when it's a
> > dynamic memory manager). In practice it'll work as well as what we have
> > currently, which is similarly inconsistent, except with per-driver locks
> > instead of shared locks from shmem helpers or dma-buf, so less obvious
> > that things are inconsistent.
> >
> > So yeah if it's too messy maybe the approach is to have a separate lock
> > for vmap for now, land things, and then fix up dma_buf_vmap in a follow up
> > series.
>
> The amdgpu driver was the fist who introduced the concept of movable
> memory for dma-bufs. Now we want to support it for DRM SHMEM too. For
> both amdgpu ttm and shmem drivers we will want to hold the reservation
> lock when we're touching moveable buffers. The current way of denoting
> that dma-buf is movable is to implement the pin/unpin callbacks of the
> dma-buf ops, should be doable for shmem.
Hm that sounds like a bridge too far? I don't think we want to start
adding moveable dma-bufs for shmem, thus far at least no one asked for
that. Goal here is just to streamline the locking a bit and align across
all the different ways of doing buffers in drm.
Or do you mean something else and I'm just completely lost?
> A day ago I found that mapping of imported dma-bufs is broken at least
> for the Tegra DRM driver (and likely for others too) because driver
> doesn't assume that anyone will try to mmap imported buffer and just
> doesn't handle this case at all, so we're getting a hard lockup on
> touching mapped memory because we're mapping something else than the
> dma-buf.
Huh that sounds bad, how does this happen? Pretty much all pieces of
dma-buf (cpu vmap, userspace mmap, heck even dma_buf_attach) are optional
or at least can fail for various reasons. So exporters not providing mmap
support is fine, but importers then dying is not.
> My plan is to move the dma-buf management code to the level of DRM core
> and make it aware of the reservation locks for the dynamic dma-bufs.
> This way we will get the proper locking for dma-bufs and fix mapping of
> imported dma-bufs for Tegra and other drivers.
So maybe we're completely talking past each another, or coffee is not
working here on my end, but I've no idea what you mean.
We do have some helpers for taking care of the dma_resv_lock dance, and
Christian K?nig has an rfc patch set to maybe unify this further. But that
should be fairly orthogonal to reworking shmem (it might help a bit with
reworking shmem though).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 5/5/22 11:12, Daniel Vetter wrote:
> On Wed, May 04, 2022 at 06:56:09PM +0300, Dmitry Osipenko wrote:
>> On 5/4/22 11:21, Daniel Vetter wrote:
>> ...
>>>>> - Maybe also do what you suggest and keep a separate lock for this, but
>>>>> the fundamental issue is that this doesn't really work - if you share
>>>>> buffers both ways with two drivers using shmem helpers, then the
>>>>> ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and
>>>>> you can get some nice deadlocks. So not a great approach (and also the
>>>>> reason why we really need to get everyone to move towards dma_resv_lock
>>>>> as _the_ buffer object lock, since otherwise we'll never get a
>>>>> consistent lock nesting hierarchy).
>>>>
>>>> The separate locks should work okay because it will be always the
>>>> exporter that takes the dma_resv_lock. But I agree that it's less ideal
>>>> than defining the new rules for dma-bufs since sometime you will take
>>>> the resv lock and sometime not, potentially hiding bugs related to lockings.
>>>
>>> That's the issue, some importers need to take the dma_resv_lock for
>>> dma_buf_vmap too (e.g. to first nail the buffer in place when it's a
>>> dynamic memory manager). In practice it'll work as well as what we have
>>> currently, which is similarly inconsistent, except with per-driver locks
>>> instead of shared locks from shmem helpers or dma-buf, so less obvious
>>> that things are inconsistent.
>>>
>>> So yeah if it's too messy maybe the approach is to have a separate lock
>>> for vmap for now, land things, and then fix up dma_buf_vmap in a follow up
>>> series.
>>
>> The amdgpu driver was the fist who introduced the concept of movable
>> memory for dma-bufs. Now we want to support it for DRM SHMEM too. For
>> both amdgpu ttm and shmem drivers we will want to hold the reservation
>> lock when we're touching moveable buffers. The current way of denoting
>> that dma-buf is movable is to implement the pin/unpin callbacks of the
>> dma-buf ops, should be doable for shmem.
>
> Hm that sounds like a bridge too far? I don't think we want to start
> adding moveable dma-bufs for shmem, thus far at least no one asked for
> that. Goal here is just to streamline the locking a bit and align across
> all the different ways of doing buffers in drm.
>
> Or do you mean something else and I'm just completely lost?
I'm talking about aligning DRM locks with the dma-buf locks. The problem
is that the convention of dma-bufs isn't specified yet. In particular
there is no convention for the mapping operations.
If we want to switch vmapping of shmem to use reservation lock, then
somebody will have to hold this lock for dma_buf_vmap() and the locking
convention needs to be specified firmly.
In case of dynamic buffers, we will also need to specify whether
dma_buf_vmap() should imply the implicit pinning by exporter or the
buffer must be pinned explicitly by importer before dma_buf_vmap() is
invoked.
Perhaps I indeed shouldn't care about this for this patchset. The
complete locking model of dma-bufs must be specified first.
>> A day ago I found that mapping of imported dma-bufs is broken at least
>> for the Tegra DRM driver (and likely for others too) because driver
>> doesn't assume that anyone will try to mmap imported buffer and just
>> doesn't handle this case at all, so we're getting a hard lockup on
>> touching mapped memory because we're mapping something else than the
>> dma-buf.
>
> Huh that sounds bad, how does this happen? Pretty much all pieces of
> dma-buf (cpu vmap, userspace mmap, heck even dma_buf_attach) are optional
> or at least can fail for various reasons. So exporters not providing mmap
> support is fine, but importers then dying is not.
Those drivers that die don't have userspace that uses dma-bufs
extensively. I noticed it only because was looking at this code too much
for the last days.
Drivers that don't die either map imported BOs properly or don't allow
mapping at all.
>> My plan is to move the dma-buf management code to the level of DRM core
>> and make it aware of the reservation locks for the dynamic dma-bufs.
>> This way we will get the proper locking for dma-bufs and fix mapping of
>> imported dma-bufs for Tegra and other drivers.
>
> So maybe we're completely talking past each another, or coffee is not
> working here on my end, but I've no idea what you mean.
>
> We do have some helpers for taking care of the dma_resv_lock dance, and
> Christian König has an rfc patch set to maybe unify this further. But that
> should be fairly orthogonal to reworking shmem (it might help a bit with
> reworking shmem though).
The reservation lock itself doesn't help much shmem, IMO. It should help
only in the context of dynamic dma-bufs and today we don't have a need
in the dynamic shmem dma-bufs.
You were talking about making DRM locks consistent with dma-buf locks,
so I thought that yours main point of making use of reservation locks
for shmem is to prepare to the new locking scheme.
I wanted to try to specify the dma-buf locking convention for mapping
operations because it's missing right now and it should affect how DRM
should take the reservation locks, but this is not easy to do as I see now.
Could you please point at the Christian's RFC patch? He posted too many
patches, can't find it :) I'm curious to take a look.
--
Best regards,
Dmitry
On Fri, May 06, 2022 at 01:49:12AM +0300, Dmitry Osipenko wrote:
> On 5/5/22 11:12, Daniel Vetter wrote:
> > On Wed, May 04, 2022 at 06:56:09PM +0300, Dmitry Osipenko wrote:
> >> On 5/4/22 11:21, Daniel Vetter wrote:
> >> ...
> >>>>> - Maybe also do what you suggest and keep a separate lock for this, but
> >>>>> the fundamental issue is that this doesn't really work - if you share
> >>>>> buffers both ways with two drivers using shmem helpers, then the
> >>>>> ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and
> >>>>> you can get some nice deadlocks. So not a great approach (and also the
> >>>>> reason why we really need to get everyone to move towards dma_resv_lock
> >>>>> as _the_ buffer object lock, since otherwise we'll never get a
> >>>>> consistent lock nesting hierarchy).
> >>>>
> >>>> The separate locks should work okay because it will be always the
> >>>> exporter that takes the dma_resv_lock. But I agree that it's less ideal
> >>>> than defining the new rules for dma-bufs since sometime you will take
> >>>> the resv lock and sometime not, potentially hiding bugs related to lockings.
> >>>
> >>> That's the issue, some importers need to take the dma_resv_lock for
> >>> dma_buf_vmap too (e.g. to first nail the buffer in place when it's a
> >>> dynamic memory manager). In practice it'll work as well as what we have
> >>> currently, which is similarly inconsistent, except with per-driver locks
> >>> instead of shared locks from shmem helpers or dma-buf, so less obvious
> >>> that things are inconsistent.
> >>>
> >>> So yeah if it's too messy maybe the approach is to have a separate lock
> >>> for vmap for now, land things, and then fix up dma_buf_vmap in a follow up
> >>> series.
> >>
> >> The amdgpu driver was the fist who introduced the concept of movable
> >> memory for dma-bufs. Now we want to support it for DRM SHMEM too. For
> >> both amdgpu ttm and shmem drivers we will want to hold the reservation
> >> lock when we're touching moveable buffers. The current way of denoting
> >> that dma-buf is movable is to implement the pin/unpin callbacks of the
> >> dma-buf ops, should be doable for shmem.
> >
> > Hm that sounds like a bridge too far? I don't think we want to start
> > adding moveable dma-bufs for shmem, thus far at least no one asked for
> > that. Goal here is just to streamline the locking a bit and align across
> > all the different ways of doing buffers in drm.
> >
> > Or do you mean something else and I'm just completely lost?
>
> I'm talking about aligning DRM locks with the dma-buf locks. The problem
> is that the convention of dma-bufs isn't specified yet. In particular
> there is no convention for the mapping operations.
>
> If we want to switch vmapping of shmem to use reservation lock, then
> somebody will have to hold this lock for dma_buf_vmap() and the locking
> convention needs to be specified firmly.
Ah yes that makes sense.
> In case of dynamic buffers, we will also need to specify whether
> dma_buf_vmap() should imply the implicit pinning by exporter or the
> buffer must be pinned explicitly by importer before dma_buf_vmap() is
> invoked.
>
> Perhaps I indeed shouldn't care about this for this patchset. The
> complete locking model of dma-bufs must be specified first.
Hm I thought vmap is meant to pin itself, and not rely on any other
pinning done already. And from a quick look through the long call chain
for amd (which is currently the only driver supporting dynamic dma-buf)
that seems to be the case.
But yeah the locking isn't specificied yet, and that makes it a bit a mess
:-(
> >> A day ago I found that mapping of imported dma-bufs is broken at least
> >> for the Tegra DRM driver (and likely for others too) because driver
> >> doesn't assume that anyone will try to mmap imported buffer and just
> >> doesn't handle this case at all, so we're getting a hard lockup on
> >> touching mapped memory because we're mapping something else than the
> >> dma-buf.
> >
> > Huh that sounds bad, how does this happen? Pretty much all pieces of
> > dma-buf (cpu vmap, userspace mmap, heck even dma_buf_attach) are optional
> > or at least can fail for various reasons. So exporters not providing mmap
> > support is fine, but importers then dying is not.
>
> Those drivers that die don't have userspace that uses dma-bufs
> extensively. I noticed it only because was looking at this code too much
> for the last days.
>
> Drivers that don't die either map imported BOs properly or don't allow
> mapping at all.
Ah yeah driver bugs as explanation makes sense :-/
> >> My plan is to move the dma-buf management code to the level of DRM core
> >> and make it aware of the reservation locks for the dynamic dma-bufs.
> >> This way we will get the proper locking for dma-bufs and fix mapping of
> >> imported dma-bufs for Tegra and other drivers.
> >
> > So maybe we're completely talking past each another, or coffee is not
> > working here on my end, but I've no idea what you mean.
> >
> > We do have some helpers for taking care of the dma_resv_lock dance, and
> > Christian K?nig has an rfc patch set to maybe unify this further. But that
> > should be fairly orthogonal to reworking shmem (it might help a bit with
> > reworking shmem though).
>
> The reservation lock itself doesn't help much shmem, IMO. It should help
> only in the context of dynamic dma-bufs and today we don't have a need
> in the dynamic shmem dma-bufs.
>
> You were talking about making DRM locks consistent with dma-buf locks,
> so I thought that yours main point of making use of reservation locks
> for shmem is to prepare to the new locking scheme.
>
> I wanted to try to specify the dma-buf locking convention for mapping
> operations because it's missing right now and it should affect how DRM
> should take the reservation locks, but this is not easy to do as I see now.
>
> Could you please point at the Christian's RFC patch? He posted too many
> patches, can't find it :) I'm curious to take a look.
https://lore.kernel.org/dri-devel/[email protected]/
Wrt this patch series here I'm wondering whether we could do an interim
solution that side-steps the dma_buf_vmap mess.
- in shmem helpers pin any vmapped buffer (it's how dma-buf works too),
and that pinning would be done under dma_resv_lock (like with other
drivers using dma_resv_lock for bo protection)
- switch over everything else except vmap code to dma_resv_lock, but leave
vmap locking as-is
- shrinker then only needs to trylock dma_resv_trylock in the shrinker,
which can check for pinned buffer and that's good enough to exclude
vmap'ed buffer. And it avoids mixing the vmap locking into the new
shrinker code and driver interfaces.
This still leaves the vmap locking mess as-is, but I think that's a mess
that's orthogonal to shrinker work.
Thoughts?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 5/9/22 16:42, Daniel Vetter wrote:
> On Fri, May 06, 2022 at 01:49:12AM +0300, Dmitry Osipenko wrote:
>> On 5/5/22 11:12, Daniel Vetter wrote:
>>> On Wed, May 04, 2022 at 06:56:09PM +0300, Dmitry Osipenko wrote:
>>>> On 5/4/22 11:21, Daniel Vetter wrote:
>>>> ...
>>>>>>> - Maybe also do what you suggest and keep a separate lock for this, but
>>>>>>> the fundamental issue is that this doesn't really work - if you share
>>>>>>> buffers both ways with two drivers using shmem helpers, then the
>>>>>>> ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and
>>>>>>> you can get some nice deadlocks. So not a great approach (and also the
>>>>>>> reason why we really need to get everyone to move towards dma_resv_lock
>>>>>>> as _the_ buffer object lock, since otherwise we'll never get a
>>>>>>> consistent lock nesting hierarchy).
>>>>>>
>>>>>> The separate locks should work okay because it will be always the
>>>>>> exporter that takes the dma_resv_lock. But I agree that it's less ideal
>>>>>> than defining the new rules for dma-bufs since sometime you will take
>>>>>> the resv lock and sometime not, potentially hiding bugs related to lockings.
>>>>>
>>>>> That's the issue, some importers need to take the dma_resv_lock for
>>>>> dma_buf_vmap too (e.g. to first nail the buffer in place when it's a
>>>>> dynamic memory manager). In practice it'll work as well as what we have
>>>>> currently, which is similarly inconsistent, except with per-driver locks
>>>>> instead of shared locks from shmem helpers or dma-buf, so less obvious
>>>>> that things are inconsistent.
>>>>>
>>>>> So yeah if it's too messy maybe the approach is to have a separate lock
>>>>> for vmap for now, land things, and then fix up dma_buf_vmap in a follow up
>>>>> series.
>>>>
>>>> The amdgpu driver was the fist who introduced the concept of movable
>>>> memory for dma-bufs. Now we want to support it for DRM SHMEM too. For
>>>> both amdgpu ttm and shmem drivers we will want to hold the reservation
>>>> lock when we're touching moveable buffers. The current way of denoting
>>>> that dma-buf is movable is to implement the pin/unpin callbacks of the
>>>> dma-buf ops, should be doable for shmem.
>>>
>>> Hm that sounds like a bridge too far? I don't think we want to start
>>> adding moveable dma-bufs for shmem, thus far at least no one asked for
>>> that. Goal here is just to streamline the locking a bit and align across
>>> all the different ways of doing buffers in drm.
>>>
>>> Or do you mean something else and I'm just completely lost?
>>
>> I'm talking about aligning DRM locks with the dma-buf locks. The problem
>> is that the convention of dma-bufs isn't specified yet. In particular
>> there is no convention for the mapping operations.
>>
>> If we want to switch vmapping of shmem to use reservation lock, then
>> somebody will have to hold this lock for dma_buf_vmap() and the locking
>> convention needs to be specified firmly.
>
> Ah yes that makes sense.
>
>> In case of dynamic buffers, we will also need to specify whether
>> dma_buf_vmap() should imply the implicit pinning by exporter or the
>> buffer must be pinned explicitly by importer before dma_buf_vmap() is
>> invoked.
>>
>> Perhaps I indeed shouldn't care about this for this patchset. The
>> complete locking model of dma-bufs must be specified first.
>
> Hm I thought vmap is meant to pin itself, and not rely on any other
> pinning done already. And from a quick look through the long call chain
> for amd (which is currently the only driver supporting dynamic dma-buf)
> that seems to be the case.
The vmapping behaviour is implementation-defined until it's documented
explicitly, IMO.
> But yeah the locking isn't specificied yet, and that makes it a bit a mess
> :-(
>
>>>> A day ago I found that mapping of imported dma-bufs is broken at least
>>>> for the Tegra DRM driver (and likely for others too) because driver
>>>> doesn't assume that anyone will try to mmap imported buffer and just
>>>> doesn't handle this case at all, so we're getting a hard lockup on
>>>> touching mapped memory because we're mapping something else than the
>>>> dma-buf.
>>>
>>> Huh that sounds bad, how does this happen? Pretty much all pieces of
>>> dma-buf (cpu vmap, userspace mmap, heck even dma_buf_attach) are optional
>>> or at least can fail for various reasons. So exporters not providing mmap
>>> support is fine, but importers then dying is not.
>>
>> Those drivers that die don't have userspace that uses dma-bufs
>> extensively. I noticed it only because was looking at this code too much
>> for the last days.
>>
>> Drivers that don't die either map imported BOs properly or don't allow
>> mapping at all.
>
> Ah yeah driver bugs as explanation makes sense :-/
>
>>>> My plan is to move the dma-buf management code to the level of DRM core
>>>> and make it aware of the reservation locks for the dynamic dma-bufs.
>>>> This way we will get the proper locking for dma-bufs and fix mapping of
>>>> imported dma-bufs for Tegra and other drivers.
>>>
>>> So maybe we're completely talking past each another, or coffee is not
>>> working here on my end, but I've no idea what you mean.
>>>
>>> We do have some helpers for taking care of the dma_resv_lock dance, and
>>> Christian König has an rfc patch set to maybe unify this further. But that
>>> should be fairly orthogonal to reworking shmem (it might help a bit with
>>> reworking shmem though).
>>
>> The reservation lock itself doesn't help much shmem, IMO. It should help
>> only in the context of dynamic dma-bufs and today we don't have a need
>> in the dynamic shmem dma-bufs.
>>
>> You were talking about making DRM locks consistent with dma-buf locks,
>> so I thought that yours main point of making use of reservation locks
>> for shmem is to prepare to the new locking scheme.
>>
>> I wanted to try to specify the dma-buf locking convention for mapping
>> operations because it's missing right now and it should affect how DRM
>> should take the reservation locks, but this is not easy to do as I see now.
>>
>> Could you please point at the Christian's RFC patch? He posted too many
>> patches, can't find it :) I'm curious to take a look.
>
> https://lore.kernel.org/dri-devel/[email protected]/
>
> Wrt this patch series here I'm wondering whether we could do an interim
> solution that side-steps the dma_buf_vmap mess.
>
> - in shmem helpers pin any vmapped buffer (it's how dma-buf works too),
> and that pinning would be done under dma_resv_lock (like with other
> drivers using dma_resv_lock for bo protection)
>
> - switch over everything else except vmap code to dma_resv_lock, but leave
> vmap locking as-is
>
> - shrinker then only needs to trylock dma_resv_trylock in the shrinker,
> which can check for pinned buffer and that's good enough to exclude
> vmap'ed buffer. And it avoids mixing the vmap locking into the new
> shrinker code and driver interfaces.
>
> This still leaves the vmap locking mess as-is, but I think that's a mess
> that's orthogonal to shrinker work.
>
> Thoughts?
Since vmapping implies implicit pinning, we can't use a separate lock in
drm_gem_shmem_vmap() because we need to protect the
drm_gem_shmem_get_pages(), which is invoked by drm_gem_shmem_vmap() to
pin the pages and requires the dma_resv_lock to be locked.
Hence the problem is:
1. If dma-buf importer holds the dma_resv_lock and invokes
dma_buf_vmap() -> drm_gem_shmem_vmap(), then drm_gem_shmem_vmap() shall
not take the dma_resv_lock.
2. Since dma-buf locking convention isn't specified, we can't assume
that dma-buf importer holds the dma_resv_lock around dma_buf_vmap().
The possible solutions are:
1. Specify the dma_resv_lock convention for dma-bufs and make all
drivers to follow it.
2. Make only DRM drivers to hold dma_resv_lock around dma_buf_vmap().
Other non-DRM drivers will get the lockdep warning.
3. Make drm_gem_shmem_vmap() to take the dma_resv_lock and get deadlock
if dma-buf importer holds the lock.
...
There are actually very few drivers in kernel that use dma_buf_vmap()
[1], so perhaps it's not really a big deal to first try to define the
locking and pinning convention for the dma-bufs? At least for
dma_buf_vmap()? Let me try to do this.
[1] https://elixir.bootlin.com/linux/v5.18-rc6/C/ident/dma_buf_vmap
I envision that the extra dma_resv_locks for dma-bufs potentially may
create unnecessary bottlenecks for some drivers if locking isn't really
necessary by a specific driver, so drivers will need to keep this in
mind. On the other hand, I don't think that any of the today's drivers
will notice the additional resv locks in practice.
--
Best regards,
Dmitry
On Wed, May 11, 2022 at 04:24:28PM +0200, Christian K?nig wrote:
> Am 11.05.22 um 15:00 schrieb Daniel Vetter:
> > On Tue, May 10, 2022 at 04:39:53PM +0300, Dmitry Osipenko wrote:
> > > [SNIP]
> > > Since vmapping implies implicit pinning, we can't use a separate lock in
> > > drm_gem_shmem_vmap() because we need to protect the
> > > drm_gem_shmem_get_pages(), which is invoked by drm_gem_shmem_vmap() to
> > > pin the pages and requires the dma_resv_lock to be locked.
> > >
> > > Hence the problem is:
> > >
> > > 1. If dma-buf importer holds the dma_resv_lock and invokes
> > > dma_buf_vmap() -> drm_gem_shmem_vmap(), then drm_gem_shmem_vmap() shall
> > > not take the dma_resv_lock.
> > >
> > > 2. Since dma-buf locking convention isn't specified, we can't assume
> > > that dma-buf importer holds the dma_resv_lock around dma_buf_vmap().
> > >
> > > The possible solutions are:
> > >
> > > 1. Specify the dma_resv_lock convention for dma-bufs and make all
> > > drivers to follow it.
> > >
> > > 2. Make only DRM drivers to hold dma_resv_lock around dma_buf_vmap().
> > > Other non-DRM drivers will get the lockdep warning.
> > >
> > > 3. Make drm_gem_shmem_vmap() to take the dma_resv_lock and get deadlock
> > > if dma-buf importer holds the lock.
> > >
> > > ...
> > Yeah this is all very annoying.
>
> Ah, yes that topic again :)
>
> I think we could relatively easily fix that by just defining and enforcing
> that the dma_resv_lock must have be taken by the caller when dma_buf_vmap()
> is called.
>
> A two step approach should work:
> 1. Move the call to dma_resv_lock() into the dma_buf_vmap() function and
> remove all lock taking from the vmap callback implementations.
> 2. Move the call to dma_resv_lock() into the callers of dma_buf_vmap() and
> enforce that the function is called with the lock held.
>
> It shouldn't be that hard to clean up. The last time I looked into it my
> main problem was that we didn't had any easy unit test for it.
Yeah I think it's doable or at least a lot less work than the map/unmap
side, which really was unfixable without just pinning at import time to
avoid the locking fun. But vmap is used a lot less, and mostly by display
drivers (where locking is a lot easier against dma_resv_lock), so it might
be possible to pull off.
-Daniel
>
> Regards,
> Christian.
>
> >
> > > There are actually very few drivers in kernel that use dma_buf_vmap()
> > > [1], so perhaps it's not really a big deal to first try to define the
> > > locking and pinning convention for the dma-bufs? At least for
> > > dma_buf_vmap()? Let me try to do this.
> > >
> > > [1] https://elixir.bootlin.com/linux/v5.18-rc6/C/ident/dma_buf_vmap
> > Yeah looking through the code there's largely two classes of drivers that
> > need vmap:
> >
> > - display drivers that need to do cpu upload (usb, spi, i2c displays).
> > Those generally set up the vmap at import time or when creating the
> > drm_framebuffer object (e.g. see
> > drm_gem_cma_prime_import_sg_table_vmap()), because that's really the
> > only place where you can safely do that without running into locking
> > inversion issues sooner or later
> >
> > - lots of other drivers (and shmem helpers) seem to do dma_buf_vmap just
> > because they can, but only actually ever use vmap on native objects,
> > never on imported objects. Or at least I think so.
> >
> > So maybe another approach here:
> >
> > 1. In general drivers which need a vmap need to set that up at dma_buf
> > import time - the same way we pin the buffers at import time for
> > non-dynamic importers because that's the only place where across all
> > drivers it's ok to just take dma_resv_lock.
> >
> > 2. We remove the "just because we can" dma_buf_vmap support from
> > helpers/drivers - the paths all already can cope with NULL since
> > dma_buf_vmap can fail. vmap will only work on native objects, not imported
> > ones.
> >
> > 3. If there is any driver using shmem helpers that absolutely needs vmap
> > to also work on imported it needs a special import function (like cma
> > helpers) which sets up the vmap at import time.
> >
> > So since this is all very tricky ... what did I miss this time around?
> >
> > > I envision that the extra dma_resv_locks for dma-bufs potentially may
> > > create unnecessary bottlenecks for some drivers if locking isn't really
> > > necessary by a specific driver, so drivers will need to keep this in
> > > mind. On the other hand, I don't think that any of the today's drivers
> > > will notice the additional resv locks in practice.
> > Nah I don't think the extra locking will ever create a bottleneck,
> > especially not for vmap. Generally vmap is a fallback or at least cpu
> > operation, so at that point you're already going very slow.
> > -Daniel
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, May 11, 2022 at 06:14:00PM +0300, Dmitry Osipenko wrote:
> On 5/11/22 17:24, Christian K?nig wrote:
> > Am 11.05.22 um 15:00 schrieb Daniel Vetter:
> >> On Tue, May 10, 2022 at 04:39:53PM +0300, Dmitry Osipenko wrote:
> >>> [SNIP]
> >>> Since vmapping implies implicit pinning, we can't use a separate lock in
> >>> drm_gem_shmem_vmap() because we need to protect the
> >>> drm_gem_shmem_get_pages(), which is invoked by drm_gem_shmem_vmap() to
> >>> pin the pages and requires the dma_resv_lock to be locked.
> >>>
> >>> Hence the problem is:
> >>>
> >>> 1. If dma-buf importer holds the dma_resv_lock and invokes
> >>> dma_buf_vmap() -> drm_gem_shmem_vmap(), then drm_gem_shmem_vmap() shall
> >>> not take the dma_resv_lock.
> >>>
> >>> 2. Since dma-buf locking convention isn't specified, we can't assume
> >>> that dma-buf importer holds the dma_resv_lock around dma_buf_vmap().
> >>>
> >>> The possible solutions are:
> >>>
> >>> 1. Specify the dma_resv_lock convention for dma-bufs and make all
> >>> drivers to follow it.
> >>>
> >>> 2. Make only DRM drivers to hold dma_resv_lock around dma_buf_vmap().
> >>> Other non-DRM drivers will get the lockdep warning.
> >>>
> >>> 3. Make drm_gem_shmem_vmap() to take the dma_resv_lock and get deadlock
> >>> if dma-buf importer holds the lock.
> >>>
> >>> ...
> >> Yeah this is all very annoying.
> >
> > Ah, yes that topic again :)
> >
> > I think we could relatively easily fix that by just defining and
> > enforcing that the dma_resv_lock must have be taken by the caller when
> > dma_buf_vmap() is called.
> >
> > A two step approach should work:
> > 1. Move the call to dma_resv_lock() into the dma_buf_vmap() function and
> > remove all lock taking from the vmap callback implementations.
> > 2. Move the call to dma_resv_lock() into the callers of dma_buf_vmap()
> > and enforce that the function is called with the lock held.
>
> I've doubts about the need to move out the dma_resv_lock() into the
> callers of dma_buf_vmap()..
>
> I looked through all the dma_buf_vmap() users and neither of them
> interacts with dma_resv_lock() at all, i.e. nobody takes the lock
> in/outside of dma_buf_vmap(). Hence it's easy and more practical to make
> dma_buf_mmap/vmap() to take the dma_resv_lock by themselves.
i915_gem_dmabuf_vmap -> i915_gem_object_pin_map_unlocked ->
i915_gem_object_lock -> dma_resv_lock
And all the ttm drivers should work similarly. So there's definitely
drivers which grab dma_resv_lock from their vmap callback.
> It's unclear to me which driver may ever want to do the mapping under
> the dma_resv_lock. But if we will ever have such a driver that will need
> to map imported buffer under dma_resv_lock, then we could always add the
> dma_buf_vmap_locked() variant of the function. In this case the locking
> rule will sound like this:
>
> "All dma-buf importers are responsible for holding the dma-reservation
> lock around the dmabuf->ops->mmap/vmap() calls."
>
> > It shouldn't be that hard to clean up. The last time I looked into it my
> > main problem was that we didn't had any easy unit test for it.
>
> Do we have any tests for dma-bufs at all? It's unclear to me what you
> are going to test in regards to the reservation locks, could you please
> clarify?
Unfortunately not really :-/ Only way really is to grab a driver which
needs vmap (those are mostly display drivers) on an imported buffer, and
see what happens.
2nd best is liberally sprinkling lockdep annotations all over the place
and throwing it at intel ci (not sure amd ci is accessible to the public)
and then hoping that's good enough. Stuff like might_lock and
dma_resv_assert_held.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 5/11/22 17:24, Christian König wrote:
> Am 11.05.22 um 15:00 schrieb Daniel Vetter:
>> On Tue, May 10, 2022 at 04:39:53PM +0300, Dmitry Osipenko wrote:
>>> [SNIP]
>>> Since vmapping implies implicit pinning, we can't use a separate lock in
>>> drm_gem_shmem_vmap() because we need to protect the
>>> drm_gem_shmem_get_pages(), which is invoked by drm_gem_shmem_vmap() to
>>> pin the pages and requires the dma_resv_lock to be locked.
>>>
>>> Hence the problem is:
>>>
>>> 1. If dma-buf importer holds the dma_resv_lock and invokes
>>> dma_buf_vmap() -> drm_gem_shmem_vmap(), then drm_gem_shmem_vmap() shall
>>> not take the dma_resv_lock.
>>>
>>> 2. Since dma-buf locking convention isn't specified, we can't assume
>>> that dma-buf importer holds the dma_resv_lock around dma_buf_vmap().
>>>
>>> The possible solutions are:
>>>
>>> 1. Specify the dma_resv_lock convention for dma-bufs and make all
>>> drivers to follow it.
>>>
>>> 2. Make only DRM drivers to hold dma_resv_lock around dma_buf_vmap().
>>> Other non-DRM drivers will get the lockdep warning.
>>>
>>> 3. Make drm_gem_shmem_vmap() to take the dma_resv_lock and get deadlock
>>> if dma-buf importer holds the lock.
>>>
>>> ...
>> Yeah this is all very annoying.
>
> Ah, yes that topic again :)
>
> I think we could relatively easily fix that by just defining and
> enforcing that the dma_resv_lock must have be taken by the caller when
> dma_buf_vmap() is called.
>
> A two step approach should work:
> 1. Move the call to dma_resv_lock() into the dma_buf_vmap() function and
> remove all lock taking from the vmap callback implementations.
> 2. Move the call to dma_resv_lock() into the callers of dma_buf_vmap()
> and enforce that the function is called with the lock held.
I've doubts about the need to move out the dma_resv_lock() into the
callers of dma_buf_vmap()..
I looked through all the dma_buf_vmap() users and neither of them
interacts with dma_resv_lock() at all, i.e. nobody takes the lock
in/outside of dma_buf_vmap(). Hence it's easy and more practical to make
dma_buf_mmap/vmap() to take the dma_resv_lock by themselves.
It's unclear to me which driver may ever want to do the mapping under
the dma_resv_lock. But if we will ever have such a driver that will need
to map imported buffer under dma_resv_lock, then we could always add the
dma_buf_vmap_locked() variant of the function. In this case the locking
rule will sound like this:
"All dma-buf importers are responsible for holding the dma-reservation
lock around the dmabuf->ops->mmap/vmap() calls."
> It shouldn't be that hard to clean up. The last time I looked into it my
> main problem was that we didn't had any easy unit test for it.
Do we have any tests for dma-bufs at all? It's unclear to me what you
are going to test in regards to the reservation locks, could you please
clarify?
--
Best regards,
Dmitry
Am 11.05.22 um 15:00 schrieb Daniel Vetter:
> On Tue, May 10, 2022 at 04:39:53PM +0300, Dmitry Osipenko wrote:
>> [SNIP]
>> Since vmapping implies implicit pinning, we can't use a separate lock in
>> drm_gem_shmem_vmap() because we need to protect the
>> drm_gem_shmem_get_pages(), which is invoked by drm_gem_shmem_vmap() to
>> pin the pages and requires the dma_resv_lock to be locked.
>>
>> Hence the problem is:
>>
>> 1. If dma-buf importer holds the dma_resv_lock and invokes
>> dma_buf_vmap() -> drm_gem_shmem_vmap(), then drm_gem_shmem_vmap() shall
>> not take the dma_resv_lock.
>>
>> 2. Since dma-buf locking convention isn't specified, we can't assume
>> that dma-buf importer holds the dma_resv_lock around dma_buf_vmap().
>>
>> The possible solutions are:
>>
>> 1. Specify the dma_resv_lock convention for dma-bufs and make all
>> drivers to follow it.
>>
>> 2. Make only DRM drivers to hold dma_resv_lock around dma_buf_vmap().
>> Other non-DRM drivers will get the lockdep warning.
>>
>> 3. Make drm_gem_shmem_vmap() to take the dma_resv_lock and get deadlock
>> if dma-buf importer holds the lock.
>>
>> ...
> Yeah this is all very annoying.
Ah, yes that topic again :)
I think we could relatively easily fix that by just defining and
enforcing that the dma_resv_lock must have be taken by the caller when
dma_buf_vmap() is called.
A two step approach should work:
1. Move the call to dma_resv_lock() into the dma_buf_vmap() function and
remove all lock taking from the vmap callback implementations.
2. Move the call to dma_resv_lock() into the callers of dma_buf_vmap()
and enforce that the function is called with the lock held.
It shouldn't be that hard to clean up. The last time I looked into it my
main problem was that we didn't had any easy unit test for it.
Regards,
Christian.
>
>> There are actually very few drivers in kernel that use dma_buf_vmap()
>> [1], so perhaps it's not really a big deal to first try to define the
>> locking and pinning convention for the dma-bufs? At least for
>> dma_buf_vmap()? Let me try to do this.
>>
>> [1] https://elixir.bootlin.com/linux/v5.18-rc6/C/ident/dma_buf_vmap
> Yeah looking through the code there's largely two classes of drivers that
> need vmap:
>
> - display drivers that need to do cpu upload (usb, spi, i2c displays).
> Those generally set up the vmap at import time or when creating the
> drm_framebuffer object (e.g. see
> drm_gem_cma_prime_import_sg_table_vmap()), because that's really the
> only place where you can safely do that without running into locking
> inversion issues sooner or later
>
> - lots of other drivers (and shmem helpers) seem to do dma_buf_vmap just
> because they can, but only actually ever use vmap on native objects,
> never on imported objects. Or at least I think so.
>
> So maybe another approach here:
>
> 1. In general drivers which need a vmap need to set that up at dma_buf
> import time - the same way we pin the buffers at import time for
> non-dynamic importers because that's the only place where across all
> drivers it's ok to just take dma_resv_lock.
>
> 2. We remove the "just because we can" dma_buf_vmap support from
> helpers/drivers - the paths all already can cope with NULL since
> dma_buf_vmap can fail. vmap will only work on native objects, not imported
> ones.
>
> 3. If there is any driver using shmem helpers that absolutely needs vmap
> to also work on imported it needs a special import function (like cma
> helpers) which sets up the vmap at import time.
>
> So since this is all very tricky ... what did I miss this time around?
>
>> I envision that the extra dma_resv_locks for dma-bufs potentially may
>> create unnecessary bottlenecks for some drivers if locking isn't really
>> necessary by a specific driver, so drivers will need to keep this in
>> mind. On the other hand, I don't think that any of the today's drivers
>> will notice the additional resv locks in practice.
> Nah I don't think the extra locking will ever create a bottleneck,
> especially not for vmap. Generally vmap is a fallback or at least cpu
> operation, so at that point you're already going very slow.
> -Daniel
On 5/11/22 18:29, Daniel Vetter wrote:
> On Wed, May 11, 2022 at 06:14:00PM +0300, Dmitry Osipenko wrote:
>> On 5/11/22 17:24, Christian König wrote:
>>> Am 11.05.22 um 15:00 schrieb Daniel Vetter:
>>>> On Tue, May 10, 2022 at 04:39:53PM +0300, Dmitry Osipenko wrote:
>>>>> [SNIP]
>>>>> Since vmapping implies implicit pinning, we can't use a separate lock in
>>>>> drm_gem_shmem_vmap() because we need to protect the
>>>>> drm_gem_shmem_get_pages(), which is invoked by drm_gem_shmem_vmap() to
>>>>> pin the pages and requires the dma_resv_lock to be locked.
>>>>>
>>>>> Hence the problem is:
>>>>>
>>>>> 1. If dma-buf importer holds the dma_resv_lock and invokes
>>>>> dma_buf_vmap() -> drm_gem_shmem_vmap(), then drm_gem_shmem_vmap() shall
>>>>> not take the dma_resv_lock.
>>>>>
>>>>> 2. Since dma-buf locking convention isn't specified, we can't assume
>>>>> that dma-buf importer holds the dma_resv_lock around dma_buf_vmap().
>>>>>
>>>>> The possible solutions are:
>>>>>
>>>>> 1. Specify the dma_resv_lock convention for dma-bufs and make all
>>>>> drivers to follow it.
>>>>>
>>>>> 2. Make only DRM drivers to hold dma_resv_lock around dma_buf_vmap().
>>>>> Other non-DRM drivers will get the lockdep warning.
>>>>>
>>>>> 3. Make drm_gem_shmem_vmap() to take the dma_resv_lock and get deadlock
>>>>> if dma-buf importer holds the lock.
>>>>>
>>>>> ...
>>>> Yeah this is all very annoying.
>>> Ah, yes that topic again :)
>>>
>>> I think we could relatively easily fix that by just defining and
>>> enforcing that the dma_resv_lock must have be taken by the caller when
>>> dma_buf_vmap() is called.
>>>
>>> A two step approach should work:
>>> 1. Move the call to dma_resv_lock() into the dma_buf_vmap() function and
>>> remove all lock taking from the vmap callback implementations.
>>> 2. Move the call to dma_resv_lock() into the callers of dma_buf_vmap()
>>> and enforce that the function is called with the lock held.
>> I've doubts about the need to move out the dma_resv_lock() into the
>> callers of dma_buf_vmap()..
>>
>> I looked through all the dma_buf_vmap() users and neither of them
>> interacts with dma_resv_lock() at all, i.e. nobody takes the lock
>> in/outside of dma_buf_vmap(). Hence it's easy and more practical to make
>> dma_buf_mmap/vmap() to take the dma_resv_lock by themselves.
> i915_gem_dmabuf_vmap -> i915_gem_object_pin_map_unlocked ->
> i915_gem_object_lock -> dma_resv_lock
>
> And all the ttm drivers should work similarly. So there's definitely
> drivers which grab dma_resv_lock from their vmap callback.
Grr.. I'll take another look.
>> It's unclear to me which driver may ever want to do the mapping under
>> the dma_resv_lock. But if we will ever have such a driver that will need
>> to map imported buffer under dma_resv_lock, then we could always add the
>> dma_buf_vmap_locked() variant of the function. In this case the locking
>> rule will sound like this:
>>
>> "All dma-buf importers are responsible for holding the dma-reservation
>> lock around the dmabuf->ops->mmap/vmap() calls."
Are you okay with this rule?
>>> It shouldn't be that hard to clean up. The last time I looked into it my
>>> main problem was that we didn't had any easy unit test for it.
>> Do we have any tests for dma-bufs at all? It's unclear to me what you
>> are going to test in regards to the reservation locks, could you please
>> clarify?
> Unfortunately not really :-/ Only way really is to grab a driver which
> needs vmap (those are mostly display drivers) on an imported buffer, and
> see what happens.
>
> 2nd best is liberally sprinkling lockdep annotations all over the place
> and throwing it at intel ci (not sure amd ci is accessible to the public)
> and then hoping that's good enough. Stuff like might_lock and
> dma_resv_assert_held.
Alright
--
Best regards,
Dmitry
On Tue, May 10, 2022 at 04:39:53PM +0300, Dmitry Osipenko wrote:
> On 5/9/22 16:42, Daniel Vetter wrote:
> > On Fri, May 06, 2022 at 01:49:12AM +0300, Dmitry Osipenko wrote:
> >> On 5/5/22 11:12, Daniel Vetter wrote:
> >>> On Wed, May 04, 2022 at 06:56:09PM +0300, Dmitry Osipenko wrote:
> >>>> On 5/4/22 11:21, Daniel Vetter wrote:
> >>>> ...
> >>>>>>> - Maybe also do what you suggest and keep a separate lock for this, but
> >>>>>>> the fundamental issue is that this doesn't really work - if you share
> >>>>>>> buffers both ways with two drivers using shmem helpers, then the
> >>>>>>> ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and
> >>>>>>> you can get some nice deadlocks. So not a great approach (and also the
> >>>>>>> reason why we really need to get everyone to move towards dma_resv_lock
> >>>>>>> as _the_ buffer object lock, since otherwise we'll never get a
> >>>>>>> consistent lock nesting hierarchy).
> >>>>>>
> >>>>>> The separate locks should work okay because it will be always the
> >>>>>> exporter that takes the dma_resv_lock. But I agree that it's less ideal
> >>>>>> than defining the new rules for dma-bufs since sometime you will take
> >>>>>> the resv lock and sometime not, potentially hiding bugs related to lockings.
> >>>>>
> >>>>> That's the issue, some importers need to take the dma_resv_lock for
> >>>>> dma_buf_vmap too (e.g. to first nail the buffer in place when it's a
> >>>>> dynamic memory manager). In practice it'll work as well as what we have
> >>>>> currently, which is similarly inconsistent, except with per-driver locks
> >>>>> instead of shared locks from shmem helpers or dma-buf, so less obvious
> >>>>> that things are inconsistent.
> >>>>>
> >>>>> So yeah if it's too messy maybe the approach is to have a separate lock
> >>>>> for vmap for now, land things, and then fix up dma_buf_vmap in a follow up
> >>>>> series.
> >>>>
> >>>> The amdgpu driver was the fist who introduced the concept of movable
> >>>> memory for dma-bufs. Now we want to support it for DRM SHMEM too. For
> >>>> both amdgpu ttm and shmem drivers we will want to hold the reservation
> >>>> lock when we're touching moveable buffers. The current way of denoting
> >>>> that dma-buf is movable is to implement the pin/unpin callbacks of the
> >>>> dma-buf ops, should be doable for shmem.
> >>>
> >>> Hm that sounds like a bridge too far? I don't think we want to start
> >>> adding moveable dma-bufs for shmem, thus far at least no one asked for
> >>> that. Goal here is just to streamline the locking a bit and align across
> >>> all the different ways of doing buffers in drm.
> >>>
> >>> Or do you mean something else and I'm just completely lost?
> >>
> >> I'm talking about aligning DRM locks with the dma-buf locks. The problem
> >> is that the convention of dma-bufs isn't specified yet. In particular
> >> there is no convention for the mapping operations.
> >>
> >> If we want to switch vmapping of shmem to use reservation lock, then
> >> somebody will have to hold this lock for dma_buf_vmap() and the locking
> >> convention needs to be specified firmly.
> >
> > Ah yes that makes sense.
> >
> >> In case of dynamic buffers, we will also need to specify whether
> >> dma_buf_vmap() should imply the implicit pinning by exporter or the
> >> buffer must be pinned explicitly by importer before dma_buf_vmap() is
> >> invoked.
> >>
> >> Perhaps I indeed shouldn't care about this for this patchset. The
> >> complete locking model of dma-bufs must be specified first.
> >
> > Hm I thought vmap is meant to pin itself, and not rely on any other
> > pinning done already. And from a quick look through the long call chain
> > for amd (which is currently the only driver supporting dynamic dma-buf)
> > that seems to be the case.
>
> The vmapping behaviour is implementation-defined until it's documented
> explicitly, IMO.
>
> > But yeah the locking isn't specificied yet, and that makes it a bit a mess
> > :-(
> >
> >>>> A day ago I found that mapping of imported dma-bufs is broken at least
> >>>> for the Tegra DRM driver (and likely for others too) because driver
> >>>> doesn't assume that anyone will try to mmap imported buffer and just
> >>>> doesn't handle this case at all, so we're getting a hard lockup on
> >>>> touching mapped memory because we're mapping something else than the
> >>>> dma-buf.
> >>>
> >>> Huh that sounds bad, how does this happen? Pretty much all pieces of
> >>> dma-buf (cpu vmap, userspace mmap, heck even dma_buf_attach) are optional
> >>> or at least can fail for various reasons. So exporters not providing mmap
> >>> support is fine, but importers then dying is not.
> >>
> >> Those drivers that die don't have userspace that uses dma-bufs
> >> extensively. I noticed it only because was looking at this code too much
> >> for the last days.
> >>
> >> Drivers that don't die either map imported BOs properly or don't allow
> >> mapping at all.
> >
> > Ah yeah driver bugs as explanation makes sense :-/
> >
> >>>> My plan is to move the dma-buf management code to the level of DRM core
> >>>> and make it aware of the reservation locks for the dynamic dma-bufs.
> >>>> This way we will get the proper locking for dma-bufs and fix mapping of
> >>>> imported dma-bufs for Tegra and other drivers.
> >>>
> >>> So maybe we're completely talking past each another, or coffee is not
> >>> working here on my end, but I've no idea what you mean.
> >>>
> >>> We do have some helpers for taking care of the dma_resv_lock dance, and
> >>> Christian K?nig has an rfc patch set to maybe unify this further. But that
> >>> should be fairly orthogonal to reworking shmem (it might help a bit with
> >>> reworking shmem though).
> >>
> >> The reservation lock itself doesn't help much shmem, IMO. It should help
> >> only in the context of dynamic dma-bufs and today we don't have a need
> >> in the dynamic shmem dma-bufs.
> >>
> >> You were talking about making DRM locks consistent with dma-buf locks,
> >> so I thought that yours main point of making use of reservation locks
> >> for shmem is to prepare to the new locking scheme.
> >>
> >> I wanted to try to specify the dma-buf locking convention for mapping
> >> operations because it's missing right now and it should affect how DRM
> >> should take the reservation locks, but this is not easy to do as I see now.
> >>
> >> Could you please point at the Christian's RFC patch? He posted too many
> >> patches, can't find it :) I'm curious to take a look.
> >
> > https://lore.kernel.org/dri-devel/[email protected]/
> >
> > Wrt this patch series here I'm wondering whether we could do an interim
> > solution that side-steps the dma_buf_vmap mess.
> >
> > - in shmem helpers pin any vmapped buffer (it's how dma-buf works too),
> > and that pinning would be done under dma_resv_lock (like with other
> > drivers using dma_resv_lock for bo protection)
> >
> > - switch over everything else except vmap code to dma_resv_lock, but leave
> > vmap locking as-is
> >
> > - shrinker then only needs to trylock dma_resv_trylock in the shrinker,
> > which can check for pinned buffer and that's good enough to exclude
> > vmap'ed buffer. And it avoids mixing the vmap locking into the new
> > shrinker code and driver interfaces.
> >
> > This still leaves the vmap locking mess as-is, but I think that's a mess
> > that's orthogonal to shrinker work.
> >
> > Thoughts?
>
> Since vmapping implies implicit pinning, we can't use a separate lock in
> drm_gem_shmem_vmap() because we need to protect the
> drm_gem_shmem_get_pages(), which is invoked by drm_gem_shmem_vmap() to
> pin the pages and requires the dma_resv_lock to be locked.
>
> Hence the problem is:
>
> 1. If dma-buf importer holds the dma_resv_lock and invokes
> dma_buf_vmap() -> drm_gem_shmem_vmap(), then drm_gem_shmem_vmap() shall
> not take the dma_resv_lock.
>
> 2. Since dma-buf locking convention isn't specified, we can't assume
> that dma-buf importer holds the dma_resv_lock around dma_buf_vmap().
>
> The possible solutions are:
>
> 1. Specify the dma_resv_lock convention for dma-bufs and make all
> drivers to follow it.
>
> 2. Make only DRM drivers to hold dma_resv_lock around dma_buf_vmap().
> Other non-DRM drivers will get the lockdep warning.
>
> 3. Make drm_gem_shmem_vmap() to take the dma_resv_lock and get deadlock
> if dma-buf importer holds the lock.
>
> ...
Yeah this is all very annoying.
> There are actually very few drivers in kernel that use dma_buf_vmap()
> [1], so perhaps it's not really a big deal to first try to define the
> locking and pinning convention for the dma-bufs? At least for
> dma_buf_vmap()? Let me try to do this.
>
> [1] https://elixir.bootlin.com/linux/v5.18-rc6/C/ident/dma_buf_vmap
Yeah looking through the code there's largely two classes of drivers that
need vmap:
- display drivers that need to do cpu upload (usb, spi, i2c displays).
Those generally set up the vmap at import time or when creating the
drm_framebuffer object (e.g. see
drm_gem_cma_prime_import_sg_table_vmap()), because that's really the
only place where you can safely do that without running into locking
inversion issues sooner or later
- lots of other drivers (and shmem helpers) seem to do dma_buf_vmap just
because they can, but only actually ever use vmap on native objects,
never on imported objects. Or at least I think so.
So maybe another approach here:
1. In general drivers which need a vmap need to set that up at dma_buf
import time - the same way we pin the buffers at import time for
non-dynamic importers because that's the only place where across all
drivers it's ok to just take dma_resv_lock.
2. We remove the "just because we can" dma_buf_vmap support from
helpers/drivers - the paths all already can cope with NULL since
dma_buf_vmap can fail. vmap will only work on native objects, not imported
ones.
3. If there is any driver using shmem helpers that absolutely needs vmap
to also work on imported it needs a special import function (like cma
helpers) which sets up the vmap at import time.
So since this is all very tricky ... what did I miss this time around?
> I envision that the extra dma_resv_locks for dma-bufs potentially may
> create unnecessary bottlenecks for some drivers if locking isn't really
> necessary by a specific driver, so drivers will need to keep this in
> mind. On the other hand, I don't think that any of the today's drivers
> will notice the additional resv locks in practice.
Nah I don't think the extra locking will ever create a bottleneck,
especially not for vmap. Generally vmap is a fallback or at least cpu
operation, so at that point you're already going very slow.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, May 11, 2022 at 06:40:32PM +0300, Dmitry Osipenko wrote:
> On 5/11/22 18:29, Daniel Vetter wrote:
> > On Wed, May 11, 2022 at 06:14:00PM +0300, Dmitry Osipenko wrote:
> >> On 5/11/22 17:24, Christian K?nig wrote:
> >>> Am 11.05.22 um 15:00 schrieb Daniel Vetter:
> >>>> On Tue, May 10, 2022 at 04:39:53PM +0300, Dmitry Osipenko wrote:
> >>>>> [SNIP]
> >>>>> Since vmapping implies implicit pinning, we can't use a separate lock in
> >>>>> drm_gem_shmem_vmap() because we need to protect the
> >>>>> drm_gem_shmem_get_pages(), which is invoked by drm_gem_shmem_vmap() to
> >>>>> pin the pages and requires the dma_resv_lock to be locked.
> >>>>>
> >>>>> Hence the problem is:
> >>>>>
> >>>>> 1. If dma-buf importer holds the dma_resv_lock and invokes
> >>>>> dma_buf_vmap() -> drm_gem_shmem_vmap(), then drm_gem_shmem_vmap() shall
> >>>>> not take the dma_resv_lock.
> >>>>>
> >>>>> 2. Since dma-buf locking convention isn't specified, we can't assume
> >>>>> that dma-buf importer holds the dma_resv_lock around dma_buf_vmap().
> >>>>>
> >>>>> The possible solutions are:
> >>>>>
> >>>>> 1. Specify the dma_resv_lock convention for dma-bufs and make all
> >>>>> drivers to follow it.
> >>>>>
> >>>>> 2. Make only DRM drivers to hold dma_resv_lock around dma_buf_vmap().
> >>>>> Other non-DRM drivers will get the lockdep warning.
> >>>>>
> >>>>> 3. Make drm_gem_shmem_vmap() to take the dma_resv_lock and get deadlock
> >>>>> if dma-buf importer holds the lock.
> >>>>>
> >>>>> ...
> >>>> Yeah this is all very annoying.
> >>> Ah, yes that topic again :)
> >>>
> >>> I think we could relatively easily fix that by just defining and
> >>> enforcing that the dma_resv_lock must have be taken by the caller when
> >>> dma_buf_vmap() is called.
> >>>
> >>> A two step approach should work:
> >>> 1. Move the call to dma_resv_lock() into the dma_buf_vmap() function and
> >>> remove all lock taking from the vmap callback implementations.
> >>> 2. Move the call to dma_resv_lock() into the callers of dma_buf_vmap()
> >>> and enforce that the function is called with the lock held.
> >> I've doubts about the need to move out the dma_resv_lock() into the
> >> callers of dma_buf_vmap()..
> >>
> >> I looked through all the dma_buf_vmap() users and neither of them
> >> interacts with dma_resv_lock() at all, i.e. nobody takes the lock
> >> in/outside of dma_buf_vmap(). Hence it's easy and more practical to make
> >> dma_buf_mmap/vmap() to take the dma_resv_lock by themselves.
> > i915_gem_dmabuf_vmap -> i915_gem_object_pin_map_unlocked ->
> > i915_gem_object_lock -> dma_resv_lock
> >
> > And all the ttm drivers should work similarly. So there's definitely
> > drivers which grab dma_resv_lock from their vmap callback.
>
> Grr.. I'll take another look.
>
> >> It's unclear to me which driver may ever want to do the mapping under
> >> the dma_resv_lock. But if we will ever have such a driver that will need
> >> to map imported buffer under dma_resv_lock, then we could always add the
> >> dma_buf_vmap_locked() variant of the function. In this case the locking
> >> rule will sound like this:
> >>
> >> "All dma-buf importers are responsible for holding the dma-reservation
> >> lock around the dmabuf->ops->mmap/vmap() calls."
>
> Are you okay with this rule?
Yeah I think long-term it's where we want to be, just trying to find
clever ways to get there.
And I think Christian agrees with that?
> >>> It shouldn't be that hard to clean up. The last time I looked into it my
> >>> main problem was that we didn't had any easy unit test for it.
> >> Do we have any tests for dma-bufs at all? It's unclear to me what you
> >> are going to test in regards to the reservation locks, could you please
> >> clarify?
> > Unfortunately not really :-/ Only way really is to grab a driver which
> > needs vmap (those are mostly display drivers) on an imported buffer, and
> > see what happens.
> >
> > 2nd best is liberally sprinkling lockdep annotations all over the place
> > and throwing it at intel ci (not sure amd ci is accessible to the public)
> > and then hoping that's good enough. Stuff like might_lock and
> > dma_resv_assert_held.
>
> Alright
So throwing it at intel-gfx-ci can't hurt I think, but that only covers
i915 so doesn't really help with the bigger issue of catching all the
drivers.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, May 12, 2022 at 09:29:35AM +0200, Christian K?nig wrote:
> Am 11.05.22 um 21:05 schrieb Daniel Vetter:
> > [SNIP]
> > > > > It's unclear to me which driver may ever want to do the mapping under
> > > > > the dma_resv_lock. But if we will ever have such a driver that will need
> > > > > to map imported buffer under dma_resv_lock, then we could always add the
> > > > > dma_buf_vmap_locked() variant of the function. In this case the locking
> > > > > rule will sound like this:
> > > > >
> > > > > "All dma-buf importers are responsible for holding the dma-reservation
> > > > > lock around the dmabuf->ops->mmap/vmap() calls."
> > > Are you okay with this rule?
> > Yeah I think long-term it's where we want to be, just trying to find
> > clever ways to get there.
> >
> > And I think Christian agrees with that?
>
> Yes, completely.
>
> A design where most DMA-buf functions are supposed to be called with the
> reservation lock held is exactly what I have in mind for the long term.
>
> > > > > > It shouldn't be that hard to clean up. The last time I looked into it my
> > > > > > main problem was that we didn't had any easy unit test for it.
> > > > > Do we have any tests for dma-bufs at all? It's unclear to me what you
> > > > > are going to test in regards to the reservation locks, could you please
> > > > > clarify?
> > > > Unfortunately not really :-/ Only way really is to grab a driver which
> > > > needs vmap (those are mostly display drivers) on an imported buffer, and
> > > > see what happens.
> > > >
> > > > 2nd best is liberally sprinkling lockdep annotations all over the place
> > > > and throwing it at intel ci (not sure amd ci is accessible to the public)
> > > > and then hoping that's good enough. Stuff like might_lock and
> > > > dma_resv_assert_held.
> > > Alright
> > So throwing it at intel-gfx-ci can't hurt I think, but that only covers
> > i915 so doesn't really help with the bigger issue of catching all the
> > drivers.
>
> BTW: We have now somebody working on converting the existing libdrm_amdgpu
> unit tests over to igt.
This sounds awesome.
/me throws a happy dance
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Am 11.05.22 um 21:05 schrieb Daniel Vetter:
> [SNIP]
>>>> It's unclear to me which driver may ever want to do the mapping under
>>>> the dma_resv_lock. But if we will ever have such a driver that will need
>>>> to map imported buffer under dma_resv_lock, then we could always add the
>>>> dma_buf_vmap_locked() variant of the function. In this case the locking
>>>> rule will sound like this:
>>>>
>>>> "All dma-buf importers are responsible for holding the dma-reservation
>>>> lock around the dmabuf->ops->mmap/vmap() calls."
>> Are you okay with this rule?
> Yeah I think long-term it's where we want to be, just trying to find
> clever ways to get there.
>
> And I think Christian agrees with that?
Yes, completely.
A design where most DMA-buf functions are supposed to be called with the
reservation lock held is exactly what I have in mind for the long term.
>>>>> It shouldn't be that hard to clean up. The last time I looked into it my
>>>>> main problem was that we didn't had any easy unit test for it.
>>>> Do we have any tests for dma-bufs at all? It's unclear to me what you
>>>> are going to test in regards to the reservation locks, could you please
>>>> clarify?
>>> Unfortunately not really :-/ Only way really is to grab a driver which
>>> needs vmap (those are mostly display drivers) on an imported buffer, and
>>> see what happens.
>>>
>>> 2nd best is liberally sprinkling lockdep annotations all over the place
>>> and throwing it at intel ci (not sure amd ci is accessible to the public)
>>> and then hoping that's good enough. Stuff like might_lock and
>>> dma_resv_assert_held.
>> Alright
> So throwing it at intel-gfx-ci can't hurt I think, but that only covers
> i915 so doesn't really help with the bigger issue of catching all the
> drivers.
BTW: We have now somebody working on converting the existing
libdrm_amdgpu unit tests over to igt.
Regards,
Christian.
>
> Cheers, Daniel