[PATCH 0/2] drm/nouveau: Remove DRM_NOUVEAU_GSP_DEFAULT config

[PATCH 0/2] drm/nouveau: Remove DRM_NOUVEAU_GSP_DEFAULT config

[Mel Henning]

Following earlier discussion at
https://lists.freedesktop.org/archives/nouveau/2025-June/047887.html
this series removes DRM_NOUVEAU_GSP_DEFAULT.

Mel Henning (2):
  drm/nouveau: Remove DRM_NOUVEAU_GSP_DEFAULT config
  drm/nouveau: Remove nvkm_gsp_fwif.enable

 drivers/gpu/drm/nouveau/Kconfig                 | 8 --------
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/ad102.c | 4 ++--
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb100.c | 2 +-
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb202.c | 2 +-
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gh100.c | 2 +-
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h  | 1 -
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/tu102.c | 6 +-----
 7 files changed, 6 insertions(+), 19 deletions(-)

-- 
2.50.0

[PATCH 1/2] drm/nouveau: Remove DRM_NOUVEAU_GSP_DEFAULT config

[Mel Henning]

This option was originally intoduced because the GSP code path was
not well tested and we wanted to leave it up to distros which code path
they shipped by default. By now though, the GSP path is probably better
tested than the old firmware eg. Fedora ships GSP by default and we
generally run CTS on GSP. We've always been GSP-only on Ada and later.

So, this path removes the option and effectively sets the option to
always on. We still fall back to the old firmware if GSP is not found.
This change only affects Turing and Ampere.

Users can still set nouveau.config=NvGspRm=0 on the kernel command line
to force using the old firmware on Turing/Ampere.

Signed-off-by: Mel Henning <mhenning@darkrefraction.com>
---
 drivers/gpu/drm/nouveau/Kconfig                 | 8 --------
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/tu102.c | 6 +-----
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index d1587639ebb0..c88776d1e784 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -102,14 +102,6 @@ config DRM_NOUVEAU_SVM
 	  Say Y here if you want to enable experimental support for
 	  Shared Virtual Memory (SVM).
 
