[Nouveau] [PATCH 1/5] core: add firmware handling functions

[Emil Velikov]

Hi Alexandre,

On 18 January 2016 at 06:07, Alexandre Courbot <acourbot at nvidia.com> wrote:
> Add two functions nvkm_firmware_get() and nvkm_firmware_put() to load a
> firmware file and free its resources, respectively. Since firmware files
> are becoming a necessity for new GPUs, and their location has been
> standardized to nvidia/chip/, this will prevent duplicate and
> error-prone name-generation code.
>
Strictly speaking this will break video decoding (and maybe others)
although I'm not sure how much one could care. Then again pinging
people to update the scripts [1] will be good :)

[1] http://nouveau.freedesktop.org/wiki/VideoAcceleration/#firmware

> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
> ---
>  drm/nouveau/include/nvkm/core/firmware.h | 11 ++++++
>  drm/nouveau/nvkm/core/Kbuild             |  1 +
>  drm/nouveau/nvkm/core/firmware.c         | 64 ++++++++++++++++++++++++++++++++
>  3 files changed, 76 insertions(+)
>  create mode 100644 drm/nouveau/include/nvkm/core/firmware.h
>  create mode 100644 drm/nouveau/nvkm/core/firmware.c
>
> diff --git a/drm/nouveau/include/nvkm/core/firmware.h b/drm/nouveau/include/nvkm/core/firmware.h
> new file mode 100644
> index 000000000000..a626ce378f04
> --- /dev/null
> +++ b/drm/nouveau/include/nvkm/core/firmware.h
> @@ -0,0 +1,11 @@
> +#ifndef __NVKM_FIRMWARE_H__
> +#define __NVKM_FIRMWARE_H__
> +
> +#include <core/device.h>
> +
> +int nvkm_firmware_get(struct nvkm_device *device, const char *fwname,
> +                     const struct firmware **fw);
> +
> +void nvkm_firmware_put(const struct firmware *fw);
> +
> +#endif
> diff --git a/drm/nouveau/nvkm/core/Kbuild b/drm/nouveau/nvkm/core/Kbuild
> index 7f66963f305c..86a31a8e1e51 100644
> --- a/drm/nouveau/nvkm/core/Kbuild
> +++ b/drm/nouveau/nvkm/core/Kbuild
> @@ -2,6 +2,7 @@ nvkm-y := nvkm/core/client.o
>  nvkm-y += nvkm/core/engine.o
>  nvkm-y += nvkm/core/enum.o
>  nvkm-y += nvkm/core/event.o
> +nvkm-y += nvkm/core/firmware.o
>  nvkm-y += nvkm/core/gpuobj.o
>  nvkm-y += nvkm/core/ioctl.o
>  nvkm-y += nvkm/core/memory.o
> diff --git a/drm/nouveau/nvkm/core/firmware.c b/drm/nouveau/nvkm/core/firmware.c
> new file mode 100644
> index 000000000000..4a4b4a5d55a8
> --- /dev/null
> +++ b/drm/nouveau/nvkm/core/firmware.c
> @@ -0,0 +1,64 @@
> +/*
> + * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <core/device.h>
> +
> +#include <linux/firmware.h>
> +
> +/**
> + * nvkm_firmware_get - load firmware from the official nvidia/chip/ directory
> + * @device     device that will use that firmware
> + * @fwname     name of firmware file to load
> + * @fw         firmware structure to load to
> + *
> + * Use this function to load firmware files in the form nvidia/chip/fwname.bin.
> + * Firmware files released by NVIDIA will always follow this format.
> + */
> +int
> +nvkm_firmware_get(struct nvkm_device *device, const char *fwname,
> +                 const struct firmware **fw)
> +{
> +       char f[64];
> +       char cname[16];
> +       int i;
> +
> +       /* Convert device name to lowercase */
> +       strncpy(cname, device->chip->name, sizeof(cname));
> +       cname[sizeof(cname) - 1] = '\0';
> +       i = strlen(cname);
> +       while (i) {
> +               --i;
> +               cname[i] = tolower(cname[i]);
> +       }
> +
Considering there's only one user that might need this, is it worth
adding it here or just keeping it where needed ?

> +       snprintf(f, sizeof(f), "nvidia/%s/%s.bin", cname, fwname);
> +       return request_firmware(fw, f, device->dev);
> +}
> +
> +/**
> + * nvkm_firmware_put - release firmware loaded with nvkm_firmware_get
> + */
> +void
> +nvkm_firmware_put(const struct firmware *fw)
> +{
> +       release_firmware(fw);
If the above suggestion makes sense, then it might be better to keep
these two in the header as static inlines ?

Not that either of the two is a hot path, then again having a one/two
liner as separate function feels odd.

Cheers,
Emil