On 11/02/2016 02:07 PM, Ilia Mirkin wrote: > On Wed, Nov 2, 2016 at 12:54 AM, Alexandre Courbot <[email protected]> > wrote: >> Look for firmware files using the legacy ("nouveau/nvxx_fucxxxx") path >> if they cannot be found in the new, "official" path. User setups were >> broken by the switch, which is bad. >> >> There are only 4 firmware files we may want to look up that way, so >> hardcode them into the lookup function. All new firmware files should >> use the standard "nvidia/<chip>/gr/" path. >> >> Fixes: 8539b37acef7 ("drm/nouveau/gr: use NVIDIA-provided external >> firmwares") >> Signed-off-by: Alexandre Courbot <[email protected]> >> --- >> drm/nouveau/nvkm/engine/gr/gf100.c | 56 >> ++++++++++++++++++++++++++++++++++---- >> 1 file changed, 51 insertions(+), 5 deletions(-) >> >> diff --git a/drm/nouveau/nvkm/engine/gr/gf100.c >> b/drm/nouveau/nvkm/engine/gr/gf100.c >> index 157919c788e6..9e65adbab21c 100644 >> --- a/drm/nouveau/nvkm/engine/gr/gf100.c >> +++ b/drm/nouveau/nvkm/engine/gr/gf100.c >> @@ -1756,24 +1756,70 @@ gf100_gr_ = { >> }; >> >> int >> +gf100_gr_ctor_fw_legacy(struct gf100_gr *gr, const char *fwname, >> + struct gf100_gr_fuc *fuc) >> +{ >> + struct nvkm_subdev *subdev = &gr->base.engine.subdev; >> + struct nvkm_device *device = subdev->device; >> + const struct firmware *fw; >> + char f[32]; >> + int ret; >> + >> + snprintf(f, sizeof(f), "nouveau/nv%02x_%s", device->chipset, fwname); >> + ret = request_firmware(&fw, f, device->dev); >> + if (ret) { >> + snprintf(f, sizeof(f), "nouveau/%s", fwname); >> + ret = request_firmware(&fw, f, device->dev); >> + if (ret) { >> + nvkm_error(subdev, "failed to load %s\n", fwname); >> + return ret; >> + } >> + } >> + >> + fuc->size = fw->size; >> + fuc->data = kmemdup(fw->data, fuc->size, GFP_KERNEL); >> + release_firmware(fw); >> + return (fuc->data != NULL) ? 0 : -ENOMEM; >> +} >> + >> +int >> gf100_gr_ctor_fw(struct gf100_gr *gr, const char *fwname, >> struct gf100_gr_fuc *fuc) >> { >> struct nvkm_subdev *subdev = &gr->base.engine.subdev; >> struct nvkm_device *device = subdev->device; >> + const char *legacy_fwname = NULL; >> const struct firmware *fw; >> int ret; >> >> ret = nvkm_firmware_get(device, fwname, &fw); >> - if (ret) { >> + /* firmware found, success! */ >> + if (!ret) { >> + fuc->size = fw->size; >> + fuc->data = kmemdup(fw->data, fuc->size, GFP_KERNEL); >> + nvkm_firmware_put(fw); >> + return (fuc->data != NULL) ? 0 : -ENOMEM; >> + } >> + >> + /* see if this firmware has a legacy path */ >> + if (!strcmp(fwname, "fecs_inst")) >> + legacy_fwname = "fuc409c"; >> + else if (!strcmp(fwname, "fecs_data")) >> + legacy_fwname = "fuc409d"; >> + else if (!strcmp(fwname, "gpccs_inst")) >> + legacy_fwname = "fuc41ac"; >> + else if (!strcmp(fwname, "gpccs_data")) >> + legacy_fwname = "fuc41ad"; > > As I mentioned on IRC, I find this strcmp thing a little jarring. It > should be pretty easy to just pass the legacy fwname into > gf100_gr_ctor_fw directly - there are only a handful of callers, and > most would just pass NULL in as the legacy fwname - only the ones in > gf100.c would pass the "old" names in.
Yeah, that was the original approach actually. I switched to this for the following reasons: - We don't want to encourage using this legacy path, hence hiding it from callers - There are only 4 possible legacy files - it's ugly but still manageable and contained in a single function Of course, if the general consensus is that this is too ugly, it would be trivial to turn it the way you suggested, so I don't feel too strongly for one approach over the other. _______________________________________________ Nouveau mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/nouveau
