by Thomas Zimmermann

[permalink] [raw]

Subject: Re: [PATCH v12 10/11] drm/virtio: Support memory shrinking

Hi

Am 05.03.23 um 23:10 schrieb Dmitry Osipenko:
[...]
>
> *bo_ptr = bo;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index 4c09e313bebc..3f21512ff153 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -238,20 +238,32 @@ 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))
> +
> + if (virtio_gpu_is_shmem(bo)) {

Not really a problem with this patchset, but having such branches looks
like a bug in the driver's GEM design. Whatever your GEM object needs or
does, it should be hidden in the implementation. Why is virtio doing this?

> + err = drm_gem_pin(&bo->base.base);

As the driver uses GEM SHMEM, please call drm_gem_shmem_object_pin()
directly and remove patch [09/11]. Same goes for the _unpin call below.

The problem with generic pinning interfaces is that there's no way of
specifying where to pin to BO. The problem is most apparent with TTM,
where hardware often has multiple locations were buffer can be placed
(VRAM, GART, system memory). So it's really a detail between the driver
and the memory manager.

These generic internal GEM interfaces should only be used by DRM core
and helpers. Drivers should use their memory manager's interface.

Best regards
Thomas

> + 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) {
> + if (virtio_gpu_is_shmem(bo))
> + drm_gem_unpin(&bo->base.base);
> +
> return -ENOMEM;
> + }
> }
>
> return 0;
> @@ -261,15 +273,21 @@ static void virtio_gpu_plane_cleanup_fb(struct drm_plane *plane,
> struct drm_plane_state *state)
> {
> struct virtio_gpu_framebuffer *vgfb;
> + struct virtio_gpu_object *bo;
>
> if (!state->fb)
> return;
>
> vgfb = to_virtio_gpu_framebuffer(state->fb);
> + bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
> +
> if (vgfb->fence) {
> dma_fence_put(&vgfb->fence->f);
> vgfb->fence = NULL;
> }
> +
> + if (virtio_gpu_is_shmem(bo))
> + drm_gem_unpin(&bo->base.base);
> }
>
> 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 b1a00c0c25a7..14ab470f413a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -545,6 +545,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,
> @@ -645,6 +660,23 @@ virtio_gpu_cmd_resource_attach_backing(struct virtio_gpu_device *vgdev,
> virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, fence);
> }
>
> +static void
> +virtio_gpu_cmd_resource_detach_backing(struct virtio_gpu_device *vgdev,
> + u32 resource_id,
> + struct virtio_gpu_fence *fence)
> +{
> + struct virtio_gpu_resource_attach_backing *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_DETACH_BACKING);
> + cmd_p->resource_id = cpu_to_le32(resource_id);
> +
> + virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, fence);
> +}
> +
> static void virtio_gpu_cmd_get_display_info_cb(struct virtio_gpu_device *vgdev,
> struct virtio_gpu_vbuffer *vbuf)
> {
> @@ -1107,6 +1139,14 @@ void virtio_gpu_object_attach(struct virtio_gpu_device *vgdev,
> ents, nents, NULL);
> }
>
> +void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev,
> + struct virtio_gpu_object *obj,
> + struct virtio_gpu_fence *fence)
> +{
> + virtio_gpu_cmd_resource_detach_backing(vgdev, obj->hw_res_handle,
> + fence);
> +}
> +
> void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev,
> struct virtio_gpu_output *output)
> {
> diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
> index 7b158fcb02b4..9fb38ad16120 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
> @@ -197,6 +198,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
> @@ -247,6 +257,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

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachments:

OpenPGP_signature (840.00 B)
OpenPGP digital signature

2023-03-07 10:45:07

On 3/7/23 21:25, Dmitry Osipenko wrote:
>> Not really a problem with this patchset, but having such branches looks
>> like a bug in the driver's GEM design. Whatever your GEM object needs or
>> does, it should be hidden in the implementation. Why is virtio doing this?
> There is another "VRAM" VirtIO-GPU BO type that doesn't implement the
> pin/unpin callbacks. Perhaps another option was to add the callbacks.

Although, the pin/unpin are optional. So yes, there was no need for the
extra branch, good catch.

--
Best regards,
Dmitry