Re: [PATCH v18 24/26] drm/virtio: Attach shmem BOs dynamically
On 11/23/23 01:37, Dmitry Osipenko wrote: > On 11/13/23 12:57, Boris Brezillon wrote: >> On Mon, 30 Oct 2023 02:02:03 +0300 >> Dmitry Osipenko wrote: >> >>> Prepare for addition of memory shrinker support by attaching shmem pages >>> to host dynamically on first use. Previously the attachment vq command >>> wasn't fenced and there was no vq kick made in the BO creation code path, >>> hence the attachment already was happening dynamically, but implicitly. >>> Making attachment explicitly dynamic will allow to simplify and reuse more >>> code when shrinker will be added. The virtio_gpu_object_shmem_init() now >>> works under the held reservation lock, which will be important to have for >>> shrinker to avoid moving pages while they are in active use by the driver. >> Ah, this commit might actually help getting rid of the workaround >> introduced in "drm/shmem-helper: Add common memory shrinker". > > Not really. The virtio_gpu_free_object() is unchanged, it's only > allocation that is being deferred and it's only done for a one BO type > (virtio-gpu has multiple BO types). s/allocation/attachment/ Pages are still allocated by virtio_gpu_object_create(). -- Best regards, Dmitry
Re: [PATCH v18 24/26] drm/virtio: Attach shmem BOs dynamically
On 11/13/23 12:57, Boris Brezillon wrote: > On Mon, 30 Oct 2023 02:02:03 +0300 > Dmitry Osipenko wrote: > >> Prepare for addition of memory shrinker support by attaching shmem pages >> to host dynamically on first use. Previously the attachment vq command >> wasn't fenced and there was no vq kick made in the BO creation code path, >> hence the attachment already was happening dynamically, but implicitly. >> Making attachment explicitly dynamic will allow to simplify and reuse more >> code when shrinker will be added. The virtio_gpu_object_shmem_init() now >> works under the held reservation lock, which will be important to have for >> shrinker to avoid moving pages while they are in active use by the driver. > Ah, this commit might actually help getting rid of the workaround > introduced in "drm/shmem-helper: Add common memory shrinker". Not really. The virtio_gpu_free_object() is unchanged, it's only allocation that is being deferred and it's only done for a one BO type (virtio-gpu has multiple BO types). -- Best regards, Dmitry
Re: [PATCH v18 24/26] drm/virtio: Attach shmem BOs dynamically
On Mon, 30 Oct 2023 02:02:03 +0300
Dmitry Osipenko wrote:
> Prepare for addition of memory shrinker support by attaching shmem pages
> to host dynamically on first use. Previously the attachment vq command
> wasn't fenced and there was no vq kick made in the BO creation code path,
> hence the attachment already was happening dynamically, but implicitly.
> Making attachment explicitly dynamic will allow to simplify and reuse more
> code when shrinker will be added. The virtio_gpu_object_shmem_init() now
> works under the held reservation lock, which will be important to have for
> shrinker to avoid moving pages while they are in active use by the driver.
Ah, this commit might actually help getting rid of the workaround
introduced in "drm/shmem-helper: Add common memory shrinker".
>
> Acked-by: Gerd Hoffmann
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.h| 7 +++
> drivers/gpu/drm/virtio/virtgpu_gem.c| 26 +
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 32 +++
> drivers/gpu/drm/virtio/virtgpu_object.c | 73 -
> drivers/gpu/drm/virtio/virtgpu_submit.c | 15 -
> 5 files changed, 125 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 56269814fb6d..421f524ae1de 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -89,6 +89,7 @@ struct virtio_gpu_object {
> uint32_t hw_res_handle;
> bool dumb;
> bool created;
> + bool detached;
> bool host3d_blob, guest_blob;
> uint32_t blob_mem, blob_flags;
>
> @@ -313,6 +314,8 @@ 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_prepare(struct virtio_gpu_device *vgdev,
> + struct virtio_gpu_object_array *objs);
> int virtio_gpu_gem_pin(struct virtio_gpu_object *bo);
> void virtio_gpu_gem_unpin(struct virtio_gpu_object *bo);
>
> @@ -453,6 +456,10 @@ int virtio_gpu_object_create(struct virtio_gpu_device
> *vgdev,
>
> bool virtio_gpu_is_shmem(struct virtio_gpu_object *bo);
>
> +int virtio_gpu_reattach_shmem_object_locked(struct virtio_gpu_object *bo);
> +
> +int virtio_gpu_reattach_shmem_object(struct virtio_gpu_object *bo);
> +
> int virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev,
> uint32_t *resid);
> /* virtgpu_prime.c */
> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c
> b/drivers/gpu/drm/virtio/virtgpu_gem.c
> index 625c05d625bf..97e67064c97e 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
> @@ -295,6 +295,26 @@ void virtio_gpu_array_put_free_work(struct work_struct
> *work)
> spin_unlock(&vgdev->obj_free_lock);
> }
>
> +int virtio_gpu_array_prepare(struct virtio_gpu_device *vgdev,
> + struct virtio_gpu_object_array *objs)
> +{
> + struct virtio_gpu_object *bo;
> + int ret = 0;
> + u32 i;
> +
> + for (i = 0; i < objs->nents; i++) {
> + bo = gem_to_virtio_gpu_obj(objs->objs[i]);
> +
> + if (virtio_gpu_is_shmem(bo) && bo->detached) {
> + ret = virtio_gpu_reattach_shmem_object_locked(bo);
> + if (ret)
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> int virtio_gpu_gem_pin(struct virtio_gpu_object *bo)
> {
> int err;
> @@ -303,6 +323,12 @@ int virtio_gpu_gem_pin(struct virtio_gpu_object *bo)
> err = drm_gem_shmem_pin(&bo->base);
> if (err)
> return err;
> +
> + err = virtio_gpu_reattach_shmem_object(bo);
> + if (err) {
> + drm_gem_shmem_unpin(&bo->base);
> + return err;
> + }
> }
>
> return 0;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index b24b11f25197..070c29cea26a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -246,6 +246,10 @@ static int virtio_gpu_transfer_from_host_ioctl(struct
> drm_device *dev,
> if (ret != 0)
> goto err_put_free;
>
> + ret = virtio_gpu_array_prepare(vgdev, objs);
> + if (ret)
> + goto err_unlock;
> +
> fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context, 0);
> if (!fence) {
> ret = -ENOMEM;
> @@ -288,11 +292,25 @@ static int virtio_gpu_transfer_to_host_ioctl(struct
> drm_device *dev, void *data,
> goto err_put_free;
> }
>
> + ret = virtio_gpu_array_lock_resv(objs);
> + if (ret
[PATCH v18 24/26] drm/virtio: Attach shmem BOs dynamically
Prepare for addition of memory shrinker support by attaching shmem pages
to host dynamically on first use. Previously the attachment vq command
wasn't fenced and there was no vq kick made in the BO creation code path,
hence the attachment already was happening dynamically, but implicitly.
Making attachment explicitly dynamic will allow to simplify and reuse more
code when shrinker will be added. The virtio_gpu_object_shmem_init() now
works under the held reservation lock, which will be important to have for
shrinker to avoid moving pages while they are in active use by the driver.
Acked-by: Gerd Hoffmann
Signed-off-by: Dmitry Osipenko
---
drivers/gpu/drm/virtio/virtgpu_drv.h| 7 +++
drivers/gpu/drm/virtio/virtgpu_gem.c| 26 +
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 32 +++
drivers/gpu/drm/virtio/virtgpu_object.c | 73 -
drivers/gpu/drm/virtio/virtgpu_submit.c | 15 -
5 files changed, 125 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 56269814fb6d..421f524ae1de 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -89,6 +89,7 @@ struct virtio_gpu_object {
uint32_t hw_res_handle;
bool dumb;
bool created;
+ bool detached;
bool host3d_blob, guest_blob;
uint32_t blob_mem, blob_flags;
@@ -313,6 +314,8 @@ 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_prepare(struct virtio_gpu_device *vgdev,
+struct virtio_gpu_object_array *objs);
int virtio_gpu_gem_pin(struct virtio_gpu_object *bo);
void virtio_gpu_gem_unpin(struct virtio_gpu_object *bo);
@@ -453,6 +456,10 @@ int virtio_gpu_object_create(struct virtio_gpu_device
*vgdev,
bool virtio_gpu_is_shmem(struct virtio_gpu_object *bo);
+int virtio_gpu_reattach_shmem_object_locked(struct virtio_gpu_object *bo);
+
+int virtio_gpu_reattach_shmem_object(struct virtio_gpu_object *bo);
+
int virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev,
uint32_t *resid);
/* virtgpu_prime.c */
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c
b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 625c05d625bf..97e67064c97e 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -295,6 +295,26 @@ void virtio_gpu_array_put_free_work(struct work_struct
*work)
spin_unlock(&vgdev->obj_free_lock);
}
+int virtio_gpu_array_prepare(struct virtio_gpu_device *vgdev,
+struct virtio_gpu_object_array *objs)
+{
+ struct virtio_gpu_object *bo;
+ int ret = 0;
+ u32 i;
+
+ for (i = 0; i < objs->nents; i++) {
+ bo = gem_to_virtio_gpu_obj(objs->objs[i]);
+
+ if (virtio_gpu_is_shmem(bo) && bo->detached) {
+ ret = virtio_gpu_reattach_shmem_object_locked(bo);
+ if (ret)
+ break;
+ }
+ }
+
+ return ret;
+}
+
int virtio_gpu_gem_pin(struct virtio_gpu_object *bo)
{
int err;
@@ -303,6 +323,12 @@ int virtio_gpu_gem_pin(struct virtio_gpu_object *bo)
err = drm_gem_shmem_pin(&bo->base);
if (err)
return err;
+
+ err = virtio_gpu_reattach_shmem_object(bo);
+ if (err) {
+ drm_gem_shmem_unpin(&bo->base);
+ return err;
+ }
}
return 0;
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index b24b11f25197..070c29cea26a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -246,6 +246,10 @@ static int virtio_gpu_transfer_from_host_ioctl(struct
drm_device *dev,
if (ret != 0)
goto err_put_free;
+ ret = virtio_gpu_array_prepare(vgdev, objs);
+ if (ret)
+ goto err_unlock;
+
fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context, 0);
if (!fence) {
ret = -ENOMEM;
@@ -288,11 +292,25 @@ static int virtio_gpu_transfer_to_host_ioctl(struct
drm_device *dev, void *data,
goto err_put_free;
}
+ ret = virtio_gpu_array_lock_resv(objs);
+ if (ret != 0)
+ goto err_put_free;
+
+ ret = virtio_gpu_array_prepare(vgdev, objs);
+ if (ret)
+ goto err_unlock;
+
+ fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context, 0);
+ if (!fence) {
+ ret = -ENOMEM;
+ goto err_unlock;
+ }
+
if (!vgdev->has
