Re: [PATCH v5] drm/virtio: Add drm_panic support

[Jocelyn Falempe <jfalempe@redhat.com>]

On 02/12/2024 17:19, Dmitry Osipenko wrote:
> On 12/2/24 17:29, Dmitry Osipenko wrote:
>> On 11/29/24 15:24, Ryosuke Yasuoka wrote:
>> ....
>>> +static int virtio_drm_get_scanout_buffer(struct drm_plane *plane,
>>> +					 struct drm_scanout_buffer *sb)
>>> +{
>>> +	struct virtio_gpu_object *bo;
>>> +
>>> +	if (!plane->state || !plane->state->fb || !plane->state->visible)
>>> +		return -ENODEV;
>>> +
>>> +	bo = gem_to_virtio_gpu_obj(plane->state->fb->obj[0]);
>>> +	if (virtio_gpu_is_vram(bo))
>>> +		return -ENODEV;
>>
>> VirtIO-GPU now supports importing external dmabufs, we should reject
>> bo->base.base.import_attach here now too.
>>
>>> +
>>> +	/* try to vmap it if possible */
>>> +	if (!bo->base.vaddr) {
>>> +		int ret;
>>> +
>>> +		ret = drm_gem_shmem_vmap(&bo->base, &sb->map[0]);
>>> +		if (ret)
>>> +			return ret;
>>
>> I've now noticed that drm_gem_shmem_vmap() expects BO reservation lock
>> to be held and we can't take lock it at a panic time.
>>
>> https://elixir.bootlin.com/linux/v6.12.1/source/drivers/gpu/drm/drm_gem_shmem_helper.c#L330
>>
>> This resv warning isn't triggered because bo->base.vaddr is set for VT
>> framebufffer BO when panic happens.

I usually test using gnome desktop in the VM, and in this case vaddr is 
not set, and calling vmap() is needed. (and I don't get the warning, I 
will check why).
>>
>> We actually should reject all BOs that don't have bo->base.vaddr set at
>> the panic time. Please correct it in v6 and rebase on top of a recent
>> drm-next tree.

The lock is there to prevent race conditions between concurrent tasks. 
In the panic handler, no other tasks can run in parallel. Also the 
buffer object is the one currently displayed, so it can't be in the 
middle of a free/resize or move operation. So I think it's safe to call 
vmap() from the panic handler.

> 
> Think ideally we need to have a pre-allocated and pre-mapped BO. Then
> when panic happens, use that BO and tell host to display it, i.e. not to
> reuse currently displayed BO. This will make panic display work in all
> conditions. WDYT?
> 

This means a full framebuffer will be allocated only for the panic use 
case. For GPU with a small amount of VRAM, it's not possible. But 
virtio-gpu is a bit special as it uses system RAM, so it might be less 
problematic to do that.

An alternate solution would be to make sure the framebuffer is vmapped 
(at least if it's shmem) so the panic handler won't need to call vmap().

Best regards,

-- 

Jocelyn



-- 

Jocelyn