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

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

On Thu, Jan 21, 2016 at 8:41 PM, Emil Velikov <emil.l.velikov at
gmail.com> wrote:
> 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
Yeah, these functions are actually designed to load NVIDIA-provided
firmware. I thought it would be nice to have a single function for all
of Nouveau's firmware needs though.
>
>> 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 ?
So for now GR uses it as GK20A (and hopefully soon, all GM20Xs) have
firmware provided by NVIDIA. GR is the only user currently, but the
secure-boot feature also makes heavy use of this.

Depending on how much firmware we can release, we may also update
other firmware consumers to use these. As far as I understand, there
is real "official" location for firmware files at the moment.

On 25 January 2016 at 02:11, Alexandre Courbot <gnurou at gmail.com>
wrote:
> On Thu, Jan 21, 2016 at 8:41 PM, Emil Velikov <emil.l.velikov at
gmail.com> wrote:
>> 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
>
> Yeah, these functions are actually designed to load NVIDIA-provided
> firmware. I thought it would be nice to have a single function for all
> of Nouveau's firmware needs though.
>
>>
>>> 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 ?
>
> So for now GR uses it as GK20A (and hopefully soon, all GM20Xs) have
> firmware provided by NVIDIA. GR is the only user currently, but the
> secure-boot feature also makes heavy use of this.
>
> Depending on how much firmware we can release, we may also update
> other firmware consumers to use these. As far as I understand, there
> is real "official" location for firmware files at the moment.
Silly brain - managed to get cname, fwname and name all in the wrong order.

Pardon for the noise.
Emil