| Message ID | 20240822172338.698922-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/virtio: Defer the host dumb buffer creation | expand |
> +static void 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.width = mode_cmd->width; > + params.height = mode_cmd->height; > + params.size = params.height * params.width * 4; > + params.size = ALIGN(params.size, PAGE_SIZE); > + params.dumb = true; I'd suggest to simply store the complete virtio_gpu_object_params struct in virtio_gpu_object instead of re-creating it here. > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h > index 64c236169db88..8d1e8dcfa8c15 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_drv.h > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h > @@ -93,6 +93,9 @@ struct virtio_gpu_object { > bool dumb; > bool created; > bool host3d_blob, guest_blob; > + bool deferred; > + struct virtio_gpu_mem_entry *ents; > + unsigned int nents; > uint32_t blob_mem, blob_flags; Put them into a new block separated by newline, add a comment saying those are needed for virtio_gpu_deferred_create? > @@ -229,9 +231,14 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev, > 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); > + /* Fence is used only with blob or virgl */ > + WARN_ONCE(fence != NULL, "deferred doesn't support fence\n"); Additionally check for param->dumb to take the deferred code path? That should make sure there is no fence. take care, Gerd
On 23/08/2024 09:04, Gerd Hoffmann wrote: >> +static void 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.width = mode_cmd->width; >> + params.height = mode_cmd->height; >> + params.size = params.height * params.width * 4; >> + params.size = ALIGN(params.size, PAGE_SIZE); >> + params.dumb = true; > > I'd suggest to simply store the complete virtio_gpu_object_params struct > in virtio_gpu_object instead of re-creating it here. The struct params is much bigger than the struct virtio_gpu_object, so I though it would waste too much memory. Using a pointer would add an alloc/free pair, and a potential source of memleak. And as we have the required parameters in struct drm_mode_fb_cmd2, I found it better this way. > >> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h >> index 64c236169db88..8d1e8dcfa8c15 100644 >> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h >> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h >> @@ -93,6 +93,9 @@ struct virtio_gpu_object { >> bool dumb; >> bool created; >> bool host3d_blob, guest_blob; >> + bool deferred; >> + struct virtio_gpu_mem_entry *ents; >> + unsigned int nents; >> uint32_t blob_mem, blob_flags; > > Put them into a new block separated by newline, add a comment saying > those are needed for virtio_gpu_deferred_create? Yes, I will do that in v2 > >> @@ -229,9 +231,14 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev, >> 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); >> + /* Fence is used only with blob or virgl */ >> + WARN_ONCE(fence != NULL, "deferred doesn't support fence\n"); > > Additionally check for param->dumb to take the deferred code path? That > should make sure there is no fence. I think I can also duplicate virtio_gpu_object_create(), and have one version only for deferred dumb buffer. I will see if that doesn't make too much code duplication. > > take care, > Gerd > Thanks for the review,
On Fri, Aug 23, 2024 at 05:35:39PM GMT, Jocelyn Falempe wrote: > On 23/08/2024 09:04, Gerd Hoffmann wrote: > > > +static void 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.width = mode_cmd->width; > > > + params.height = mode_cmd->height; > > > + params.size = params.height * params.width * 4; > > > + params.size = ALIGN(params.size, PAGE_SIZE); > > > + params.dumb = true; > > > > I'd suggest to simply store the complete virtio_gpu_object_params struct > > in virtio_gpu_object instead of re-creating it here. > > The struct params is much bigger than the struct virtio_gpu_object, so I > though it would waste too much memory. Using a pointer would add an > alloc/free pair, and a potential source of memleak. And as we have the > required parameters in struct drm_mode_fb_cmd2, I found it better this way. You have to sanity-check the size then. take care, Gerd
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index 64baf2f22d9f0..3f18ee0fd5155 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -290,6 +290,26 @@ static int vgdev_output_init(struct virtio_gpu_device *vgdev, int index) return 0; } +static void 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.width = mode_cmd->width; + params.height = mode_cmd->height; + params.size = params.height * params.width * 4; + params.size = ALIGN(params.size, PAGE_SIZE); + params.dumb = true; + + 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; +} + static struct drm_framebuffer * virtio_gpu_user_framebuffer_create(struct drm_device *dev, struct drm_file *file_priv, @@ -297,6 +317,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 +330,10 @@ 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) + virtio_gpu_deferred_create(bo, vgdev, mode_cmd); + 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..8d1e8dcfa8c15 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.h +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h @@ -93,6 +93,9 @@ struct virtio_gpu_object { bool dumb; bool created; bool host3d_blob, guest_blob; + bool deferred; + struct virtio_gpu_mem_entry *ents; + unsigned int nents; uint32_t blob_mem, blob_flags; int uuid_state; 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); 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..507a90681a779 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); @@ -229,9 +231,14 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev, 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); + /* Fence is used only with blob or virgl */ + WARN_ONCE(fence != NULL, "deferred doesn't support fence\n"); + /* 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; } *bo_ptr = bo;
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@redhat.com> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/virtio/virtgpu_display.c | 26 ++++++++++++++++++++++++ drivers/gpu/drm/virtio/virtgpu_drv.h | 3 +++ drivers/gpu/drm/virtio/virtgpu_gem.c | 1 - drivers/gpu/drm/virtio/virtgpu_object.c | 13 +++++++++--- 4 files changed, 39 insertions(+), 4 deletions(-) base-commit: 04b5b362bc2a36f1dfe5cad52c83b1ea9d25b87c