On 11/04/2016 07:29 AM, Ben Skeggs wrote: > * PGP Signed by an unknown key > > 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.
Makes sense - will respin later today. Thanks! Alex. _______________________________________________ Nouveau mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/nouveau
