[PATCH v2 0/8] Add memory shrinker to VirtIO-GPU DRM driver

[PATCH v2 0/8] Add memory shrinker to VirtIO-GPU DRM driver

[Dmitry Osipenko]

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. 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 includes couple fixes for problems of VirtIO-GPU driver that
I found while was working on the shrinker, it also includes prerequisite
DMA API usage improvement needed by the shrinker.

The Mesa and IGT patches will be kept on hold until this kernel series
will be approved and applied.

This patchset was tested using Qemu and crosvm, including both cases of
IOMMU off/on.

Note that this patchset only enables initial shrinking of the guest memory,
shrinking of the host memory is unsupported yet.

Mesa: https://gitlab.freedesktop.org/digetx/mesa/-/commits/virgl-madvise
IGT:  https://gitlab.freedesktop.org/digetx/igt-gpu-tools/-/tree/virtio-madvise

Changelog:

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 (8):
  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 in error code path
  drm/virtio: Improve DMA API usage for shmem BOs
  drm/shmem-helper: Correct doc-comment of drm_gem_shmem_get_sg_table()
  drm/shmem-helper: Add generic memory shrinker
  drm/virtio: Support memory shrinking
  drm/panfrost: Switch to generic memory shrinker

 drivers/gpu/drm/drm_gem_shmem_helper.c     | 196 ++++++++++++++++++++-
 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    |  27 +--
 drivers/gpu/drm/panfrost/panfrost_gem.h    |   9 -
 drivers/gpu/drm/panfrost/panfrost_job.c    |  22 ++-
 drivers/gpu/drm/virtio/virtgpu_drv.c       |  22 ++-
 drivers/gpu/drm/virtio/virtgpu_drv.h       |  26 ++-
 drivers/gpu/drm/virtio/virtgpu_gem.c       |  96 ++++++++++
 drivers/gpu/drm/virtio/virtgpu_ioctl.c     |  37 ++++
 drivers/gpu/drm/virtio/virtgpu_kms.c       |  17 +-
 drivers/gpu/drm/virtio/virtgpu_object.c    |  78 ++++----
 drivers/gpu/drm/virtio/virtgpu_plane.c     |  17 +-
 drivers/gpu/drm/virtio/virtgpu_vq.c        |  30 +++-
 include/drm/drm_device.h                   |   4 +
 include/drm/drm_gem.h                      |  11 ++
 include/drm/drm_gem_shmem_helper.h         |  25 +++
 include/uapi/drm/virtgpu_drm.h             |  14 ++
 19 files changed, 544 insertions(+), 111 deletions(-)

-- 
2.35.1

[PATCH v2 1/8] drm/virtio: Correct drm_gem_shmem_get_sg_table() error handling

[Dmitry Osipenko]

drm_gem_shmem_get_sg_table() never ever returned NULL on error. Correct
the error handling to avoid crash on OOM.

Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 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..bea7806a3ae3 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(shmem->pages);
+	if (ret) {
 		drm_gem_shmem_unpin(&bo->base);
-		return -EINVAL;
+		shmem->pages = NULL;
+		return ret;
 	}
 
 	if (use_dma_api) {
-- 
2.35.1

Re: [PATCH v2 1/8] drm/virtio: Correct drm_gem_shmem_get_sg_table() error handling

[Dmitry Osipenko]

On 3/15/22 01:42, Dmitry Osipenko wrote:
> drm_gem_shmem_get_sg_table() never ever returned NULL on error. Correct
> the error handling to avoid crash on OOM.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  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..bea7806a3ae3 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(shmem->pages);

This actually needs to be PTR_ERR_OR_ZERO. This code is changed to use
drm_gem_shmem_get_pages_sgt()+IS_ERR() by the further patch, so I just
missed the typo previously. I'll correct it in v3.

[PATCH v2 2/8] drm/virtio: Check whether transferred 2D BO is shmem

[Dmitry Osipenko]

Transferred 2D BO always must be a shmem BO. Add check for that to prevent
NULL dereference if userspace passes a VRAM BO.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/virtio/virtgpu_vq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 7c052efe8836..2edf31806b74 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -595,7 +595,7 @@ void virtio_gpu_cmd_transfer_to_host_2d(struct virtio_gpu_device *vgdev,
 	bool use_dma_api = !virtio_has_dma_quirk(vgdev->vdev);
 	struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);
 
-	if (use_dma_api)
+	if (virtio_gpu_is_shmem(bo) && use_dma_api)
 		dma_sync_sgtable_for_device(vgdev->vdev->dev.parent,
 					    shmem->pages, DMA_TO_DEVICE);
 
-- 
2.35.1

[PATCH v2 3/8] drm/virtio: Unlock GEM reservations in error code path

[Dmitry Osipenko]

Unlock reservations in the error code path of virtio_gpu_object_create()
to silence debug warning splat produced by ww_mutex_destroy(&obj->lock)
when GEM is released with the held lock.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/virtio/virtgpu_object.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index bea7806a3ae3..0b8cbb87f8d8 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -250,6 +250,8 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
 
 	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;
-- 
2.35.1

[PATCH v2 4/8] drm/virtio: Improve DMA API usage for shmem BOs

[Dmitry Osipenko]

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 <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.c    | 22 +++++++---
 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, 37 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 5f25a8d15464..8449dad3e65c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -46,9 +46,9 @@ 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];
