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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id CE28BC25B0E for ; Tue, 16 Aug 2022 13:55:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233632AbiHPNzj (ORCPT ); Tue, 16 Aug 2022 09:55:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37408 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235871AbiHPNzc (ORCPT ); Tue, 16 Aug 2022 09:55:32 -0400 Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 60B8A7C1B4 for ; Tue, 16 Aug 2022 06:55:25 -0700 (PDT) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id F2E4C3458A; Tue, 16 Aug 2022 13:55:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1660658123; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=uZ1rhP5h+o+pAIkaqiAfxqHKk/DnvqzaGpIhfBTwdSk=; b=ZJkccRT2TYgphKsNtn16YFOKH8VqK+/TzxCnRlbpEt4db+J6fxEF8OywuP3HooFGsVddB7 /tqrYYD1CTvA6EQPPvH0rD44tHxMk93Lkjo4IGDNmqyDSdBovD54K163g5FEPL7HL57ARY ZjI52vZfSIuSqxSU6c0RLo/ffm/z19w= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1660658124; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=uZ1rhP5h+o+pAIkaqiAfxqHKk/DnvqzaGpIhfBTwdSk=; b=k3NpvueGl1OZf71ck/Cbnuaxb7FBdBcrGqzhAhaMT8l+B/nkpQ8z+siVEIIrwtLY7xYQqm dRBpXeyrPjo8JLBA== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id D2FAC139B7; Tue, 16 Aug 2022 13:55:23 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id gcq7Msuh+2KadgAAMHmgww (envelope-from ); Tue, 16 Aug 2022 13:55:23 +0000 Date: Tue, 16 Aug 2022 15:55:23 +0200 Message-ID: <87fshwxph0.wl-tiwai@suse.de> From: Takashi Iwai To: Thomas Zimmermann Cc: Dave Airlie , Sean Paul , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect In-Reply-To: <875yj1wz8d.wl-tiwai@suse.de> References: <20220804075826.27036-1-tiwai@suse.de> <20220804075826.27036-4-tiwai@suse.de> <87h72lx4yw.wl-tiwai@suse.de> <2a307221-62a8-a5f8-354f-d92e90f74f04@suse.de> <87a68dwzzi.wl-tiwai@suse.de> <32d98960-a952-b3ab-c3fb-0d615626a3d1@suse.de> <875yj1wz8d.wl-tiwai@suse.de> User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0 MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 09 Aug 2022 11:19:30 +0200, Takashi Iwai wrote: > > On Tue, 09 Aug 2022 11:13:46 +0200, > Thomas Zimmermann wrote: > > > > Hi > > > > Am 09.08.22 um 11:03 schrieb Takashi Iwai: > > > On Tue, 09 Aug 2022 09:41:19 +0200, > > > Thomas Zimmermann wrote: > > >> > > >> Hi > > >> > > >> Am 09.08.22 um 09:15 schrieb Takashi Iwai: > > >>> On Tue, 09 Aug 2022 09:13:16 +0200, > > >>> Thomas Zimmermann wrote: > > >>>> > > >>>> Hi > > >>>> > > >>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai: > > >>>>> At both suspend and disconnect, we should rather cancel the pending > > >>>>> URBs immediately. For the suspend case, the display will be turned > > >>>>> off, so it makes no sense to process the rendering. And for the > > >>>>> disconnect case, the device may be no longer accessible, hence we > > >>>>> shouldn't do any submission. > > >>>>> > > >>>>> Tested-by: Thomas Zimmermann > > >>>>> Signed-off-by: Takashi Iwai > > >>>>> --- > > >>>>> drivers/gpu/drm/udl/udl_drv.h | 2 ++ > > >>>>> drivers/gpu/drm/udl/udl_main.c | 25 ++++++++++++++++++++++--- > > >>>>> drivers/gpu/drm/udl/udl_modeset.c | 2 ++ > > >>>>> 3 files changed, 26 insertions(+), 3 deletions(-) > > >>>>> > > >>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h > > >>>>> index f01e50c5b7b7..28aaf75d71cf 100644 > > >>>>> --- a/drivers/gpu/drm/udl/udl_drv.h > > >>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h > > >>>>> @@ -39,6 +39,7 @@ struct urb_node { > > >>>>> struct urb_list { > > >>>>> struct list_head list; > > >>>>> + struct list_head in_flight; > > >>>>> spinlock_t lock; > > >>>>> wait_queue_head_t sleep; > > >>>>> int available; > > >>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev) > > >>>>> int udl_submit_urb(struct drm_device *dev, struct urb *urb, > > >>>>> size_t len); > > >>>>> int udl_sync_pending_urbs(struct drm_device *dev); > > >>>>> +void udl_kill_pending_urbs(struct drm_device *dev); > > >>>>> void udl_urb_completion(struct urb *urb); > > >>>>> int udl_init(struct udl_device *udl); > > >>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c > > >>>>> index 93615648414b..47204b7eb10e 100644 > > >>>>> --- a/drivers/gpu/drm/udl/udl_main.c > > >>>>> +++ b/drivers/gpu/drm/udl/udl_main.c > > >>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb) > > >>>>> urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */ > > >>>>> spin_lock_irqsave(&udl->urbs.lock, flags); > > >>>>> - list_add_tail(&unode->entry, &udl->urbs.list); > > >>>>> + list_move(&unode->entry, &udl->urbs.list); > > >>>>> udl->urbs.available++; > > >>>>> spin_unlock_irqrestore(&udl->urbs.lock, flags); > > >>>>> @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct > > >>>>> drm_device *dev, int count, size_t size) > > >>>>> retry: > > >>>>> udl->urbs.size = size; > > >>>>> INIT_LIST_HEAD(&udl->urbs.list); > > >>>>> + INIT_LIST_HEAD(&udl->urbs.in_flight); > > >>>>> init_waitqueue_head(&udl->urbs.sleep); > > >>>>> udl->urbs.count = 0; > > >>>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout) > > >>>>> } > > >>>>> unode = list_first_entry(&udl->urbs.list, struct urb_node, > > >>>>> entry); > > >>>>> - list_del_init(&unode->entry); > > >>>>> + list_move(&unode->entry, &udl->urbs.in_flight); > > >>>>> udl->urbs.available--; > > >>>>> unlock: > > >>>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev) > > >>>>> spin_lock_irq(&udl->urbs.lock); > > >>>>> /* 2 seconds as a sane timeout */ > > >>>>> if (!wait_event_lock_irq_timeout(udl->urbs.sleep, > > >>>>> - udl->urbs.available == udl->urbs.count, > > >>>>> + list_empty(&udl->urbs.in_flight), > > >>>>> udl->urbs.lock, > > >>>>> msecs_to_jiffies(2000))) > > >>>>> ret = -ETIMEDOUT; > > >>>>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev) > > >>>>> return ret; > > >>>>> } > > >>>>> +/* kill pending URBs */ > > >>>>> +void udl_kill_pending_urbs(struct drm_device *dev) > > >>>>> +{ > > >>>>> + struct udl_device *udl = to_udl(dev); > > >>>>> + struct urb_node *unode; > > >>>>> + > > >>>>> + spin_lock_irq(&udl->urbs.lock); > > >>>>> + while (!list_empty(&udl->urbs.in_flight)) { > > >>>>> + unode = list_first_entry(&udl->urbs.in_flight, > > >>>>> + struct urb_node, entry); > > >>>>> + spin_unlock_irq(&udl->urbs.lock); > > >>>>> + usb_kill_urb(unode->urb); > > >>>>> + spin_lock_irq(&udl->urbs.lock); > > >>>>> + } > > >>>>> + spin_unlock_irq(&udl->urbs.lock); > > >>>>> +} > > >>>>> + > > >>>>> int udl_init(struct udl_device *udl) > > >>>>> { > > >>>>> struct drm_device *dev = &udl->drm; > > >>>>> @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev) > > >>>>> { > > >>>>> struct udl_device *udl = to_udl(dev); > > >>>>> + udl_kill_pending_urbs(dev); > > >>>>> udl_free_urb_list(dev); > > >>>>> put_device(udl->dmadev); > > >>>>> udl->dmadev = NULL; > > >>>>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c > > >>>>> index 50025606b6ad..169110d8fc2e 100644 > > >>>>> --- a/drivers/gpu/drm/udl/udl_modeset.c > > >>>>> +++ b/drivers/gpu/drm/udl/udl_modeset.c > > >>>>> @@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe) > > >>>>> struct urb *urb; > > >>>>> char *buf; > > >>>>> + udl_kill_pending_urbs(dev); > > >>>>> + > > >>>> > > >>>> I already reviewed the patchset, but I have another comment. I think > > >>>> we should only kill urbs from within the suspend handler. Same for the > > >>>> call to the URB-sync function in patch 2. > > >>>> > > >>>> This disable function is part of the regular modeset path. It's > > >>>> probably not appropriate to outright remove pending URBs here. This > > >>>> can lead to failed modesets, which would have succeeded otherwise. > > >>> > > >>> Well, the device shall be turned off right after that point, so the > > >>> all pending rendering makes little sense, no? > > >> > > >> udl_simple_display_pipe_disable() only disables the display, but not > > >> the device. The kill operation here could potentially kill some valid > > >> modeset operation that was still going on. And who knows what the > > >> device state is after that. > > > > > > But udl_simple_display_pipe_disable() invokes UDL_BLANK_MODE_POWERDOWN > > > command right after the place I've put udl_kill_pending_urbs(). So it > > > shall blank / turn off the power (of the device, as it has a single > > > output). And the URB completion doesn't do any error handling but > > > just re-links URB chain and wakes up the queue. So killing a pending > > > URB would nothing but canceling the in-flight URBs, and there should > > > be no disturbance to the modeset operation itself, as the screen will > > > be blanked immediately. > > > > The blank mode is essentially DPMS. It's unrelated to the device's > > display mode. > > The function invokes the UDL_BLANK_MODE_POWERDOWN command; that will > discard the whole rendered picture. And, the counterpart, > udl_simple_display_pipe_enable(), re-initializes the mode fully from > the scratch again. > So what's the point to continue rendering that is immediately cleared > (from the screen and from the device state)? Killing pending URBs > doesn't influence on the internal (modeset) state of the driver. In anyway, this patchset seems problematic around the disconnection, and maybe this particular one is no much improvement, better to drop for now. I'll resubmit the v2 patch set including your resume fixes later. thanks, Takashi