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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 42F03C54E58 for ; Mon, 11 Mar 2024 13:59:45 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 46D09112AA2; Mon, 11 Mar 2024 13:59:44 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=collabora.com header.i=@collabora.com header.b="nmi29W0p"; dkim-atps=neutral Received: from madrid.collaboradmins.com (madrid.collaboradmins.com [46.235.227.194]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5BE67112A9F for ; Mon, 11 Mar 2024 13:59:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1710165581; bh=8JQNJ5clFuukJR4cfjmw93POF0TTVVw1V0s7AhKaLTc=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=nmi29W0pHIRtCvoe2r5cGUZT8I9pQH6Nms4lrIFn8iuV2CGjtpSeeekjU6KDFnER5 l4XPGsNQ094EIrUZuMIQFYOf6/vRE2KHhVZ5SniZuxMgDGypTrK71IM4ivCZYT9HZh Yh9bQDrWIhEAEj4FLNbHWyztW5MB2gUTQZJvVpVkucGt7mp4bHJPceZBTSqLKhb53L CRBNzYqdEH2x2crbNq9c0x2G+xhQUH7HyFD8IpBiHYBvksLBZ3vB7teRf/3MrLQ2qC mSYv5Bo3Dn0n21i5PADThJV0AnmEhJDZNExSBli9+YzlATx0tyGHyo85J8sC4hLeMm tN8YC0nCESkfA== Received: from localhost (cola.collaboradmins.com [195.201.22.229]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by madrid.collaboradmins.com (Postfix) with ESMTPSA id B7F023782017; Mon, 11 Mar 2024 13:59:40 +0000 (UTC) Date: Mon, 11 Mar 2024 14:59:39 +0100 From: Boris Brezillon To: Steven Price Cc: Robin Murphy , Jani Nikula , Liviu Dudau , dri-devel@lists.freedesktop.org, kernel@collabora.com, kernel test robot Subject: Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue Message-ID: <20240311145939.2f529a3f@collabora.com> In-Reply-To: <20240311145837.4254b504@collabora.com> References: <20240304090812.3941084-1-boris.brezillon@collabora.com> <20240304090812.3941084-4-boris.brezillon@collabora.com> <87il1tt4f6.fsf@intel.com> <20240311124634.2ee63052@collabora.com> <87frwxt2cb.fsf@intel.com> <20240311125259.6bf317a5@collabora.com> <20240311142236.05139059@collabora.com> <6bb76bfe-2be8-4724-a5db-34a779e571ad@arm.com> <4f4552fe-e2b7-44d0-9d46-448be908472c@arm.com> <20240311145837.4254b504@collabora.com> Organization: Collabora X-Mailer: Claws Mail 4.2.0 (GTK 3.24.38; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Mon, 11 Mar 2024 14:58:37 +0100 Boris Brezillon wrote: > On Mon, 11 Mar 2024 13:46:23 +0000 > Steven Price wrote: >=20 > > On 11/03/2024 13:36, Robin Murphy wrote: =20 > > > On 2024-03-11 1:22 pm, Boris Brezillon wrote: =20 > > >> On Mon, 11 Mar 2024 13:11:28 +0000 > > >> Robin Murphy wrote: > > >> =20 > > >>> On 2024-03-11 11:52 am, Boris Brezillon wrote: =20 > > >>>> On Mon, 11 Mar 2024 13:49:56 +0200 > > >>>> Jani Nikula wrote: > > >>>> =C2=A0=C2=A0 =20 > > >>>>> On Mon, 11 Mar 2024, Boris Brezillon > > >>>>> wrote: =20 > > >>>>>> On Mon, 11 Mar 2024 13:05:01 +0200 > > >>>>>> Jani Nikula wrote: > > >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0 =20 > > >>>>>>> This breaks the config for me: > > >>>>>>> > > >>>>>>> =C2=A0=C2=A0=C2=A0 SYNC=C2=A0=C2=A0=C2=A0 include/config/auto.c= onf.cmd > > >>>>>>> =C2=A0=C2=A0=C2=A0 GEN=C2=A0=C2=A0=C2=A0=C2=A0 Makefile > > >>>>>>> drivers/iommu/Kconfig:14:error: recursive dependency detected! > > >>>>>>> drivers/iommu/Kconfig:14:=C2=A0=C2=A0=C2=A0 symbol IOMMU_SUPPOR= T is selected by > > >>>>>>> DRM_PANTHOR > > >>>>>>> drivers/gpu/drm/panthor/Kconfig:3:=C2=A0=C2=A0=C2=A0 symbol DRM= _PANTHOR depends > > >>>>>>> on PM > > >>>>>>> kernel/power/Kconfig:183:=C2=A0=C2=A0=C2=A0 symbol PM is select= ed by PM_SLEEP > > >>>>>>> kernel/power/Kconfig:117:=C2=A0=C2=A0=C2=A0 symbol PM_SLEEP dep= ends on > > >>>>>>> HIBERNATE_CALLBACKS > > >>>>>>> kernel/power/Kconfig:35:=C2=A0=C2=A0=C2=A0 symbol HIBERNATE_CAL= LBACKS is > > >>>>>>> selected by XEN_SAVE_RESTORE > > >>>>>>> arch/x86/xen/Kconfig:67:=C2=A0=C2=A0=C2=A0 symbol XEN_SAVE_REST= ORE depends on XEN > > >>>>>>> arch/x86/xen/Kconfig:6:=C2=A0=C2=A0=C2=A0 symbol XEN depends on= PARAVIRT > > >>>>>>> arch/x86/Kconfig:781:=C2=A0=C2=A0=C2=A0 symbol PARAVIRT is sele= cted by HYPERV > > >>>>>>> drivers/hv/Kconfig:5:=C2=A0=C2=A0=C2=A0 symbol HYPERV depends o= n X86_LOCAL_APIC > > >>>>>>> arch/x86/Kconfig:1106:=C2=A0=C2=A0=C2=A0 symbol X86_LOCAL_APIC = depends on > > >>>>>>> X86_UP_APIC > > >>>>>>> arch/x86/Kconfig:1081:=C2=A0=C2=A0=C2=A0 symbol X86_UP_APIC pro= mpt is visible > > >>>>>>> depending on PCI_MSI > > >>>>>>> drivers/pci/Kconfig:39:=C2=A0=C2=A0=C2=A0 symbol PCI_MSI is sel= ected by AMD_IOMMU > > >>>>>>> drivers/iommu/amd/Kconfig:3:=C2=A0=C2=A0=C2=A0 symbol AMD_IOMMU= depends on > > >>>>>>> IOMMU_SUPPORT =20 > > >>>>>> > > >>>>>> Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "sel= ect > > >>>>>> IOMMU_SUPPORT" in panthor then. =20 > > >>>>> > > >>>>> That works for me. =20 > > >>>> > > >>>> Let's revert the faulty commit first. We'll see if Steve has a > > >>>> different solution for the original issue. =20 > > >>> > > >>> FWIW, the reasoning in the offending commit seems incredibly tenuou= s. > > >>> There are far more practical reasons for building an arm/arm64 kern= el > > >>> without PM - for debugging or whatever, and where one may even still > > >>> want a usable GPU, let alone just a non-broken build - than there a= re > > >>> for building this driver for x86. Using pm_ptr() is trivial, and if= you > > >>> want to support COMPILE_TEST then there's really no justifiable exc= use > > >>> not to. =20 > > >> > > >> The problem is not just about using pm_ptr(), but also making sure > > >> panthor_device_resume/suspend() are called called in the init/unplug > > >> path when !PM, as I don't think the PM helpers automate that for us.= I > > >> was just aiming for a simple fix that wouldn't force me to test the = !PM > > >> case... =20 > > > Fair enough, at worst we could always have a runtime check and refuse= to > > > probe in conditions we don't think are worth the bother of implementi= ng > > > fully-functional support for. However if we want to make an argument = for > > > only supporting "realistic" configs at build time then that is an > > > argument for dropping COMPILE_TEST as well. =20 > >=20 > > Can we just replace the "depends on PM" with "select PM"? In my > > (admittedly very limited) testing this works. Otherwise I think we need > > to bite the bullet and support !PM in some way (maybe just as Robin > > suggests with a runtime bail out). =20 >=20 > I won't have time to test it this week, but if someone is interested, > here's a diff implementing manual resume/suspend in the init/unplug > path: >=20 > --->8--- =20 This time with the diff :-) --->8--- diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/pan= thor/panthor_device.c index 69deb8e17778..3d05e7358f0e 100644 --- a/drivers/gpu/drm/panthor/panthor_device.c +++ b/drivers/gpu/drm/panthor/panthor_device.c @@ -87,6 +87,10 @@ void panthor_device_unplug(struct panthor_device *ptdev) pm_runtime_dont_use_autosuspend(ptdev->base.dev); pm_runtime_put_sync_suspend(ptdev->base.dev); =20 + /* If PM is disabled, we need to call the suspend handler manually.= */ + if (!IS_ENABLED(CONFIG_PM)) + panthor_device_suspend(ptdev->base.dev); + /* Report the unplug operation as done to unblock concurrent * panthor_device_unplug() callers. */ @@ -218,6 +222,13 @@ int panthor_device_init(struct panthor_device *ptdev) if (ret) return ret; =20 + /* If PM is disabled, we need to call panthor_device_resume() manua= lly. */ + if (!IS_ENABLED(CONFIG_PM)) { + ret =3D panthor_device_resume(ptdev->base.dev); + if (ret) + return ret; + } + ret =3D panthor_gpu_init(ptdev); if (ret) goto err_rpm_put; diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/pantho= r/panthor_drv.c index ff484506229f..2ea6a9f436db 100644 --- a/drivers/gpu/drm/panthor/panthor_drv.c +++ b/drivers/gpu/drm/panthor/panthor_drv.c @@ -1407,17 +1407,19 @@ static const struct of_device_id dt_match[] =3D { }; MODULE_DEVICE_TABLE(of, dt_match); =20 +#ifdef CONFIG_PM static DEFINE_RUNTIME_DEV_PM_OPS(panthor_pm_ops, panthor_device_suspend, panthor_device_resume, NULL); +#endif =20 static struct platform_driver panthor_driver =3D { .probe =3D panthor_probe, .remove_new =3D panthor_remove, .driver =3D { .name =3D "panthor", - .pm =3D &panthor_pm_ops, + .pm =3D pm_ptr(&panthor_pm_ops), .of_match_table =3D dt_match, }, };