[PATCH] drm/nouveau: select FW caching

[PATCH] drm/nouveau: select FW caching

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

nouveau tries to load some firmware during suspend that it loaded earlier, but with
fw caching disabled it hangs suspend, so just rely on FW cache enabling instead of
working around it in the driver.

Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/nouveau/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index ce840300578d8..1050a4617fc15 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -4,6 +4,7 @@ config DRM_NOUVEAU
 	depends on DRM && PCI && MMU
 	select IOMMU_API
 	select FW_LOADER
+	select FW_CACHE if PM_SLEEP
 	select DRM_CLIENT_SELECTION
 	select DRM_DISPLAY_DP_HELPER
 	select DRM_DISPLAY_HDMI_HELPER
-- 
2.48.1

Re: [PATCH] drm/nouveau: select FW caching

[Danilo Krummrich]

(+ Luis, Russ)

On Fri, Feb 07, 2025 at 11:25:31AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> nouveau tries to load some firmware during suspend that it loaded earlier, but with
> fw caching disabled it hangs suspend, so just rely on FW cache enabling instead of
> working around it in the driver.
> 
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/nouveau/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
> index ce840300578d8..1050a4617fc15 100644
> --- a/drivers/gpu/drm/nouveau/Kconfig
> +++ b/drivers/gpu/drm/nouveau/Kconfig
> @@ -4,6 +4,7 @@ config DRM_NOUVEAU
>  	depends on DRM && PCI && MMU
>  	select IOMMU_API
>  	select FW_LOADER
> +	select FW_CACHE if PM_SLEEP

CONFIG_FW_CACHE was added, as the Kconfig says, it "can prevent suspend on many
platforms".

@Luis, Russ: I assume this mostly means embedded platforms?

I wonder if we should not insist on FW_CACHE if NOUVEAU_PLATFORM_DRIVER, or even
only force FW_CACHE if DRM_NOUVEAU_GSP_DEFAULT?

Re: [PATCH] drm/nouveau: select FW caching

[Danilo Krummrich]

On Fri, Feb 14, 2025 at 02:05:36PM +0100, Danilo Krummrich wrote:
> only force FW_CACHE if DRM_NOUVEAU_GSP_DEFAULT?

Please scratch that, it was a horrible idea.

Re: [PATCH] drm/nouveau: select FW caching

[Luis Chamberlain]

On Fri, Feb 14, 2025 at 02:10:27PM +0100, Danilo Krummrich wrote:
> On Fri, Feb 14, 2025 at 02:05:36PM +0100, Danilo Krummrich wrote:
> > only force FW_CACHE if DRM_NOUVEAU_GSP_DEFAULT?
> 
> Please scratch that, it was a horrible idea.

What I recommend is to look into why we disable it by default, I think
its sold old obscure reasoning but now suspect it was udev being dumb,
in line with why we also try to defensively try to ignore duplicate
udev requests for module requests causing a flood. Refer to commit
9b9879fc03275ff ("modules: catch concurrent module loads, treat them as
idempotent") for details.

  Luis

Re: [PATCH] drm/nouveau: select FW caching

[Danilo Krummrich]

On Tue, Feb 18, 2025 at 06:28:53AM -0800, Luis Chamberlain wrote:
> 
> What I recommend is to look into why we disable it by default, I think

I think FW_CACHE is enabled by default, no?

> its sold old obscure reasoning but now suspect it was udev being dumb,
> in line with why we also try to defensively try to ignore duplicate
> udev requests for module requests causing a flood. Refer to commit
> 9b9879fc03275ff ("modules: catch concurrent module loads, treat them as
> idempotent") for details.

Thanks, that helps a lot.

Given that this commit landed in v6.5 I guess there's not really a concern to
force enable FW_CACHE for Nouveau as a fix for commit 176fdcbddfd2
("drm/nouveau/gsp/r535: add support for booting GSP-RM"), which landed in v6.7.

Re: [PATCH] drm/nouveau: select FW caching

[Luis Chamberlain]

On Tue, Feb 18, 2025 at 06:24:43PM +0100, Danilo Krummrich wrote:
> On Tue, Feb 18, 2025 at 06:28:53AM -0800, Luis Chamberlain wrote:
> > 
> > What I recommend is to look into why we disable it by default, I think
> 
> I think FW_CACHE is enabled by default, no?
> 
> > its sold old obscure reasoning but now suspect it was udev being dumb,
> > in line with why we also try to defensively try to ignore duplicate
> > udev requests for module requests causing a flood. Refer to commit
> > 9b9879fc03275ff ("modules: catch concurrent module loads, treat them as
> > idempotent") for details.
> 
> Thanks, that helps a lot.
> 
> Given that this commit landed in v6.5

That commit lets us ignore the udev floods for superfluous module
requests only.

> I guess there's not really a concern to
> force enable FW_CACHE for Nouveau as a fix for commit 176fdcbddfd2
> ("drm/nouveau/gsp/r535: add support for booting GSP-RM"), which landed in v6.7.

I'd recommmend to look into *why* folks didn't like fw cache to look at
disabling it in the first place, and look and see if udev was acting up
and doing stupid things preventing suspend.

 Luis

Re: [PATCH] drm/nouveau: select FW caching

[Dave Airlie]

On Wed, 19 Feb 2025 at 07:39, Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Tue, Feb 18, 2025 at 06:24:43PM +0100, Danilo Krummrich wrote:
> > On Tue, Feb 18, 2025 at 06:28:53AM -0800, Luis Chamberlain wrote:
> > >
> > > What I recommend is to look into why we disable it by default, I think
> >
> > I think FW_CACHE is enabled by default, no?
> >
> > > its sold old obscure reasoning but now suspect it was udev being dumb,
> > > in line with why we also try to defensively try to ignore duplicate
> > > udev requests for module requests causing a flood. Refer to commit
> > > 9b9879fc03275ff ("modules: catch concurrent module loads, treat them as
> > > idempotent") for details.
> >
> > Thanks, that helps a lot.
> >
> > Given that this commit landed in v6.5
>
> That commit lets us ignore the udev floods for superfluous module
> requests only.
>
> > I guess there's not really a concern to
> > force enable FW_CACHE for Nouveau as a fix for commit 176fdcbddfd2
> > ("drm/nouveau/gsp/r535: add support for booting GSP-RM"), which landed in v6.7.
>
> I'd recommmend to look into *why* folks didn't like fw cache to look at
> disabling it in the first place, and look and see if udev was acting up
> and doing stupid things preventing suspend.

This has been on in Fedora for ever, Linus said it was originally on
by default and Greg thinks Android folks had a regression, I don't
think the intersections of nouveau and platforms which had problems a
long time ago is interesting enough to care,

All distros should enable this, it fixes problems, if Android finds a
regression with nouveau it'll bisect to here.

Dave.
>
>  Luis