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.

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

*/

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

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

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

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.

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);

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.