[RESEND] amdkfd: always include uapi header in priv.h

[Zhu Lingshan]

On 11/5/2024 4:43 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.
exactly! usr/include/linux/kfd_ioctl.h is not shown until we build the kernel, it is a duplication of the uapi header generated by
kernel "make", that is why we did not see it until "make". If you run git blame usr/include/linux/kfd_ioctl.h,
git will tell us: fatal: no such path 'usr/include/linux/kfd_ioctl.h' in HEAD

In a fresh cloned kernel tree, without "make", in usr/include, try [lszhu at localhost include]$ find -name kfd_ioctl.h
shows nothing, usr/include/linux/kfd_ioct.h is not there until build the kernel.

Means 'usr/include/linux/kfd_ioctl.h' is not a source code file, thus we should include the original uapi one in our code.

>
> 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.
the usr one is an extra copy, kernel build system copies headers from uapi folder to usr/include/linux, so the uapi headers are the original ones.

Thanks
Lingshan
>
> I still don't think we need to change the code to fix a problem specific to your build system.
>
> 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 at amd.com>
>>>> ---
>>>>  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>