[Public]
Thanks,
Candice
-----Original Message-----
From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
Sent: Thursday, April 21, 2022 7:01 PM
To: Li, Candice <Candice.Li@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Clements, John <John.Clements@xxxxxxx>
Subject: Re: [PATCH v2 2/2] drm/amdgpu: Add debugfs TA load/unload/invoke support
On 4/21/2022 4:08 PM, Li, Candice wrote:
> [Public]
>
>
>
>
>
> Thanks,
> Candice
>
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Thursday, April 21, 2022 6:03 PM
> To: Li, Candice <Candice.Li@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Clements, John <John.Clements@xxxxxxx>
> Subject: Re: [PATCH v2 2/2] drm/amdgpu: Add debugfs TA load/unload/invoke support
>
>
>
> On 4/21/2022 3:28 PM, Li, Candice wrote:
>> [Public]
>>
>>
>>
>>
>>
>> Thanks,
>> Candice
>>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
>> Sent: Thursday, April 21, 2022 5:23 PM
>> To: Li, Candice <Candice.Li@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>> Cc: Clements, John <John.Clements@xxxxxxx>
>> Subject: Re: [PATCH v2 2/2] drm/amdgpu: Add debugfs TA load/unload/invoke support
>>
>>
>>
>> On 4/20/2022 4:43 PM, Candice Li wrote:
>>> v1:
>>> Add debugfs support to load/unload/invoke TA in runtime.
>>>
>>> v2:
>>> 1. Update some variables to static.
>>> 2. Use PAGE_ALIGN to calculate shared buf size directly.
>>> 3. Remove fp check.
>>> 4. Update debugfs from read to write.
>>>
>>> Signed-off-by: John Clements <john.clements@xxxxxxx>
>>> Signed-off-by: Candice Li <candice.li@xxxxxxx>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/Makefile | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 2 +
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.c | 308 ++++++++++++++++++++
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.h | 30 ++
>>> 4 files changed, 341 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.c
>>> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.h
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> index 7d7af43a258f83..b525f9be9326f4 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>>> @@ -58,7 +58,7 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
>>> amdgpu_vm_sdma.o amdgpu_discovery.o amdgpu_ras_eeprom.o amdgpu_nbio.o \
>>> amdgpu_umc.o smu_v11_0_i2c.o amdgpu_fru_eeprom.o amdgpu_rap.o \
>>> amdgpu_fw_attestation.o amdgpu_securedisplay.o \
>>> - amdgpu_eeprom.o amdgpu_mca.o
>>> + amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o
>>>
>>> amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> index 13e4d8f9b87449..eedb12f6b8a32d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> @@ -38,6 +38,7 @@
>>> #include "amdgpu_umr.h"
>>>
>>> #include "amdgpu_reset.h"
>>> +#include "amdgpu_psp_ta.h"
>>>
>>> #if defined(CONFIG_DEBUG_FS)
>>>
>>> @@ -1767,6 +1768,7 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>>> DRM_ERROR("registering register debugfs failed (%d).\n", r);
>>>
>>> amdgpu_debugfs_firmware_init(adev);
>>> + amdgpu_ta_if_debugfs_init(adev);
>>>
>>> #if defined(CONFIG_DRM_AMD_DC)
>>> if (amdgpu_device_has_dc_support(adev))
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.c
>>> new file mode 100644
>>> index 00000000000000..247a476e63544c
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.c
>>> @@ -0,0 +1,308 @@
>>> +/*
>>> + * Copyright 2022 Advanced Micro Devices, Inc.
>>> + *
>>> + * 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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 "amdgpu.h"
>>> +#include "amdgpu_psp_ta.h"
>>> +
>>> +static const char *TA_IF_FS_NAME = "ta_if";
>>> +
>>> +struct dentry *dir;
>>> +static struct dentry *ta_load_debugfs_dentry;
>>> +static struct dentry *ta_unload_debugfs_dentry;
>>> +static struct dentry *ta_invoke_debugfs_dentry;
>>> +
>>> +static ssize_t ta_if_load_debugfs_write(struct file *fp, const char *buf,
>>> + size_t len, loff_t *off);
>>> +static ssize_t ta_if_unload_debugfs_write(struct file *fp, const char *buf,
>>> + size_t len, loff_t *off);
>>> +static ssize_t ta_if_invoke_debugfs_write(struct file *fp, const char *buf,
>>> + size_t len, loff_t *off);
>>> +
>>> +
>>> +static uint32_t get_bin_version(const uint8_t *bin)
>>> +{
>>> + const struct common_firmware_header *hdr =
>>> + (const struct common_firmware_header *)bin;
>>> +
>>> + return hdr->ucode_version;
>>> +}
>>> +
>>> +static void prep_ta_mem_context(struct psp_context *psp,
>>> + struct ta_context *context,
>>> + uint8_t *shared_buf,
>>> + uint32_t shared_buf_len)
>>> +{
>>> + context->mem_context.shared_mem_size = PAGE_ALIGN(shared_buf_len);
>>> + psp_ta_init_shared_buf(psp, &context->mem_context);
>>> +
>>> + memcpy((void *)context->mem_context.shared_buf, shared_buf, shared_buf_len);
>>> +}
>>> +
>>> +static bool is_ta_type_valid(enum ta_type_id ta_type)
>>> +{
>>> + bool ret = false;
>>> +
>>> + switch (ta_type) {
>>> + case TA_TYPE_RAS:
>>> + ret = true;
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static const struct file_operations ta_load_debugfs_fops = {
>>> + .write = ta_if_load_debugfs_write,
>>> + .llseek = default_llseek,
>>> + .owner = THIS_MODULE
>>> +};
>>> +
>>> +static const struct file_operations ta_unload_debugfs_fops = {
>>> + .write = ta_if_unload_debugfs_write,
>>> + .llseek = default_llseek,
>>> + .owner = THIS_MODULE
>>> +};
>>> +
>>> +static const struct file_operations ta_invoke_debugfs_fops = {
>>> + .write = ta_if_invoke_debugfs_write,
>>> + .llseek = default_llseek,
>>> + .owner = THIS_MODULE
>>> +};
>>> +
>>> +
>>> +/**
>>> + * DOC: AMDGPU TA debugfs interfaces
>>> + *
>>> + * Three debugfs interfaces can be opened by a program to
>>> + * load/invoke/unload TA,
>>> + *
>>> + * - /sys/kernel/debug/dri/<N>/ta_if/ta_load
>>> + * - /sys/kernel/debug/dri/<N>/ta_if/ta_invoke
>>> + * - /sys/kernel/debug/dri/<N>/ta_if/ta_unload
>>> + *
>>> + * How to use the interfaces in a program?
>>> + *
>>> + * A program needs to provide transmit buffer to the interfaces
>>> + * and will receive buffer from the interfaces below,
>>> + *
>>> + * - For TA load debugfs interface:
>>> + * Transmit buffer:
>>> + * - TA type (4bytes)
>>> + * - TA bin length (4bytes)
>>> + * - TA bin
>>> + * Receive buffer:
>>> + * - TA ID (4bytes)
>>> + *
>>> + * - For TA invoke debugfs interface:
>>> + * Transmit buffer:
>>> + * - TA ID (4bytes)
>>> + * - TA CMD ID (4bytes)
>>> + * - TA shard buf length (4bytes)
>>> + * - TA shared buf
>>> + * Receive buffer:
>>> + * - TA shared buf
>>> + *
>>> + * - For TA unload debugfs interface:
>>> + * Transmit buffer:
>>> + * - TA ID (4bytes)
>>> + */
>>> +
>>> +static ssize_t ta_if_load_debugfs_write(struct file *fp, const char *buf, size_t len, loff_t *off)
>>> +{
>>> + uint32_t ta_type = 0;
>>> + uint32_t ta_bin_len = 0;
>>> + uint8_t *ta_bin = NULL;
>>> + uint32_t copy_pos = 0;
>>> + int ret = 0;
>>> +
>>> + struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(fp)->i_private;
>>> + struct psp_context *psp = &adev->psp;
>>> + struct ta_context context = {0};
>>> +
>>> + if (!buf)
>>> + return -EINVAL;
>>> +
>>> + ret = copy_from_user((void *)&ta_type, &buf[copy_pos], sizeof(uint32_t));
>>> + if (ret || (!is_ta_type_valid(ta_type)))
>>> + return -EINVAL;
>>> +
>>> + copy_pos += sizeof(uint32_t);
>>> +
>>> + ret = copy_from_user((void *)&ta_bin_len, &buf[copy_pos], sizeof(uint32_t));
>>> + if (ret)
>>> + return -EINVAL;
>>> +
>>> + copy_pos += sizeof(uint32_t);
>>> +
>>> + ta_bin = kzalloc(ta_bin_len, GFP_KERNEL);
>>> + if (!ta_bin)
>>> + ret = -ENOMEM;
>>> + ret = copy_from_user((void *)ta_bin, &buf[copy_pos], ta_bin_len);
Missed another point here - during your tests, do you see the write
calls coming multiple times while passing TA binary from userspace?
Usually libc maintains buffers and if the size of the file is large, it
will be calling write multiple times with different offsets.
From the code, it looks like there is an assumption that TAs are small
enough that the entire TA can be passed in one shot.
Thanks,
Lijo
[Candice] Currently the interface is limited to RAS TA, so I only passed the RAS TA in my tests.
I didn't see it called write for multiple times during my tests.
>>> + if (ret)
>>> + goto err_free_bin;
>>> +
>>> + ret = psp_ras_terminate(psp);
>>> + if (ret) {
>>> + dev_err(adev->dev, "Failed to unload embedded RAS TA\n");
>>> + goto err_free_bin;
>>> + }
>>> +
>>> + context.ta_type = ta_type;
>>> + context.ta_load_type = GFX_CMD_ID_LOAD_TA;
>>> + context.bin_desc.fw_version = get_bin_version(ta_bin);
>>> + context.bin_desc.size_bytes = ta_bin_len;
>>> + context.bin_desc.start_addr = ta_bin;
>>> +
>>> + ret = psp_ta_load(psp, &context);
>>> +
>>> + if (ret || context.resp_status) {
>>> + dev_err(adev->dev, "TA load via debugfs failed (%d) status %d\n",
>>> + ret, context.resp_status);
>>> + goto err_free_bin;
>>> + }
>>> +
>>> + context.initialized = true;
>>> + ret = copy_to_user((char *)buf, (void *)&context.session_id, sizeof(uint32_t));
>>> +
>>> +err_free_bin:
>>> + kfree(ta_bin);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static ssize_t ta_if_unload_debugfs_write(struct file *fp, const char *buf, size_t len, loff_t *off)
>>> +{
>>> + uint32_t ta_id = 0;
>>> + int ret = 0;
>>> +
>>> + struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(fp)->i_private;
>>> + struct psp_context *psp = &adev->psp;
>>> + struct ta_context context = {0};
>>> +
>>> + if (!buf)
>>> + return -EINVAL;
>>> +
>>> + ret = copy_from_user((void *)&ta_id, buf, sizeof(uint32_t));
>>> + if (ret)
>>> + return -EINVAL;
>>> +
>>> + context.session_id = ta_id;
>>> +
>>> + ret = psp_ta_unload(psp, &context);
>>> + if (!ret)
>>> + context.initialized = false;
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static ssize_t ta_if_invoke_debugfs_write(struct file *fp, const char *buf, size_t len, loff_t *off)
>>> +{
>>> + uint32_t ta_id = 0;
>>> + uint32_t cmd_id = 0;
>>> + uint32_t shared_buf_len = 0;
>>> + uint8_t *shared_buf = NULL;
>>> + uint32_t copy_pos = 0;
>>> + int ret = 0;
>>> +
>>> + struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(fp)->i_private;
>>> + struct psp_context *psp = &adev->psp;
>>> + struct ta_context context = {0};
>>> +
>>> + if (!buf)
>>> + return -EINVAL;
>>> +
>>> + ret = copy_from_user((void *)&ta_id, &buf[copy_pos], sizeof(uint32_t));
>>> + if (ret)
>>> + return -EINVAL;
>>> + copy_pos += sizeof(uint32_t);
>>> +
>>> + ret = copy_from_user((void *)&cmd_id, &buf[copy_pos], sizeof(uint32_t));
>>> + if (ret)
>>> + return -EINVAL;
>>> + copy_pos += sizeof(uint32_t);
>>> +
>>> + ret = copy_from_user((void *)&shared_buf_len, &buf[copy_pos], sizeof(uint32_t));
>>> + if (ret)
>>> + return -EINVAL;
>>> + copy_pos += sizeof(uint32_t);
>>> +
>>> + shared_buf = kzalloc(shared_buf_len, GFP_KERNEL);
>>> + if (!shared_buf)
>>> + ret = -ENOMEM;
>>> + ret = copy_from_user((void *)shared_buf, &buf[copy_pos], shared_buf_len);
>>> + if (ret)
>>> + goto err_free_shared_buf;
>>> +
>>> + context.session_id = ta_id;
>>> +
>>> + prep_ta_mem_context(psp, &context, shared_buf, shared_buf_len);
>>> +
>>> + ret = psp_ta_invoke_indirect(psp, cmd_id, &context);
>>> +
>>> + if (ret || context.resp_status) {
>>> + dev_err(adev->dev, "TA invoke via debugfs failed (%d) status %d\n",
>>> + ret, context.resp_status);
>>> + goto err_free_ta_shared_buf;
>>> + }
>>> +
>>> + ret = copy_to_user((char *)buf, context.mem_context.shared_buf, shared_buf_len);
>>> +
>>> +err_free_ta_shared_buf:
>>> + psp_ta_free_shared_buf(&context.mem_context);
>>> +
>>> +err_free_shared_buf:
>>> + kfree(shared_buf);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static struct dentry *amdgpu_ta_if_debugfs_create(struct amdgpu_device *adev)
>>> +{
>>> + struct drm_minor *minor = adev_to_drm(adev)->primary;
>>> +
>>> + dir = debugfs_create_dir(TA_IF_FS_NAME, minor->debugfs_root);
>>> +
>>> + ta_load_debugfs_dentry = debugfs_create_file("ta_load", 0200, dir, adev,
>>> + &ta_load_debugfs_fops);
>>> +
>>> + ta_unload_debugfs_dentry = debugfs_create_file("ta_unload", 0200, dir,
>>> + adev, &ta_unload_debugfs_fops);
>>> +
>>> + ta_invoke_debugfs_dentry = debugfs_create_file("ta_invoke", 0200, dir,
>>> + adev, &ta_invoke_debugfs_fops);
>>
>> This uses global variables, is this tested on a system with multiple GPUs?
>>
>> Thanks,
>> Lijo
>>
>> [Candice] Yes, it has been tested on mGPU system and works.
>>
>
> Did you also cover amdgpu_ta_if_debugfs_remove() during your tests? I'm
> wondering how it removes the directory of all devices as by above logic
> 'dir' will always point to the entry of the last probed device.
>
> Thanks,
> Lijo
>
> [Candice] That's a good point. Not yet for degbufs remove function, it has not been called in the initial patch.
>
>>> + return dir;
>>> +}
>>> +
>>> +void amdgpu_ta_if_debugfs_init(struct amdgpu_device *adev)
>>> +{
>>> +#if defined(CONFIG_DEBUG_FS)
>>> + dir = amdgpu_ta_if_debugfs_create(adev);
>>> +#endif
>>> +}
>>> +
>>> +void amdgpu_ta_if_debugfs_remove(void)
>>> +{
>>> + debugfs_remove_recursive(dir);
>>> +}
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.h
>>> new file mode 100644
>>> index 00000000000000..883f89d57616d0
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp_ta.h
>>> @@ -0,0 +1,30 @@
>>> +/*
>>> + * Copyright 2022 Advanced Micro Devices, Inc.
>>> + *
>>> + * 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
>>> + *
>>> + */
>>> +
>>> +#ifndef __AMDGPU_PSP_TA_H__
>>> +#define __AMDGPU_PSP_TA_H__
>>> +
>>> +void amdgpu_ta_if_debugfs_init(struct amdgpu_device *adev);
>>> +void amdgpu_ta_if_debugfs_remove(void);
>>> +
>>> +#endif
>>>
