| Message ID | 20240903075414.297622-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/virtio: Defer the host dumb buffer creation | expand |
Hi, > - } 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; > } else { return -EINVAL; } I think this should not happen, because non-dumb buffers are only created with virgl being active. Nevertheless we should catch this case and return an error, maybe even have a WARN_ONCE() there. Otherwise looks good to me, so with that fixed: Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> take care, Gerd
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@redhat.com> > Signed-off-by: Jocelyn Falempe <jfalempe@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. > 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? 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.
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@redhat.com> >> Signed-off-by: Jocelyn Falempe <jfalempe@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,
Hi, > > +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.format = virtio_gpu_translate_format(DRM_FORMAT_HOST_XRGB8888); > > This will break the guest blob code path in virtio_gpu_object_create(), > AFAICT. Good point. > > + } 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? virtio_gpu_deferred_create re-creates params from scratch, so it must set "params.dumb = true" too. That looks fine to me. > 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. If we can solve this issue with a guest-side fix I'd very much prefer that. When adding device features for that the driver will have to support both old + new devices, which I don't think will be better in the end. take care, Gerd PS: Figured how using blob resources on s390x should work (tested in emulation only). ----------------------- cut here ----------------------- --- fedora-s390x-gfx.xml 2024-09-11 11:29:57.864900808 +0200 +++ fedora-s390x-gfx-blob.xml 2024-09-11 11:29:54.662935454 +0200 @@ -1,8 +1,11 @@ <domain type='qemu'> - <name>fedora-s390x-gfx</name> - <uuid>00e1b01c-f220-4d95-9d25-cd52c4284238</uuid> + <name>fedora-s390x-gfx-blob</name> + <uuid>2dcfa930-019d-4c25-8636-65abeed741c0</uuid> <memory unit='KiB'>4194304</memory> <currentMemory unit='KiB'>4194304</currentMemory> + <memoryBacking> + <source type='memfd'/> + </memoryBacking> <vcpu placement='static'>2</vcpu> <os> <type arch='s390x' machine='s390-ccw-virtio-6.2'>hvm</type> @@ -59,7 +62,7 @@ </graphics> <audio id='1' type='none'/> <video> - <model type='virtio' heads='1' primary='yes'/> + <model type='virtio' heads='1' primary='yes' blob='on'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'> <zpci uid='0x0001' fid='0x00000000'/> </address>
On 9/11/24 11:04, Jocelyn Falempe wrote: ... >>> --- 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. Indeed >>> 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). The commit message of patch #2 says that it's fbdev that doesn't work because it uses BGRX8888, i.e. wayland should work without this patch, isn't it? Please try to find why colors are wrong, could become that we won't need this patch anymore once you'll figure out the root of the problem.
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); 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; } *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> --- 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(-) base-commit: 9345e3aab7fef06b8908308634974ea32a29e276