[PATCH v2 1/2] virtio: New virtio_gpu driver

[Jiaxun Yang]

在2024年7月21日七月 下午6:08，Simon Glass写道：
[...]
>> >> > +
>> >> > +     set_scanout_in = malloc(sizeof(struct virtio_gpu_set_scanout));
>> >
>> > Could these structs be inside priv instead of allocating each one?
>>
>> Those structs are all used only once at initialization.
>>
>> We can save some runtime memory by freeing them here :-)
>
> Then you can just use a local var.

That will yield a huge stack frame that breaks booting on RISC-V virt board :-(

Thanks
- Jiaxun

>
>>
>> >
>> >> > +     while (scanout_mask) {
>> >> > +             u32 active_scanout = ffs(scanout_mask) - 1;
>> >> > +
>> >> > +             set_scanout_in->r.x = 0;
>> >> > +             set_scanout_in->r.y = 0;
>> >> > +             set_scanout_in->r.width = cpu_to_virtio32(dev, uc_priv->xsize);
>> >> > +             set_scanout_in->r.height = cpu_to_virtio32(dev, uc_priv->ysize);
>> >> > +             set_scanout_in->scanout_id = cpu_to_virtio32(dev, active_scanout);
>> >> > +             set_scanout_in->resource_id = cpu_to_virtio32(dev,
>> >> > priv->scanout_res_id);
>> >> > +
>
> [..]
>
>> >> > +/* data passed in the cursor vq */
>> >> > +
>> >> > +struct virtio_gpu_cursor_pos {
>> >> > +     __le32 scanout_id;
>> >> > +     __le32 x;
>> >> > +     __le32 y;
>> >> > +     __le32 padding;
>> >> > +};
>> >> > +
>> >> > +/* VIRTIO_GPU_CMD_UPDATE_CURSOR, VIRTIO_GPU_CMD_MOVE_CURSOR */
>> >> > +struct virtio_gpu_update_cursor {
>> >> > +     struct virtio_gpu_ctrl_hdr hdr;
>> >> > +     struct virtio_gpu_cursor_pos pos;  /* update & move */
>> >> > +     __le32 resource_id;           /* update only */
>> >> > +     __le32 hot_x;                 /* update only */
>> >> > +     __le32 hot_y;                 /* update only */
>> >> > +     __le32 padding;
>> >> > +};
>> >> > +
>> >> > +/* data passed in the control vq, 2d related */
>> >
>> > But there are no comments so we don't know what these structs are for
>> > or what the fields do. Can you add documentation, or a pointer to
>> > somewhere where it exists?
>>
>> The whole header is copied from Linux kernel so I’m not really sure if we want to edit it.
>>
>> Maybe I can mention about the specification at start of the driver code?
>
> Yes that would help.
>
> I thought Linux's aversion to comments had softened a little?
>
> Regards,
> Simon

-- 
- Jiaxun