@@ -101,6 +101,7 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev, struct virtio_device *vd
 static int virtio_gpu_probe(struct virtio_device *vdev)
 {
 	struct drm_device *dev;
+	struct device *dma_dev;
 	int ret;
 
 	if (drm_firmware_drivers_only() && virtio_gpu_modeset == -1)
@@ -109,18 +110,29 @@ static int virtio_gpu_probe(struct virtio_device *vdev)
 	if (virtio_gpu_modeset == 0)
 		return -EINVAL;
 
-	dev = drm_dev_alloc(&driver, &vdev->dev);
+	/*
+	 * If GPU's parent is a PCI device, then we will use this PCI device
+	 * for the DRM's driver device because GPU won't have PCI's IOMMU DMA
+	 * ops in this case since GPU device is sitting on a separate (from PCI)
+	 * virtio-bus.
+	 */
+	if (!strcmp(vdev->dev.parent->bus->name, "pci"))
+		dma_dev = vdev->dev.parent;
+	else
+		dma_dev = &vdev->dev;
+
+	dev = drm_dev_alloc(&driver, dma_dev);
 	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 0b8cbb87f8d8..1964c0d8b51f 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(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

Re: [PATCH v2 4/8] drm/virtio: Improve DMA API usage for shmem BOs

[Robin Murphy]

On 2022-03-14 22:42, Dmitry Osipenko wrote:
> 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 <dmitry.osipenko@collabora.com>
> ---
>   drivers/gpu/drm/virtio/virtgpu_drv.c    | 22 +++++++---
>   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, 37 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index 5f25a8d15464..8449dad3e65c 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -46,9 +46,9 @@ 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];
> @@ -101,6 +101,7 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev, struct virtio_device *vd
>   static int virtio_gpu_probe(struct virtio_device *vdev)
>   {
>   	struct drm_device *dev;
> +	struct device *dma_dev;
>   	int ret;
>   
>   	if (drm_firmware_drivers_only() && virtio_gpu_modeset == -1)
> @@ -109,18 +110,29 @@ static int virtio_gpu_probe(struct virtio_device *vdev)
>   	if (virtio_gpu_modeset == 0)
>   		return -EINVAL;
>   
> -	dev = drm_dev_alloc(&driver, &vdev->dev);
> +	/*
> +	 * If GPU's parent is a PCI device, then we will use this PCI device
> +	 * for the DRM's driver device because GPU won't have PCI's IOMMU DMA
> +	 * ops in this case since GPU device is sitting on a separate (from PCI)
> +	 * virtio-bus.
> +	 */
> +	if (!strcmp(vdev->dev.parent->bus->name, "pci"))

Nit: dev_is_pci() ?

However, what about other VirtIO transports? Wouldn't virtio-mmio with 
F_ACCESS_PLATFORM be in a similar situation?

Robin.

> +		dma_dev = vdev->dev.parent;
> +	else
> +		dma_dev = &vdev->dev;
> +
> +	dev = drm_dev_alloc(&driver, dma_dev);
>   	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 0b8cbb87f8d8..1964c0d8b51f 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(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));

Re: [PATCH v2 4/8] drm/virtio: Improve DMA API usage for shmem BOs

[Dmitry Osipenko]

On 3/16/22 15:41, Robin Murphy wrote:
> On 2022-03-14 22:42, Dmitry Osipenko wrote:
>> 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 <dmitry.osipenko@collabora.com>
>> ---
>>   drivers/gpu/drm/virtio/virtgpu_drv.c    | 22 +++++++---
>>   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, 37 insertions(+), 66 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
>> b/drivers/gpu/drm/virtio/virtgpu_drv.c
>> index 5f25a8d15464..8449dad3e65c 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
>> @@ -46,9 +46,9 @@ 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];
>> @@ -101,6 +101,7 @@ static int virtio_gpu_pci_quirk(struct drm_device
>> *dev, struct virtio_device *vd
>>   static int virtio_gpu_probe(struct virtio_device *vdev)
>>   {
>>       struct drm_device *dev;
>> +    struct device *dma_dev;
>>       int ret;
>>         if (drm_firmware_drivers_only() && virtio_gpu_modeset == -1)
>> @@ -109,18 +110,29 @@ static int virtio_gpu_probe(struct virtio_device
>> *vdev)
>>       if (virtio_gpu_modeset == 0)
>>           return -EINVAL;
>>   -    dev = drm_dev_alloc(&driver, &vdev->dev);
>> +    /*
>> +     * If GPU's parent is a PCI device, then we will use this PCI device
>> +     * for the DRM's driver device because GPU won't have PCI's IOMMU
>> DMA
>> +     * ops in this case since GPU device is sitting on a separate
>> (from PCI)
>> +     * virtio-bus.
>> +     */
>> +    if (!strcmp(vdev->dev.parent->bus->name, "pci"))
> 
> Nit: dev_is_pci() ?

Yes, thank you.

> However, what about other VirtIO transports? Wouldn't virtio-mmio with
> F_ACCESS_PLATFORM be in a similar situation?

I couldn't find anyone using virtio-mmio for the GPU, both Qemu and
crosvm support only PCI transport for GPU.

But I'm now looking at virtio_mmio_probe() and see that virtio-mmio
devices actually should be in the exactly same position as PCI devices.
So you should be right and we need to use vdev->dev.parent for the DRM
device universally. I'll improve it in the v3, thank you again.

Re: [PATCH v2 4/8] drm/virtio: Improve DMA API usage for shmem BOs

[Dmitry Osipenko]

On 3/15/22 01:42, Dmitry Osipenko wrote:
> 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 <dmitry.osipenko@collabora.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.c    | 22 +++++++---
>  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, 37 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index 5f25a8d15464..8449dad3e65c 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -46,9 +46,9 @@ 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)

Somehow I missed that virtio_gpu_pci_quirk() contains comment telling
about why dev.parent isn't used for the DRM's device.

	/*
	 * 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.
	 */

There is no I2C, nor prime import support and framebuffer_alloc hasn't
been used for years now. I guess prime import actually will want the
real device and not the virtio_device if GEM importing will be ever
supported by VirtIO-GPU driver. Apparently this comment was outdated a
long time ago and the "visual changes" aren't too important, the
messages now says "virtio-pci 0000:00:02.0" instead of "virtio_gpu virtio0".

I'll remove the comment and drm_dev_set_unique() quirk in v3.

[PATCH v2 5/8] drm/shmem-helper: Correct doc-comment of drm_gem_shmem_get_sg_table()

[Dmitry Osipenko]

drm_gem_shmem_get_sg_table() never returns NULL on error, but a ERR_PTR.
Correct the doc comment which says that it returns NULL on error.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8ad0e02991ca..37009418cd28 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -662,7 +662,7 @@ EXPORT_SYMBOL(drm_gem_shmem_print_info);
  * drm_gem_shmem_get_pages_sgt() instead.
  *
  * Returns:
- * A pointer to the scatter/gather table of pinned pages or NULL on failure.
+ * A pointer to the scatter/gather table of pinned pages or errno on failure.
  */
 struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem)
 {
-- 
2.35.1

[PATCH v2 6/8] drm/shmem-helper: Add generic memory shrinker

[Dmitry Osipenko]

Introduce a common DRM SHMEM shrinker. It allows to reduce code
duplication among DRM drivers, it also handles complicated lockings
for the drivers. This is initial version of the shrinker that covers
basic needs of GPU drivers.

This patch is based on a couple ideas borrowed from Rob's Clark MSM
shrinker and Thomas' Zimmermann variant of SHMEM shrinker.

GPU drivers that want to use generic DRM memory shrinker must support
generic GEM reservations.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 194 +++++++++++++++++++++++++
 include/drm/drm_device.h               |   4 +
 include/drm/drm_gem.h                  |  11 ++
 include/drm/drm_gem_shmem_helper.h     |  25 ++++
 4 files changed, 234 insertions(+)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 37009418cd28..35be2ee98f11 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -139,6 +139,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
 {
 	struct drm_gem_object *obj = &shmem->base;
 
+	/* take out shmem GEM object from the memory shrinker */
+	drm_gem_shmem_madvise(shmem, 0);
+
 	WARN_ON(shmem->vmap_use_count);
 
 	if (obj->import_attach) {
@@ -163,6 +166,42 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
 
+static void drm_gem_shmem_update_purgeable_status(struct drm_gem_shmem_object *shmem)
+{
+	struct drm_gem_object *obj = &shmem->base;
+	struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
+	size_t page_count = obj->size >> PAGE_SHIFT;
+
+	if (!gem_shrinker || obj->import_attach || !obj->funcs->purge)
+		return;
+
+	mutex_lock(&shmem->vmap_lock);
+	mutex_lock(&shmem->pages_lock);
+	mutex_lock(&gem_shrinker->lock);
+
+	if (shmem->madv < 0) {
+		list_del_init(&shmem->madv_list);
+		goto unlock;
+	} else if (shmem->madv > 0) {
+		if (!list_empty(&shmem->madv_list))
+			goto unlock;
+
+		WARN_ON(gem_shrinker->shrinkable_count + page_count < page_count);
+		gem_shrinker->shrinkable_count += page_count;
+
+		list_add_tail(&shmem->madv_list, &gem_shrinker->lru);
+	} else if (!list_empty(&shmem->madv_list)) {
+		list_del_init(&shmem->madv_list);
+
+		WARN_ON(gem_shrinker->shrinkable_count < page_count);
+		gem_shrinker->shrinkable_count -= page_count;
+	}
+unlock:
+	mutex_unlock(&gem_shrinker->lock);
+	mutex_unlock(&shmem->pages_lock);
+	mutex_unlock(&shmem->vmap_lock);
+}
+
 static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
 {
 	struct drm_gem_object *obj = &shmem->base;
@@ -366,6 +405,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
 	ret = drm_gem_shmem_vmap_locked(shmem, map);
 	mutex_unlock(&shmem->vmap_lock);
 
+	drm_gem_shmem_update_purgeable_status(shmem);
+
 	return ret;
 }
 EXPORT_SYMBOL(drm_gem_shmem_vmap);
@@ -409,6 +450,8 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
 	mutex_lock(&shmem->vmap_lock);
 	drm_gem_shmem_vunmap_locked(shmem, map);
 	mutex_unlock(&shmem->vmap_lock);
+
+	drm_gem_shmem_update_purgeable_status(shmem);
 }
 EXPORT_SYMBOL(drm_gem_shmem_vunmap);
 
@@ -451,6 +494,8 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
 
 	mutex_unlock(&shmem->pages_lock);
 
+	drm_gem_shmem_update_purgeable_status(shmem);
+
 	return (madv >= 0);
 }
 EXPORT_SYMBOL(drm_gem_shmem_madvise);
@@ -763,6 +808,155 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
 
+static struct drm_gem_shmem_shrinker *
+to_drm_shrinker(struct shrinker *shrinker)
+{
+	return container_of(shrinker, struct drm_gem_shmem_shrinker, base);
+}
+
+static unsigned long
+drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker,
+				     struct shrink_control *sc)
+{
+	struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
+	u64 count = gem_shrinker->shrinkable_count;
+
+	if (count >= SHRINK_EMPTY)
+		return SHRINK_EMPTY - 1;
+
+	return count ?: SHRINK_EMPTY;
+}
+
+static unsigned long
+drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker,
+				    struct shrink_control *sc)
+{
+	struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
+	struct drm_gem_shmem_object *shmem;
+	struct list_head still_in_list;
+	bool lock_contention = true;
+	struct drm_gem_object *obj;
+	unsigned long freed = 0;
+
+	INIT_LIST_HEAD(&still_in_list);
+
+	mutex_lock(&gem_shrinker->lock);
+
+	while (freed < sc->nr_to_scan) {
+		shmem = list_first_entry_or_null(&gem_shrinker->lru,
+						 typeof(*shmem), madv_list);
+		if (!shmem)
+			break;
+
+		obj = &shmem->base;
+		list_move_tail(&shmem->madv_list, &still_in_list);
+
+		/*
+		 * If it's in the process of being freed, gem_object->free()
+		 * may be blocked on lock waiting to remove it.  So just
+		 * skip it.
+		 */
+		if (!kref_get_unless_zero(&obj->refcount))
+			continue;
+
+		mutex_unlock(&gem_shrinker->lock);
+
+		/* prevent racing with job submission code paths */
+		if (!dma_resv_trylock(obj->resv))
+			goto shrinker_lock;
+
+		/* prevent racing with the dma-buf exporting */
+		if (!mutex_trylock(&gem_shrinker->dev->object_name_lock))
+			goto resv_unlock;
+
+		if (!mutex_trylock(&shmem->vmap_lock))
+			goto object_name_unlock;
+
+		if (!mutex_trylock(&shmem->pages_lock))
+			goto vmap_unlock;
+
+		lock_contention = false;
+
+		/* check whether h/w uses this object */
+		if (!dma_resv_test_signaled(obj->resv, true))
+			goto pages_unlock;
+
+		/* GEM may've become unpurgeable while shrinker was unlocked */
+		if (!drm_gem_shmem_is_purgeable(shmem))
+			goto pages_unlock;
+
+		freed += obj->funcs->purge(obj);
+pages_unlock:
+		mutex_unlock(&shmem->pages_lock);
+vmap_unlock:
+		mutex_unlock(&shmem->vmap_lock);
+object_name_unlock:
+		mutex_unlock(&gem_shrinker->dev->object_name_lock);
+resv_unlock:
+		dma_resv_unlock(obj->resv);
+shrinker_lock:
+		drm_gem_object_put(&shmem->base);
+		mutex_lock(&gem_shrinker->lock);
+	}
+
+	list_splice_tail(&still_in_list, &gem_shrinker->lru);
+	WARN_ON(gem_shrinker->shrinkable_count < freed);
+	gem_shrinker->shrinkable_count -= freed;
+
+	mutex_unlock(&gem_shrinker->lock);
+
+	if (!freed && !lock_contention)
+		return SHRINK_STOP;
+
+	return freed;
+}
+
+int drm_gem_shmem_shrinker_register(struct drm_device *dev)
+{
+	struct drm_gem_shmem_shrinker *gem_shrinker;
+	int err;
+
+	if (WARN_ON(dev->shmem_shrinker))
+		return -EBUSY;
+
+	gem_shrinker = kzalloc(sizeof(*gem_shrinker), GFP_KERNEL);
+	if (!gem_shrinker)
+		return -ENOMEM;
+
+	gem_shrinker->base.count_objects = drm_gem_shmem_shrinker_count_objects;
+	gem_shrinker->base.scan_objects = drm_gem_shmem_shrinker_scan_objects;
+	gem_shrinker->base.seeks = DEFAULT_SEEKS;
+	gem_shrinker->dev = dev;
+
+	INIT_LIST_HEAD(&gem_shrinker->lru);
+	mutex_init(&gem_shrinker->lock);
+
+	dev->shmem_shrinker = gem_shrinker;
+
+	err = register_shrinker(&gem_shrinker->base);
+	if (err) {
+		dev->shmem_shrinker = NULL;
+		kfree(gem_shrinker);
+		return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_register);
+
+void drm_gem_shmem_shrinker_unregister(struct drm_device *dev)
+{
+	struct drm_gem_shmem_shrinker *gem_shrinker = dev->shmem_shrinker;
+
+	if (gem_shrinker) {
+		unregister_shrinker(&gem_shrinker->base);
+		mutex_destroy(&gem_shrinker->lock);
+		dev->shmem_shrinker = NULL;
+		kfree(gem_shrinker);
+	}
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_unregister);
+
 MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
 MODULE_IMPORT_NS(DMA_BUF);
 MODULE_LICENSE("GPL v2");
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 9923c7a6885e..929546cad894 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -16,6 +16,7 @@ struct drm_vblank_crtc;
 struct drm_vma_offset_manager;
 struct drm_vram_mm;
 struct drm_fb_helper;
+struct drm_gem_shmem_shrinker;
 
 struct inode;
 
@@ -277,6 +278,9 @@ struct drm_device {
 	/** @vram_mm: VRAM MM memory manager */
 	struct drm_vram_mm *vram_mm;
 
+	/** @shmem_shrinker: SHMEM GEM memory shrinker */
+	struct drm_gem_shmem_shrinker *shmem_shrinker;
+
 	/**
 	 * @switch_power_state:
 	 *
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index e2941cee14b6..cdb99cfbf0bc 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -172,6 +172,17 @@ struct drm_gem_object_funcs {
 	 * This is optional but necessary for mmap support.
 	 */
 	const struct vm_operations_struct *vm_ops;
+
+	/**
+	 * @purge:
+	 *
+	 * Releases the GEM object's allocated backing storage to the system.
+	 *
+	 * Returns the number of pages that have been freed by purging the GEM object.
+	 *
+	 * This callback is used by the GEM shrinker.
+	 */
+	unsigned long (*purge)(struct drm_gem_object *obj);
 };
 
 /**
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index d0a57853c188..455254f131f6 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -6,6 +6,7 @@
 #include <linux/fs.h>
 #include <linux/mm.h>
 #include <linux/mutex.h>
+#include <linux/shrinker.h>
 
 #include <drm/drm_file.h>
 #include <drm/drm_gem.h>
@@ -15,6 +16,7 @@
 struct dma_buf_attachment;
 struct drm_mode_create_dumb;
 struct drm_printer;
+struct drm_device;
 struct sg_table;
 
 /**
@@ -272,6 +274,29 @@ static inline int drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct v
 	return drm_gem_shmem_mmap(shmem, vma);
 }
 
+/**
+ * struct drm_gem_shmem_shrinker - Generic memory shrinker for shmem GEMs
+ */
+struct drm_gem_shmem_shrinker {
+	/** @base: Shrinker for purging shmem GEM objects */
+	struct shrinker base;
+
+	/** @lock: Protects @lru */
+	struct mutex lock;
+
+	/** @lru: List of shmem GEM objects available for purging */
+	struct list_head lru;
+
+	/** @dev: DRM device that uses this shrinker */
+	struct drm_device *dev;
+
+	/** @shrinkable_count: Count of shmem GEM pages to be purged */
+	u64 shrinkable_count;
+};
+
+int drm_gem_shmem_shrinker_register(struct drm_device *dev);
+void drm_gem_shmem_shrinker_unregister(struct drm_device *dev);
+
 /*
  * Driver ops
  */
-- 
2.35.1

Re: [PATCH v2 6/8] drm/shmem-helper: Add generic memory shrinker

[Steven Price]

On 14/03/2022 22:42, Dmitry Osipenko wrote:
> Introduce a common DRM SHMEM shrinker. It allows to reduce code
> duplication among DRM drivers, it also handles complicated lockings
> for the drivers. This is initial version of the shrinker that covers
> basic needs of GPU drivers.
> 
> This patch is based on a couple ideas borrowed from Rob's Clark MSM
> shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
> 
> GPU drivers that want to use generic DRM memory shrinker must support
> generic GEM reservations.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

This looks fine to me, but one nitpick: you should update the comment in
struct drm_gem_shmem_object:

> 	/**
> 	 * @madv: State for madvise
> 	 *
> 	 * 0 is active/inuse.
> 	 * A negative value is the object is purged.
> 	 * Positive values are driver specific and not used by the helpers.
> 	 */
> 	int madv;

This is adding a helper which cares about the positive values.

Steve

> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 194 +++++++++++++++++++++++++
>  include/drm/drm_device.h               |   4 +
>  include/drm/drm_gem.h                  |  11 ++
>  include/drm/drm_gem_shmem_helper.h     |  25 ++++
>  4 files changed, 234 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 37009418cd28..35be2ee98f11 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -139,6 +139,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
>  {
>  	struct drm_gem_object *obj = &shmem->base;
>  
> +	/* take out shmem GEM object from the memory shrinker */
> +	drm_gem_shmem_madvise(shmem, 0);
> +
>  	WARN_ON(shmem->vmap_use_count);
>  
>  	if (obj->import_attach) {
> @@ -163,6 +166,42 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
>  
> +static void drm_gem_shmem_update_purgeable_status(struct drm_gem_shmem_object *shmem)
> +{
> +	struct drm_gem_object *obj = &shmem->base;
> +	struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
> +	size_t page_count = obj->size >> PAGE_SHIFT;
> +
> +	if (!gem_shrinker || obj->import_attach || !obj->funcs->purge)
> +		return;
> +
> +	mutex_lock(&shmem->vmap_lock);
> +	mutex_lock(&shmem->pages_lock);
> +	mutex_lock(&gem_shrinker->lock);
> +
> +	if (shmem->madv < 0) {
> +		list_del_init(&shmem->madv_list);
> +		goto unlock;
> +	} else if (shmem->madv > 0) {
> +		if (!list_empty(&shmem->madv_list))
> +			goto unlock;
> +
> +		WARN_ON(gem_shrinker->shrinkable_count + page_count < page_count);
> +		gem_shrinker->shrinkable_count += page_count;
> +
> +		list_add_tail(&shmem->madv_list, &gem_shrinker->lru);
> +	} else if (!list_empty(&shmem->madv_list)) {
> +		list_del_init(&shmem->madv_list);
> +
> +		WARN_ON(gem_shrinker->shrinkable_count < page_count);
> +		gem_shrinker->shrinkable_count -= page_count;
> +	}
> +unlock:
> +	mutex_unlock(&gem_shrinker->lock);
> +	mutex_unlock(&shmem->pages_lock);
> +	mutex_unlock(&shmem->vmap_lock);
> +}
> +
>  static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
>  {
>  	struct drm_gem_object *obj = &shmem->base;
> @@ -366,6 +405,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>  	ret = drm_gem_shmem_vmap_locked(shmem, map);
>  	mutex_unlock(&shmem->vmap_lock);
>  
> +	drm_gem_shmem_update_purgeable_status(shmem);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_gem_shmem_vmap);
> @@ -409,6 +450,8 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
>  	mutex_lock(&shmem->vmap_lock);
>  	drm_gem_shmem_vunmap_locked(shmem, map);
>  	mutex_unlock(&shmem->vmap_lock);
> +
> +	drm_gem_shmem_update_purgeable_status(shmem);
>  }
>  EXPORT_SYMBOL(drm_gem_shmem_vunmap);
>  
> @@ -451,6 +494,8 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
>  
>  	mutex_unlock(&shmem->pages_lock);
>  
> +	drm_gem_shmem_update_purgeable_status(shmem);
> +
>  	return (madv >= 0);
>  }
>  EXPORT_SYMBOL(drm_gem_shmem_madvise);
> @@ -763,6 +808,155 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>  
> +static struct drm_gem_shmem_shrinker *
> +to_drm_shrinker(struct shrinker *shrinker)
> +{
> +	return container_of(shrinker, struct drm_gem_shmem_shrinker, base);
> +}
> +
> +static unsigned long
> +drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker,
> +				     struct shrink_control *sc)
> +{
> +	struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
> +	u64 count = gem_shrinker->shrinkable_count;
> +
> +	if (count >= SHRINK_EMPTY)
> +		return SHRINK_EMPTY - 1;
> +
> +	return count ?: SHRINK_EMPTY;
> +}
> +
> +static unsigned long
> +drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker,
> +				    struct shrink_control *sc)
> +{
> +	struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
> +	struct drm_gem_shmem_object *shmem;
> +	struct list_head still_in_list;
> +	bool lock_contention = true;
> +	struct drm_gem_object *obj;
> +	unsigned long freed = 0;
> +
> +	INIT_LIST_HEAD(&still_in_list);
> +
> +	mutex_lock(&gem_shrinker->lock);
> +
> +	while (freed < sc->nr_to_scan) {
> +		shmem = list_first_entry_or_null(&gem_shrinker->lru,
> +						 typeof(*shmem), madv_list);
> +		if (!shmem)
> +			break;
> +
> +		obj = &shmem->base;
> +		list_move_tail(&shmem->madv_list, &still_in_list);
> +
> +		/*
> +		 * If it's in the process of being freed, gem_object->free()
> +		 * may be blocked on lock waiting to remove it.  So just
> +		 * skip it.
> +		 */
> +		if (!kref_get_unless_zero(&obj->refcount))
> +			continue;
> +
> +		mutex_unlock(&gem_shrinker->lock);
> +
> +		/* prevent racing with job submission code paths */
> +		if (!dma_resv_trylock(obj->resv))
> +			goto shrinker_lock;
> +
> +		/* prevent racing with the dma-buf exporting */
> +		if (!mutex_trylock(&gem_shrinker->dev->object_name_lock))
> +			goto resv_unlock;
> +
> +		if (!mutex_trylock(&shmem->vmap_lock))
> +			goto object_name_unlock;
> +
> +		if (!mutex_trylock(&shmem->pages_lock))
> +			goto vmap_unlock;
> +
> +		lock_contention = false;
> +
> +		/* check whether h/w uses this object */
> +		if (!dma_resv_test_signaled(obj->resv, true))
> +			goto pages_unlock;
> +
> +		/* GEM may've become unpurgeable while shrinker was unlocked */
> +		if (!drm_gem_shmem_is_purgeable(shmem))
> +			goto pages_unlock;
> +
> +		freed += obj->funcs->purge(obj);
> +pages_unlock:
> +		mutex_unlock(&shmem->pages_lock);
> +vmap_unlock:
> +		mutex_unlock(&shmem->vmap_lock);
> +object_name_unlock:
> +		mutex_unlock(&gem_shrinker->dev->object_name_lock);
> +resv_unlock:
> +		dma_resv_unlock(obj->resv);
> +shrinker_lock:
> +		drm_gem_object_put(&shmem->base);
> +		mutex_lock(&gem_shrinker->lock);
> +	}
> +
> +	list_splice_tail(&still_in_list, &gem_shrinker->lru);
> +	WARN_ON(gem_shrinker->shrinkable_count < freed);
> +	gem_shrinker->shrinkable_count -= freed;
> +
> +	mutex_unlock(&gem_shrinker->lock);
> +
> +	if (!freed && !lock_contention)
> +		return SHRINK_STOP;
> +
> +	return freed;
> +}
> +
> +int drm_gem_shmem_shrinker_register(struct drm_device *dev)
> +{
> +	struct drm_gem_shmem_shrinker *gem_shrinker;
> +	int err;
> +
> +	if (WARN_ON(dev->shmem_shrinker))
> +		return -EBUSY;
> +
> +	gem_shrinker = kzalloc(sizeof(*gem_shrinker), GFP_KERNEL);
> +	if (!gem_shrinker)
> +		return -ENOMEM;
> +
> +	gem_shrinker->base.count_objects = drm_gem_shmem_shrinker_count_objects;
> +	gem_shrinker->base.scan_objects = drm_gem_shmem_shrinker_scan_objects;
> +	gem_shrinker->base.seeks = DEFAULT_SEEKS;
> +	gem_shrinker->dev = dev;
> +
> +	INIT_LIST_HEAD(&gem_shrinker->lru);
> +	mutex_init(&gem_shrinker->lock);
> +
> +	dev->shmem_shrinker = gem_shrinker;
> +
> +	err = register_shrinker(&gem_shrinker->base);
> +	if (err) {
> +		dev->shmem_shrinker = NULL;
> +		kfree(gem_shrinker);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_register);
> +
> +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev)
> +{
> +	struct drm_gem_shmem_shrinker *gem_shrinker = dev->shmem_shrinker;
> +
> +	if (gem_shrinker) {
> +		unregister_shrinker(&gem_shrinker->base);
> +		mutex_destroy(&gem_shrinker->lock);
> +		dev->shmem_shrinker = NULL;
> +		kfree(gem_shrinker);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_unregister);
> +
>  MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
>  MODULE_IMPORT_NS(DMA_BUF);
>  MODULE_LICENSE("GPL v2");
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 9923c7a6885e..929546cad894 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -16,6 +16,7 @@ struct drm_vblank_crtc;
>  struct drm_vma_offset_manager;
>  struct drm_vram_mm;
>  struct drm_fb_helper;
> +struct drm_gem_shmem_shrinker;
>  
>  struct inode;
>  
> @@ -277,6 +278,9 @@ struct drm_device {
>  	/** @vram_mm: VRAM MM memory manager */
>  	struct drm_vram_mm *vram_mm;
>  
> +	/** @shmem_shrinker: SHMEM GEM memory shrinker */
> +	struct drm_gem_shmem_shrinker *shmem_shrinker;
> +
>  	/**
>  	 * @switch_power_state:
>  	 *
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index e2941cee14b6..cdb99cfbf0bc 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -172,6 +172,17 @@ struct drm_gem_object_funcs {
>  	 * This is optional but necessary for mmap support.
>  	 */
>  	const struct vm_operations_struct *vm_ops;
> +
> +	/**
> +	 * @purge:
> +	 *
> +	 * Releases the GEM object's allocated backing storage to the system.
> +	 *
> +	 * Returns the number of pages that have been freed by purging the GEM object.
> +	 *
> +	 * This callback is used by the GEM shrinker.
> +	 */
> +	unsigned long (*purge)(struct drm_gem_object *obj);
>  };
>  
>  /**
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index d0a57853c188..455254f131f6 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -6,6 +6,7 @@
>  #include <linux/fs.h>
>  #include <linux/mm.h>
>  #include <linux/mutex.h>
> +#include <linux/shrinker.h>
>  
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
> @@ -15,6 +16,7 @@
>  struct dma_buf_attachment;
>  struct drm_mode_create_dumb;
>  struct drm_printer;
> +struct drm_device;
>  struct sg_table;
>  
>  /**
> @@ -272,6 +274,29 @@ static inline int drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct v
>  	return drm_gem_shmem_mmap(shmem, vma);
>  }
>  
> +/**
> + * struct drm_gem_shmem_shrinker - Generic memory shrinker for shmem GEMs
> + */
> +struct drm_gem_shmem_shrinker {
> +	/** @base: Shrinker for purging shmem GEM objects */
> +	struct shrinker base;
> +
> +	/** @lock: Protects @lru */
> +	struct mutex lock;
> +
> +	/** @lru: List of shmem GEM objects available for purging */
> +	struct list_head lru;
> +
> +	/** @dev: DRM device that uses this shrinker */
> +	struct drm_device *dev;
> +
> +	/** @shrinkable_count: Count of shmem GEM pages to be purged */
> +	u64 shrinkable_count;
> +};
> +
> +int drm_gem_shmem_shrinker_register(struct drm_device *dev);
> +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev);
> +
>  /*
>   * Driver ops
>   */

