| Message ID | 1434638872-29061-2-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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)?
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 --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; }
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(-)