From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5B67BC48BDF for ; Tue, 15 Jun 2021 09:24:37 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E759B6115C for ; Tue, 15 Jun 2021 09:24:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E759B6115C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=stuge.se Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7332E89BAB; Tue, 15 Jun 2021 09:24:36 +0000 (UTC) X-Greylist: delayed 398 seconds by postgrey-1.36 at gabe; Tue, 15 Jun 2021 09:24:34 UTC Received: from foo.stuge.se (foo.stuge.se [212.116.89.98]) by gabe.freedesktop.org (Postfix) with ESMTPS id AB81489BAB for ; Tue, 15 Jun 2021 09:24:34 +0000 (UTC) Received: (qmail 27368 invoked by uid 1000); 15 Jun 2021 09:17:51 -0000 Message-ID: <20210615091751.27367.qmail@stuge.se> Date: Tue, 15 Jun 2021 09:17:51 +0000 From: Peter Stuge To: Noralf =?utf-8?Q?Tr=C3=B8nnes?= Subject: Re: [PATCH 2/2] drm/gud: Use scatter-gather USB bulk transfer References: <20210329180120.27380-1-noralf@tronnes.org> <20210329180120.27380-2-noralf@tronnes.org> <0c688720-08d5-452a-31d1-db5020075d23@tronnes.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0c688720-08d5-452a-31d1-db5020075d23@tronnes.org> X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "open list:DRM PANEL DRIVERS" Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Hi Noralf, Noralf Trønnes wrote: > >> +static int gud_usb_bulk(struct gud_device *gdrm, size_t len) .. > >> + 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; .. > > Mention in the commit message that sending USB bulk transfers with > > an sglist could be unstable Can you explain a bit about /how/ it is unstable? As you write, usb_bulk_msg() (as used before) has a timeout which is passed to the host controller hardware and implemented there. I haven't used SG with kernel USB but I would expect such a timeout to still be available with SG? > 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. The device not responding to bulk packets scheduled and sent by the host is a real error /in the device/ and thus not neccessarily something the kernel must handle gracefully.. I think it's quite nice to do so, but one can argue that it's not strictly required. But more importantly: Remember that bulk transfer has no delivery time guarantee. It can take indefinitely long until a bulk transfer is scheduled by the host on a busy bus which is starved with more important things (control, interrupt, iso transfers) - that's not an error at all, and may be indistinguishable from the device not responding to packets actually sent by the host. Having a timeout is important, I just expect the USB SG interface to support it since it is the hardware that times out in the non-SG case. And since this is essentially real time data maybe a shorter timeout is better? 3 seconds seems really long. The timeout must include all latency for a frame, so e.g. 16ms (60 Hz) is too short for sure. But maybe something like 500ms? //Peter