Re: [PATCH v2 6/8] drm/shmem-helper: Add generic memory shrinker

[Dmitry Osipenko]

On 3/16/22 18:04, Steven Price wrote:
> On 14/03/2022 22:42, Dmitry Osipenko wrote:
>> Introduce a common DRM SHMEM shrinker. It allows to reduce code
>> duplication among DRM drivers, it also handles complicated lockings
>> for the drivers. This is initial version of the shrinker that covers
>> basic needs of GPU drivers.
>>
>> This patch is based on a couple ideas borrowed from Rob's Clark MSM
>> shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
>>
>> GPU drivers that want to use generic DRM memory shrinker must support
>> generic GEM reservations.
>>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> This looks fine to me, but one nitpick: you should update the comment in
> struct drm_gem_shmem_object:
> 
>> 	/**
>> 	 * @madv: State for madvise
>> 	 *
>> 	 * 0 is active/inuse.
>> 	 * A negative value is the object is purged.
>> 	 * Positive values are driver specific and not used by the helpers.
>> 	 */
>> 	int madv;
> This is adding a helper which cares about the positive values.

Good catch, I'll update the comment in v3.

Re: [PATCH v2 6/8] drm/shmem-helper: Add generic memory shrinker

[Rob Clark]

On Mon, Mar 14, 2022 at 3:44 PM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> Introduce a common DRM SHMEM shrinker. It allows to reduce code
> duplication among DRM drivers, it also handles complicated lockings
> for the drivers. This is initial version of the shrinker that covers
> basic needs of GPU drivers.
>
> This patch is based on a couple ideas borrowed from Rob's Clark MSM
> shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
>
> GPU drivers that want to use generic DRM memory shrinker must support
> generic GEM reservations.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 194 +++++++++++++++++++++++++
>  include/drm/drm_device.h               |   4 +
>  include/drm/drm_gem.h                  |  11 ++
>  include/drm/drm_gem_shmem_helper.h     |  25 ++++
>  4 files changed, 234 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 37009418cd28..35be2ee98f11 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -139,6 +139,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
>  {
>         struct drm_gem_object *obj = &shmem->base;
>
> +       /* take out shmem GEM object from the memory shrinker */
> +       drm_gem_shmem_madvise(shmem, 0);
> +
>         WARN_ON(shmem->vmap_use_count);
>
>         if (obj->import_attach) {
> @@ -163,6 +166,42 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
>
> +static void drm_gem_shmem_update_purgeable_status(struct drm_gem_shmem_object *shmem)
> +{
> +       struct drm_gem_object *obj = &shmem->base;
> +       struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
> +       size_t page_count = obj->size >> PAGE_SHIFT;
> +
> +       if (!gem_shrinker || obj->import_attach || !obj->funcs->purge)
> +               return;
> +
> +       mutex_lock(&shmem->vmap_lock);
> +       mutex_lock(&shmem->pages_lock);
> +       mutex_lock(&gem_shrinker->lock);
> +
> +       if (shmem->madv < 0) {
> +               list_del_init(&shmem->madv_list);
> +               goto unlock;
> +       } else if (shmem->madv > 0) {
> +               if (!list_empty(&shmem->madv_list))
> +                       goto unlock;
> +
> +               WARN_ON(gem_shrinker->shrinkable_count + page_count < page_count);
> +               gem_shrinker->shrinkable_count += page_count;
> +
> +               list_add_tail(&shmem->madv_list, &gem_shrinker->lru);
> +       } else if (!list_empty(&shmem->madv_list)) {
> +               list_del_init(&shmem->madv_list);
> +
> +               WARN_ON(gem_shrinker->shrinkable_count < page_count);
> +               gem_shrinker->shrinkable_count -= page_count;
> +       }
> +unlock:
> +       mutex_unlock(&gem_shrinker->lock);
> +       mutex_unlock(&shmem->pages_lock);
> +       mutex_unlock(&shmem->vmap_lock);
> +}
> +
>  static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
>  {
>         struct drm_gem_object *obj = &shmem->base;
> @@ -366,6 +405,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>         ret = drm_gem_shmem_vmap_locked(shmem, map);
>         mutex_unlock(&shmem->vmap_lock);
>
> +       drm_gem_shmem_update_purgeable_status(shmem);
> +
>         return ret;
>  }
>  EXPORT_SYMBOL(drm_gem_shmem_vmap);
> @@ -409,6 +450,8 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
>         mutex_lock(&shmem->vmap_lock);
>         drm_gem_shmem_vunmap_locked(shmem, map);
>         mutex_unlock(&shmem->vmap_lock);
> +
> +       drm_gem_shmem_update_purgeable_status(shmem);
>  }
>  EXPORT_SYMBOL(drm_gem_shmem_vunmap);
>
> @@ -451,6 +494,8 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
>
>         mutex_unlock(&shmem->pages_lock);
>
> +       drm_gem_shmem_update_purgeable_status(shmem);
> +
>         return (madv >= 0);
>  }
>  EXPORT_SYMBOL(drm_gem_shmem_madvise);
> @@ -763,6 +808,155 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>
> +static struct drm_gem_shmem_shrinker *
> +to_drm_shrinker(struct shrinker *shrinker)
> +{
> +       return container_of(shrinker, struct drm_gem_shmem_shrinker, base);
> +}
> +
> +static unsigned long
> +drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker,
> +                                    struct shrink_control *sc)
> +{
> +       struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
> +       u64 count = gem_shrinker->shrinkable_count;
> +
> +       if (count >= SHRINK_EMPTY)
> +               return SHRINK_EMPTY - 1;
> +
> +       return count ?: SHRINK_EMPTY;
> +}
> +
> +static unsigned long
> +drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker,
> +                                   struct shrink_control *sc)
> +{
> +       struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
> +       struct drm_gem_shmem_object *shmem;
> +       struct list_head still_in_list;
> +       bool lock_contention = true;
> +       struct drm_gem_object *obj;
> +       unsigned long freed = 0;
> +
> +       INIT_LIST_HEAD(&still_in_list);
> +
> +       mutex_lock(&gem_shrinker->lock);
> +
> +       while (freed < sc->nr_to_scan) {
> +               shmem = list_first_entry_or_null(&gem_shrinker->lru,
> +                                                typeof(*shmem), madv_list);
> +               if (!shmem)
> +                       break;
> +
> +               obj = &shmem->base;
> +               list_move_tail(&shmem->madv_list, &still_in_list);
> +
> +               /*
> +                * If it's in the process of being freed, gem_object->free()
> +                * may be blocked on lock waiting to remove it.  So just
> +                * skip it.
> +                */
> +               if (!kref_get_unless_zero(&obj->refcount))
> +                       continue;
> +
> +               mutex_unlock(&gem_shrinker->lock);
> +
> +               /* prevent racing with job submission code paths */
> +               if (!dma_resv_trylock(obj->resv))
> +                       goto shrinker_lock;

jfwiw, the trylock here is in the msm code isn't so much for madvise
(it is an error to submit jobs that reference DONTNEED objects), but
instead for the case of evicting WILLNEED but inactive objects to
swap.  Ie. in the case that we need to move bo's back in to memory, we
don't want to unpin/evict a buffer that is later on the list for the
same job.. msm shrinker re-uses the same scan loop for both
inactive_dontneed (purge) and inactive_willneed (evict)

I suppose using trylock is not technically wrong, and it would be a
good idea if the shmem helpers supported eviction as well.  But I
think in the madvise/purge case if you lose the trylock then there is
something else bad going on.

Anyways, from the PoV of minimizing lock contention when under memory
pressure, this all looks good to me.

BR,
-R

> +
> +               /* prevent racing with the dma-buf exporting */
> +               if (!mutex_trylock(&gem_shrinker->dev->object_name_lock))
> +                       goto resv_unlock;
> +
> +               if (!mutex_trylock(&shmem->vmap_lock))
> +                       goto object_name_unlock;
> +
> +               if (!mutex_trylock(&shmem->pages_lock))
> +                       goto vmap_unlock;
> +
> +               lock_contention = false;
> +
> +               /* check whether h/w uses this object */
> +               if (!dma_resv_test_signaled(obj->resv, true))
> +                       goto pages_unlock;
> +
> +               /* GEM may've become unpurgeable while shrinker was unlocked */
> +               if (!drm_gem_shmem_is_purgeable(shmem))
> +                       goto pages_unlock;
> +
> +               freed += obj->funcs->purge(obj);
> +pages_unlock:
> +               mutex_unlock(&shmem->pages_lock);
> +vmap_unlock:
> +               mutex_unlock(&shmem->vmap_lock);
> +object_name_unlock:
> +               mutex_unlock(&gem_shrinker->dev->object_name_lock);
> +resv_unlock:
> +               dma_resv_unlock(obj->resv);
> +shrinker_lock:
> +               drm_gem_object_put(&shmem->base);
> +               mutex_lock(&gem_shrinker->lock);
> +       }
> +
> +       list_splice_tail(&still_in_list, &gem_shrinker->lru);
> +       WARN_ON(gem_shrinker->shrinkable_count < freed);
> +       gem_shrinker->shrinkable_count -= freed;
> +
> +       mutex_unlock(&gem_shrinker->lock);
> +
> +       if (!freed && !lock_contention)
> +               return SHRINK_STOP;
> +
> +       return freed;
> +}
> +
> +int drm_gem_shmem_shrinker_register(struct drm_device *dev)
> +{
> +       struct drm_gem_shmem_shrinker *gem_shrinker;
> +       int err;
> +
> +       if (WARN_ON(dev->shmem_shrinker))
> +               return -EBUSY;
> +
> +       gem_shrinker = kzalloc(sizeof(*gem_shrinker), GFP_KERNEL);
> +       if (!gem_shrinker)
> +               return -ENOMEM;
> +
> +       gem_shrinker->base.count_objects = drm_gem_shmem_shrinker_count_objects;
> +       gem_shrinker->base.scan_objects = drm_gem_shmem_shrinker_scan_objects;
> +       gem_shrinker->base.seeks = DEFAULT_SEEKS;
> +       gem_shrinker->dev = dev;
> +
> +       INIT_LIST_HEAD(&gem_shrinker->lru);
> +       mutex_init(&gem_shrinker->lock);
> +
> +       dev->shmem_shrinker = gem_shrinker;
> +
> +       err = register_shrinker(&gem_shrinker->base);
> +       if (err) {
> +               dev->shmem_shrinker = NULL;
> +               kfree(gem_shrinker);
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_register);
> +
> +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev)
> +{
> +       struct drm_gem_shmem_shrinker *gem_shrinker = dev->shmem_shrinker;
> +
> +       if (gem_shrinker) {
> +               unregister_shrinker(&gem_shrinker->base);
> +               mutex_destroy(&gem_shrinker->lock);
> +               dev->shmem_shrinker = NULL;
> +               kfree(gem_shrinker);
> +       }
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_unregister);
> +
>  MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
>  MODULE_IMPORT_NS(DMA_BUF);
>  MODULE_LICENSE("GPL v2");
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 9923c7a6885e..929546cad894 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -16,6 +16,7 @@ struct drm_vblank_crtc;
>  struct drm_vma_offset_manager;
>  struct drm_vram_mm;
>  struct drm_fb_helper;
> +struct drm_gem_shmem_shrinker;
>
>  struct inode;
>
> @@ -277,6 +278,9 @@ struct drm_device {
>         /** @vram_mm: VRAM MM memory manager */
>         struct drm_vram_mm *vram_mm;
>
> +       /** @shmem_shrinker: SHMEM GEM memory shrinker */
> +       struct drm_gem_shmem_shrinker *shmem_shrinker;
> +
>         /**
>          * @switch_power_state:
>          *
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index e2941cee14b6..cdb99cfbf0bc 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -172,6 +172,17 @@ struct drm_gem_object_funcs {
>          * This is optional but necessary for mmap support.
>          */
>         const struct vm_operations_struct *vm_ops;
> +
> +       /**
> +        * @purge:
> +        *
> +        * Releases the GEM object's allocated backing storage to the system.
> +        *
> +        * Returns the number of pages that have been freed by purging the GEM object.
> +        *
> +        * This callback is used by the GEM shrinker.
> +        */
> +       unsigned long (*purge)(struct drm_gem_object *obj);
>  };
>
>  /**
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index d0a57853c188..455254f131f6 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -6,6 +6,7 @@
>  #include <linux/fs.h>
>  #include <linux/mm.h>
>  #include <linux/mutex.h>
> +#include <linux/shrinker.h>
>
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
> @@ -15,6 +16,7 @@
>  struct dma_buf_attachment;
>  struct drm_mode_create_dumb;
>  struct drm_printer;
> +struct drm_device;
>  struct sg_table;
>
>  /**
> @@ -272,6 +274,29 @@ static inline int drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct v
>         return drm_gem_shmem_mmap(shmem, vma);
>  }
>
> +/**
> + * struct drm_gem_shmem_shrinker - Generic memory shrinker for shmem GEMs
> + */
> +struct drm_gem_shmem_shrinker {
> +       /** @base: Shrinker for purging shmem GEM objects */
> +       struct shrinker base;
> +
> +       /** @lock: Protects @lru */
> +       struct mutex lock;
> +
> +       /** @lru: List of shmem GEM objects available for purging */
> +       struct list_head lru;
> +
> +       /** @dev: DRM device that uses this shrinker */
> +       struct drm_device *dev;
> +
> +       /** @shrinkable_count: Count of shmem GEM pages to be purged */
> +       u64 shrinkable_count;
> +};
> +
> +int drm_gem_shmem_shrinker_register(struct drm_device *dev);
> +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev);
> +
>  /*
>   * Driver ops
>   */
> --
> 2.35.1
>

