diff mbox

[1/6] gr: support for NVIDIA-provided firmwares

Message ID 1434638872-29061-2-git-send-email-acourbot@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alexandre Courbot June 18, 2015, 2:47 p.m. UTC
NVIDIA will officially start providing signed firmwares through
linux-firmware. Change the GR firmware lookup function to look them up
in addition to the extracted firmwares.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drm/nouveau/nvkm/engine/gr/gf100.c | 66 +++++++++++++++++++++++++++++---------
 1 file changed, 51 insertions(+), 15 deletions(-)

Comments

Ilia Mirkin June 18, 2015, 3:57 p.m. UTC | #1
Why did you change request_firmware to request_firmware_direct?

On Thu, Jun 18, 2015 at 10:47 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> NVIDIA will officially start providing signed firmwares through
> linux-firmware. Change the GR firmware lookup function to look them up
> in addition to the extracted firmwares.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drm/nouveau/nvkm/engine/gr/gf100.c | 66 +++++++++++++++++++++++++++++---------
>  1 file changed, 51 insertions(+), 15 deletions(-)
>
> diff --git a/drm/nouveau/nvkm/engine/gr/gf100.c b/drm/nouveau/nvkm/engine/gr/gf100.c
> index ca11ddb..39d482f 100644
> --- a/drm/nouveau/nvkm/engine/gr/gf100.c
> +++ b/drm/nouveau/nvkm/engine/gr/gf100.c
> @@ -1544,26 +1544,62 @@ gf100_gr_dtor_fw(struct gf100_gr_fuc *fuc)
>         fuc->data = NULL;
>  }
>
> +/**
> + * gf100_gr_ctor_fw - helper for loading external GR firmwares
> + *
> + * A firmware can either be officially provided by NVIDIA (in which case it will
> + * use a "NVIDIA name", or be extracted from the binary blob (and use a
> + * "Nouveau name". The fwname and nvfwname are to be given the Nouveau and
> + * NVIDIA names of a given firmware, respectively. This function will then
> + * try to load the NVIDIA firmware, then the extracted one, in that order.
> + *
> + */
>  int
>  gf100_gr_ctor_fw(struct gf100_gr_priv *priv, const char *fwname,
> -                struct gf100_gr_fuc *fuc)
> +                const char *nvfwname, struct gf100_gr_fuc *fuc)
>  {
>         struct nvkm_device *device = nv_device(priv);
>         const struct firmware *fw;
> -       char f[32];
> -       int ret;
> +       char f[64];
> +       int ret = -EINVAL;
> +       int i;
>
> -       snprintf(f, sizeof(f), "nouveau/nv%02x_%s", device->chipset, fwname);
> -       ret = request_firmware(&fw, f, nv_device_base(device));
> -       if (ret) {
> -               snprintf(f, sizeof(f), "nouveau/%s", fwname);
> -               ret = request_firmware(&fw, f, nv_device_base(device));
> -               if (ret) {
> -                       nv_error(priv, "failed to load %s\n", fwname);
> -                       return ret;
> +       /*
> +        * NVIDIA firmware name provided - try to load it
> +        * We try this first since most chips that require external firmware
> +        * are supported by NVIDIA
> +        */
> +       if (nvfwname) {
> +               snprintf(f, sizeof(f), "nvidia/%s/%s.bin", device->cname,
> +                        nvfwname);
> +               i = strlen(f);
> +               while (i) {
> +                       --i;
> +                       f[i] = tolower(f[i]);
>                 }
> +               ret = request_firmware_direct(&fw, f, nv_device_base(device));
> +               if (!ret)
> +                       goto found;
> +       }
> +
> +       /* Nouveau firmware name provided - try to load it */
> +       if (fwname) {
> +               snprintf(f, sizeof(f), "nouveau/nv%02x_%s", device->chipset,
> +                        fwname);
> +               ret = request_firmware_direct(&fw, f, nv_device_base(device));
> +               if (!ret)
> +                       goto found;
> +
> +               snprintf(f, sizeof(f), "nouveau/%s", fwname);
> +               ret = request_firmware_direct(&fw, f, nv_device_base(device));
> +               if (!ret)
> +                       goto found;
>         }
>
> +       nv_error(priv, "failed to load %s / %s\n", fwname, nvfwname);
> +       return ret;
> +
> +found:
>         fuc->size = fw->size;
>         fuc->data = kmemdup(fw->data, fuc->size, GFP_KERNEL);
>         release_firmware(fw);
> @@ -1615,10 +1651,10 @@ gf100_gr_ctor(struct nvkm_object *parent, struct nvkm_object *engine,
>
>         if (use_ext_fw) {
>                 nv_info(priv, "using external firmware\n");
> -               if (gf100_gr_ctor_fw(priv, "fuc409c", &priv->fuc409c) ||
> -                   gf100_gr_ctor_fw(priv, "fuc409d", &priv->fuc409d) ||
> -                   gf100_gr_ctor_fw(priv, "fuc41ac", &priv->fuc41ac) ||
> -                   gf100_gr_ctor_fw(priv, "fuc41ad", &priv->fuc41ad))
> +               if (gf100_gr_ctor_fw(priv, "fuc409c", "fecs_inst", &priv->fuc409c) ||
> +                   gf100_gr_ctor_fw(priv, "fuc409d", "fecs_data", &priv->fuc409d) ||
> +                   gf100_gr_ctor_fw(priv, "fuc41ac", "gpccs_inst", &priv->fuc41ac) ||
> +                   gf100_gr_ctor_fw(priv, "fuc41ad", "gpccs_data", &priv->fuc41ad))
>                         return -ENODEV;
>                 priv->firmware = true;
>         }
> --
> 2.4.3
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau
Alexandre Courbot June 19, 2015, 4:04 a.m. UTC | #2
On Fri, Jun 19, 2015 at 12:57 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> Why did you change request_firmware to request_firmware_direct?

request_firmware() outputs an error message if the firmware file does
not exist, whereas request_firmware_direct() doesn't. Since the
firmware file can be in one of two locations, that seems to be the
most appropriate behavior.

However, request_firmware_direct() also does not invoke the userspace
helper. In the case of Nouveau I don't believe we need this, but if I
overlooked something let me know and I will think of something else.
Ben Skeggs June 19, 2015, 4:40 a.m. UTC | #3
On 19 June 2015 at 00:47, Alexandre Courbot <gnurou@gmail.com> wrote:
> NVIDIA will officially start providing signed firmwares through
> linux-firmware. Change the GR firmware lookup function to look them up
> in addition to the extracted firmwares.
I wonder if perhaps we should just replace the mechanism entirely, and
remove the support for nouveau/fuc* as we add "official" support for
NVIDIA's ucode.  The existing code is actually partially broken
anyway, and mostly works by luck and was intended as a development aid
/ workaround anyway.  There are no chipsets (aside from GM2xx...)
which we don't currently support using our own ucode, so the impact of
removing it will be very minimal.

Thoughts?

Ben.

>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drm/nouveau/nvkm/engine/gr/gf100.c | 66 +++++++++++++++++++++++++++++---------
>  1 file changed, 51 insertions(+), 15 deletions(-)
>
> diff --git a/drm/nouveau/nvkm/engine/gr/gf100.c b/drm/nouveau/nvkm/engine/gr/gf100.c
> index ca11ddb..39d482f 100644
> --- a/drm/nouveau/nvkm/engine/gr/gf100.c
> +++ b/drm/nouveau/nvkm/engine/gr/gf100.c
> @@ -1544,26 +1544,62 @@ gf100_gr_dtor_fw(struct gf100_gr_fuc *fuc)
>         fuc->data = NULL;
>  }
>
> +/**
> + * gf100_gr_ctor_fw - helper for loading external GR firmwares
> + *
> + * A firmware can either be officially provided by NVIDIA (in which case it will
> + * use a "NVIDIA name", or be extracted from the binary blob (and use a
> + * "Nouveau name". The fwname and nvfwname are to be given the Nouveau and
> + * NVIDIA names of a given firmware, respectively. This function will then
> + * try to load the NVIDIA firmware, then the extracted one, in that order.
> + *
> + */
>  int
>  gf100_gr_ctor_fw(struct gf100_gr_priv *priv, const char *fwname,
> -                struct gf100_gr_fuc *fuc)
> +                const char *nvfwname, struct gf100_gr_fuc *fuc)
>  {
>         struct nvkm_device *device = nv_device(priv);
>         const struct firmware *fw;
> -       char f[32];
> -       int ret;
> +       char f[64];
> +       int ret = -EINVAL;
> +       int i;
>
> -       snprintf(f, sizeof(f), "nouveau/nv%02x_%s", device->chipset, fwname);
> -       ret = request_firmware(&fw, f, nv_device_base(device));
> -       if (ret) {
> -               snprintf(f, sizeof(f), "nouveau/%s", fwname);
> -               ret = request_firmware(&fw, f, nv_device_base(device));
> -               if (ret) {
> -                       nv_error(priv, "failed to load %s\n", fwname);
> -                       return ret;
> +       /*
> +        * NVIDIA firmware name provided - try to load it
> +        * We try this first since most chips that require external firmware
> +        * are supported by NVIDIA
> +        */
> +       if (nvfwname) {
> +               snprintf(f, sizeof(f), "nvidia/%s/%s.bin", device->cname,
> +                        nvfwname);
> +               i = strlen(f);
> +               while (i) {
> +                       --i;
> +                       f[i] = tolower(f[i]);
>                 }
> +               ret = request_firmware_direct(&fw, f, nv_device_base(device));
> +               if (!ret)
> +                       goto found;
> +       }
> +
> +       /* Nouveau firmware name provided - try to load it */
> +       if (fwname) {
> +               snprintf(f, sizeof(f), "nouveau/nv%02x_%s", device->chipset,
> +                        fwname);
> +               ret = request_firmware_direct(&fw, f, nv_device_base(device));
> +               if (!ret)
> +                       goto found;
> +
> +               snprintf(f, sizeof(f), "nouveau/%s", fwname);
> +               ret = request_firmware_direct(&fw, f, nv_device_base(device));
> +               if (!ret)
> +                       goto found;
>         }
>
> +       nv_error(priv, "failed to load %s / %s\n", fwname, nvfwname);
> +       return ret;
> +
> +found:
>         fuc->size = fw->size;
>         fuc->data = kmemdup(fw->data, fuc->size, GFP_KERNEL);
>         release_firmware(fw);
> @@ -1615,10 +1651,10 @@ gf100_gr_ctor(struct nvkm_object *parent, struct nvkm_object *engine,
>
>         if (use_ext_fw) {
>                 nv_info(priv, "using external firmware\n");
> -               if (gf100_gr_ctor_fw(priv, "fuc409c", &priv->fuc409c) ||
> -                   gf100_gr_ctor_fw(priv, "fuc409d", &priv->fuc409d) ||
> -                   gf100_gr_ctor_fw(priv, "fuc41ac", &priv->fuc41ac) ||
> -                   gf100_gr_ctor_fw(priv, "fuc41ad", &priv->fuc41ad))
> +               if (gf100_gr_ctor_fw(priv, "fuc409c", "fecs_inst", &priv->fuc409c) ||
> +                   gf100_gr_ctor_fw(priv, "fuc409d", "fecs_data", &priv->fuc409d) ||
> +                   gf100_gr_ctor_fw(priv, "fuc41ac", "gpccs_inst", &priv->fuc41ac) ||
> +                   gf100_gr_ctor_fw(priv, "fuc41ad", "gpccs_data", &priv->fuc41ad))
>                         return -ENODEV;
>                 priv->firmware = true;
>         }
> --
> 2.4.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Alexandre Courbot June 19, 2015, 5:22 a.m. UTC | #4
On Fri, Jun 19, 2015 at 1:40 PM, Ben Skeggs <skeggsb@gmail.com> wrote:
> On 19 June 2015 at 00:47, Alexandre Courbot <gnurou@gmail.com> wrote:
>> NVIDIA will officially start providing signed firmwares through
>> linux-firmware. Change the GR firmware lookup function to look them up
>> in addition to the extracted firmwares.
> I wonder if perhaps we should just replace the mechanism entirely, and
> remove the support for nouveau/fuc* as we add "official" support for
> NVIDIA's ucode.  The existing code is actually partially broken
> anyway, and mostly works by luck and was intended as a development aid
> / workaround anyway.  There are no chipsets (aside from GM2xx...)
> which we don't currently support using our own ucode, so the impact of
> removing it will be very minimal.
>
> Thoughts?

I'm all for making things simpler, and if someone needs to use an
external firmware for a Nouveau-supported GPU they can always put it
under the nvidia/ firmware directory. So if you agree with it I will
remove support for firmwares in nouveau/ in GR. I am not sure whether
your statement also applies to other firmwares (falcon, xtensa)?
Ben Skeggs June 19, 2015, 5:28 a.m. UTC | #5
On 19 June 2015 at 15:22, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Fri, Jun 19, 2015 at 1:40 PM, Ben Skeggs <skeggsb@gmail.com> wrote:
>> On 19 June 2015 at 00:47, Alexandre Courbot <gnurou@gmail.com> wrote:
>>> NVIDIA will officially start providing signed firmwares through
>>> linux-firmware. Change the GR firmware lookup function to look them up
>>> in addition to the extracted firmwares.
>> I wonder if perhaps we should just replace the mechanism entirely, and
>> remove the support for nouveau/fuc* as we add "official" support for
>> NVIDIA's ucode.  The existing code is actually partially broken
>> anyway, and mostly works by luck and was intended as a development aid
>> / workaround anyway.  There are no chipsets (aside from GM2xx...)
>> which we don't currently support using our own ucode, so the impact of
>> removing it will be very minimal.
>>
>> Thoughts?
>
> I'm all for making things simpler, and if someone needs to use an
> external firmware for a Nouveau-supported GPU they can always put it
> under the nvidia/ firmware directory. So if you agree with it I will
> remove support for firmwares in nouveau/ in GR. I am not sure whether
> your statement also applies to other firmwares (falcon, xtensa)?
I'd say leave the non-gr engines for now, at least until we know what
NVIDIA plans on doing with dGPU firmwares.
diff mbox

Patch

diff --git a/drm/nouveau/nvkm/engine/gr/gf100.c b/drm/nouveau/nvkm/engine/gr/gf100.c
index ca11ddb..39d482f 100644
--- a/drm/nouveau/nvkm/engine/gr/gf100.c
+++ b/drm/nouveau/nvkm/engine/gr/gf100.c
@@ -1544,26 +1544,62 @@  gf100_gr_dtor_fw(struct gf100_gr_fuc *fuc)
 	fuc->data = NULL;
 }
 
+/**
+ * gf100_gr_ctor_fw - helper for loading external GR firmwares
+ *
+ * A firmware can either be officially provided by NVIDIA (in which case it will
+ * use a "NVIDIA name", or be extracted from the binary blob (and use a
+ * "Nouveau name". The fwname and nvfwname are to be given the Nouveau and
+ * NVIDIA names of a given firmware, respectively. This function will then
+ * try to load the NVIDIA firmware, then the extracted one, in that order.
+ *
+ */
 int
 gf100_gr_ctor_fw(struct gf100_gr_priv *priv, const char *fwname,
-		 struct gf100_gr_fuc *fuc)
+		 const char *nvfwname, struct gf100_gr_fuc *fuc)
 {
 	struct nvkm_device *device = nv_device(priv);
 	const struct firmware *fw;
-	char f[32];
-	int ret;
+	char f[64];
+	int ret = -EINVAL;
+	int i;
 
-	snprintf(f, sizeof(f), "nouveau/nv%02x_%s", device->chipset, fwname);
-	ret = request_firmware(&fw, f, nv_device_base(device));
-	if (ret) {
-		snprintf(f, sizeof(f), "nouveau/%s", fwname);
-		ret = request_firmware(&fw, f, nv_device_base(device));
-		if (ret) {
-			nv_error(priv, "failed to load %s\n", fwname);
-			return ret;
+	/*
+	 * NVIDIA firmware name provided - try to load it
+	 * We try this first since most chips that require external firmware
+	 * are supported by NVIDIA
+	 */
+	if (nvfwname) {
+		snprintf(f, sizeof(f), "nvidia/%s/%s.bin", device->cname,
+			 nvfwname);
+		i = strlen(f);
+		while (i) {
+			--i;
+			f[i] = tolower(f[i]);
 		}
+		ret = request_firmware_direct(&fw, f, nv_device_base(device));
+		if (!ret)
+			goto found;
+	}
+
+	/* Nouveau firmware name provided - try to load it */
+	if (fwname) {
+		snprintf(f, sizeof(f), "nouveau/nv%02x_%s", device->chipset,
+			 fwname);
+		ret = request_firmware_direct(&fw, f, nv_device_base(device));
+		if (!ret)
+			goto found;
+
+		snprintf(f, sizeof(f), "nouveau/%s", fwname);
+		ret = request_firmware_direct(&fw, f, nv_device_base(device));
+		if (!ret)
+			goto found;
 	}
 
+	nv_error(priv, "failed to load %s / %s\n", fwname, nvfwname);
+	return ret;
+
+found:
 	fuc->size = fw->size;
 	fuc->data = kmemdup(fw->data, fuc->size, GFP_KERNEL);
 	release_firmware(fw);
@@ -1615,10 +1651,10 @@  gf100_gr_ctor(struct nvkm_object *parent, struct nvkm_object *engine,
 
 	if (use_ext_fw) {
 		nv_info(priv, "using external firmware\n");
-		if (gf100_gr_ctor_fw(priv, "fuc409c", &priv->fuc409c) ||
-		    gf100_gr_ctor_fw(priv, "fuc409d", &priv->fuc409d) ||
-		    gf100_gr_ctor_fw(priv, "fuc41ac", &priv->fuc41ac) ||
-		    gf100_gr_ctor_fw(priv, "fuc41ad", &priv->fuc41ad))
+		if (gf100_gr_ctor_fw(priv, "fuc409c", "fecs_inst", &priv->fuc409c) ||
+		    gf100_gr_ctor_fw(priv, "fuc409d", "fecs_data", &priv->fuc409d) ||
+		    gf100_gr_ctor_fw(priv, "fuc41ac", "gpccs_inst", &priv->fuc41ac) ||
+		    gf100_gr_ctor_fw(priv, "fuc41ad", "gpccs_data", &priv->fuc41ad))
 			return -ENODEV;
 		priv->firmware = true;
 	}