On 11/02/2016 03:52 PM, Alexandre Courbot wrote: > 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 I agree.
> > 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. As it is a legacy path, I'm more for hiding it away in ctor_legacy() as Emil suggests. Ben.
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Nouveau mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/nouveau
