[PATCH v2 1/2] drm/virtio: Defer the host dumb buffer creation
Jocelyn Falempe
jfalempe at redhat.com
Wed Sep 11 08:04:26 UTC 2024
On 11/09/2024 00:09, Dmitry Osipenko wrote:
> On 9/3/24 10:48, Jocelyn Falempe wrote:
>> The host dumb buffer command needs a format, but the
>> DRM_IOCTL_MODE_CREATE_DUMB only provides a buffer size.
>> So wait for the DRM_IOCTL_MODE_ADDFB(2), to get the format, and create
>> the host resources at this time.
>>
>> This will allow virtio-gpu to support multiple pixel formats.
>>
>> This problem was noticed in commit 42fd9e6c29b39 ("drm/virtio: fix
>> DRM_FORMAT_* handling")
>>
>> Suggested-by: Gerd Hoffmann <kraxel at redhat.com>
>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
>> ---
>>
>> v2:
>> * Move virtio_gpu_object deferred field to its own block (Geerd Hoffmann)
>> * Check that the size of the dumb buffer can hold the framebuffer (Geerd Hoffmann)
>>
>> drivers/gpu/drm/virtio/virtgpu_display.c | 33 ++++++++++++++++++++++++
>> drivers/gpu/drm/virtio/virtgpu_drv.h | 5 ++++
>> drivers/gpu/drm/virtio/virtgpu_gem.c | 1 -
>> drivers/gpu/drm/virtio/virtgpu_object.c | 13 +++++++---
>> 4 files changed, 47 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
>> index 64baf2f22d9f0..5e8ca742c6d00 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
>> @@ -290,6 +290,30 @@ static int vgdev_output_init(struct virtio_gpu_device *vgdev, int index)
>> return 0;
>> }
>>
>> +static int virtio_gpu_deferred_create(struct virtio_gpu_object *bo,
>> + struct virtio_gpu_device *vgdev,
>> + const struct drm_mode_fb_cmd2 *mode_cmd)
>> +{
>> + struct virtio_gpu_object_params params = { 0 };
>> +
>> + params.format = virtio_gpu_translate_format(mode_cmd->pixel_format);
>> + params.dumb = true;
>> + params.width = mode_cmd->width;
>> + params.height = mode_cmd->height;
>> + params.size = params.height * params.width * 4;
>> + params.size = ALIGN(params.size, PAGE_SIZE);
>> +
>> + if (params.size > bo->base.base.size)
>> + return -EINVAL;
>> +
>> + virtio_gpu_cmd_create_resource(vgdev, bo, ¶ms, NULL, NULL);
>> + virtio_gpu_object_attach(vgdev, bo, bo->ents, bo->nents);
>> +
>> + bo->deferred = false;
>> + bo->ents = NULL;
>> + return 0;
>> +}
>> +
>> static struct drm_framebuffer *
>> virtio_gpu_user_framebuffer_create(struct drm_device *dev,
>> struct drm_file *file_priv,
>> @@ -297,6 +321,8 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev,
>> {
>> struct drm_gem_object *obj = NULL;
>> struct virtio_gpu_framebuffer *virtio_gpu_fb;
>> + struct virtio_gpu_device *vgdev = dev->dev_private;
>> + struct virtio_gpu_object *bo;
>> int ret;
>>
>> if (mode_cmd->pixel_format != DRM_FORMAT_HOST_XRGB8888 &&
>> @@ -308,6 +334,13 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev,
>> if (!obj)
>> return ERR_PTR(-EINVAL);
>>
>> + bo = gem_to_virtio_gpu_obj(obj);
>> + if (bo->deferred) {
>> + ret = virtio_gpu_deferred_create(bo, vgdev, mode_cmd);
>> + if (ret)
>> + return ERR_PTR(ret);
>> + }
>> +
>> virtio_gpu_fb = kzalloc(sizeof(*virtio_gpu_fb), GFP_KERNEL);
>> if (virtio_gpu_fb == NULL) {
>> drm_gem_object_put(obj);
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
>> index 64c236169db88..4302933e5067c 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
>> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
>> @@ -95,6 +95,11 @@ struct virtio_gpu_object {
>> bool host3d_blob, guest_blob;
>> uint32_t blob_mem, blob_flags;
>>
>> + /* For deferred dumb buffer creation */
>> + bool deferred;
>> + struct virtio_gpu_mem_entry *ents;
>> + unsigned int nents;
>> +
>> int uuid_state;
>> uuid_t uuid;
>> };
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
>> index 7db48d17ee3a8..33ad15fed30f6 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
>> @@ -75,7 +75,6 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
>> args->size = pitch * args->height;
>> args->size = ALIGN(args->size, PAGE_SIZE);
>>
>> - params.format = virtio_gpu_translate_format(DRM_FORMAT_HOST_XRGB8888);
>
> This will break the guest blob code path in virtio_gpu_object_create(),
> AFAICT.
The params.format is not used in virtio_gpu_cmd_resource_create_3d() nor
in virtio_gpu_cmd_resource_create_blob(), it's only used for dumb buffer
creation. So it shouldn't break the guest blob path.
>
>> params.width = args->width;
>> params.height = args->height;
>> params.size = args->size;
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
>> index c7e74cf130221..b5a537a761294 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
>> @@ -67,6 +67,8 @@ 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)) {
>> + if (bo->deferred)
>> + kvfree(bo->ents);
>> drm_gem_shmem_free(&bo->base);
>> } else if (virtio_gpu_is_vram(bo)) {
>> struct virtio_gpu_object_vram *vram = to_virtio_gpu_vram(bo);
>> @@ -228,10 +230,13 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
>> virtio_gpu_cmd_resource_create_3d(vgdev, bo, params,
>> objs, fence);
>> virtio_gpu_object_attach(vgdev, bo, ents, nents);
>> - } else {
>> - virtio_gpu_cmd_create_resource(vgdev, bo, params,
>> - objs, fence);
>> - virtio_gpu_object_attach(vgdev, bo, ents, nents);
>> + } else if (params->dumb) {
>> + /* Create the host resource in virtio_gpu_user_framebuffer_create()
>> + * because the pixel format is not specified yet
>> + */
>> + bo->ents = ents;
>> + bo->nents = nents;
>> + bo->deferred = true;
>> }
>
> AFAICS, the "params.dumb = true" should be set in
> virtio_gpu_mode_dumb_create() and not in virtio_gpu_deferred_create().
> Was this patch tested?
You're right that params.dumb is not used in
virtio_gpu_cmd_create_resource() so it's useless to set it in
virtio_gpu_deferred_create(). I can remove that line.
But it's also set in virtio_gpu_mode_dumb_create() and I didn't change
that with this patch.
I'm testing on a s390x machine, and it allows wayland compositor to
work. (The colors are still a mess, but at least it starts with this patch).
>
> Overall, this deferred resource creation doesn't look robust. Could be
> better to either add SET_SCANOUT2 with the format info or add cmd that
> overrides the res fmt.
>
This would also need modification in the qemu code, so it looks much
more complex than this patch.
Best regards,
--
Jocelyn
More information about the dri-devel
mailing list