On 3/7/23 13:42, Thomas Zimmermann wrote:
> 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?
There is another "VRAM" VirtIO-GPU BO type that doesn't implement the
pin/unpin callbacks. Perhaps another option was to add the callbacks.
>> + 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.
I'll switch to use drm_gem_shmem_object_pin() directly in v13, maybe add
virtio_gpu_gem_pin() helper for that. Thanks!
--
Best regards,
Dmitry
