From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BCD556093287 for ; Mon, 2 Nov 2026 15:12:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key, unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=f2oaTzZa Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 22DDA10E63B; Mon, 2 Nov 2026 15:11:57 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.18]) by gabe.freedesktop.org (Postfix) with ESMTPS id 155BE10EF75 for ; Mon, 2 Nov 2026 15:11:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1793632311; x=1825168311; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=A1Z8YVADStasxeVGsvKlCx9NW4+VyRaCaoIqgq9WuaI=; b=f2oaTzZaWQqmk9rkZSoItVo7cUqrW8SFgTljfYBp18JZY3qyi790Ifg9 BeuGr99V9EVbDP0KBixdtm7MRg/Fuc+VXPzOXRa7HH3CmIVDTJpCx7TSi Pug2jM5ZVgayce20DLYtBTRrDZEDzzg1s/VUuQnzuUFW5XL5hrM81x/bv lps6F8WJEJHj5vWrCXp+9aBNwbGerpaRXWnKgO1GxmIa+Egc+rKyBAl3A HTub9YwLFPu8jGa9I9Tab6VMWWXJsomyhePaeI1RLYi7cMH8s5gsPPvgg osBNTd4a5L9CvH/F+CEztIGNVhFMtIL+ItVsuvNG9MINHjhOoJlydycdD w==; X-CSE-ConnectionGUID: DZ25XpI1T2KrJzhiGpSmxQ== X-CSE-MsgGUID: Im0tAPF2SfekTcpU79xS9Q== X-IronPort-AV: E=McAfee;i="6800,10657,11954"; a="2338026" X-IronPort-AV: E=Sophos;i="6.27,191,1787036400"; d="scan'208";a="2338026" Received: from fmviesa002.fm.intel.com ([10.60.135.142]) by orvoesa110.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Nov 2026 07:11:50 -0800 X-CSE-ConnectionGUID: 14KMMsgeT4qr9BW02qAynQ== X-CSE-MsgGUID: 0sF9zJAHQhaktVbw3e4vzQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.27,191,1787036400"; d="scan'208";a="6315995" Received: from hrotuna-mobl2.ger.corp.intel.com (HELO localhost) ([10.245.245.10]) by smtpauth.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Nov 2026 07:11:48 -0800 Date: Mon, 2 Nov 2026 17:11:43 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Thomas Zimmermann Cc: maarten.lankhorst@linux.intel.com, mripard@kernel.org, airlied@gmail.com, simona@ffwll.ch, javierm@redhat.com, louis.chauvet@bootlin.com, dri-devel@lists.freedesktop.org, sashiko-reviews@lists.linux.dev, stable@vger.kernel.org Subject: Re: [PATCH] drm/vblank: Restart enabling vblank timer on possible deadlock Message-ID: References: <20261102143919.1672293-1-tzimmermann@suse.de> X-Archived-By: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20261102143919.1672293-1-tzimmermann@suse.de> X-Patchwork-Hint: comment Organization: Intel Finland Oy - BIC 0357606-4 - c/o Alberga Business Park, 6 krs Bertel Jungin Aukio 5, 02600 Espoo, Finland X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "dri-devel" X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=4.0.1 X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on aws-us-west-2-korg-listserv-alma10-1 On Mon, Nov 02, 2026 at 03:38:52PM +0100, Thomas Zimmermann wrote: > Return -EDEADLK when trying to cancel a running hrtimer as part of > enable_vblank(). Reacquire vblank_time_lock and try again. > > The vblank helper drm_vblank_disable_and_save() and the vblank IRQ > handler hold the spin lock vblank_time_lock at some point. This > can result in a deadlock if vblank timer's disable code in > drm_crtc_vblank_cancel_timer() calls hrtimer_cancel() directly: the > helper wais for the timer function to complete and the timer function > waits for the helper to release vblank.time_lock. > > drm_crtc_vblank_cancel_timer() works around this problem by calling > hrtimer_try_to_cancel() instead. For re-enabling the vblank timer, > drm_crtc_vblank_start_timer() must wait for the vblank timer to stop. > It runs into the same deadlock problem while waiting for the timer > function to complete. > > The solution to this problem is to release and reacquire the spin > lock vblank_time_lock if hrtimer_try_to_cancel() signals a running > timer function. Releasing the lock allows for the timer function to > make progress. > > Signal the possible deadlock with -EDEADLK and perform the necessary > steps. No other driver uses -EDEADLK in their enable_vblank, so the > change should not interfere. > > Tested with bochs on qemu. > > Closes: https://lore.kernel.org/dri-devel/20260914111129.300691F00898@smtp.kernel.org/ > Signed-off-by: Thomas Zimmermann > Fixes: 74afeb812850 ("drm/vblank: Add vblank timer") > Cc: Thomas Zimmermann > Cc: Louis Chauvet > Cc: Javier Martinez Canillas > Cc: David Airlie > Cc: Simona Vetter > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: dri-devel@lists.freedesktop.org > Cc: # v6.19+ > --- > I also tried to move hrtimer_cancel() out of the critical section. The > result was fairly fragile. So returning an error and re-acquiring the > lock was the easier solution. > --- > drivers/gpu/drm/drm_vblank.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 5162250e8813..135efde86298 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -1182,10 +1182,12 @@ static int __enable_vblank(struct drm_device *dev, unsigned int pipe) > static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe) > { > struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe); > - int ret = 0; > + int ret; > > assert_spin_locked(&dev->vbl_lock); > > +retry: > + ret = 0; > spin_lock(&dev->vblank_time_lock); > > if (!vblank->enabled) { > @@ -1199,7 +1201,11 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe) > ret = __enable_vblank(dev, pipe); > drm_dbg_core(dev, "enabling vblank on crtc %u, ret: %d\n", > pipe, ret); > - if (ret) { > + if (ret == -EDEADLK) { > + spin_unlock(&dev->vblank_time_lock); > + cpu_relax(); > + goto retry; > + } else if (ret) { > atomic_dec(&vblank->refcount); > } else { > drm_update_vblank_count(dev, pipe, 0); > @@ -2232,11 +2238,14 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc) > CLOCK_MONOTONIC, HRTIMER_MODE_REL); > } else { > /* > - * Timer should not be active. If it is, wait for the > - * previous cancel operations to finish. > + * Timer should not be running. If it is, signal cancellation > + * again and restart to release locks. > */ > - while (hrtimer_active(&vtimer->timer)) > - hrtimer_try_to_cancel(&vtimer->timer); Why do you even have to cancel the timer here? Just let it run? > + if (hrtimer_try_to_cancel(&vtimer->timer) == -1) { > + scoped_guard(spinlock_irqsave, &vtimer->interval_lock) > + vtimer->interval = 0; Does this interval_lock serve some actual purpose? Looks like it's just protecting a single variable, and there are no read-modify-writes happening to it that I can see. > + return -EDEADLK; > + } > } > > drm_calc_timestamping_constants(crtc, &crtc->mode); > -- > 2.56.0 -- Ville Syrjälä Intel