Re: [PATCH 1/2] Revert "include/uapi/drm/amdgpu_drm.h: use __u32 and __u64 from <linux/types.h>"

[Marek OlÅÃk]

On Fri, Aug 19, 2016 at 7:12 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Fri, Aug 19, 2016 at 7:11 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
>> On Fri, Aug 19, 2016 at 5:22 PM, Marek OlÅÃk <maraeo@xxxxxxxxx> wrote:
>>> On Fri, Aug 19, 2016 at 4:52 PM, Mikko Rapeli <mikko.rapeli@xxxxxx> wrote:
>>>> On Fri, Aug 19, 2016 at 04:26:40PM +0200, Christian KÃnig wrote:
>>>>> Am 19.08.2016 um 15:50 schrieb Marek OlÅÃk:
>>>>> >From: Marek OlÅÃk <marek.olsak@xxxxxxx>
>>>>> >
>>>>> >This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f.
>>>>> >
>>>>> >See the comment in the code. Basically, don't do cleanups in this header.
>>>>> >
>>>>> >Signed-off-by: Marek OlÅÃk <marek.olsak@xxxxxxx>
>>>>>
>>>>> I completely agree with you that this was a bad move, but I fear that we
>>>>> will run into opposition with that.
>>>>>
>>>>> Adding Mikko Rapeli who made the reverted patch to comment.
>>>>
>>>> But this header is part of Linux kernel uapi. Remove it from there too then.
>>>
>>> That's a good idea, but it really is a uapi header in the sense that
>>> it defines the kernel driver interface for a specific kernel version.
>>> However, it is not a header that the userspace stack should include,
>>> because userspace should get it from libdrm. (it makes userspace more
>>> independent from the currently running kernel)
>>
>> Please don't remove it from the kernel side if it's included in
>> libdrm. Emil&I just spent a bit of time making sure those two sets of
>> headers match when we produce them using the make headers_install
>> target.
>>
>> drm is indeed special, as in our headers aren't shipped with the
>> kernel-headers package but libdrm. But otherwise they are supposed to
>> work exactly like any other uapi headers. No need to move them out of
>> the uapi headers - I'd even say that would be bad since it removes the
>> visibility and clear marker that this header must be considered uapi!
>>
>> Also the very loud comment at the top is definitely misleading,
>> there's nothing that guarnatees that the libdrm and kernel copy are in
>> sync when you build or run userspace. Also maybe for context, but what
>> exactly  is the problem with the __ types?
>>
>> Adding Emil.
>
> Adding Emil for real ...

My understanding is that the problem was that userspace had to include
stdint.h before including these uapi headers. That's not a problem for
2 reasons:

1) Userspace doesn't include these headers, but includes the libdrm
headers instead.

2) The few userspace drivers and tools that include the libdrm headers
can also include stdint.h. User-friendliness isn't required here.

Marek