On 11/5/2024 6:35 PM, Lazar, Lijo wrote: > > On 11/5/2024 2:13 AM, Felix Kuehling wrote: >> On 2024-10-31 22:15, Zhu Lingshan wrote: >>> On 10/31/2024 11:27 PM, Felix Kuehling wrote: >>>> On 2024-10-31 6:47, Zhu Lingshan wrote: >>>>> The header usr/linux/kfd_ioctl.h is a duplicate of uapi/linux/kfd_ioctl.h. >>>> I don't see usr/linux/kfd_ioctl.h. Which branch are you looking at? >>> The mainline master branch: >>> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/amd/amdkfd/kfd_priv.h#L35 >>> #include <linux/kfd_ioctl.h> >>>>> And it is actually not a file in the source code tree. >>>> If it's a file that only there on your local working tree, maybe just clean up your working tree. >>> Yes, this is a file on the local working tree, it is generated when build, it is not in source code tree. >>> The is even no folder usr/include/linux before building. >>> >>> For a quick verify: >>> 1) it is not a source code file from mainline: >>> $git blame ./usr/include/linux/kfd_ioctl.h >>> fatal: no such path 'usr/include/linux/kfd_ioctl.h' in HEAD >>> >>> 2) it is generated when build: >>> remove usr/include/linux/kfd_ioctl.h, then build. >>> If we remove a required header, kfd would not >>> build and there will be error messages. >>> >>> but here we will see these lines: >>> HDRINST usr/include/linux/kfd_ioctl.h >>> HDRTEST usr/include/linux/kfd_ioctl.h, >>> no build errors, and usr/include/linux/kfd_ioctl.h is >>> generated by duplicating the uapi one. >>> >>> >>> 2) linux/kfd_ioctl.h is usr/include/linux/kfd_ioctl.h >>> add random characters in usr/include/linux/kfd_ioctl.h, then build >>> we will see errors like this: >>> >>> In file included from <command-line>: >>> ./usr/include/linux/kfd_ioctl.h:65:9: error: expected ‘;’ before ‘struct’ >>> 65 | abcd1234 >>> | ^ >>> | ; >>> 66 | >>> 67 | struct kfd_ioctl_create_queue_args { >>> | ~~~~~~ >>>>> Ideally, the usr version should be updated whenever the source code is recompiled. >>>> If the usr version is not in the git repository, it doesn't need to be updated. I don't know where this is coming from on your local tree. >>> the usr one would be installed to the system when run make install or install_headers, >>> it is for user space, we should include the uapi one instead of usr one in our source code files >> I did not see the folder in my tree because I build with O=... so usr/include/linux/kfd_ioctl.h shows up in my build output tree. >> >> The extra copy of the user mode headers seems to be an artifact of the Linux kernel build system. The fact that the build picks up user mode headers from <OUT>/usr/include/... seems intentional. If your tree has an outdated version of kfd_ioctl.h there, it's probably something broken with your build tree. Maybe broken file timestamps. It should be easy to fix with a "make mrproper" to force it to update all the build artifacts. >> >> I still don't think we need to change the code to fix a problem specific to your build system. >> > Looking at the number of occurrences with "#include <uapi/", it appears > like explicitly mentioning as uapi/linux for header location is a better > practice. Gentle ping, Felix Thanks Lingshan > > Thanks, > Lijo > >> Regards, >> Felix >> >>> Thanks >>> Lingshan >>>> Regards, >>>> Felix >>>> >>>>> However, I have noticed a discrepancy between the two headers >>>>> even after rebuilding the kernel. >>>>> >>>>> This commit modifies kfd_priv.h to always include the header from uapi to ensure >>>>> the latest changes are reflected. We should always include the source >>>>> code header other than the duplication. >>>>> >>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@xxxxxxx> >>>>> --- >>>>> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >>>>> index 26e48fdc8728..78de7ac09e8a 100644 >>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h >>>>> @@ -32,7 +32,7 @@ >>>>> #include <linux/atomic.h> >>>>> #include <linux/workqueue.h> >>>>> #include <linux/spinlock.h> >>>>> -#include <linux/kfd_ioctl.h> >>>>> +#include <uapi/linux/kfd_ioctl.h> >>>>> #include <linux/idr.h> >>>>> #include <linux/kfifo.h> >>>>> #include <linux/seq_file.h>
