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

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 11/19/24 13:21, Jocelyn Falempe wrote:
...
>> In the virtio_panic_flush() below,
>> virtio_gpu_panic_cmd_transfer_to_host_2d() is invoked only for dumb BOs.
>> Thus, only dumb BO supports panic output and should be accepted by
>> get_scanout_buffer(), other should be rejected with ENODEV here, AFAICT.
>> Correct?
> 
> Yes, it's correct

Please fix in v5.

...
>>> +struct virtio_gpu_panic_object_array {
>>> +    struct ww_acquire_ctx ticket;
>>> +    struct list_head next;
>>> +    u32 nents, total;
>>> +    struct drm_gem_object *objs;
>>> +};
>>
>> This virtio_gpu_panic_object_array struct is a hack, use
>> virtio_gpu_array_alloc(). Maybe add atomic variant of the array_alloc().
> 
> We can't allocate memory in the panic handler, but maybe it can be pre-
> allocated, like the virtio_panic_buffer ?

We certainly can allocate memory from any context using GFP_ATOMIC flag
for the allocation. You already doing that in
virtio_gpu_panic_queue_ctrl_sgs().

>>> +static void virtio_gpu_panic_put_vbuf(struct virtqueue *vq,
>>> +                      struct virtio_gpu_vbuffer *vbuf,
>>> +                      struct virtio_gpu_object_array *objs)
>>> +{
>>> +    unsigned int len;
>>> +    int i;
>>> +
>>> +    /* waiting vbuf to be used */
>>> +    for (i = 0; i < 500; i++) {
>>> +        if (vbuf == virtqueue_get_buf(vq, &len)) {
>>
>> Is it guaranteed that virtio_gpu_dequeue_ctrl_func() never runs in
>> parallel here?
> 
> Yes, in the panic handler, only one CPU remains active, and no other
> task can be scheduled.

Awesome. thanks for the clarification.

...
>>> +    ret = virtio_gpu_panic_resource_flush(plane, vbuf_resource_flush,
>>> +                          plane->state->src_x >> 16,
>>> +                          plane->state->src_y >> 16,
>>> +                          plane->state->src_w >> 16,
>>> +                          plane->state->src_h >> 16);
>>> +    if (ret) {
>>> +        virtio_gpu_panic_put_vbuf(vgdev->ctrlq.vq,
>>> +                      vbuf_dumb_bo,
>>> +                      (struct virtio_gpu_object_array *)&objs);
>>
>> The virtio_gpu_panic_notify() hasn't been invoked here, thus this
>> put_vbuf should always time out because vq hasn't been kicked. Again,
>> best to leak resources on error than to have broken/untested error
>> handling code paths.
> 
> agreed

Please change it in v5.

...
>> If there is more than one virtio-gpu display, then this panic buffer is
>> reused by other displays. It seems to work okay, but potential
>> implications are unclear. Won't it be more robust to have a panic buffer
>> per CRTC?
> 
> The drm panic handler call each plane sequentially, so it's not a
> problem. Having one buffer per CRTC would just add more complexity.

Please add a clarifying comment for that to the code.

-- 
Best regards,
Dmitry