Re: [PATCH 2/2] drm/gud: Use scatter-gather USB bulk transfer

["Noralf Trønnes" <noralf@tronnes.org>]

Den 14.06.2021 22.54, skrev Linus Walleij:
> Hi Noralf,
> 
> On Mon, Mar 29, 2021 at 8:01 PM Noralf Trønnes <noralf@tronnes.org> 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@tronnes.org>
> 
>> +       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.
> 

Yes it's the same, I just copied this from elsewhere in the kernel where
a vmalloc buffer is turned into an sg list. I can change that.

>> +       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".
> 

A timeout is detected in gud_usb_bulk() which will return -ETIMEDOUT if
the timer did fire. gud_flush_work() will print an error message.

>> +}
>> +
>> +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.
> 

There are 5 users of usb_sg_wait() in the kernel:
drivers/input/touchscreen/sur40.c
drivers/misc/cardreader/rtsx_usb.c
drivers/mmc/host/vub300.c
drivers/usb/misc/usbtest.c
drivers/usb/storage/transport.c

3 of those wrap it in a timer:
drivers/misc/cardreader/rtsx_usb.c: rtsx_usb_bulk_transfer_sglist()
drivers/mmc/host/vub300.c: __command_write_data()
drivers/usb/misc/usbtest.c: perform_sglist()

And it looks to me like usb/storage has some timeout handling through
the scsi layer:
/drivers/usb/storage/scsiglue.c: command_abort() ->
usb_stor_stop_transport() -> usb_sg_cancel()

This leaves 1 out of 5 users without timeout handling?

usb_bulk_msg() has builtin timeout handling and during development of a
microcontroller gadget implementation I've triggered this timeout
several times when the uC usb interrupts stopped firing.

I can add a comment in the commit message about the timer.

Noralf.

> With these comments fixed up:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Yours,
> Linus Walleij
>