-config DRM_NOUVEAU_GSP_DEFAULT
-	bool "Use GSP firmware for Turing/Ampere (needs firmware installed)"
-	depends on DRM_NOUVEAU
-	default n
-	help
-	  Say Y here if you want to use the GSP codepaths by default on
-	  Turing and Ampere GPUs.
-
 config DRM_NOUVEAU_CH7006
 	tristate "Chrontel ch7006 TV encoder"
 	depends on DRM_NOUVEAU
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/tu102.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/tu102.c
index 58e233bc53b1..81e56da0474a 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/tu102.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/tu102.c
@@ -383,13 +383,9 @@ int
 tu102_gsp_load_rm(struct nvkm_gsp *gsp, const struct nvkm_gsp_fwif *fwif)
 {
 	struct nvkm_subdev *subdev = &gsp->subdev;
-	bool enable_gsp = fwif->enable;
 	int ret;
 
-#if IS_ENABLED(CONFIG_DRM_NOUVEAU_GSP_DEFAULT)
-	enable_gsp = true;
-#endif
-	if (!nvkm_boolopt(subdev->device->cfgopt, "NvGspRm", enable_gsp))
+	if (!nvkm_boolopt(subdev->device->cfgopt, "NvGspRm", true))
 		return -EINVAL;
 
 	ret = nvkm_gsp_load_fw(gsp, "gsp", fwif->ver, &gsp->fws.rm);
-- 
2.50.0

[PATCH 2/2] drm/nouveau: Remove nvkm_gsp_fwif.enable

[Mel Henning]

This struct element is no longer used.

Signed-off-by: Mel Henning <mhenning@darkrefraction.com>
---
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/ad102.c | 4 ++--
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb100.c | 2 +-
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb202.c | 2 +-
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gh100.c | 2 +-
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h  | 1 -
 5 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/ad102.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/ad102.c
index eb765da0876e..35d1fcef520b 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/ad102.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/ad102.c
@@ -41,8 +41,8 @@ ad102_gsp = {
 
 static struct nvkm_gsp_fwif
 ad102_gsps[] = {
-	{ 1, tu102_gsp_load, &ad102_gsp, &r570_rm_ga102, "570.144", true },
-	{ 0, tu102_gsp_load, &ad102_gsp, &r535_rm_ga102, "535.113.01", true },
+	{ 1, tu102_gsp_load, &ad102_gsp, &r570_rm_ga102, "570.144" },
+	{ 0, tu102_gsp_load, &ad102_gsp, &r535_rm_ga102, "535.113.01" },
 	{}
 };
 
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb100.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb100.c
index 12a3f2c1ed82..1b3b31b95ce4 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb100.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb100.c
@@ -20,7 +20,7 @@ gb100_gsp = {
 
 static struct nvkm_gsp_fwif
 gb100_gsps[] = {
-	{ 0, gh100_gsp_load, &gb100_gsp, &r570_rm_gb10x, "570.144", true },
+	{ 0, gh100_gsp_load, &gb100_gsp, &r570_rm_gb10x, "570.144" },
 	{}
 };
 
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb202.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb202.c
index c1d718172ddf..51384c63148c 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb202.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gb202.c
@@ -20,7 +20,7 @@ gb202_gsp = {
 
 static struct nvkm_gsp_fwif
 gb202_gsps[] = {
-	{ 0, gh100_gsp_load, &gb202_gsp, &r570_rm_gb20x, "570.144", true },
+	{ 0, gh100_gsp_load, &gb202_gsp, &r570_rm_gb20x, "570.144" },
 	{}
 };
 
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gh100.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gh100.c
index ce31e8248807..b0dd5fce7bad 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gh100.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/gh100.c
@@ -344,7 +344,7 @@ gh100_gsp_load(struct nvkm_gsp *gsp, int ver, const struct nvkm_gsp_fwif *fwif)
 
 static struct nvkm_gsp_fwif
 gh100_gsps[] = {
-	{ 0, gh100_gsp_load, &gh100_gsp, &r570_rm_gh100, "570.144", true },
+	{ 0, gh100_gsp_load, &gh100_gsp, &r570_rm_gh100, "570.144" },
 	{}
 };
 
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
index 4f14e85fc69e..c3494b7ac572 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
@@ -14,7 +14,6 @@ struct nvkm_gsp_fwif {
 	const struct nvkm_gsp_func *func;
 	const struct nvkm_rm_impl *rm;
 	const char *ver;
-	bool enable;
 };
 
 int nvkm_gsp_load_fw(struct nvkm_gsp *, const char *name, const char *ver,
-- 
2.50.0

Re: [PATCH 2/2] drm/nouveau: Remove nvkm_gsp_fwif.enable

[Timur Tabi]

On Mon, 2025-06-23 at 18:04 -0400, Mel Henning wrote:
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
> b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
> index 4f14e85fc69e..c3494b7ac572 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
> @@ -14,7 +14,6 @@ struct nvkm_gsp_fwif {
>  	const struct nvkm_gsp_func *func;
>  	const struct nvkm_rm_impl *rm;
>  	const char *ver;
> -	bool enable;
>  };

Instead of removing it, I think we should rename it to indicate whether GSP-RM is required.  You
cannot boot Ada or later without GSP-RM, so on those platforms, nouveau.config=NvGspRm=0 should be
ignored, and a pr_warn should be issued that it is being ignored.

Re: [PATCH 2/2] drm/nouveau: Remove nvkm_gsp_fwif.enable

[M Henning]

On Tue, Jun 24, 2025 at 1:28 PM Timur Tabi <ttabi@nvidia.com> wrote:
> Instead of removing it, I think we should rename it to indicate whether GSP-RM is required.  You
> cannot boot Ada or later without GSP-RM, so on those platforms, nouveau.config=NvGspRm=0 should be
> ignored, and a pr_warn should be issued that it is being ignored.

We did no such error checking before this series (in fact, most of
these options have almost no error checking). Are you saying you want
to see this added in this patch series?

Re: [PATCH 2/2] drm/nouveau: Remove nvkm_gsp_fwif.enable

[Timur Tabi]

On Tue, 2025-06-24 at 15:01 -0400, M Henning wrote:
> We did no such error checking before this series (in fact, most of
> these options have almost no error checking). Are you saying you want
> to see this added in this patch series?

You have a good point, but I think your change, in effect, necessitates my request.  Previously, the
default was no GSP-RM unless needed.  Now it's yes GSP-RM, and the concept of "need" has been
removed.  So there's no indication any more that some GPUs need GSP-RM and some do not.

So to address that, I think it makes sense to add a warning if someone tries disable GSP-RM on a GPU
that is not supported in that configuration.

Now, whether or not we should ignore NvGspRm=0 on Ada+ is up for debate.  If I understand the code
correctly, today (and still with your patches), Ada+ would fail to boot.  I can't say whether or not
that's a good idea.  But I think a warning should be printed either way.

Re: [PATCH 2/2] drm/nouveau: Remove nvkm_gsp_fwif.enable

[M Henning]

On Tue, Jun 24, 2025 at 3:13 PM Timur Tabi <ttabi@nvidia.com> wrote:
> You have a good point, but I think your change, in effect, necessitates my request.  Previously, the
> default was no GSP-RM unless needed.  Now it's yes GSP-RM, and the concept of "need" has been
> removed.  So there's no indication any more that some GPUs need GSP-RM and some do not.
>
> So to address that, I think it makes sense to add a warning if someone tries disable GSP-RM on a GPU
> that is not supported in that configuration.
>
> Now, whether or not we should ignore NvGspRm=0 on Ada+ is up for debate.  If I understand the code
> correctly, today (and still with your patches), Ada+ would fail to boot.  I can't say whether or not
> that's a good idea.  But I think a warning should be printed either way.

This patch behaves exactly the same as DRM_NOUVEAU_GSP_DEFAULT=y
kernels already behave.

That being said, I'm not against the additional error checking here
and can add it to the next version of this series.

Re: [PATCH 2/2] drm/nouveau: Remove nvkm_gsp_fwif.enable

[Ben Skeggs]

On 6/27/25 03:15, M Henning wrote:

> On Tue, Jun 24, 2025 at 3:13 PM Timur Tabi <ttabi@nvidia.com> wrote:
>> You have a good point, but I think your change, in effect, necessitates my request.  Previously, the
>> default was no GSP-RM unless needed.  Now it's yes GSP-RM, and the concept of "need" has been
>> removed.  So there's no indication any more that some GPUs need GSP-RM and some do not.
>>
>> So to address that, I think it makes sense to add a warning if someone tries disable GSP-RM on a GPU
>> that is not supported in that configuration.
>>
>> Now, whether or not we should ignore NvGspRm=0 on Ada+ is up for debate.  If I understand the code
>> correctly, today (and still with your patches), Ada+ would fail to boot.  I can't say whether or not
>> that's a good idea.  But I think a warning should be printed either way.
> This patch behaves exactly the same as DRM_NOUVEAU_GSP_DEFAULT=y
> kernels already behave.
>
> That being said, I'm not against the additional error checking here
> and can add it to the next version of this series.

Yeah, the GPUs that don't support GSP-RM can't hit the code that used 
fwif.enable anyway, so the series should be fine as it is.

Feel free to add my:

Reviewed-by: Ben Skeggs <bskeggs@nvidia.com>

Re: [PATCH 2/2] drm/nouveau: Remove nvkm_gsp_fwif.enable

[M Henning]

On Sun, Jul 13, 2025 at 11:19 PM Ben Skeggs <bskeggs@nvidia.com> wrote:
> Yeah, the GPUs that don't support GSP-RM can't hit the code that used
> fwif.enable anyway, so the series should be fine as it is.

We're actually talking about the reverse case. That is, on Ada what
happens if you set nouveau.config=NvGspRm=0 on the kernel command
line?

Re: [PATCH 2/2] drm/nouveau: Remove nvkm_gsp_fwif.enable

[Ben Skeggs]

On 7/15/25 02:04, M Henning wrote:

> On Sun, Jul 13, 2025 at 11:19 PM Ben Skeggs <bskeggs@nvidia.com> wrote:
>> Yeah, the GPUs that don't support GSP-RM can't hit the code that used
>> fwif.enable anyway, so the series should be fine as it is.
> We're actually talking about the reverse case. That is, on Ada what
> happens if you set nouveau.config=NvGspRm=0 on the kernel command
> line?

The last entry in fwif[] for GPUs prior to Ada is one that always 
succeeds, and sets up some minimal GSP stuff that's needed for booting 
the pre-GSP-RM firmwares.  Ada onwards lack this, and the driver will 
cleanly fail to load as if no firmware were present.

The error message could be better for sure, and you can just add an 
nvkm_error(&gsp->subdev, ...) after the attempt to call 
nvkm_firmware_load() in subdev/gsp/base.c.

This should work fine without needing to add any kind of "needs GSP-RM" 
field anywhere.

Ben.