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
