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 6EDD5C25B0E for ; Tue, 16 Aug 2022 14:10:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233363AbiHPOKW (ORCPT ); Tue, 16 Aug 2022 10:10:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57998 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233073AbiHPOKS (ORCPT ); Tue, 16 Aug 2022 10:10:18 -0400 Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5F1981C116 for ; Tue, 16 Aug 2022 07:10:17 -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-out2.suse.de (Postfix) with ESMTPS id 0291F1F951; Tue, 16 Aug 2022 14:10:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1660659016; 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=wOfV7MBpUDVV0gdupil0HRPP+eQ90hzDFPkjycxDYpo=; b=GhJAYNnHfM3YFSZGdI2l2Eey5a8FCBCoRwXoAS5gYy1whDdAF63oRrhCiWODspYF7wRYTy cD1c7HBb0WzHtotbq8Rlfp5cqOnaS+Byzs8WNz2Bbiz3kjPD0oDVXYhLNu7AchU7CHAVR7 a2oxHRs/uXdlFSXOavGMoJ0vz4TFtjg= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1660659016; 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=wOfV7MBpUDVV0gdupil0HRPP+eQ90hzDFPkjycxDYpo=; b=xGUpQtSNtar6TBnjAK43euA2A1s/gln7qySuxpqnjEh9/oCckU/fPzZcby/BItcNiKyEUg IgiUHIQerS9dSUAg== 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 D35DE139B7; Tue, 16 Aug 2022 14:10:15 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id ddzfMkel+2KsfQAAMHmgww (envelope-from ); Tue, 16 Aug 2022 14:10:15 +0000 Date: Tue, 16 Aug 2022 16:10:15 +0200 Message-ID: <87a684xos8.wl-tiwai@suse.de> From: Takashi Iwai To: Thomas Zimmermann Cc: Takashi Iwai , 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: 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> <87fshwxph0.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, 16 Aug 2022 16:01:34 +0200, Thomas Zimmermann wrote: > > Hi Takashi > > Am 16.08.22 um 15:55 schrieb Takashi Iwai: > > 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. > > I already merged the patches before seeing the discussion on the rsp > bug report. If you submit an update, maybe you can do so with Fixes > tags? Oh sure, will check then. Takashi