Re: [PATCH v2 6/8] drm/shmem-helper: Add generic memory shrinker

[Dmitry Osipenko]

On 3/16/22 23:00, Rob Clark wrote:
> On Mon, Mar 14, 2022 at 3:44 PM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
>>
>> Introduce a common DRM SHMEM shrinker. It allows to reduce code
>> duplication among DRM drivers, it also handles complicated lockings
>> for the drivers. This is initial version of the shrinker that covers
>> basic needs of GPU drivers.
>>
>> This patch is based on a couple ideas borrowed from Rob's Clark MSM
>> shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
>>
>> GPU drivers that want to use generic DRM memory shrinker must support
>> generic GEM reservations.
>>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> ---
>>  drivers/gpu/drm/drm_gem_shmem_helper.c | 194 +++++++++++++++++++++++++
>>  include/drm/drm_device.h               |   4 +
>>  include/drm/drm_gem.h                  |  11 ++
>>  include/drm/drm_gem_shmem_helper.h     |  25 ++++
>>  4 files changed, 234 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index 37009418cd28..35be2ee98f11 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -139,6 +139,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
>>  {
>>         struct drm_gem_object *obj = &shmem->base;
>>
>> +       /* take out shmem GEM object from the memory shrinker */
>> +       drm_gem_shmem_madvise(shmem, 0);
>> +
>>         WARN_ON(shmem->vmap_use_count);
>>
>>         if (obj->import_attach) {
>> @@ -163,6 +166,42 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
>>  }
>>  EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
>>
>> +static void drm_gem_shmem_update_purgeable_status(struct drm_gem_shmem_object *shmem)
>> +{
>> +       struct drm_gem_object *obj = &shmem->base;
>> +       struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
>> +       size_t page_count = obj->size >> PAGE_SHIFT;
>> +
>> +       if (!gem_shrinker || obj->import_attach || !obj->funcs->purge)
>> +               return;
>> +
>> +       mutex_lock(&shmem->vmap_lock);
>> +       mutex_lock(&shmem->pages_lock);
>> +       mutex_lock(&gem_shrinker->lock);
>> +
>> +       if (shmem->madv < 0) {
>> +               list_del_init(&shmem->madv_list);
>> +               goto unlock;
>> +       } else if (shmem->madv > 0) {
>> +               if (!list_empty(&shmem->madv_list))
>> +                       goto unlock;
>> +
>> +               WARN_ON(gem_shrinker->shrinkable_count + page_count < page_count);
>> +               gem_shrinker->shrinkable_count += page_count;
>> +
>> +               list_add_tail(&shmem->madv_list, &gem_shrinker->lru);
>> +       } else if (!list_empty(&shmem->madv_list)) {
>> +               list_del_init(&shmem->madv_list);
>> +
>> +               WARN_ON(gem_shrinker->shrinkable_count < page_count);
>> +               gem_shrinker->shrinkable_count -= page_count;
>> +       }
>> +unlock:
>> +       mutex_unlock(&gem_shrinker->lock);
>> +       mutex_unlock(&shmem->pages_lock);
>> +       mutex_unlock(&shmem->vmap_lock);
>> +}
>> +
>>  static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
>>  {
>>         struct drm_gem_object *obj = &shmem->base;
>> @@ -366,6 +405,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>>         ret = drm_gem_shmem_vmap_locked(shmem, map);
>>         mutex_unlock(&shmem->vmap_lock);
>>
>> +       drm_gem_shmem_update_purgeable_status(shmem);
>> +
>>         return ret;
>>  }
>>  EXPORT_SYMBOL(drm_gem_shmem_vmap);
>> @@ -409,6 +450,8 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
>>         mutex_lock(&shmem->vmap_lock);
>>         drm_gem_shmem_vunmap_locked(shmem, map);
>>         mutex_unlock(&shmem->vmap_lock);
>> +
>> +       drm_gem_shmem_update_purgeable_status(shmem);
>>  }
>>  EXPORT_SYMBOL(drm_gem_shmem_vunmap);
>>
>> @@ -451,6 +494,8 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
>>
>>         mutex_unlock(&shmem->pages_lock);
>>
>> +       drm_gem_shmem_update_purgeable_status(shmem);
>> +
>>         return (madv >= 0);
>>  }
>>  EXPORT_SYMBOL(drm_gem_shmem_madvise);
>> @@ -763,6 +808,155 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>  }
>>  EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>>
>> +static struct drm_gem_shmem_shrinker *
>> +to_drm_shrinker(struct shrinker *shrinker)
>> +{
>> +       return container_of(shrinker, struct drm_gem_shmem_shrinker, base);
>> +}
>> +
>> +static unsigned long
>> +drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker,
>> +                                    struct shrink_control *sc)
>> +{
>> +       struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
>> +       u64 count = gem_shrinker->shrinkable_count;
>> +
>> +       if (count >= SHRINK_EMPTY)
>> +               return SHRINK_EMPTY - 1;
>> +
>> +       return count ?: SHRINK_EMPTY;
>> +}
>> +
>> +static unsigned long
>> +drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker,
>> +                                   struct shrink_control *sc)
>> +{
>> +       struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
>> +       struct drm_gem_shmem_object *shmem;
>> +       struct list_head still_in_list;
>> +       bool lock_contention = true;
>> +       struct drm_gem_object *obj;
>> +       unsigned long freed = 0;
>> +
>> +       INIT_LIST_HEAD(&still_in_list);
>> +
>> +       mutex_lock(&gem_shrinker->lock);
>> +
>> +       while (freed < sc->nr_to_scan) {
>> +               shmem = list_first_entry_or_null(&gem_shrinker->lru,
>> +                                                typeof(*shmem), madv_list);
>> +               if (!shmem)
>> +                       break;
>> +
>> +               obj = &shmem->base;
>> +               list_move_tail(&shmem->madv_list, &still_in_list);
>> +
>> +               /*
>> +                * If it's in the process of being freed, gem_object->free()
>> +                * may be blocked on lock waiting to remove it.  So just
>> +                * skip it.
>> +                */
>> +               if (!kref_get_unless_zero(&obj->refcount))
>> +                       continue;
>> +
>> +               mutex_unlock(&gem_shrinker->lock);
>> +
>> +               /* prevent racing with job submission code paths */
>> +               if (!dma_resv_trylock(obj->resv))
>> +                       goto shrinker_lock;
> 
> jfwiw, the trylock here is in the msm code isn't so much for madvise
> (it is an error to submit jobs that reference DONTNEED objects), but
> instead for the case of evicting WILLNEED but inactive objects to
> swap.  Ie. in the case that we need to move bo's back in to memory, we
> don't want to unpin/evict a buffer that is later on the list for the
> same job.. msm shrinker re-uses the same scan loop for both
> inactive_dontneed (purge) and inactive_willneed (evict)

I don't see connection between the objects on the shrinker's list and
the job's BOs. Jobs indeed must not have any objects marked as DONTNEED,
this case should never happen in practice, but we still need to protect
from it.

> I suppose using trylock is not technically wrong, and it would be a
> good idea if the shmem helpers supported eviction as well.  But I
> think in the madvise/purge case if you lose the trylock then there is
> something else bad going on.

This trylock is intended for protecting job's submission path from
racing with madvise ioctl invocation followed by immediate purging of
BOs while job is in a process of submission, i.e. it protects from a
use-after-free.

If you'll lose this trylock, then shrinker can't use
dma_resv_test_signaled() reliably anymore and shrinker may purge BO
before job had a chance to add fence to the BO's reservation.

> Anyways, from the PoV of minimizing lock contention when under memory
> pressure, this all looks good to me.

Thank you. I may try to add generic eviction support to the v3.

Re: [PATCH v2 6/8] drm/shmem-helper: Add generic memory shrinker

[Rob Clark]

On Wed, Mar 16, 2022 at 5:13 PM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 3/16/22 23:00, Rob Clark wrote:
> > On Mon, Mar 14, 2022 at 3:44 PM Dmitry Osipenko
> > <dmitry.osipenko@collabora.com> wrote:
> >>
> >> Introduce a common DRM SHMEM shrinker. It allows to reduce code
> >> duplication among DRM drivers, it also handles complicated lockings
> >> for the drivers. This is initial version of the shrinker that covers
> >> basic needs of GPU drivers.
> >>
> >> This patch is based on a couple ideas borrowed from Rob's Clark MSM
> >> shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
> >>
> >> GPU drivers that want to use generic DRM memory shrinker must support
> >> generic GEM reservations.
> >>
> >> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >> ---
> >>  drivers/gpu/drm/drm_gem_shmem_helper.c | 194 +++++++++++++++++++++++++
> >>  include/drm/drm_device.h               |   4 +
> >>  include/drm/drm_gem.h                  |  11 ++
> >>  include/drm/drm_gem_shmem_helper.h     |  25 ++++
> >>  4 files changed, 234 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> index 37009418cd28..35be2ee98f11 100644
> >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> @@ -139,6 +139,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
> >>  {
> >>         struct drm_gem_object *obj = &shmem->base;
> >>
> >> +       /* take out shmem GEM object from the memory shrinker */
> >> +       drm_gem_shmem_madvise(shmem, 0);
> >> +
> >>         WARN_ON(shmem->vmap_use_count);
> >>
> >>         if (obj->import_attach) {
> >> @@ -163,6 +166,42 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
> >>  }
> >>  EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
> >>
> >> +static void drm_gem_shmem_update_purgeable_status(struct drm_gem_shmem_object *shmem)
> >> +{
> >> +       struct drm_gem_object *obj = &shmem->base;
> >> +       struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
> >> +       size_t page_count = obj->size >> PAGE_SHIFT;
> >> +
> >> +       if (!gem_shrinker || obj->import_attach || !obj->funcs->purge)
> >> +               return;
> >> +
> >> +       mutex_lock(&shmem->vmap_lock);
> >> +       mutex_lock(&shmem->pages_lock);
> >> +       mutex_lock(&gem_shrinker->lock);
> >> +
> >> +       if (shmem->madv < 0) {
> >> +               list_del_init(&shmem->madv_list);
> >> +               goto unlock;
> >> +       } else if (shmem->madv > 0) {
> >> +               if (!list_empty(&shmem->madv_list))
> >> +                       goto unlock;
> >> +
> >> +               WARN_ON(gem_shrinker->shrinkable_count + page_count < page_count);
> >> +               gem_shrinker->shrinkable_count += page_count;
> >> +
> >> +               list_add_tail(&shmem->madv_list, &gem_shrinker->lru);
> >> +       } else if (!list_empty(&shmem->madv_list)) {
> >> +               list_del_init(&shmem->madv_list);
> >> +
> >> +               WARN_ON(gem_shrinker->shrinkable_count < page_count);
> >> +               gem_shrinker->shrinkable_count -= page_count;
> >> +       }
> >> +unlock:
> >> +       mutex_unlock(&gem_shrinker->lock);
> >> +       mutex_unlock(&shmem->pages_lock);
> >> +       mutex_unlock(&shmem->vmap_lock);
> >> +}
> >> +
> >>  static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
> >>  {
> >>         struct drm_gem_object *obj = &shmem->base;
> >> @@ -366,6 +405,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
> >>         ret = drm_gem_shmem_vmap_locked(shmem, map);
> >>         mutex_unlock(&shmem->vmap_lock);
> >>
> >> +       drm_gem_shmem_update_purgeable_status(shmem);
> >> +
> >>         return ret;
> >>  }
> >>  EXPORT_SYMBOL(drm_gem_shmem_vmap);
> >> @@ -409,6 +450,8 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
> >>         mutex_lock(&shmem->vmap_lock);
> >>         drm_gem_shmem_vunmap_locked(shmem, map);
> >>         mutex_unlock(&shmem->vmap_lock);
> >> +
> >> +       drm_gem_shmem_update_purgeable_status(shmem);
> >>  }
> >>  EXPORT_SYMBOL(drm_gem_shmem_vunmap);
> >>
> >> @@ -451,6 +494,8 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
> >>
> >>         mutex_unlock(&shmem->pages_lock);
> >>
> >> +       drm_gem_shmem_update_purgeable_status(shmem);
> >> +
> >>         return (madv >= 0);
> >>  }
> >>  EXPORT_SYMBOL(drm_gem_shmem_madvise);
> >> @@ -763,6 +808,155 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
> >>  }
> >>  EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
> >>
> >> +static struct drm_gem_shmem_shrinker *
> >> +to_drm_shrinker(struct shrinker *shrinker)
> >> +{
> >> +       return container_of(shrinker, struct drm_gem_shmem_shrinker, base);
> >> +}
> >> +
> >> +static unsigned long
> >> +drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker,
> >> +                                    struct shrink_control *sc)
> >> +{
> >> +       struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
> >> +       u64 count = gem_shrinker->shrinkable_count;
> >> +
> >> +       if (count >= SHRINK_EMPTY)
> >> +               return SHRINK_EMPTY - 1;
> >> +
> >> +       return count ?: SHRINK_EMPTY;
> >> +}
> >> +
> >> +static unsigned long
> >> +drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker,
> >> +                                   struct shrink_control *sc)
> >> +{
> >> +       struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
> >> +       struct drm_gem_shmem_object *shmem;
> >> +       struct list_head still_in_list;
> >> +       bool lock_contention = true;
> >> +       struct drm_gem_object *obj;
> >> +       unsigned long freed = 0;
> >> +
> >> +       INIT_LIST_HEAD(&still_in_list);
> >> +
> >> +       mutex_lock(&gem_shrinker->lock);
> >> +
> >> +       while (freed < sc->nr_to_scan) {
> >> +               shmem = list_first_entry_or_null(&gem_shrinker->lru,
> >> +                                                typeof(*shmem), madv_list);
> >> +               if (!shmem)
> >> +                       break;
> >> +
> >> +               obj = &shmem->base;
> >> +               list_move_tail(&shmem->madv_list, &still_in_list);
> >> +
> >> +               /*
> >> +                * If it's in the process of being freed, gem_object->free()
> >> +                * may be blocked on lock waiting to remove it.  So just
> >> +                * skip it.
> >> +                */
> >> +               if (!kref_get_unless_zero(&obj->refcount))
> >> +                       continue;
> >> +
> >> +               mutex_unlock(&gem_shrinker->lock);
> >> +
> >> +               /* prevent racing with job submission code paths */
> >> +               if (!dma_resv_trylock(obj->resv))
> >> +                       goto shrinker_lock;
> >
> > jfwiw, the trylock here is in the msm code isn't so much for madvise
> > (it is an error to submit jobs that reference DONTNEED objects), but
> > instead for the case of evicting WILLNEED but inactive objects to
> > swap.  Ie. in the case that we need to move bo's back in to memory, we
> > don't want to unpin/evict a buffer that is later on the list for the
> > same job.. msm shrinker re-uses the same scan loop for both
> > inactive_dontneed (purge) and inactive_willneed (evict)
>
> I don't see connection between the objects on the shrinker's list and
> the job's BOs. Jobs indeed must not have any objects marked as DONTNEED,
> this case should never happen in practice, but we still need to protect
> from it.

Hmm, let me try to explain with a simple example.. hopefully this makes sense.

Say you have a job with two bo's, A and B..  bo A is not backed with
memory (either hasn't been used before or was evicted.  Allocating
pages for A triggers shrinker.  But B is still on the
inactive_willneed list, however it is already locked (because we don't
want to evict B to obtain backing pages for A).

>
> > I suppose using trylock is not technically wrong, and it would be a
> > good idea if the shmem helpers supported eviction as well.  But I
> > think in the madvise/purge case if you lose the trylock then there is
> > something else bad going on.
>
> This trylock is intended for protecting job's submission path from
> racing with madvise ioctl invocation followed by immediate purging of
> BOs while job is in a process of submission, i.e. it protects from a
> use-after-free.

ahh, ok

> If you'll lose this trylock, then shrinker can't use
> dma_resv_test_signaled() reliably anymore and shrinker may purge BO
> before job had a chance to add fence to the BO's reservation.
>
> > Anyways, from the PoV of minimizing lock contention when under memory
> > pressure, this all looks good to me.
>
> Thank you. I may try to add generic eviction support to the v3.

eviction is a trickier thing to get right, I wouldn't blame you for
splitting that out into it's own patchset ;-)

You probably also would want to make it a thing that is opt-in for
drivers using the shmem helpers

BR,
-R

Re: [PATCH v2 6/8] drm/shmem-helper: Add generic memory shrinker

[Dmitry Osipenko]

On 3/17/22 19:13, Rob Clark wrote:
...
>>>> +               /* prevent racing with job submission code paths */
>>>> +               if (!dma_resv_trylock(obj->resv))
>>>> +                       goto shrinker_lock;
>>>
>>> jfwiw, the trylock here is in the msm code isn't so much for madvise
>>> (it is an error to submit jobs that reference DONTNEED objects), but
>>> instead for the case of evicting WILLNEED but inactive objects to
>>> swap.  Ie. in the case that we need to move bo's back in to memory, we
>>> don't want to unpin/evict a buffer that is later on the list for the
>>> same job.. msm shrinker re-uses the same scan loop for both
>>> inactive_dontneed (purge) and inactive_willneed (evict)
>>
>> I don't see connection between the objects on the shrinker's list and
>> the job's BOs. Jobs indeed must not have any objects marked as DONTNEED,
>> this case should never happen in practice, but we still need to protect
>> from it.
> 
> Hmm, let me try to explain with a simple example.. hopefully this makes sense.
> 
> Say you have a job with two bo's, A and B..  bo A is not backed with
> memory (either hasn't been used before or was evicted.  Allocating
> pages for A triggers shrinker.  But B is still on the
> inactive_willneed list, however it is already locked (because we don't
> want to evict B to obtain backing pages for A).

I see now what you're talking about, thanks. My intention of locking the
reservations is different since eviction isn't supported by this v2. But
we probably will be able to re-use this try_lock for protecting from
swapping out job's BOs as well.

>>> I suppose using trylock is not technically wrong, and it would be a
>>> good idea if the shmem helpers supported eviction as well.  But I
>>> think in the madvise/purge case if you lose the trylock then there is
>>> something else bad going on.
>>
>> This trylock is intended for protecting job's submission path from
>> racing with madvise ioctl invocation followed by immediate purging of
>> BOs while job is in a process of submission, i.e. it protects from a
>> use-after-free.
> 
> ahh, ok
> 
>> If you'll lose this trylock, then shrinker can't use
>> dma_resv_test_signaled() reliably anymore and shrinker may purge BO
>> before job had a chance to add fence to the BO's reservation.
>>
>>> Anyways, from the PoV of minimizing lock contention when under memory
>>> pressure, this all looks good to me.
>>
>> Thank you. I may try to add generic eviction support to the v3.
> 
> eviction is a trickier thing to get right, I wouldn't blame you for
> splitting that out into it's own patchset ;-)
> 
> You probably also would want to make it a thing that is opt-in for
> drivers using the shmem helpers

I had the same thoughts, will see.

Re: [PATCH v2 6/8] drm/shmem-helper: Add generic memory shrinker

[Daniel Vetter]

On Tue, Mar 15, 2022 at 01:42:51AM +0300, Dmitry Osipenko wrote:
> Introduce a common DRM SHMEM shrinker. It allows to reduce code
> duplication among DRM drivers, it also handles complicated lockings
> for the drivers. This is initial version of the shrinker that covers
> basic needs of GPU drivers.
> 
> This patch is based on a couple ideas borrowed from Rob's Clark MSM
> shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
> 
> GPU drivers that want to use generic DRM memory shrinker must support
> generic GEM reservations.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 194 +++++++++++++++++++++++++
>  include/drm/drm_device.h               |   4 +
>  include/drm/drm_gem.h                  |  11 ++
>  include/drm/drm_gem_shmem_helper.h     |  25 ++++
>  4 files changed, 234 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 37009418cd28..35be2ee98f11 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -139,6 +139,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
>  {
>  	struct drm_gem_object *obj = &shmem->base;
>  
> +	/* take out shmem GEM object from the memory shrinker */
> +	drm_gem_shmem_madvise(shmem, 0);
> +
>  	WARN_ON(shmem->vmap_use_count);
>  
>  	if (obj->import_attach) {
> @@ -163,6 +166,42 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
>  
> +static void drm_gem_shmem_update_purgeable_status(struct drm_gem_shmem_object *shmem)
> +{
> +	struct drm_gem_object *obj = &shmem->base;
> +	struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
> +	size_t page_count = obj->size >> PAGE_SHIFT;
> +
> +	if (!gem_shrinker || obj->import_attach || !obj->funcs->purge)
> +		return;
> +
> +	mutex_lock(&shmem->vmap_lock);
> +	mutex_lock(&shmem->pages_lock);
> +	mutex_lock(&gem_shrinker->lock);

Uh this is just terrible I think.

Can't we move shmem helpers over to reasonable locking, i.e. per-object
dma_resv_lock for everything? I know it's a pile of work, but I think
we're way past the point with things like this popping up where we should
just bite that bullet.

I discussed the full thing with Daniel Stone, but maybe a joint refresher
on irc would be a good thing.
-Daniel

> +
> +	if (shmem->madv < 0) {
> +		list_del_init(&shmem->madv_list);
> +		goto unlock;
> +	} else if (shmem->madv > 0) {
> +		if (!list_empty(&shmem->madv_list))
> +			goto unlock;
> +
> +		WARN_ON(gem_shrinker->shrinkable_count + page_count < page_count);
> +		gem_shrinker->shrinkable_count += page_count;
> +
> +		list_add_tail(&shmem->madv_list, &gem_shrinker->lru);
> +	} else if (!list_empty(&shmem->madv_list)) {
> +		list_del_init(&shmem->madv_list);
> +
> +		WARN_ON(gem_shrinker->shrinkable_count < page_count);
> +		gem_shrinker->shrinkable_count -= page_count;
> +	}
> +unlock:
> +	mutex_unlock(&gem_shrinker->lock);
> +	mutex_unlock(&shmem->pages_lock);
> +	mutex_unlock(&shmem->vmap_lock);
> +}
> +
>  static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
>  {
>  	struct drm_gem_object *obj = &shmem->base;
> @@ -366,6 +405,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>  	ret = drm_gem_shmem_vmap_locked(shmem, map);
>  	mutex_unlock(&shmem->vmap_lock);
>  
> +	drm_gem_shmem_update_purgeable_status(shmem);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_gem_shmem_vmap);
> @@ -409,6 +450,8 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
>  	mutex_lock(&shmem->vmap_lock);
>  	drm_gem_shmem_vunmap_locked(shmem, map);
>  	mutex_unlock(&shmem->vmap_lock);
> +
> +	drm_gem_shmem_update_purgeable_status(shmem);
>  }
>  EXPORT_SYMBOL(drm_gem_shmem_vunmap);
>  
> @@ -451,6 +494,8 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
>  
>  	mutex_unlock(&shmem->pages_lock);
>  
> +	drm_gem_shmem_update_purgeable_status(shmem);
> +
>  	return (madv >= 0);
>  }
>  EXPORT_SYMBOL(drm_gem_shmem_madvise);
> @@ -763,6 +808,155 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>  
> +static struct drm_gem_shmem_shrinker *
> +to_drm_shrinker(struct shrinker *shrinker)
> +{
> +	return container_of(shrinker, struct drm_gem_shmem_shrinker, base);
> +}
> +
> +static unsigned long
> +drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker,
> +				     struct shrink_control *sc)
> +{
> +	struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
> +	u64 count = gem_shrinker->shrinkable_count;
> +
> +	if (count >= SHRINK_EMPTY)
> +		return SHRINK_EMPTY - 1;
> +
> +	return count ?: SHRINK_EMPTY;
> +}
> +
> +static unsigned long
> +drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker,
> +				    struct shrink_control *sc)
> +{
> +	struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
> +	struct drm_gem_shmem_object *shmem;
> +	struct list_head still_in_list;
> +	bool lock_contention = true;
> +	struct drm_gem_object *obj;
> +	unsigned long freed = 0;
> +
> +	INIT_LIST_HEAD(&still_in_list);
> +
> +	mutex_lock(&gem_shrinker->lock);
> +
> +	while (freed < sc->nr_to_scan) {
> +		shmem = list_first_entry_or_null(&gem_shrinker->lru,
> +						 typeof(*shmem), madv_list);
> +		if (!shmem)
> +			break;
> +
> +		obj = &shmem->base;
> +		list_move_tail(&shmem->madv_list, &still_in_list);
> +
> +		/*
> +		 * If it's in the process of being freed, gem_object->free()
> +		 * may be blocked on lock waiting to remove it.  So just
> +		 * skip it.
> +		 */
> +		if (!kref_get_unless_zero(&obj->refcount))
> +			continue;
> +
> +		mutex_unlock(&gem_shrinker->lock);
> +
> +		/* prevent racing with job submission code paths */
> +		if (!dma_resv_trylock(obj->resv))
> +			goto shrinker_lock;
> +
> +		/* prevent racing with the dma-buf exporting */
> +		if (!mutex_trylock(&gem_shrinker->dev->object_name_lock))
> +			goto resv_unlock;
> +
> +		if (!mutex_trylock(&shmem->vmap_lock))
> +			goto object_name_unlock;
> +
> +		if (!mutex_trylock(&shmem->pages_lock))
> +			goto vmap_unlock;
> +
> +		lock_contention = false;
> +
> +		/* check whether h/w uses this object */
> +		if (!dma_resv_test_signaled(obj->resv, true))
> +			goto pages_unlock;
> +
> +		/* GEM may've become unpurgeable while shrinker was unlocked */
> +		if (!drm_gem_shmem_is_purgeable(shmem))
> +			goto pages_unlock;
> +
> +		freed += obj->funcs->purge(obj);
> +pages_unlock:
> +		mutex_unlock(&shmem->pages_lock);
> +vmap_unlock:
> +		mutex_unlock(&shmem->vmap_lock);
> +object_name_unlock:
> +		mutex_unlock(&gem_shrinker->dev->object_name_lock);
> +resv_unlock:
> +		dma_resv_unlock(obj->resv);
> +shrinker_lock:
> +		drm_gem_object_put(&shmem->base);
> +		mutex_lock(&gem_shrinker->lock);
> +	}
> +
> +	list_splice_tail(&still_in_list, &gem_shrinker->lru);
> +	WARN_ON(gem_shrinker->shrinkable_count < freed);
> +	gem_shrinker->shrinkable_count -= freed;
> +
> +	mutex_unlock(&gem_shrinker->lock);
> +
> +	if (!freed && !lock_contention)
> +		return SHRINK_STOP;
> +
> +	return freed;
> +}
> +
> +int drm_gem_shmem_shrinker_register(struct drm_device *dev)
> +{
> +	struct drm_gem_shmem_shrinker *gem_shrinker;
> +	int err;
> +
> +	if (WARN_ON(dev->shmem_shrinker))
> +		return -EBUSY;
> +
> +	gem_shrinker = kzalloc(sizeof(*gem_shrinker), GFP_KERNEL);
> +	if (!gem_shrinker)
> +		return -ENOMEM;
> +
> +	gem_shrinker->base.count_objects = drm_gem_shmem_shrinker_count_objects;
> +	gem_shrinker->base.scan_objects = drm_gem_shmem_shrinker_scan_objects;
> +	gem_shrinker->base.seeks = DEFAULT_SEEKS;
> +	gem_shrinker->dev = dev;
> +
> +	INIT_LIST_HEAD(&gem_shrinker->lru);
> +	mutex_init(&gem_shrinker->lock);
> +
> +	dev->shmem_shrinker = gem_shrinker;
> +
> +	err = register_shrinker(&gem_shrinker->base);
> +	if (err) {
> +		dev->shmem_shrinker = NULL;
> +		kfree(gem_shrinker);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_register);
> +
> +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev)
> +{
> +	struct drm_gem_shmem_shrinker *gem_shrinker = dev->shmem_shrinker;
> +
> +	if (gem_shrinker) {
> +		unregister_shrinker(&gem_shrinker->base);
> +		mutex_destroy(&gem_shrinker->lock);
> +		dev->shmem_shrinker = NULL;
> +		kfree(gem_shrinker);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_unregister);
> +
>  MODULE_DESCRIPTION("DRM SHMEM memory-management helpers");
>  MODULE_IMPORT_NS(DMA_BUF);
>  MODULE_LICENSE("GPL v2");
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 9923c7a6885e..929546cad894 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -16,6 +16,7 @@ struct drm_vblank_crtc;
>  struct drm_vma_offset_manager;
>  struct drm_vram_mm;
>  struct drm_fb_helper;
> +struct drm_gem_shmem_shrinker;
>  
>  struct inode;
>  
> @@ -277,6 +278,9 @@ struct drm_device {
>  	/** @vram_mm: VRAM MM memory manager */
>  	struct drm_vram_mm *vram_mm;
>  
> +	/** @shmem_shrinker: SHMEM GEM memory shrinker */
> +	struct drm_gem_shmem_shrinker *shmem_shrinker;
> +
>  	/**
>  	 * @switch_power_state:
>  	 *
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index e2941cee14b6..cdb99cfbf0bc 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -172,6 +172,17 @@ struct drm_gem_object_funcs {
>  	 * This is optional but necessary for mmap support.
>  	 */
>  	const struct vm_operations_struct *vm_ops;
> +
> +	/**
> +	 * @purge:
> +	 *
> +	 * Releases the GEM object's allocated backing storage to the system.
> +	 *
> +	 * Returns the number of pages that have been freed by purging the GEM object.
> +	 *
> +	 * This callback is used by the GEM shrinker.
> +	 */
> +	unsigned long (*purge)(struct drm_gem_object *obj);
>  };
>  
>  /**
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index d0a57853c188..455254f131f6 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -6,6 +6,7 @@
>  #include <linux/fs.h>
>  #include <linux/mm.h>
>  #include <linux/mutex.h>
> +#include <linux/shrinker.h>
>  
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
> @@ -15,6 +16,7 @@
>  struct dma_buf_attachment;
>  struct drm_mode_create_dumb;
>  struct drm_printer;
> +struct drm_device;
>  struct sg_table;
>  
>  /**
> @@ -272,6 +274,29 @@ static inline int drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct v
>  	return drm_gem_shmem_mmap(shmem, vma);
>  }
>  
> +/**
> + * struct drm_gem_shmem_shrinker - Generic memory shrinker for shmem GEMs
> + */
> +struct drm_gem_shmem_shrinker {
> +	/** @base: Shrinker for purging shmem GEM objects */
> +	struct shrinker base;
> +
> +	/** @lock: Protects @lru */
> +	struct mutex lock;
> +
> +	/** @lru: List of shmem GEM objects available for purging */
> +	struct list_head lru;
> +
> +	/** @dev: DRM device that uses this shrinker */
> +	struct drm_device *dev;
> +
> +	/** @shrinkable_count: Count of shmem GEM pages to be purged */
> +	u64 shrinkable_count;
> +};
> +
> +int drm_gem_shmem_shrinker_register(struct drm_device *dev);
> +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev);
> +
>  /*
>   * Driver ops
>   */
> -- 
> 2.35.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 6/8] drm/shmem-helper: Add generic memory shrinker

[Dmitry Osipenko]

On 3/17/22 20:32, Daniel Vetter wrote:
>> +static void drm_gem_shmem_update_purgeable_status(struct drm_gem_shmem_object *shmem)
>> +{
>> +	struct drm_gem_object *obj = &shmem->base;
>> +	struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
>> +	size_t page_count = obj->size >> PAGE_SHIFT;
>> +
>> +	if (!gem_shrinker || obj->import_attach || !obj->funcs->purge)
>> +		return;
>> +
>> +	mutex_lock(&shmem->vmap_lock);
>> +	mutex_lock(&shmem->pages_lock);
>> +	mutex_lock(&gem_shrinker->lock);
> Uh this is just terrible I think.
> 
> Can't we move shmem helpers over to reasonable locking, i.e. per-object
> dma_resv_lock for everything? I know it's a pile of work, but I think
> we're way past the point with things like this popping up where we should
> just bite that bullet.
> 
> I discussed the full thing with Daniel Stone, but maybe a joint refresher
> on irc would be a good thing.

Aha! Perhaps I saw bits of that discussion, but it wasn't entirely clear
to me what was discussed in fact. Sounds like a good idea to try to use
the reservation lock everywhere, thank you for the suggestion.

[PATCH v2 7/8] drm/virtio: Support memory shrinking

[Dmitry Osipenko]

Add memory shrinker support and new madvise IOCTL to the VirtIO-GPU
driver. Userspace (BO cache manager of Mesa driver) will mark BOs as
"don't need" using the new IOCTL to let shrinker purge the marked BOs
on OOM, thus shrinker will lower memory pressure and prevent OOM kills.

For the starter only support of handling guest-side memory pressure is
implemented.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h    | 21 +++++-
 drivers/gpu/drm/virtio/virtgpu_gem.c    | 96 +++++++++++++++++++++++++
 drivers/gpu/drm/virtio/virtgpu_ioctl.c  | 37 ++++++++++
 drivers/gpu/drm/virtio/virtgpu_kms.c    | 10 +++
 drivers/gpu/drm/virtio/virtgpu_object.c | 22 ++++++
 drivers/gpu/drm/virtio/virtgpu_plane.c  | 17 ++++-
 drivers/gpu/drm/virtio/virtgpu_vq.c     | 15 ++++
 include/uapi/drm/virtgpu_drm.h          | 14 ++++
 8 files changed, 229 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index b2d93cb12ebf..86e5c7b83ec5 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -94,6 +94,9 @@ struct virtio_gpu_object {
 
 	int uuid_state;
 	uuid_t uuid;
+
+	/* object's backing memory will stay pinned while count > 0 */
+	unsigned int mem_pin_count;
 };
 #define gem_to_virtio_gpu_obj(gobj) \
 	container_of((gobj), struct virtio_gpu_object, base.base)
@@ -261,6 +264,9 @@ struct virtio_gpu_device {
 	spinlock_t resource_export_lock;
 	/* protects map state and host_visible_mm */
 	spinlock_t host_visible_lock;
+
+	/* protects all memory management operations */
+	struct mutex mm_lock;
 };
 
 struct virtio_gpu_fpriv {
@@ -274,7 +280,7 @@ struct virtio_gpu_fpriv {
 };
 
 /* virtgpu_ioctl.c */
-#define DRM_VIRTIO_NUM_IOCTLS 12
+#define DRM_VIRTIO_NUM_IOCTLS 13
 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);
 
@@ -310,6 +316,13 @@ void virtio_gpu_array_put_free(struct virtio_gpu_object_array *objs);
 void virtio_gpu_array_put_free_delayed(struct virtio_gpu_device *vgdev,
 				       struct virtio_gpu_object_array *objs);
 void virtio_gpu_array_put_free_work(struct work_struct *work);
+int virtio_gpu_array_validate(struct virtio_gpu_device *vgdev,
+			      struct virtio_gpu_object_array *objs);
+int virtio_gpu_gem_host_mem_release(struct virtio_gpu_object *bo);
+bool virtio_gpu_gem_madvise(struct virtio_gpu_object *obj, int madv);
+int virtio_gpu_gem_pin(struct virtio_gpu_object *bo);
+void virtio_gpu_gem_unpin(struct virtio_gpu_object *bo);
+bool virtio_gpu_gem_is_pinned(struct virtio_gpu_object *bo);
 
 /* virtgpu_vq.c */
 int virtio_gpu_alloc_vbufs(struct virtio_gpu_device *vgdev);
@@ -321,6 +334,8 @@ void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
 				    struct virtio_gpu_fence *fence);
 void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev,
 				   struct virtio_gpu_object *bo);
