Hi Noralf,
On Mon, Mar 29, 2021 at 8:01 PM Noralf Trønnes <noralf@xxxxxxxxxxx> wrote:
> There'a limit to how big a kmalloc buffer can be, and as memory gets
> fragmented it becomes more difficult to get big buffers. The downside of
> smaller buffers is that the driver has to split the transfer up which
> hampers performance. Compression might also take a hit because of the
> splitting.
>
> Solve this by allocating the transfer buffer using vmalloc and create a
> SG table to be passed on to the USB subsystem. vmalloc_32() is used to
> avoid DMA bounce buffers on USB controllers that can only access 32-bit
> addresses.
>
> This also solves the problem that split transfers can give host side
> tearing since flushing is decoupled from rendering.
>
> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> + num_pages = PAGE_ALIGN(gdrm->bulk_len) >> PAGE_SHIFT;
Isn't it the same to write:
num_pages = round_up(gdrm->bulk_len, PAGE_SIZE)?
Slightly easier to read IMO.
> + if (max_buffer_size > SZ_64M)
> + max_buffer_size = SZ_64M; /* safeguard */
Explain this choice of max buffer in the commit message
or as a comment please because I don't get why this size
is the roof.
> +struct gud_usb_bulk_context {
> + struct timer_list timer;
> + struct usb_sg_request sgr;
> +};
> +
> +static void gud_usb_bulk_timeout(struct timer_list *t)
> +{
> + struct gud_usb_bulk_context *timer = from_timer(timer, t, timer);
> +
> + usb_sg_cancel(&timer->sgr);
Error message here? "Timeout on sg bulk transfer".
> +}
> +
> +static int gud_usb_bulk(struct gud_device *gdrm, size_t len)
> +{
> + struct gud_usb_bulk_context ctx;
> + int ret;
> +
> + ret = usb_sg_init(&ctx.sgr, gud_to_usb_device(gdrm), gdrm->bulk_pipe, 0,
> + gdrm->bulk_sgt.sgl, gdrm->bulk_sgt.nents, len, GFP_KERNEL);
> + if (ret)
> + return ret;
> +
> + timer_setup_on_stack(&ctx.timer, gud_usb_bulk_timeout, 0);
> + mod_timer(&ctx.timer, jiffies + msecs_to_jiffies(3000));
> +
> + usb_sg_wait(&ctx.sgr);
> +
> + if (!del_timer_sync(&ctx.timer))
> + ret = -ETIMEDOUT;
> + else if (ctx.sgr.status < 0)
> + ret = ctx.sgr.status;
> + else if (ctx.sgr.bytes != len)
> + ret = -EIO;
> +
> + destroy_timer_on_stack(&ctx.timer);
> +
> + return ret;
> +}
Mention in the commit message that sending USB bulk transfers with
an sglist could be unstable so you set up a timeout around
usb_sg_wait() (did this happen to you? then write that)
The other users of usb_sg_wait() in the kernel do not have these
timeout wrappers, I suspect the reasoning is something like
"it's graphics, not storage, so if we timeout and lose an update,
too bad but let's just continue hoping the lost graphics will be
less than noticeable" so then we should write that as a comment
about that in the code or something.
With these comments fixed up:
Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
Yours,
Linus Walleij
