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.
--
Best regards,
Dmitry