+int virtio_gpu_cmd_release_resource(struct virtio_gpu_device *vgdev,
+				    struct virtio_gpu_object *bo);
 void virtio_gpu_cmd_transfer_to_host_2d(struct virtio_gpu_device *vgdev,
 					uint64_t offset,
 					uint32_t width, uint32_t height,
@@ -483,4 +498,8 @@ void virtio_gpu_vram_unmap_dma_buf(struct device *dev,
 				   struct sg_table *sgt,
 				   enum dma_data_direction dir);
 
+/* virtgpu_gem_shrinker.c */
+int virtio_gpu_gem_shrinker_init(struct virtio_gpu_device *vgdev);
+void virtio_gpu_gem_shrinker_fini(struct virtio_gpu_device *vgdev);
+
 #endif
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 48d3c9955f0d..62cdc00f3009 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -282,3 +282,99 @@ void virtio_gpu_array_put_free_work(struct work_struct *work)
 	}
 	spin_unlock(&vgdev->obj_free_lock);
 }
+
+int virtio_gpu_array_validate(struct virtio_gpu_device *vgdev,
+			      struct virtio_gpu_object_array *objs)
+{
+	struct drm_gem_shmem_object *shmem;
+	int ret = 0;
+	u32 i;
+
+	mutex_lock(&vgdev->mm_lock);
+
+	for (i = 0; i < objs->nents; i++) {
+		shmem = to_drm_gem_shmem_obj(objs->objs[i]);
+		if (shmem->madv) {
+			ret = -ENOMEM;
+			break;
+		}
+	}
+
+	mutex_unlock(&vgdev->mm_lock);
+
+	return ret;
+}
+
+bool virtio_gpu_gem_madvise(struct virtio_gpu_object *bo, int madv)
+{
+	struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;
+	bool retained;
+
+	/*
+	 * For now we support only purging BOs that are backed by guest's
+	 * memory.
+	 */
+	if (!virtio_gpu_is_shmem(bo))
+		return true;
+
+	mutex_lock(&vgdev->mm_lock);
+	retained = drm_gem_shmem_madvise(&bo->base, madv);
+	mutex_unlock(&vgdev->mm_lock);
+
+	return retained;
+}
+
+int virtio_gpu_gem_host_mem_release(struct virtio_gpu_object *bo)
+{
+	struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;
+	int err;
+
+	if (bo->created) {
+		err = virtio_gpu_cmd_release_resource(vgdev, bo);
+		if (err)
+			return err;
+
+		virtio_gpu_notify(vgdev);
+		bo->created = false;
+	}
+
+	return 0;
+}
+
+int virtio_gpu_gem_pin(struct virtio_gpu_object *bo)
+{
+	struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;
+	int ret = 0;
+
+	mutex_lock(&vgdev->mm_lock);
+
+	if (bo->base.madv == VIRTGPU_MADV_WILLNEED)
+		bo->mem_pin_count++;
+	else
+		ret = -ENOMEM;
+
+	mutex_unlock(&vgdev->mm_lock);
+
+	return ret;
+}
+
+void virtio_gpu_gem_unpin(struct virtio_gpu_object *bo)
+{
+	struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;
+
+	mutex_lock(&vgdev->mm_lock);
+	WARN_ON(!bo->mem_pin_count--);
+	mutex_unlock(&vgdev->mm_lock);
+}
+
+bool virtio_gpu_gem_is_pinned(struct virtio_gpu_object *bo)
+{
+	struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;
+	bool ret;
+
+	mutex_lock(&vgdev->mm_lock);
+	ret = bo->mem_pin_count > 0;
+	mutex_unlock(&vgdev->mm_lock);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index c708bab555c6..bb5369eee425 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -217,6 +217,10 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 		ret = virtio_gpu_array_lock_resv(buflist);
 		if (ret)
 			goto out_memdup;
+
+		ret = virtio_gpu_array_validate(vgdev, buflist);
+		if (ret)
+			goto out_unresv;
 	}
 
 	out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
@@ -423,6 +427,10 @@ static int virtio_gpu_transfer_from_host_ioctl(struct drm_device *dev,
 	if (ret != 0)
 		goto err_put_free;
 
+	ret = virtio_gpu_array_validate(vgdev, objs);
+	if (ret)
+		goto err_unlock;
+
 	fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context, 0);
 	if (!fence) {
 		ret = -ENOMEM;
@@ -482,6 +490,10 @@ static int virtio_gpu_transfer_to_host_ioctl(struct drm_device *dev, void *data,
 		if (ret != 0)
 			goto err_put_free;
 
+		ret = virtio_gpu_array_validate(vgdev, objs);
+		if (ret)
+			goto err_unlock;
+
 		ret = -ENOMEM;
 		fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context,
 					       0);
@@ -836,6 +848,28 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,
 	return ret;
 }
 
+static int virtio_gpu_madvise_ioctl(struct drm_device *dev,
+				    void *data,
+				    struct drm_file *file)
+{
+	struct drm_virtgpu_madvise *args = data;
+	struct virtio_gpu_object *bo;
+	struct drm_gem_object *obj;
+
+	if (args->madv > VIRTGPU_MADV_DONTNEED)
+		return -EOPNOTSUPP;
+
+	obj = drm_gem_object_lookup(file, args->bo_handle);
+	if (!obj)
+		return -ENOENT;
+
+	bo = gem_to_virtio_gpu_obj(obj);
+	args->retained = virtio_gpu_gem_madvise(bo, args->madv);
+	drm_gem_object_put(obj);
+
+	return 0;
+}
+
 struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
 	DRM_IOCTL_DEF_DRV(VIRTGPU_MAP, virtio_gpu_map_ioctl,
 			  DRM_RENDER_ALLOW),
@@ -875,4 +909,7 @@ struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
 
 	DRM_IOCTL_DEF_DRV(VIRTGPU_CONTEXT_INIT, virtio_gpu_context_init_ioctl,
 			  DRM_RENDER_ALLOW),
+
+	DRM_IOCTL_DEF_DRV(VIRTGPU_MADVISE, virtio_gpu_madvise_ioctl,
+			  DRM_RENDER_ALLOW),
 };
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 0d1e3eb61bee..3a94d5d5fbed 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -134,6 +134,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
 	dev->dev_private = vgdev;
 	vgdev->vdev = vdev;
 
+	mutex_init(&vgdev->mm_lock);
 	spin_lock_init(&vgdev->display_info_lock);
 	spin_lock_init(&vgdev->resource_export_lock);
 	spin_lock_init(&vgdev->host_visible_lock);
@@ -238,6 +239,12 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
 		goto err_scanouts;
 	}
 
+	ret = drm_gem_shmem_shrinker_register(dev);
+	if (ret) {
+		DRM_ERROR("shrinker init failed\n");
+		goto err_modeset;
+	}
+
 	virtio_device_ready(vgdev->vdev);
 
 	if (num_capsets)
@@ -250,6 +257,8 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
 			   5 * HZ);
 	return 0;
 
+err_modeset:
+	virtio_gpu_modeset_fini(vgdev);
 err_scanouts:
 	virtio_gpu_free_vbufs(vgdev);
 err_vbufs:
@@ -289,6 +298,7 @@ void virtio_gpu_release(struct drm_device *dev)
 	if (!vgdev)
 		return;
 
+	drm_gem_shmem_shrinker_unregister(dev);
 	virtio_gpu_modeset_fini(vgdev);
 	virtio_gpu_free_vbufs(vgdev);
 	virtio_gpu_cleanup_cap_cache(vgdev);
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index 1964c0d8b51f..4133c8a71bd6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -97,6 +97,27 @@ static void virtio_gpu_free_object(struct drm_gem_object *obj)
 	virtio_gpu_cleanup_object(bo);
 }
 
+static unsigned long virtio_gpu_purge_object(struct drm_gem_object *obj)
+{
+	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
+	int err;
+
+	if (virtio_gpu_gem_is_pinned(bo))
+		return 0;
+
+	/*
+	 * Release host's memory before guest's memory is gone to ensure that
+	 * host won't touch released memory of the guest.
+	 */
+	err = virtio_gpu_gem_host_mem_release(bo);
+	if (err)
+		return 0;
+
+	drm_gem_shmem_purge_locked(&bo->base);
+
+	return obj->size >> PAGE_SHIFT;
+}
+
 static const struct drm_gem_object_funcs virtio_gpu_shmem_funcs = {
 	.free = virtio_gpu_free_object,
 	.open = virtio_gpu_gem_object_open,
@@ -110,6 +131,7 @@ static const struct drm_gem_object_funcs virtio_gpu_shmem_funcs = {
 	.vunmap = drm_gem_shmem_object_vunmap,
 	.mmap = drm_gem_shmem_object_mmap,
 	.vm_ops = &drm_gem_shmem_vm_ops,
+	.purge = &virtio_gpu_purge_object,
 };
 
 bool virtio_gpu_is_shmem(struct virtio_gpu_object *bo)
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index 6d3cc9e238a4..597ef1645bf2 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -246,20 +246,28 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct virtio_gpu_framebuffer *vgfb;
 	struct virtio_gpu_object *bo;
+	int err;
 
 	if (!new_state->fb)
 		return 0;
 
 	vgfb = to_virtio_gpu_framebuffer(new_state->fb);
 	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
-	if (!bo || (plane->type == DRM_PLANE_TYPE_PRIMARY && !bo->guest_blob))
+
+	err = virtio_gpu_gem_pin(bo);
+	if (err)
+		return err;
+
+	if (plane->type == DRM_PLANE_TYPE_PRIMARY && !bo->guest_blob)
 		return 0;
 
 	if (bo->dumb && (plane->state->fb != new_state->fb)) {
 		vgfb->fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context,
 						     0);
-		if (!vgfb->fence)
+		if (!vgfb->fence) {
+			virtio_gpu_gem_unpin(bo);
 			return -ENOMEM;
+		}
 	}
 
 	return 0;
@@ -269,15 +277,20 @@ static void virtio_gpu_plane_cleanup_fb(struct drm_plane *plane,
 					struct drm_plane_state *old_state)
 {
 	struct virtio_gpu_framebuffer *vgfb;
+	struct virtio_gpu_object *bo;
 
 	if (!plane->state->fb)
 		return;
 
 	vgfb = to_virtio_gpu_framebuffer(plane->state->fb);
+	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
+
 	if (vgfb->fence) {
 		dma_fence_put(&vgfb->fence->f);
 		vgfb->fence = NULL;
 	}
+
+	virtio_gpu_gem_unpin(bo);
 }
 
 static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 06566e44307d..c55c2fc8ecc0 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -536,6 +536,21 @@ void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev,
 		virtio_gpu_cleanup_object(bo);
 }
 
+int virtio_gpu_cmd_release_resource(struct virtio_gpu_device *vgdev,
+				    struct virtio_gpu_object *bo)
+{
+	struct virtio_gpu_resource_unref *cmd_p;
+	struct virtio_gpu_vbuffer *vbuf;
+
+	cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
+	memset(cmd_p, 0, sizeof(*cmd_p));
+
+	cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_UNREF);
+	cmd_p->resource_id = cpu_to_le32(bo->hw_res_handle);
+
+	return virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
+}
+
 void virtio_gpu_cmd_set_scanout(struct virtio_gpu_device *vgdev,
 				uint32_t scanout_id, uint32_t resource_id,
 				uint32_t width, uint32_t height,
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index 0512fde5e697..12197d8e9759 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -48,6 +48,7 @@ extern "C" {
 #define DRM_VIRTGPU_GET_CAPS  0x09
 #define DRM_VIRTGPU_RESOURCE_CREATE_BLOB 0x0a
 #define DRM_VIRTGPU_CONTEXT_INIT 0x0b
+#define DRM_VIRTGPU_MADVISE 0x0c
 
 #define VIRTGPU_EXECBUF_FENCE_FD_IN	0x01
 #define VIRTGPU_EXECBUF_FENCE_FD_OUT	0x02
@@ -196,6 +197,15 @@ struct drm_virtgpu_context_init {
 	__u64 ctx_set_params;
 };
 
+#define VIRTGPU_MADV_WILLNEED 0
+#define VIRTGPU_MADV_DONTNEED 1
+struct drm_virtgpu_madvise {
+	__u32 bo_handle;
+	__u32 retained; /* out, non-zero if BO can be used */
+	__u32 madv;
+	__u32 pad;
+};
+
 /*
  * Event code that's given when VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK is in
  * effect.  The event size is sizeof(drm_event), since there is no additional
@@ -246,6 +256,10 @@ struct drm_virtgpu_context_init {
 	DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_CONTEXT_INIT,		\
 		struct drm_virtgpu_context_init)
 
+#define DRM_IOCTL_VIRTGPU_MADVISE \
+	DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_MADVISE, \
+		 struct drm_virtgpu_madvise)
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.35.1

Re: [PATCH v2 7/8] drm/virtio: Support memory shrinking

[Emil Velikov]

Greetings everyone,

Food for thought: Would it make sense to have the madvise ioctl as
generic DRM one?
Looking around - i915, msm & panfrost already have one and the virtio
implementation [below] seems as generic as it gets.

On Mon, 14 Mar 2022 at 22:44, Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:

> +#define VIRTGPU_MADV_WILLNEED 0
> +#define VIRTGPU_MADV_DONTNEED 1
> +struct drm_virtgpu_madvise {
> +       __u32 bo_handle;
> +       __u32 retained; /* out, non-zero if BO can be used */
> +       __u32 madv;
> +       __u32 pad;

This seems to be based on panfrost/msm yet names (bo_handle vs
handle), layout and documentation varies.
Why is that - copy/paste is cheap :-P

HTH

-Emil

Re: [PATCH v2 7/8] drm/virtio: Support memory shrinking

[Dmitry Osipenko]

On 3/15/22 15:43, Emil Velikov wrote:
> Greetings everyone,
> 
> Food for thought: Would it make sense to have the madvise ioctl as
> generic DRM one?
> Looking around - i915, msm & panfrost already have one and the virtio
> implementation [below] seems as generic as it gets.
> 
> On Mon, 14 Mar 2022 at 22:44, Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
> 
>> +#define VIRTGPU_MADV_WILLNEED 0
>> +#define VIRTGPU_MADV_DONTNEED 1
>> +struct drm_virtgpu_madvise {
>> +       __u32 bo_handle;
>> +       __u32 retained; /* out, non-zero if BO can be used */
>> +       __u32 madv;
>> +       __u32 pad;
> 
> This seems to be based on panfrost/msm yet names (bo_handle vs
> handle), layout and documentation varies.
> Why is that - copy/paste is cheap :-P

Indeed, there is copy/pasting among drivers. But I'm doubtful about
making madvise a generic ioctl because some drivers already have own
ioctl for that and h/w capabilities vary, so some drivers may want to
have extra features later on and then this doesn't feel like a common
thing anymore.

[PATCH v2 8/8] drm/panfrost: Switch to generic memory shrinker

[Dmitry Osipenko]

Replace Panfrost's memory shrinker with a generic DRM memory shrinker.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 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    | 27 ++++++++++++++--------
 drivers/gpu/drm/panfrost/panfrost_gem.h    |  9 --------
 drivers/gpu/drm/panfrost/panfrost_job.c    | 22 +++++++++++++++++-
 6 files changed, 40 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
index b71935862417..ecf0864cb515 100644
--- a/drivers/gpu/drm/panfrost/Makefile
+++ b/drivers/gpu/drm/panfrost/Makefile
@@ -5,7 +5,6 @@ panfrost-y := \
 	panfrost_device.o \
 	panfrost_devfreq.o \
 	panfrost_gem.o \
-	panfrost_gem_shrinker.o \
 	panfrost_gpu.o \
 	panfrost_job.o \
 	panfrost_mmu.o \
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 8b25278f34c8..fe04b21fc044 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -115,10 +115,6 @@ struct panfrost_device {
 		atomic_t pending;
 	} reset;
 
-	struct mutex shrinker_lock;
-	struct list_head shrinker_list;
-	struct shrinker shrinker;
-
 	struct panfrost_devfreq pfdevfreq;
 };
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 94b6f0a19c83..b014dadcf51f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -160,7 +160,6 @@ panfrost_lookup_bos(struct drm_device *dev,
 			break;
 		}
 
-		atomic_inc(&bo->gpu_usecount);
 		job->mappings[i] = mapping;
 	}
 
@@ -390,7 +389,6 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
 {
 	struct panfrost_file_priv *priv = file_priv->driver_priv;
 	struct drm_panfrost_madvise *args = data;
-	struct panfrost_device *pfdev = dev->dev_private;
 	struct drm_gem_object *gem_obj;
 	struct panfrost_gem_object *bo;
 	int ret = 0;
@@ -403,7 +401,6 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
 
 	bo = to_panfrost_bo(gem_obj);
 
-	mutex_lock(&pfdev->shrinker_lock);
 	mutex_lock(&bo->mappings.lock);
 	if (args->madv == PANFROST_MADV_DONTNEED) {
 		struct panfrost_gem_mapping *first;
@@ -429,17 +426,8 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
 
 	args->retained = drm_gem_shmem_madvise(&bo->base, args->madv);
 
-	if (args->retained) {
-		if (args->madv == PANFROST_MADV_DONTNEED)
-			list_add_tail(&bo->base.madv_list,
-				      &pfdev->shrinker_list);
-		else if (args->madv == PANFROST_MADV_WILLNEED)
-			list_del_init(&bo->base.madv_list);
-	}
-
 out_unlock_mappings:
 	mutex_unlock(&bo->mappings.lock);
-	mutex_unlock(&pfdev->shrinker_lock);
 
 	drm_gem_object_put(gem_obj);
 	return ret;
@@ -570,9 +558,6 @@ static int panfrost_probe(struct platform_device *pdev)
 	ddev->dev_private = pfdev;
 	pfdev->ddev = ddev;
 
-	mutex_init(&pfdev->shrinker_lock);
-	INIT_LIST_HEAD(&pfdev->shrinker_list);
-
 	err = panfrost_device_init(pfdev);
 	if (err) {
 		if (err != -EPROBE_DEFER)
@@ -594,7 +579,7 @@ static int panfrost_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto err_out1;
 
-	panfrost_gem_shrinker_init(ddev);
+	drm_gem_shmem_shrinker_register(ddev);
 
 	return 0;
 
@@ -612,8 +597,8 @@ static int panfrost_remove(struct platform_device *pdev)
 	struct panfrost_device *pfdev = platform_get_drvdata(pdev);
 	struct drm_device *ddev = pfdev->ddev;
 
+	drm_gem_shmem_shrinker_unregister(ddev);
 	drm_dev_unregister(ddev);
-	panfrost_gem_shrinker_cleanup(ddev);
 
 	pm_runtime_get_sync(pfdev->dev);
 	pm_runtime_disable(pfdev->dev);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 293e799e2fe8..d164d05ed84e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -19,16 +19,6 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
 	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
 	struct panfrost_device *pfdev = obj->dev->dev_private;
 
-	/*
-	 * Make sure the BO is no longer inserted in the shrinker list before
-	 * taking care of the destruction itself. If we don't do that we have a
-	 * race condition between this function and what's done in
-	 * panfrost_gem_shrinker_scan().
-	 */
-	mutex_lock(&pfdev->shrinker_lock);
-	list_del_init(&bo->base.madv_list);
-	mutex_unlock(&pfdev->shrinker_lock);
-
 	/*
 	 * If we still have mappings attached to the BO, there's a problem in
 	 * our refcounting.
@@ -195,6 +185,22 @@ static int panfrost_gem_pin(struct drm_gem_object *obj)
 	return drm_gem_shmem_pin(&bo->base);
 }
 
+static unsigned long panfrost_gem_purge(struct drm_gem_object *obj)
+{
+	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
+
+	if (!mutex_trylock(&bo->mappings.lock))
+		return 0;
+
+	panfrost_gem_teardown_mappings_locked(bo);
+	drm_gem_shmem_purge_locked(&bo->base);
+
+	mutex_unlock(&bo->mappings.lock);
+
+	return shmem->base.size >> PAGE_SHIFT;
+}
+
 static const struct drm_gem_object_funcs panfrost_gem_funcs = {
 	.free = panfrost_gem_free_object,
 	.open = panfrost_gem_open,
@@ -207,6 +213,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
 	.vunmap = drm_gem_shmem_object_vunmap,
 	.mmap = drm_gem_shmem_object_mmap,
 	.vm_ops = &drm_gem_shmem_vm_ops,
+	.purge = panfrost_gem_purge,
 };
 
 /**
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index 8088d5fd8480..09da064f1c07 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -30,12 +30,6 @@ struct panfrost_gem_object {
 		struct mutex lock;
 	} mappings;
 
-	/*
-	 * Count the number of jobs referencing this BO so we don't let the
-	 * shrinker reclaim this object prematurely.
-	 */
-	atomic_t gpu_usecount;
-
 	bool noexec		:1;
 	bool is_heap		:1;
 };
@@ -84,7 +78,4 @@ panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
 void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
 void panfrost_gem_teardown_mappings_locked(struct panfrost_gem_object *bo);
 
-void panfrost_gem_shrinker_init(struct drm_device *dev);
-void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
-
 #endif /* __PANFROST_GEM_H__ */
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index a6925dbb6224..e767e526e897 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -267,6 +267,22 @@ static void panfrost_attach_object_fences(struct drm_gem_object **bos,
 		dma_resv_add_excl_fence(bos[i]->resv, fence);
 }
 
+static bool panfrost_objects_alive(struct drm_gem_object **bos, int bo_count)
+{
+	struct panfrost_gem_object *bo;
+	bool alive = true;
+
+	while (alive && bo_count--) {
+		bo = to_panfrost_bo(bos[bo_count]);
+
+		mutex_lock(&bo->mappings.lock);
+		alive = !bo->base.madv;
+		mutex_unlock(&bo->mappings.lock);
+	}
+
+	return alive;
+}
+
 int panfrost_job_push(struct panfrost_job *job)
 {
 	struct panfrost_device *pfdev = job->pfdev;
@@ -278,6 +294,11 @@ int panfrost_job_push(struct panfrost_job *job)
 	if (ret)
 		return ret;
 
+	if (!panfrost_objects_alive(job->bos, job->bo_count)) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
 	mutex_lock(&pfdev->sched_lock);
 	drm_sched_job_arm(&job->base);
 
@@ -319,7 +340,6 @@ static void panfrost_job_cleanup(struct kref *ref)
 			if (!job->mappings[i])
 				break;
 
-			atomic_dec(&job->mappings[i]->obj->gpu_usecount);
 			panfrost_gem_mapping_put(job->mappings[i]);
 		}
 		kvfree(job->mappings);
-- 
2.35.1

Re: [PATCH v2 8/8] drm/panfrost: Switch to generic memory shrinker

[Alyssa Rosenzweig]

On Tue, Mar 15, 2022 at 01:42:53AM +0300, Dmitry Osipenko wrote:
> Replace Panfrost's memory shrinker with a generic DRM memory shrinker.
> 
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  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    | 27 ++++++++++++++--------
>  drivers/gpu/drm/panfrost/panfrost_gem.h    |  9 --------
>  drivers/gpu/drm/panfrost/panfrost_job.c    | 22 +++++++++++++++++-
>  6 files changed, 40 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
> index b71935862417..ecf0864cb515 100644
> --- a/drivers/gpu/drm/panfrost/Makefile
> +++ b/drivers/gpu/drm/panfrost/Makefile
> @@ -5,7 +5,6 @@ panfrost-y := \
>  	panfrost_device.o \
>  	panfrost_devfreq.o \
>  	panfrost_gem.o \
> -	panfrost_gem_shrinker.o \
>  	panfrost_gpu.o \
>  	panfrost_job.o \
>  	panfrost_mmu.o \

I'm not sure you actually deleted gem_shrinker anywhere in this patch?
Diff stat is too small.

Re: [PATCH v2 8/8] drm/panfrost: Switch to generic memory shrinker

[Dmitry Osipenko]

On 3/15/22 02:26, Alyssa Rosenzweig wrote:
> On Tue, Mar 15, 2022 at 01:42:53AM +0300, Dmitry Osipenko wrote:
>> Replace Panfrost's memory shrinker with a generic DRM memory shrinker.
>>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> ---
>>  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    | 27 ++++++++++++++--------
>>  drivers/gpu/drm/panfrost/panfrost_gem.h    |  9 --------
>>  drivers/gpu/drm/panfrost/panfrost_job.c    | 22 +++++++++++++++++-
>>  6 files changed, 40 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
>> index b71935862417..ecf0864cb515 100644
>> --- a/drivers/gpu/drm/panfrost/Makefile
>> +++ b/drivers/gpu/drm/panfrost/Makefile
>> @@ -5,7 +5,6 @@ panfrost-y := \
>>  	panfrost_device.o \
>>  	panfrost_devfreq.o \
>>  	panfrost_gem.o \
>> -	panfrost_gem_shrinker.o \
>>  	panfrost_gpu.o \
>>  	panfrost_job.o \
>>  	panfrost_mmu.o \
> 
> I'm not sure you actually deleted gem_shrinker anywhere in this patch?
> Diff stat is too small.

Indeed, I was also confused by the diffstat once noticed it after the
patches were sent out. And yes, I missed to git-add the
panfrost_gem_shrinker.c by accident. Good catch, thank you! I'll correct
it in in the v3.

Meanwhile, will be great if you or somebody else could test this
Panfrost patch. I don't have any h/w with Mali on hands yet and only
compile-tested it.

Re: [PATCH v2 8/8] drm/panfrost: Switch to generic memory shrinker

[Steven Price]

On 14/03/2022 22:42, Dmitry Osipenko wrote:
> Replace Panfrost's memory shrinker with a generic DRM memory shrinker.
> 
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---

I gave this a spin on my Firefly-RK3288 board and everything seems to
work. So feel free to add a:

Tested-by: Steven Price <steven.price@arm.com>

As Alyssa has already pointed out you need to remove the
panfrost_gem_shrinker.c file. But otherwise everything looks fine, and
I'm very happy to see the shrinker code gone ;)

Thanks,

Steve

>  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    | 27 ++++++++++++++--------
>  drivers/gpu/drm/panfrost/panfrost_gem.h    |  9 --------
>  drivers/gpu/drm/panfrost/panfrost_job.c    | 22 +++++++++++++++++-
>  6 files changed, 40 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
> index b71935862417..ecf0864cb515 100644
> --- a/drivers/gpu/drm/panfrost/Makefile
> +++ b/drivers/gpu/drm/panfrost/Makefile
> @@ -5,7 +5,6 @@ panfrost-y := \
>  	panfrost_device.o \
>  	panfrost_devfreq.o \
>  	panfrost_gem.o \
> -	panfrost_gem_shrinker.o \
>  	panfrost_gpu.o \
>  	panfrost_job.o \
>  	panfrost_mmu.o \
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 8b25278f34c8..fe04b21fc044 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -115,10 +115,6 @@ struct panfrost_device {
>  		atomic_t pending;
>  	} reset;
>  
> -	struct mutex shrinker_lock;
> -	struct list_head shrinker_list;
> -	struct shrinker shrinker;
> -
>  	struct panfrost_devfreq pfdevfreq;
>  };
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 94b6f0a19c83..b014dadcf51f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -160,7 +160,6 @@ panfrost_lookup_bos(struct drm_device *dev,
>  			break;
>  		}
>  
> -		atomic_inc(&bo->gpu_usecount);
>  		job->mappings[i] = mapping;
>  	}
>  
> @@ -390,7 +389,6 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  {
>  	struct panfrost_file_priv *priv = file_priv->driver_priv;
>  	struct drm_panfrost_madvise *args = data;
> -	struct panfrost_device *pfdev = dev->dev_private;
>  	struct drm_gem_object *gem_obj;
>  	struct panfrost_gem_object *bo;
>  	int ret = 0;
> @@ -403,7 +401,6 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  
>  	bo = to_panfrost_bo(gem_obj);
>  
> -	mutex_lock(&pfdev->shrinker_lock);
>  	mutex_lock(&bo->mappings.lock);
>  	if (args->madv == PANFROST_MADV_DONTNEED) {
>  		struct panfrost_gem_mapping *first;
> @@ -429,17 +426,8 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
>  
>  	args->retained = drm_gem_shmem_madvise(&bo->base, args->madv);
>  
> -	if (args->retained) {
> -		if (args->madv == PANFROST_MADV_DONTNEED)
> -			list_add_tail(&bo->base.madv_list,
> -				      &pfdev->shrinker_list);
> -		else if (args->madv == PANFROST_MADV_WILLNEED)
> -			list_del_init(&bo->base.madv_list);
> -	}
> -
>  out_unlock_mappings:
>  	mutex_unlock(&bo->mappings.lock);
> -	mutex_unlock(&pfdev->shrinker_lock);
>  
>  	drm_gem_object_put(gem_obj);
>  	return ret;
> @@ -570,9 +558,6 @@ static int panfrost_probe(struct platform_device *pdev)
>  	ddev->dev_private = pfdev;
>  	pfdev->ddev = ddev;
>  
> -	mutex_init(&pfdev->shrinker_lock);
> -	INIT_LIST_HEAD(&pfdev->shrinker_list);
> -
>  	err = panfrost_device_init(pfdev);
>  	if (err) {
>  		if (err != -EPROBE_DEFER)
> @@ -594,7 +579,7 @@ static int panfrost_probe(struct platform_device *pdev)
>  	if (err < 0)
>  		goto err_out1;
>  
> -	panfrost_gem_shrinker_init(ddev);
> +	drm_gem_shmem_shrinker_register(ddev);
>  
>  	return 0;
>  
> @@ -612,8 +597,8 @@ static int panfrost_remove(struct platform_device *pdev)
>  	struct panfrost_device *pfdev = platform_get_drvdata(pdev);
>  	struct drm_device *ddev = pfdev->ddev;
>  
> +	drm_gem_shmem_shrinker_unregister(ddev);
>  	drm_dev_unregister(ddev);
> -	panfrost_gem_shrinker_cleanup(ddev);
>  
>  	pm_runtime_get_sync(pfdev->dev);
>  	pm_runtime_disable(pfdev->dev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 293e799e2fe8..d164d05ed84e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -19,16 +19,6 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
>  	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
>  	struct panfrost_device *pfdev = obj->dev->dev_private;
>  
> -	/*
> -	 * Make sure the BO is no longer inserted in the shrinker list before
> -	 * taking care of the destruction itself. If we don't do that we have a
> -	 * race condition between this function and what's done in
> -	 * panfrost_gem_shrinker_scan().
> -	 */
> -	mutex_lock(&pfdev->shrinker_lock);
> -	list_del_init(&bo->base.madv_list);
> -	mutex_unlock(&pfdev->shrinker_lock);
> -
>  	/*
>  	 * If we still have mappings attached to the BO, there's a problem in
>  	 * our refcounting.
> @@ -195,6 +185,22 @@ static int panfrost_gem_pin(struct drm_gem_object *obj)
>  	return drm_gem_shmem_pin(&bo->base);
>  }
>  
> +static unsigned long panfrost_gem_purge(struct drm_gem_object *obj)
> +{
> +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> +
> +	if (!mutex_trylock(&bo->mappings.lock))
> +		return 0;
> +
> +	panfrost_gem_teardown_mappings_locked(bo);
> +	drm_gem_shmem_purge_locked(&bo->base);
> +
> +	mutex_unlock(&bo->mappings.lock);
> +
> +	return shmem->base.size >> PAGE_SHIFT;
> +}
> +
>  static const struct drm_gem_object_funcs panfrost_gem_funcs = {
>  	.free = panfrost_gem_free_object,
>  	.open = panfrost_gem_open,
> @@ -207,6 +213,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
>  	.vunmap = drm_gem_shmem_object_vunmap,
>  	.mmap = drm_gem_shmem_object_mmap,
>  	.vm_ops = &drm_gem_shmem_vm_ops,
> +	.purge = panfrost_gem_purge,
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index 8088d5fd8480..09da064f1c07 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -30,12 +30,6 @@ struct panfrost_gem_object {
>  		struct mutex lock;
>  	} mappings;
>  
> -	/*
> -	 * Count the number of jobs referencing this BO so we don't let the
> -	 * shrinker reclaim this object prematurely.
> -	 */
> -	atomic_t gpu_usecount;
> -
>  	bool noexec		:1;
>  	bool is_heap		:1;
>  };
> @@ -84,7 +78,4 @@ panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
>  void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
>  void panfrost_gem_teardown_mappings_locked(struct panfrost_gem_object *bo);
>  
> -void panfrost_gem_shrinker_init(struct drm_device *dev);
> -void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
> -
>  #endif /* __PANFROST_GEM_H__ */
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index a6925dbb6224..e767e526e897 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -267,6 +267,22 @@ static void panfrost_attach_object_fences(struct drm_gem_object **bos,
>  		dma_resv_add_excl_fence(bos[i]->resv, fence);
>  }
>  
> +static bool panfrost_objects_alive(struct drm_gem_object **bos, int bo_count)
> +{
> +	struct panfrost_gem_object *bo;
> +	bool alive = true;
> +
> +	while (alive && bo_count--) {
> +		bo = to_panfrost_bo(bos[bo_count]);
> +
> +		mutex_lock(&bo->mappings.lock);
> +		alive = !bo->base.madv;
> +		mutex_unlock(&bo->mappings.lock);
> +	}
> +
> +	return alive;
> +}
> +
>  int panfrost_job_push(struct panfrost_job *job)
>  {
>  	struct panfrost_device *pfdev = job->pfdev;
> @@ -278,6 +294,11 @@ int panfrost_job_push(struct panfrost_job *job)
>  	if (ret)
>  		return ret;
>  
> +	if (!panfrost_objects_alive(job->bos, job->bo_count)) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
>  	mutex_lock(&pfdev->sched_lock);
>  	drm_sched_job_arm(&job->base);
>  
> @@ -319,7 +340,6 @@ static void panfrost_job_cleanup(struct kref *ref)
>  			if (!job->mappings[i])
>  				break;
>  
> -			atomic_dec(&job->mappings[i]->obj->gpu_usecount);
>  			panfrost_gem_mapping_put(job->mappings[i]);
>  		}
>  		kvfree(job->mappings);

Re: [PATCH v2 8/8] drm/panfrost: Switch to generic memory shrinker

[Dmitry Osipenko]

On 3/16/22 18:04, Steven Price wrote:
> On 14/03/2022 22:42, Dmitry Osipenko wrote:
>> Replace Panfrost's memory shrinker with a generic DRM memory shrinker.
>>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> ---
> I gave this a spin on my Firefly-RK3288 board and everything seems to
> work. So feel free to add a:
> 
> Tested-by: Steven Price <steven.price@arm.com>
> 
> As Alyssa has already pointed out you need to remove the
> panfrost_gem_shrinker.c file. But otherwise everything looks fine, and
> I'm very happy to see the shrinker code gone ;)

Awesome, thank you.

Re: [PATCH v2 8/8] drm/panfrost: Switch to generic memory shrinker

[Dmitry Osipenko]

On 3/17/22 02:04, Dmitry Osipenko wrote:
> 
> On 3/16/22 18:04, Steven Price wrote:
>> On 14/03/2022 22:42, Dmitry Osipenko wrote:
>>> Replace Panfrost's memory shrinker with a generic DRM memory shrinker.
>>>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> ---
>> I gave this a spin on my Firefly-RK3288 board and everything seems to
>> work. So feel free to add a:
>>
>> Tested-by: Steven Price <steven.price@arm.com>
>>
>> As Alyssa has already pointed out you need to remove the
>> panfrost_gem_shrinker.c file. But otherwise everything looks fine, and
>> I'm very happy to see the shrinker code gone ;)
> 
> Awesome, thank you.

Steven, could you please tell me how exactly you tested the shrinker?

I realized that today's IGT doesn't have any tests for the Panfrost's
madvise ioctl.

You may invoke "echo 2 > /proc/sys/vm/drop_caches" manually in order to
trigger shrinker while 3d app is running actively (like a game or
benchmark). Nothing crashing will be a good enough indicator that it
works okay.

I may get an RK board next week and then will be able to test it by
myself, so please don't hurry.

Re: [PATCH v2 8/8] drm/panfrost: Switch to generic memory shrinker

[Steven Price]

On 18/03/2022 14:41, Dmitry Osipenko wrote:
> 
> On 3/17/22 02:04, Dmitry Osipenko wrote:
>>
>> On 3/16/22 18:04, Steven Price wrote:
>>> On 14/03/2022 22:42, Dmitry Osipenko wrote:
>>>> Replace Panfrost's memory shrinker with a generic DRM memory shrinker.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>> ---
>>> I gave this a spin on my Firefly-RK3288 board and everything seems to
>>> work. So feel free to add a:
>>>
>>> Tested-by: Steven Price <steven.price@arm.com>
>>>
>>> As Alyssa has already pointed out you need to remove the
>>> panfrost_gem_shrinker.c file. But otherwise everything looks fine, and
>>> I'm very happy to see the shrinker code gone ;)
>>
>> Awesome, thank you.
> 
> Steven, could you please tell me how exactly you tested the shrinker?
> 
> I realized that today's IGT doesn't have any tests for the Panfrost's
> madvise ioctl.
> 
> You may invoke "echo 2 > /proc/sys/vm/drop_caches" manually in order to
> trigger shrinker while 3d app is running actively (like a game or
> benchmark). Nothing crashing will be a good enough indicator that it
> works okay.
> 
> I may get an RK board next week and then will be able to test it by
> myself, so please don't hurry.

I have to admit it wasn't a very thorough test. I run glmark on the
board with the following hack:

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index b014dadcf51f..194dec00695a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -279,6 +279,14 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
        if (ret)
                goto out_cleanup_job;
 
+       {
+       struct shrink_control sc = {
+               .nr_to_scan = 1000
+       };
+       dev->shmem_shrinker->base.scan_objects(&dev->shmem_shrinker->base,
+                       &sc);
+       }
+
        ret = panfrost_job_push(job);
        if (ret)
                goto out_cleanup_job;

That hack was specifically because I had some doubts about the removal
of the 'gpu_usecount' counter and wanted to ensure that purging as the
job is submitted wouldn't cause problems.

The drop_caches file should also work AFAIK.

Steve

Re: [PATCH v2 8/8] drm/panfrost: Switch to generic memory shrinker

[Dmitry Osipenko]

On 3/18/22 17:47, Steven Price wrote:
> On 18/03/2022 14:41, Dmitry Osipenko wrote:
>>
>> On 3/17/22 02:04, Dmitry Osipenko wrote:
>>>
>>> On 3/16/22 18:04, Steven Price wrote:
>>>> On 14/03/2022 22:42, Dmitry Osipenko wrote:
>>>>> Replace Panfrost's memory shrinker with a generic DRM memory shrinker.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>>> ---
>>>> I gave this a spin on my Firefly-RK3288 board and everything seems to
>>>> work. So feel free to add a:
>>>>
>>>> Tested-by: Steven Price <steven.price@arm.com>
>>>>
>>>> As Alyssa has already pointed out you need to remove the
>>>> panfrost_gem_shrinker.c file. But otherwise everything looks fine, and
>>>> I'm very happy to see the shrinker code gone ;)
>>>
>>> Awesome, thank you.
>>
>> Steven, could you please tell me how exactly you tested the shrinker?
>>
>> I realized that today's IGT doesn't have any tests for the Panfrost's
>> madvise ioctl.
>>
>> You may invoke "echo 2 > /proc/sys/vm/drop_caches" manually in order to
>> trigger shrinker while 3d app is running actively (like a game or
>> benchmark). Nothing crashing will be a good enough indicator that it
>> works okay.
>>
>> I may get an RK board next week and then will be able to test it by
>> myself, so please don't hurry.
> 
> I have to admit it wasn't a very thorough test. I run glmark on the
> board with the following hack:
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index b014dadcf51f..194dec00695a 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -279,6 +279,14 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
>         if (ret)
>                 goto out_cleanup_job;
>  
> +       {
> +       struct shrink_control sc = {
> +               .nr_to_scan = 1000
> +       };
> +       dev->shmem_shrinker->base.scan_objects(&dev->shmem_shrinker->base,
> +                       &sc);
> +       }
> +
>         ret = panfrost_job_push(job);
>         if (ret)
>                 goto out_cleanup_job;
> 
> That hack was specifically because I had some doubts about the removal
> of the 'gpu_usecount' counter and wanted to ensure that purging as the
> job is submitted wouldn't cause problems.
> 
> The drop_caches file should also work AFAIK.

Yours variant of testing looks okay, thanks.

We check BO's reservation dma-fence status in the shrinker's scan(), so
BO won't be purged until the last job has completed using the BO. The
'gpu_usecount' is redundant now, and thus, I removed it.

Re: [PATCH v2 0/8] Add memory shrinker to VirtIO-GPU DRM driver

[Emil Velikov]

On Mon, 14 Mar 2022 at 22:44, Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:

> Dmitry Osipenko (8):
>   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 in error code path

These three are legitimate fixes that we want regardless of the shrinker.

Please add the respective "Fixes" tag, so they find their way in the
stable trees. With that, them 3 are:
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

HTH
Emil

Re: [PATCH v2 0/8] Add memory shrinker to VirtIO-GPU DRM driver

[Dmitry Osipenko]

On 3/15/22 15:47, Emil Velikov wrote:
> On Mon, 14 Mar 2022 at 22:44, Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
> 
>> Dmitry Osipenko (8):
>>   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 in error code path
> 
> These three are legitimate fixes that we want regardless of the shrinker.
> 
> Please add the respective "Fixes" tag, so they find their way in the
> stable trees. With that, them 3 are:
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Thank you, I already added stable tag to the first patch. The other
patches aren't critical for the stable kernels, IMO, but we can tag them
too for completeness. I'll do in v3.